Skip to content

Fix #364: make one-based Month deserializer accept "12" #365

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

Merged
merged 2 commits into from
Mar 30, 2025

Conversation

bbobcik
Copy link
Contributor

@bbobcik bbobcik commented Mar 26, 2025

As described in #364 , com.fasterxml.jackson.datatype.jsr310.deser.OneBasedMonthDeserializer incorrectly assumed that all values can be converted to a Month value by the original deserializer. This assumptions fails for "12".

In this fix the OBMD is refactored in order to depend less on the original deserializer. Certain inputs (1-digit and 2-digit numbers and their string representations) are handled - validated and parsed - directly. Non-numeric inputs as well as numbers that have 3 or more digits are forwarded to the delegate, as before.

Associated tests are expanded to cover all months.

if (zeroBaseMonth == Month.JANUARY) {
throw new InvalidFormatException(parser, "Month.JANUARY value not allowed for 1-based Month.", zeroBaseMonth, Month.class);
if (_isPossibleNumericValue(token)) {
String monthSpec = parser.getText();
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this should not use getText() if we get actual number (JsonToken.VALUE_NUMBER_INT) but just call parser.getIntValue().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, reworked in 3f564fa

@cowtowncoder
Copy link
Member

First of all: thank you for reporting the issue and providing a fix! It looks good overall.

But one thing we need before merging (if not done earlier): we'd need CLA (from here: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf ).
It is needed just once before merging the first PR; works for all future contributions (so if you have sent one in the past please LMK).

The usual way is to print it, fill & sign, email to cla at fasterxml dot com.

Once we have it, we can proceed with merging.

Thanks again!

@cowtowncoder cowtowncoder added the cla-received Marker to denote that there is a CLA for pr label Mar 29, 2025
@cowtowncoder
Copy link
Member

CLA received: hoping to review and merge this weekend.

@cowtowncoder cowtowncoder merged commit 6283ad1 into FasterXML:2.19 Mar 30, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-received Marker to denote that there is a CLA for pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants