jackson-modules-java8 icon indicating copy to clipboard operation
jackson-modules-java8 copied to clipboard

InstantSerializer doesn't respect any format-related settings without replacing serializer instance

Open Thorn1089 opened this issue 5 years ago • 9 comments

InstantSerializer doesn't respect any of the following standard Jackson configuration settings:

ObjectMapper#setDateFormat @JsonFormat(pattern) on an Instant others?

Instead, I have to create and manually override the default InstantSerializer registered by the module.

This isn't well documented and is definitely confusing when using the library. Can we make InstantSerializer actually respect date-related or pattern-related options from the core library?

Looking at InstantSerializerBase, the provided context has the format from the mapper, but doesn't use it. The formatters attached to the serializer directly are both null by default.

public void serialize(T value, JsonGenerator generator, SerializerProvider provider) throws IOException {
        if (this.useTimestamp(provider)) {
            if (this.useNanoseconds(provider)) {
                generator.writeNumber(DecimalUtils.toBigDecimal(this.getEpochSeconds.applyAsLong(value), this.getNanoseconds.applyAsInt(value)));
            } else {
                generator.writeNumber(this.getEpochMillis.applyAsLong(value));
            }
        } else {
            String str;
            if (this._formatter != null) {
                str = this._formatter.format(value);
            } else if (this.defaultFormat != null) {
                str = this.defaultFormat.format(value);
            } else {
                str = value.toString();
            }

            generator.writeString(str);
        }
    }

So the SerializationFeature.WRITE_DATES_AS_TIMESTAMPS is respected but that's it.

Does it make sense to use the provider's defaultSerializeDateValue(long timestamp, JsonGenerator gen) method here and pass the timestamp from the Instant?

Thorn1089 avatar Mar 26 '20 18:03 Thorn1089

Yes, I would be happy to review, merge PR that adds configubility. Coverage is incomplete mostly because @JsonFormat.pattern was added after this module was created and support has been contributed incrementally. Depending on timing, this could still go in 2.11 (but time is getting close to 2.11.0), or 2.12 if not 2.11.

As to using defaultSerializeDateValue... ideally yes, but in practice probably not, the way things work, simply because that would use functionality of java.util date/time classes (and old formatter), which has its issues. For 3.x it would perhaps make sense to change this so that both java.util and Java 8 date/time would default to handling by latter, as it is superior implementation-wise. If anyone wants to tackle that (master is for Jackson 3.0), that would be a big bit useful undertaking.

@kupci WDYT?

cowtowncoder avatar Mar 27 '20 15:03 cowtowncoder

For 3.x it would perhaps make sense to change this so that both java.util and Java 8 date/time would default to handling by latter, as it is superior implementation-wise.

@cowtowncoder Sounds good, would this fall under the date-time-config work? Or is it big enough to be a related task by itself? (I haven't checked to see if this is under the 'new ideas' section.)

kupci avatar Mar 30 '20 20:03 kupci

@kupci my first instinct is that this would probably be separate task.

cowtowncoder avatar Mar 30 '20 23:03 cowtowncoder

Hey guys, I will take this one.

obarcelonap avatar Oct 29 '20 05:10 obarcelonap

After taking a look at the code and the comments here, not sure if this is working already and if is needed. Please can you provide some guidance on how to proceed? thanks in advance.

obarcelonap avatar Oct 29 '20 12:10 obarcelonap

@obarcelonap the original description is bit vague, but maybe you could add a unit test(s) to show that pattern and shape overrides work (to switch between timestamp/textual, custom pattern), for serialization (since this is what issue is for)?

If there are existing tests that you think covers them that's fine too, but I suspect there may be some gaps.

cowtowncoder avatar Oct 29 '20 18:10 cowtowncoder

I can get @JsonFormat to properly format my Instants, it does however seem like the ObjectMapper#setDateFormat is ignored. Reproducible in version 2.12.2.

static class Pojo {
    final Instant d;
    public Pojo(final Instant d) {
        this.d = d;
    }
}
...
final ObjectMapper mapper = new ObjectMapper()
    .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY)
    .registerModule(new JavaTimeModule())
    .setDateFormat(new SimpleDateFormat("yy-MM-dd HH:mm"));
final String s = mapper.writer().writeValueAsString(new Pojo(Instant.now()));
System.out.println(s);
> {"d":"2021-05-03T14:00:49.696022Z"}

aelgn avatar May 03 '21 14:05 aelgn

InstantSerializer cannot DateFormat since that type is only usable with pre-Java8 types, java.util.Calendar (and then java.util.Date). So that setting will not and cannot have any effect on Java 8 or Joda date/time types.

JsonFormat (either through annotation @JsonFormat or config overrides) should work on all types but will have to be registered on multiple types (with config override approach) or per-property. Config overrides are used like:

        mapper.configOverride(Instant.class)
                // Bad format, no timezone/offset... but whatever:
                .setFormat(JsonFormat.Value.forPattern("yy-MM-dd HH:mm")):

cowtowncoder avatar May 03 '21 20:05 cowtowncoder

Ah yes of course! Silly me. Thank you. On a side note; I vote to deprecate support for java.util time and merge com.fasterxml.jackson.datatype.jsr310 into jackson 3. The old java.util time types has become my own personal ie6 :)

aelgn avatar May 04 '21 08:05 aelgn