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

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

Merged

Conversation

etrandafir93
Copy link
Contributor

@etrandafir93 etrandafir93 commented Dec 1, 2023

Implements #274:

if (_oneBaseMonths) {
return Month.of(monthNo);
}
return Month.values()[monthNo];
Copy link
Member

Choose a reason for hiding this comment

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

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

@cowtowncoder
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

etrandafir93 commented Dec 1, 2023

@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
Copy link
Member

cowtowncoder commented Dec 2, 2023

@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
Copy link
Contributor Author

etrandafir93 commented Dec 4, 2023

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!

if(type.getRawClass() != Month.class) {
return deserializer;
}
return new JsonDeserializer<Enum>() {
Copy link
Member

Choose a reason for hiding this comment

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

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(...) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.deser.std.DelegatingDeserializer;
import com.fasterxml.jackson.databind.exc.InvalidFormatException;

Copy link
Member

Choose a reason for hiding this comment

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

Add @since 2.17 in Javadoc here too

}

private boolean _isNumberAsString(String text, JsonToken token) throws IOException {
String regex = "[\"']?\\d{1,2}[\"']?";
Copy link
Member

Choose a reason for hiding this comment

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

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 :) ).

/*
&& we write the enum ordinal (int) value,
via WRITE_ENUMS_USING_INDEX or other similar construct
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

@Override
public JsonDeserializer<?> modifyEnumDeserializer(DeserializationConfig config, JavaType type, BeanDescription beanDesc, JsonDeserializer<?> defaultDeserializer) {
if (type.hasRawClass(Month.class)) {
return new MonthDeserializer((JsonDeserializer<Enum>) defaultDeserializer, _oneBaseMonths);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

@Override
public JsonSerializer<?> modifyEnumSerializer(SerializationConfig config, JavaType valueType, BeanDescription beanDesc, JsonSerializer<?> serializer) {
if (valueType.hasRawClass(Month.class)) {
return new MonthSerializer((JsonSerializer<Enum>) serializer, _oneBaseMonths);
Copy link
Member

Choose a reason for hiding this comment

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

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.

private final boolean _oneBaseMonths;

public JavaTimeSerializerModifier() {
this(false);
Copy link
Member

Choose a reason for hiding this comment

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

Is this constructor needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I removed it now 👍

public class JavaTimeDeserializerModifier extends BeanDeserializerModifier {
private final boolean _oneBaseMonths;

public JavaTimeDeserializerModifier() {
Copy link
Member

Choose a reason for hiding this comment

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

Probably not needed?

@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jan 14, 2024

@Override
public void serialize(Enum<Month> value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
if (_serializeEnumsByIndex.get()) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@cowtowncoder
Copy link
Member

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

@cowtowncoder cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jan 15, 2024
@cowtowncoder
Copy link
Member

CLA received, not blocking any more.

private final boolean _oneBaseMonths;
private final Supplier<Boolean> _serializeEnumsByIndex;

public JavaTimeSerializerModifier(boolean oneBaseMonths, Supplier<Boolean> serializeEnumsByIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@cowtowncoder
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

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

@cowtowncoder cowtowncoder merged commit d8d0f5c into FasterXML:2.17 Jan 16, 2024
4 checks passed
@cowtowncoder cowtowncoder changed the title 274: add MonthDeserializer and JavaTimeFeature option Add 1-based Month[De]serializer and JavaTimeFeature option Jan 16, 2024
@cowtowncoder cowtowncoder changed the title Add 1-based Month[De]serializer and JavaTimeFeature option Add 1-based Month[De]serializer enabled with JavaTimeFeature.ONE_BASED_MONTHS option Jan 16, 2024
@cowtowncoder cowtowncoder added this to the 2.17.0 milestone Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants