Skip to content
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

Deserializing java.time.Month from an int causes an off-by-one error (0->Jan,11->Dec), because it's an enum #274

Closed
kreiger opened this issue May 3, 2023 · 5 comments
Labels
2.17 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@kreiger
Copy link

kreiger commented May 3, 2023

I found an issue with deserializing java.time.Month from an integer, or from a string containing an integer.

Since it's an enum, 0 deserializes as Month.JANUARY, 1/"01" as Month.FEBRUARY, 11/"11" as Month.DECEMBER, and so on.

I have a PR with a failing test that proves this: kreiger#1

@kreiger kreiger changed the title 'java java.time.Month deserialization from an integer is off by one, because it's an enum. May 3, 2023
@kreiger kreiger changed the title java.time.Month deserialization from an integer is off by one, because it's an enum. Deserializing java.time.Month from an int causes an off-by-one error (0->Jan,11->Dec), because it's an enum. May 3, 2023
@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label May 3, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented May 3, 2023

Looks like there is no explicit serializer/deserializer for type Month so it is dealt as a regular Enum.
I can see how this would lead to unexpected results, although at the same time it is technically compliant with Jackson behavior for Enums.

A challenge is that change in this logic is backwards incompatible change: so we'd probably need to add a configuration option -- f.ex add new config enum JavaTimeFeature, with setting like... ONE_BASED_MONTHS (default: false) and then custom serializer, deserializer (to override default Enum serializer/deserializer).

This sounds like a good idea, but quite a bit of work. Since Jackson 2.16.0 is being released any day now this probably has to wait until 2.17.

@cowtowncoder cowtowncoder added the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label May 3, 2023
@cowtowncoder
Copy link
Member

Note: when implemented, should be enabled by a feature added to JavaTimeFeature (included in 2.16 to help configurability).

@etrandafir93
Copy link
Contributor

hello @cowtowncoder, I started looking into this issue.
I have created a MR where you can see my progress: #292

I'll need a bit of guidance for the test related to the strictMapper - configOverride. Except for that, I fixed the other tests you provided and I added additional ones to check the solution is backwards compatible.

Which code formatter should I use for this project?

@cowtowncoder
Copy link
Member

@etrandafir93 Ok sounds good: I added a few comments on PR. ConfigOverrides can be handled by something similar to what YearDeserializer does (it's pretty good source as basis, actually).

As to formatter, I don't think we have one so it's more of "When in Rome" style to follow -- most Jackson projects are similar, although there are some differences (esp. for Kotlin, Scala modules).
But some basic rules:

  1. Indentation by spaces (4 per level)
  2. Imports from most generic to most specific (JDK, 3rd party lib, Jackson core, specific module)
  3. Variable naming, follow project (usually underscore-leading member variables, otherwise usual camel case)

there is a jackson-databind issue filed that asks writing down Jackson Coding Style, something I need to get done.

@cowtowncoder cowtowncoder changed the title Deserializing java.time.Month from an int causes an off-by-one error (0->Jan,11->Dec), because it's an enum. Deserializing java.time.Month from an int causes an off-by-one error (0->Jan,11->Dec), because it's an enum Jan 16, 2024
@cowtowncoder cowtowncoder added this to the 2.17.0 milestone Jan 16, 2024
@cowtowncoder
Copy link
Member

New feature merged in 2.17 for inclusion in 2.17.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants