Add 1-based `Month[De]serializer` enabled with `JavaTimeFeature.ONE_BASED_MONTHS` option

[etrandafir93]

Implements #274:

[cowtowncoder]

@etrandafir93 First of all, thank you for this contribution! I added a few suggestions, mostly to change things that could be done in better way.

One practical thing before merging (once we get code all cleaned up!) is CLA: unless we have received one before, we'd need it from here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the usual way is to print, fill & sign, scan/photo, email to cla at fasterxml dot com. This only needs to be done once, and CLA is good for all future contributions.

Thank you once again; looking forward to merging this useful new feature!

[cowtowncoder]

@etrandafir93 I figured it might be quicker for me to make suggested changes instead of explaining what and how should be changed. I hope you don't mind pushing changes to PR.

But one thing that is bit worse off now, and that needs to be figure out is this: should "Stringified" numbers be accepted like regular JSON numbers or not? Other deserializers only allow them for special case of backends that have StreamReadCapability.UNTYPED_SCALARS enabled (currently meaning XML and CSV), but not for typed formats. So I changed code in a way that makes this work: if you feel strongly this is wrong you can change that; either way, tests need to be aligned with whether these are allowed.

I did not add bounds checks for numbers; those should be done etc.

[etrandafir93]

@etrandafir93 I figured it might be quicker for me to make suggested changes instead of explaining what and how should be changed. I hope you don't mind pushing changes to PR.

But one thing that is bit worse off now, and that needs to be figure out is this: should "Stringified" numbers be accepted like regular JSON numbers or not? Other deserializers only allow them for special case of backends that have StreamReadCapability.UNTYPED_SCALARS enabled (currently meaning XML and CSV), but not for typed formats. So I changed code in a way that makes this work: if you feel strongly this is wrong you can change that; either way, tests need to be aligned with whether these are allowed.

I did not add bounds checks for numbers; those should be done etc.

@cowtowncoder - sure, feel free to commit on by branch. It's the first time I'm looking into the code and I'm not very familiar with it, but here's another idea that will allow us to skip re-implementing this parsing:

If the feature is OFF (default) we can delegate to the generic enum deserializer, making sure we are 100% backaward compatible.
If the feature is ON we can still rely on the enum generic deserializer, but then just return the previous month (so if generic deserializer returns Month.FEBRUARY, we map it to Month.JANUARY).

Basically, a very simple decorator that wraps the generic enum deserializer. What do you think?

[cowtowncoder]

@etrandafir93 Ah! Conceptually I like the idea a lot -- and it should be quite compatible with existing handling, too (since EnumDeserializer is currently used).

The challenge, then, becomes that of implementing delegating deserializer. A good starting point would be DelegatingDeserializer from jackson-databind. An instance would need to be registered via different callback: BeanDeserializerModifier method modifyEnumDeserializer() that would create delegating instance for specific case of type Month.class (passing default deserializer to DelegatingDeserializer).

And then DelegatingDeserializer implementation would need to override 2 deserialize() method and deserializeWithType() (ideally), to have special handling for 1-based case.

Above may not sound particularly simple, but that is the correct way to do it and should work very well. I can help with complications, if any.

[etrandafir93]

hello @cowtowncoder, I have made some changes to start using BeanDeserializerModifier and I believe i'm on the right track. However, I'm not sure I've fully understood your instructions. Can you take a look at the latest commit?

Is there a way of registering the BeanDeserializerModifier for a specific type better than what I did with the if type.getRawClass() != Month.class? Let me know what you think about the solution. If it's ok, I'll move on with formatting, documentation and other small refactorings.

Thanks for the support and quick replies!

[cowtowncoder]

At this point, I'd need CLA before proceeding further.

[cowtowncoder]

CLA received, not blocking any more.

[cowtowncoder]

With CLA received, looks like there are only small changes needed:

1. Since "one-based" Feature check can be done by modifiers (or if we want, even by Module as modifiers only needed for 1-based ser/deser, for now), can drop checks by Serializer/Deserializer
2. Looks like some unit test(s) fail, need to resolve those.

and then I can merge this into 2.17 branch. Thanks!

[cowtowncoder]

@etrandafir93 Ok: I fixed the one failing test (it was assuming coercion from Empty String is allowed by default; it is not for Enums so changed test). WIll see if I can make the one last change to remove passing of Supplier.

[cowtowncoder]

@etrandafir93 Ok, made all changes I wanted and ready to merge. But wanted to give a chance to see if there's anything you would want to change still. Please LMK and I'll merge this when ready!

[etrandafir93]

@cowtowncoder - nothing else to add from my side. Thank you for the guidance and support here!