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

[etrandafir93]

Implements #274:

[cowtowncoder]

Same as above, but more important as we don't want to get ArrayIndexOutOfBundsException...

[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]

Ok, this is too simplistic: my idea was to extend DelegatingDeserializer passing the default deserializer and overriding its deserialize() method -- ONLY handling special case of 1-based month and number, otherwise calling super.deserialize() (getDelegatee().deserialize(...) .

[etrandafir93]

I have applied your suggestions and a few other small changes. Can you take a look when you'll have some time?
Also, I haven't tackled the part about serialization yet - will we follow the same approach?

[cowtowncoder]

Looks pretty good so far: added couple of suggestions again, nothing major.

As to serializer, yeah, I think same pattern is usable: BeanSerializerModifier to wrap JsonSerializer.
Although may need to look at logic of EnumSerializer for clues on how handling works wrt configuration (see EnumSerializer.serialize(...)) -- I think override is only needed for case where we serialize using index; as text should be fine.

[cowtowncoder]

Add @since 2.17 in Javadoc here too

[cowtowncoder]

Ok, couple of things: parser.getText() will return contents, so no quotes around it, can leave those out.

And make sure to create pre-compiled Pattern as static member of class: otherwise RegEx state machine is re-create for every call... and that's pretty expensive (and more importantly easily avoidable :) ).

[etrandafir93]

@cowtowncoder - Hello again and sorry for the late reply, I finally got some time to continue working here.
I have applied your suggestions and refactored the code a bit.

Regarding serialization, I am not sure how to check if we ar serializing the enum as an integer (via WRITE_ENUMS_USING_INDEX ) or other similar configuration. How should I do it?

[cowtowncoder]

Hmmh. Good question. Ideally we should be able to use EnumSerializer but that seems difficult if not impossible, at least by sub-classing (method serialize() is final for one but also initialization is quite tricky).
Similarly delegation is difficult to use wrt changing of value to serialize.

But EnumSerializer has useful pieces at least.

"createContextual":

https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/ser/std/EnumSerializer.java#L136C40-L136C40

"serialize()" :

https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/ser/std/EnumSerializer.java#L168

and "_isShapeWrittenUsingIndex"

https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/ser/std/EnumSerializer.java#L263

implement logic.

[cowtowncoder]

And thinking out aloud further it'd be nice to use delegating serializer. There is StdDelegatingSerializer (but no intermediate DelegatingSerializer can't remember why not) which may or may not.

[etrandafir93]

@cowtowncoder - I have tried a few things but couldn't find the best way of doing it using these methods.
However, I managed to pass the WRITE_ENUMS_USING_INDEX feature as a Supplier that can be later checked , during the serialization - can you take a look? si there a more straight-forward way of doing this?

[cowtowncoder]

Shouldn't this only need to be returned if _oneBasedMonths is true? That might simplify handling a bit when there are fewer checks.

[etrandafir93]

@cowtowncoder - it's mostly about the naming and meaning of the classes. If we do it like this, perhaps it's better to rename the deserializer from MonthDeserializer to OneBasedMonthDeserializer and make it always updated the value returned by the delegate (without checking the _oneBasedMonths flag -- and move the check here).
What do you think?

[cowtowncoder]

Yes, I think that makes sense -- I like explicit naming.

[cowtowncoder]

Similar to deserializer: shouldn't this only need to be returned if _oneBasedMonths is true?
That might simplify handling a bit when there are fewer checks.

[cowtowncoder]

Is this constructor needed?

[etrandafir93]

no, I removed it now 👍

[cowtowncoder]

Probably not needed?

[cowtowncoder]

Use SerializerProvider.isEnabled(SerializationFeature.WRITE_ENUMS_USING_INDEX) instead of passing Supplier.

[cowtowncoder]

Actually if/when serializer only registered when feature is enabled, can drop the check altogether.

[cowtowncoder]

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

[cowtowncoder]

CLA received, not blocking any more.

[cowtowncoder]

As per other discussions, can drop Supplier and checks altogher as Serializer is only used if one-based-months feature is set.

[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!