-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix #221 Support alternate radixes for numeric values #5317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
src/main/java/com/fasterxml/jackson/databind/cfg/BaseSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/cfg/BaseSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/cfg/BaseSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/cfg/MapperConfig.java
Outdated
Show resolved
Hide resolved
...n/java/com/fasterxml/jackson/databind/deser/std/FromStringWithRadixToNumberDeserializer.java
Show resolved
Hide resolved
|
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. |
src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/DeserializationConfig.java
Outdated
Show resolved
Hide resolved
pjfanning
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some changes requested
| return Long.parseLong(text, radix); | ||
| } else { | ||
| ctxt.reportInputMismatch(handledType, | ||
| "Trying to deserialize a non-whole number with NumberToStringWithRadixSerializer"); |
There was a problem hiding this comment.
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.
src/main/java/com/fasterxml/jackson/databind/ser/std/NumberSerializer.java
Outdated
Show resolved
Hide resolved
|
Impressive PR! One big(ger) question/concern I have is the addition of but I realize there's the problem of changing radix for ALL integral types. Perhaps that could be tackled by adding actual I'd just not want to force it through all machinery via |
|
Fails CI; probably uses some Java 17 language feature (2.x still has Java 8 baseline; 3.x has 17) |
|
Just want to preface by saying this is not a final change but more of an outline where I am still using A few comments and questions:
<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 I think we have 2 options here:
|
|
Quick note on: I think it's |
|
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. |
|
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? |
|
Fudge! Was not supposed to merge; need to revert ASAP |
|
I do not know how, but somehow I managed to merge this -- not via UI. So had to revert via #5373. Not sure how to re-create PR... sorry. I hate git sometimes. Today is such a day. :-( |
|
I can just try and get it from my local branch. Should I create a new PR? |
|
New PR sounds good @tiger9800 |
|
I think the changes are still in. Though the change is incomplete, and I will finish it in with a new PR that starts using radix property of |
|
Just my opinion but I would prefer if this was removed from 2.x and only a 3.x target considered. |
|
Do you mean the new pull request or should we revert the changes from this one in 2.x? |
|
Agree wrt this targeting 3.1. |
New PR with changes targeting |
|
I see it in my 2.x local branch after pulling, but I guess there is a different 2.x branch somewhere. I will make the new PR targeting 3.x once the annotation change is merged. |
|
@tiger9800 Just merged annotations changes -- should be available via |
Main Github repo has |
|
I pulled the 3.x branch, but it does not look like radix made it to JsonFormat for some reason. Anything special I need to do? |
|
Jackson-annotations remains as a 2.x only release. Ignore any 3.x snapshots. Use com.fasterxml.jackson.core jackson-annotations |
|
Looks like you need the 2.21-SNAPSHOT |
|
When I look at the version pulled in from jackson bom, I see However, I see that it says that 2.21 is not released yet in the release notes hence databind cannot use it. |
Your PR can use jackson-annotations 2.21-SNAPSHOT - we will ensure that the jars are released in the right order. If you create a branch based on jackson-databind 3.x branch, that build should already depend on jackson-annotations 2.21-SNAPSHOT. |
|
@tiger9800 Things "should just work" when starting branch from |
|
After running mvn clean install -U, this is what I see for JsonFormat (i.e. radix missing): |
|
@tiger9800 I am not sure how you get the sources, but have a look at jar they come from, it is likely an older version: needs to be "2.21-SNAPSHOT". Jackson-annotations, branch merged via and CI-built and published to Sonatype snapshot repo. So something in your system is stale. It could be |
|
The name of the jar says it's 2.21: jackson-annotations-2.21-20250909.010129-3-sources.jar |
|
From September…. Way old 😀
```
20250909.010129-3-sources.jar
^^
```
so you need to make sure you get later/-est snapshot either built locally, or from Sonatype snapshot repo (as defined on `pom.xml` of project you are building).
Or refresh IDE etc.
…On Thu, Nov 20, 2025 at 2:54 PM Davyd Fridman ***@***.***> wrote:
*tiger9800* left a comment (FasterXML/jackson-databind#5317)
<#5317 (comment)>
The name of the jar says it's 2.21:
jackson-annotations-2.21-20250909.010129-3-sources.jar
—
Reply to this email directly, view it on GitHub
<#5317 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAANOGKFI2EUNY2557EPJPD35ZBARAVCNFSM6AAAAACGTGD3SSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKNRQGQ3TQOJTGM>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
|
I am pulling the right dependency now. I have the change, and I am trying to debug it with my test. I thought maybe it was my change but after dropping my commit I still see the same issue with other tests. Any chance you know what is happening? |
|
@tiger9800 test actions seem to be passing, so try clearing library cache |
|
@JooHyukKim I think the actions use maven and |
When writing numeric values as strings, we would like to support alternative to base-10 representations. Let's add it via
@JsonFormatannotation. Whenever the shape is STRING and pattern is a number, we serialize/deserialize annotated properties using the specified alternative radix.