Skip to content

Conversation

tiger9800
Copy link

When writing numeric values as strings, we would like to support alternative to base-10 representations. Let's add it via @JsonFormat annotation. Whenever the shape is STRING and pattern is a number, we serialize/deserialize annotated properties using the specified alternative radix.

When writing numeric values as strings, we would like to
support alternative to base-10 representations. Let's add it
via @jsonformat annotation. Whenever the shape is STRING
and pattern is a number, we serialize/deserialize annotated
properties using the specified alternative radix.
@tiger9800
Copy link
Author

I based the change on the 2.x branch. I understand that the latest release is 2.20 and this should is a backwards compatible change, so it should be done against the next minor version (2.21), but I could not find it as one of the remote branches.

Also addressing the comments now @pjfanning, thank you.

Copy link
Member

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some changes requested

@tiger9800 tiger9800 marked this pull request as draft September 16, 2025 02:57
return Long.parseLong(text, radix);
} else {
ctxt.reportInputMismatch(handledType,
"Trying to deserialize a non-whole number with NumberToStringWithRadixSerializer");
Copy link
Author

@tiger9800 tiger9800 Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered explicitly throwing here, but this seemed like a more common way to indicate an error.

@tiger9800 tiger9800 marked this pull request as ready for review September 16, 2025 20:23
@tiger9800 tiger9800 requested a review from pjfanning September 16, 2025 20:23
@tiger9800 tiger9800 requested a review from pjfanning September 16, 2025 22:53
@cowtowncoder cowtowncoder added the cla-received PR already covered by CLA (optional label) label Sep 18, 2025
@cowtowncoder
Copy link
Member

2.x is the right target branch indeed (for 2.21.x).

Impressive PR!

One big(ger) question/concern I have is the addition of radix in BaseSettings as discrete config option. I think I'd prefer requiring use of "Config Overrides" with JsonFormat.Value a la:

        mapper.configOverride(Integer.TYPE) // for `int`
            .setFormat(JsonFormat.Value.forShape(JsonFormat.Shape.STRING));
            // but with "Value.forPattern("16")"

but I realize there's the problem of changing radix for ALL integral types.

Perhaps that could be tackled by adding actual radix property for @JsonFormat annotation, and then allowing setting of base radix in ConfigOverrides, merged via ConfigOverride.
Or something along those lines.

I'd just not want to force it through all machinery via BaseSettings.

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 18, 2025

Fails CI; probably uses some Java 17 language feature (2.x still has Java 8 baseline; 3.x has 17)

@tiger9800
Copy link
Author

tiger9800 commented Sep 19, 2025

Just want to preface by saying this is not a final change but more of an outline where I am still using pattern as the field containing radix.

A few comments and questions:

  • I initially wanted to make ConfigOverrides have a JsonFormat.Value field instead of just radix, but there is a test that checks that ObjectReader is serializable. ObjectReader has a reference to ConfigOverrides through some other configs it refers to directly. As a result, we want to serialize ConfigOverrides as a part of that test and JsonFormat.Value is not serializable because Feature does not implement Serializable.

  • I refactored ConcreteBeanPropertyBase#findPropertyFormat because it seemed like an appropriate place to use _defaultFormat (that I was going to get from ConfigOverride) as a base format that gets overriden by v1 and v2. Since keeping JsonFormat.Value in ConfigOverride did not work, I just reserved to creating a JsonFormat.Value inside the method from an empty format, and I kept the refractor. To make sure the refactor works as intended, I added a unit test that uses stubs. I did not find a lot of other tests that use mockito, so I am not sure it is something you do in the repo. The problem with using mockito is that it shows a warning described here https://openjdk.org/jeps/451. The solution (described here) is adding a VM flag to test run configuration, and a flag to surefire plugin:

<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-surefire-plugin</artifactId>
	<version>3.5.2</version>
	<configuration>
		<argLine>
			-javaagent:${org.mockito:mockito-core:jar}
			-Xshare:off
		</argLine>
	</configuration>
</plugin>

However, the solution only works with newer mockito jars. The mockito-core-4.11.0.jar databind depends on does not specify Premain class in its MANIFEST.MF. I do not know which version is the first one to specify it, but here is an example of Premain being specified in mockito-core-5.19.0.jar's MANIFEST.MF: Premain-Class: org.mockito.internal.PremainAttach.
I also left a TODO saying this needs to be done once mockito is upgraded.

I think we have 2 options here:

  1. Create another issue to upgrade mockito & add the argument to the surefire plugin
  2. Remove the test
  • Finally, I was wondering what is the correct process for updating another "internal" project databind depends on to use a newly added construct from there. Specifically for adding radix field to the JsonFormat in the annotations repo, how do we get a new release of the annotations repo and then change the databind's pom to point to that new release?

@cowtowncoder
Copy link
Member

Quick note on:

JsonFormat.Value is not serializable because Feature does not implement Serializable.

I think it's Features (since Feature is Enum and should be java.io.Serializable).
But either way, could you please file an issue on jackson-annotations as we definitely do want all Value objects to be Serializable.
(change to go in 2.21 but good to solve now).

@tiger9800
Copy link
Author

Will file an issue and try to address it!

Please let me know what you think about the other questions/concerns once you get the chance.

@tiger9800
Copy link
Author

FasterXML/jackson-annotations#316 - made the pull request. Would it make sense to make the change to annotations and go with the initial approach that failed?

@cowtowncoder
Copy link
Member

Annotation stuff now fixed, should be serializable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-received PR already covered by CLA (optional label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants