NumberFormat support by hurelhuyag · Pull Request #4273 · FasterXML/jackson-databind

hurelhuyag · 2023-12-20T14:59:24Z

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java

cowtowncoder · 2023-12-20T19:30:29Z

Ah. Thank you for contributing this!

One quick note: we'd need unit tests to show how this works; class Javadocs would be useful too, to indicate expectation of using format notation of DecimalFormat

cowtowncoder · 2023-12-20T19:31:22Z

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java

            switch (format.getShape()) {
            case STRING:
+                if (format.hasPattern()) {
+                    return new NumberSerializer(handledType(), new DecimalFormat(format.getPattern()));


Probably needs a try-catch block for invalid format (and probably unit test to show handling), re-throw exception (using one of methods from SerializerProvider).

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java

cowtowncoder · 2023-12-22T05:13:28Z

I realized that there is a good reason why I'd like to see tests: I am pretty sure this will not work for most numeric fields :)
Specifically, for int/Integer, long/Long.
(I can elaborate why, but it'd be good to add tests first to see if I am right or not).

cowtowncoder · 2023-12-22T05:18:10Z

src/test/java/com/fasterxml/jackson/databind/format/NumberFormatTest.java

+
+    public void testInvalidPattern() throws Exception {
+        ObjectMapper mapper = newJsonMapper();
+        Assert.assertThrows(JsonMappingException.class, () -> {


Could you check the exception message too (so it's the expected exception)

cowtowncoder · 2023-12-22T05:27:52Z

@hurelhuyag I think we can made this work. I hope you don't mind my pushing couple of changes to streamline things.

The next thing, I think, is adding bit more testing -- tests you added look good! -- for other number types; int, long, double (assuming these can be formatted with DecimalFormat? At least double/Double should, right?).

And then I think we need to resolve use of shape wrt values other than STRING / NUMBER / NUMBER_INT / NUMBER_FLOAT. Specially, what to do with ANY / missing.

JooHyukKim

🔥

JooHyukKim · 2023-12-22T12:18:20Z

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java

+                        }
+                    } catch (IllegalArgumentException e) {
+                        return prov.reportBadDefinition(handledType(),
+                                String.format("Invalid `DecimalFormat`: \"%s\"", format.getPattern()));


Suggested change

String.format("Invalid `DecimalFormat`: \"%s\"", format.getPattern()));

String.format("Invalid 'DecimalFormat': \"%s\"", format.getPattern()));

Can we use single quote instead of backtick? I looked around the codebase and in most cases (not all) single quote is used.

JooHyukKim · 2023-12-22T12:26:09Z

src/test/java/com/fasterxml/jackson/databind/format/NumberFormatTest.java

+    public void testTypeDefaults() throws Exception
+    {
+        ObjectMapper mapper = newJsonMapper();
+        mapper.configOverride(BigDecimal.class)
+            .setFormat(new JsonFormat.Value("00,000.00", JsonFormat.Shape.STRING, (Locale) null, (TimeZone) null, null, null));
+        String json = mapper.writeValueAsString(new NumberWrapper(new BigDecimal("1234")));
+        assertEquals(a2q("{'value':'01,234.00'}"), json);
+
+        // and then read back is not supported yet.
+        /*NumberWrapper w = mapper.readValue(a2q("{'value':'01,234.00'}"), NumberWrapper.class);
+        assertNotNull(w);
+        assertEquals(new BigDecimal("1234"), w.value);*/
+    }
+
+    protected static class InvalidPatternWrapper {
+        @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "#,##0.#.#")
+        public BigDecimal value;
+
+        public InvalidPatternWrapper(BigDecimal value) {
+            this.value = value;
+        }
+    }
+
+    public void testInvalidPattern() throws Exception {
+        ObjectMapper mapper = newJsonMapper();
+        Assert.assertThrows(JsonMappingException.class, () -> {
+            mapper.writeValueAsString(new InvalidPatternWrapper(BigDecimal.ZERO));
+        });
+    }


Suggested change

public void testTypeDefaults() throws Exception

{

ObjectMapper mapper = newJsonMapper();

mapper.configOverride(BigDecimal.class)

.setFormat(new JsonFormat.Value("00,000.00", JsonFormat.Shape.STRING, (Locale) null, (TimeZone) null, null, null));

String json = mapper.writeValueAsString(new NumberWrapper(new BigDecimal("1234")));

assertEquals(a2q("{'value':'01,234.00'}"), json);

// and then read back is not supported yet.

/*NumberWrapper w = mapper.readValue(a2q("{'value':'01,234.00'}"), NumberWrapper.class);

assertNotNull(w);

assertEquals(new BigDecimal("1234"), w.value);*/

}

protected static class InvalidPatternWrapper {

@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "#,##0.#.#")

public BigDecimal value;

public InvalidPatternWrapper(BigDecimal value) {

this.value = value;

}

}

public void testInvalidPattern() throws Exception {

ObjectMapper mapper = newJsonMapper();

Assert.assertThrows(JsonMappingException.class, () -> {

mapper.writeValueAsString(new InvalidPatternWrapper(BigDecimal.ZERO));

});

}

protected static class InvalidPatternWrapper {

@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "#,##0.#.#")

public BigDecimal value;

public InvalidPatternWrapper(BigDecimal value) {

this.value = value;

}

}

@Test

public void testTypeDefaults() throws Exception

{

ObjectMapper mapper = newJsonMapper();

mapper.configOverride(BigDecimal.class)

.setFormat(new JsonFormat.Value("00,000.00", JsonFormat.Shape.STRING, (Locale) null, (TimeZone) null, null, null));

String json = mapper.writeValueAsString(new NumberWrapper(new BigDecimal("1234")));

assertEquals(a2q("{'value':'01,234.00'}"), json);

// and then read back is not supported yet.

/*NumberWrapper w = mapper.readValue(a2q("{'value':'01,234.00'}"), NumberWrapper.class);

assertNotNull(w);

assertEquals(new BigDecimal("1234"), w.value);*/

}

@Test

public void testInvalidPattern() throws Exception {

ObjectMapper mapper = newJsonMapper();

try {

mapper.writeValueAsString(new InvalidPatternWrapper(BigDecimal.ZERO));

fail();

} catch (JsonMappingException e) {

verifyException(e, "expected message part 1/2"); //

verifyException(e, "expected message part 2/2");

}

}

Couple of suggestion on test if that's okay with you 🙂

Let's use JUnit 5, (meaning remove extends BaseMapTest

Class declaration comes first then test methods (Check suggestion)

As suggested above, check message by using try-catch and verifyException (from DatabindTestUtil) combo

Above comment seems like Jackson Coding Style convention idea 🤔... WDYT, @cowtowncoder ?

JooHyukKim · 2023-12-22T12:29:15Z

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java

        if (format != null) {
            switch (format.getShape()) {
            case STRING:
+                if (format.hasPattern()) {


Suggested change

if (format.hasPattern()) {

// [databind#1114] since 2.17 : Support @JsonFormat for String, numbers, using String.format()

if (format.hasPattern()) {

JooHyukKim · 2023-12-22T12:29:24Z

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java

    @Override
    public void serialize(Number value, JsonGenerator g, SerializerProvider provider) throws IOException
    {
+        if (_format != null) {


Suggested change

if (_format != null) {

// [databind#1114] since 2.17 : Support @JsonFormat for String, numbers, using String.format()

if (_format != null) {

hurelhuyag · 2023-12-25T02:26:47Z

I realized that there is a good reason why I'd like to see tests: I am pretty sure this will not work for most numeric fields :) Specifically, for int/Integer, long/Long. (I can elaborate why, but it'd be good to add tests first to see if I am right or not).

Why would you think that? Isn't the JDK doc clear?
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/text/NumberFormat.html#format(java.lang.Object,java.lang.StringBuffer,java.text.FieldPosition)

yihtserns · 2023-12-25T17:28:26Z

Why would you think that? Isn't the JDK doc clear?

I think he's hinting that NumberSerializer does not handle the common & Atomic* number data types - this PR (in the current state) would only likely handle BigDecimal, BigInteger, and other non-common Number subclasses eg LongAdder.

yihtserns · 2023-12-25T19:28:00Z

Just want to note that current PR covers subset of #1114:

	Serialization	Deserialization
int/Integer	❌	❌
long/Long	❌	❌
byte/Byte	❌	❌
short/Short	❌	❌
float/Float	❌	❌
double/Double	❌	❌
BigDecimal	✅	❌
BigInteger	✅	❌
Other Number subclasses e.g. LongAdder	✅	❌
AtomicInteger	❌	❌
AtomicLong	❌	❌

Misc things that we may/may not want to also support:

@JsonFormat.lenient
@JsonFormat.locale

But I think it's a good start. I do think that using DecimalFormat is better mainly because the same pattern can be used for both formatting AND parsing - I don't think String.format pattern can be used for parsing.

And then I think we need to resolve use of shape wrt values other than STRING / NUMBER / NUMBER_INT / NUMBER_FLOAT. Specially, what to do with ANY / missing.

I'd suggest to only deal with Shape.STRING & (what to do with) Shape.ANY for #1114, to keep things small and focused so they can move faster. There's probably more than enough things to deal with even with just those 2 shapes.

yihtserns · 2023-12-25T19:36:45Z

One quick note: we'd need unit tests to show how this works; class Javadocs would be useful too, to indicate expectation of using format notation of DecimalFormat

Question for core maintainers: where is the doc explaining the kind of @JsonFormat.pattern that is usable for a particular data type?:

I don't see them in the javadoc of JsonFormat.pattern.
There's only (outdated?) mention of pattern for Date in the javadoc of JsonFormat.

Is the full list kept somewhere else, or they have yet to exist?

cowtowncoder · 2023-12-26T19:15:37Z

One quick note: we'd need unit tests to show how this works; class Javadocs would be useful too, to indicate expectation of using format notation of DecimalFormat

Question for core maintainers: where is the doc explaining the kind of @JsonFormat.pattern that is usable for a particular data type?:
* I don't see them in the [javadoc of JsonFormat.pattern](https://fasterxml.github.io/jackson-annotations/javadoc/2.14/com/fasterxml/jackson/annotation/JsonFormat.html#pattern--).

* There's only (outdated?) mention of pattern for Date in the [javadoc of JsonFormat](https://fasterxml.github.io/jackson-annotations/javadoc/2.14/com/fasterxml/jackson/annotation/JsonFormat.html).
Is the full list kept somewhere else, or they have yet to exist?

Such a definition does not yet exist. Part of this is the practical challenge of location.

For example, while it would be sort of natural from User POV to find the definition in @JsonFormat Javadocs, this seems bad place from maintenance perspective: actual handling is not implemented by annotation package or annotation types.

But at the same time, finding definitions from -- say -- individual deserializers' Javadocs would not be useful at all.

So I think this should probably be a combination of:

A wiki page (or README.md) in one of repos (jackson, jackson-docs or jackson-databind?)
Links from @JsonFormat Javadocs at least; and maybe from other documentation (cross-refs)

So the first thing would be to decide on (1) I think, and create a skeletal page with some of known cases, general idea behind annotations.

cowtowncoder · 2023-12-26T19:19:29Z

I realized that there is a good reason why I'd like to see tests: I am pretty sure this will not work for most numeric fields :) Specifically, for int/Integer, long/Long. (I can elaborate why, but it'd be good to add tests first to see if I am right or not).

Why would you think that? Isn't the JDK doc clear? https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/text/NumberFormat.html#format(java.lang.Object,java.lang.StringBuffer,java.text.FieldPosition)

No, this has nothing to do with Formatter being used, but rather that NumberSerializer is not used by Jackson for most field types. So I don't mean that formatter object could not be used by other number types, but that serializer being used by Jackson varies by type: there are more optimized serializers for primitive types for example.

If you test, for example

public class DoubleWrapper {
   @JsonFormat(...)
   public double value;
}

you will notice that nothing added via NumberSerializer will take effect: NumberSerializer is only used for, I think, BigDecimal and cases where nominal type of field is Number (like, List<Number>).
But if type is declared as double or Double, different serializer is used. Same goes for most or all primitive types.

NumberFormat support

adfc5c2

hurelhuyag mentioned this pull request Dec 20, 2023

Support @JsonFormat for String, numbers, using String.format() #1114

Open

cowtowncoder reviewed Dec 20, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Dec 20, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java Outdated Show resolved Hide resolved

hurelhuyag added 2 commits December 21, 2023 10:09

NumberFormat.format test

118555f

invalid pattern test

3a50d67

cowtowncoder reviewed Dec 22, 2023

View reviewed changes

Minor refactoring

35f0c60

JooHyukKim reviewed Dec 22, 2023

View reviewed changes

	String.format("Invalid `DecimalFormat`: \"%s\"", format.getPattern()));
	String.format("Invalid 'DecimalFormat': \"%s\"", format.getPattern()));

	if (format.hasPattern()) {
	// [databind#1114] since 2.17 : Support @JsonFormat for String, numbers, using String.format()
	if (format.hasPattern()) {

	if (_format != null) {
	// [databind#1114] since 2.17 : Support @JsonFormat for String, numbers, using String.format()
	if (_format != null) {

Uh oh!

Comments

Conversation

hurelhuyag commented Dec 20, 2023 • edited by cowtowncoder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cowtowncoder commented Dec 20, 2023

Uh oh!

cowtowncoder Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cowtowncoder commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cowtowncoder Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

cowtowncoder commented Dec 22, 2023

Uh oh!

JooHyukKim left a comment

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

hurelhuyag commented Dec 25, 2023

Uh oh!

yihtserns commented Dec 25, 2023

Uh oh!

yihtserns commented Dec 25, 2023

Uh oh!

yihtserns commented Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cowtowncoder commented Dec 26, 2023

Uh oh!

cowtowncoder commented Dec 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hurelhuyag commented Dec 20, 2023 •

edited by cowtowncoder

Loading

cowtowncoder commented Dec 22, 2023 •

edited

Loading

yihtserns commented Dec 25, 2023 •

edited

Loading