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

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

Open
Thorn1089 opened this issue Mar 26, 2020 · 9 comments
Labels
good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards json-format Missing handling of `@JsonFormat` property annotation

Comments

@Thorn1089
Copy link

Thorn1089 commented Mar 26, 2020

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?

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 27, 2020

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?

@kupci kupci added the documentation Issue that documentation improvements could solve or alleviate label Mar 30, 2020
@kupci
Copy link
Member

kupci commented Mar 30, 2020

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

@cowtowncoder
Copy link
Member

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

@cowtowncoder cowtowncoder added 2.12 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards json-format Missing handling of `@JsonFormat` property annotation and removed documentation Issue that documentation improvements could solve or alleviate labels Sep 30, 2020
@obarcelonap
Copy link
Contributor

Hey guys, I will take this one.

@obarcelonap
Copy link
Contributor

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.

@cowtowncoder
Copy link
Member

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

@aelgn
Copy link

aelgn commented May 3, 2021

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"}

@cowtowncoder
Copy link
Member

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

@aelgn
Copy link

aelgn commented May 4, 2021

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

@cowtowncoder cowtowncoder removed the 2.12 label Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards json-format Missing handling of `@JsonFormat` property annotation
Projects
None yet
Development

No branches or pull requests

5 participants