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
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,16 @@ public enum JavaTimeFeature implements JacksonFeature
* stringified numbers are always accepted as timestamps regardless of
* this feature.
*/
ALWAYS_ALLOW_STRINGIFIED_DATE_TIMESTAMPS(false)
ALWAYS_ALLOW_STRINGIFIED_DATE_TIMESTAMPS(false),

/**
* Feature that determines whether {@link java.time.Month} is serialized
* and deserialized as using a zero-based index (FALSE) or a one-based index (TRUE).
* For example, "1" would be serialized/deserialized as Month.JANUARY if TRUE and Month.FEBRUARY if FALSE.
*<p>
* Default setting is false, meaning that Month is serialized/deserialized as a zero-based index.
*/
ONE_BASED_MONTHS(false)
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.time.*;

import com.fasterxml.jackson.core.json.PackageVersion;
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
import com.fasterxml.jackson.core.util.JacksonFeatureSet;

import com.fasterxml.jackson.databind.*;
Expand Down Expand Up @@ -142,6 +143,11 @@ public void setupModule(SetupContext context) {
desers.addDeserializer(ZoneOffset.class, JSR310StringParsableDeserializer.ZONE_OFFSET);

context.addDeserializers(desers);

boolean oneBasedMonth = _features.isEnabled(JavaTimeFeature.ONE_BASED_MONTHS);
context.addBeanDeserializerModifier(new JavaTimeDeserializerModifier(oneBasedMonth));
context.addBeanSerializerModifier(new JavaTimeSerializerModifier(oneBasedMonth));

// 20-Nov-2023, tatu: [modules-java8#288]: someone may have directly
// added entries, need to add for backwards compatibility
if (_deserializers != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.fasterxml.jackson.datatype.jsr310.deser;

import java.time.Month;

import com.fasterxml.jackson.databind.BeanDescription;
import com.fasterxml.jackson.databind.DeserializationConfig;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.deser.BeanDeserializerModifier;

/**
* @since 2.17
*/
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?

this(false);
}

public JavaTimeDeserializerModifier(boolean oneBaseMonths) {
_oneBaseMonths = oneBaseMonths;
}

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

}
return defaultDeserializer;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package com.fasterxml.jackson.datatype.jsr310.deser;

import java.io.IOException;
import java.time.Month;
import java.util.regex.Pattern;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.DeserializationContext;
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

/**
* @since 2.17
*/
public class MonthDeserializer extends DelegatingDeserializer {
private static final Pattern HAS_ONE_OR_TWO_DIGITS = Pattern.compile("^\\d{1,2}$");
private final boolean _enableOneBaseMonths;

public MonthDeserializer(JsonDeserializer<Enum> defaultDeserializer, boolean oneBaseMonths) {
super(defaultDeserializer);
_enableOneBaseMonths = oneBaseMonths;
}

@Override
public Object deserialize(JsonParser parser, DeserializationContext context) throws IOException {
JsonToken token = parser.currentToken();
Month zeroBaseMonth = (Month) getDelegatee().deserialize(parser, context);
if (!_enableOneBaseMonths || !_isNumericValue(parser.getText(), token)) {
return zeroBaseMonth;
}
if (zeroBaseMonth == Month.JANUARY) {
throw new InvalidFormatException(parser, "Month.JANUARY value not allowed for 1-based Month.", zeroBaseMonth, Month.class);
}
return zeroBaseMonth.minus(1);
}

private boolean _isNumericValue(String text, JsonToken token) {
return token == JsonToken.VALUE_NUMBER_INT || _isNumberAsString(text, token);
}

private boolean _isNumberAsString(String text, JsonToken token) {
return token == JsonToken.VALUE_STRING && HAS_ONE_OR_TWO_DIGITS.matcher(text).find();
}

@Override
protected JsonDeserializer<?> newDelegatingInstance(JsonDeserializer<?> newDelegatee) {
return new MonthDeserializer((JsonDeserializer<Enum>) newDelegatee, _enableOneBaseMonths);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package com.fasterxml.jackson.datatype.jsr310.ser;

import java.time.Month;

import com.fasterxml.jackson.databind.BeanDescription;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.SerializationConfig;
import com.fasterxml.jackson.databind.ser.BeanSerializerModifier;

/**
* @since 2.17
*/
public class JavaTimeSerializerModifier extends BeanSerializerModifier {
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 JavaTimeSerializerModifier(boolean oneBaseMonths) {
_oneBaseMonths = oneBaseMonths;
}

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

}
return serializer;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.fasterxml.jackson.datatype.jsr310.ser;

import java.io.IOException;
import java.time.Month;

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.SerializerProvider;

/**
* @since 2.17
*/
public class MonthSerializer extends JsonSerializer<Enum<Month>> {
private final boolean _enableOneBaseMonths;
private final JsonSerializer<Enum> _defaultSerializer;

public MonthSerializer(JsonSerializer<Enum> defaultSerializer, boolean oneBaseMonths) {
_defaultSerializer = defaultSerializer;
_enableOneBaseMonths = oneBaseMonths;
}

@Override
public void serialize(Enum<Month> value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
if(_enableOneBaseMonths
/*
&& 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?

) {
gen.writeNumber(value.ordinal() + 1);
return;
}
_defaultSerializer.serialize(value, gen, serializers);
}

}
Loading
Loading