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

Allow Instant to be serialized as epochSecond without the fraction part #116

Open
bric3 opened this issue Jul 4, 2019 · 20 comments
Open
Labels
date-time-config Work related to possible larger DateTimeConfig feature new-feature

Comments

@bric3
Copy link

bric3 commented Jul 4, 2019

I have an Instant field that represents an epoch timestamp in seconds.

    @JsonProperty("registered_at")
    @JsonFormat(shape = JsonFormat.Shape.NUMBER)
    private Instant registeredAt;

the mapper is configured this way

    .featuresToDisable(ADJUST_DATES_TO_CONTEXT_TIME_ZONE,
                       FAIL_ON_UNKNOWN_PROPERTIES,
                       WRITE_DATES_AS_TIMESTAMPS,
                       SORT_PROPERTIES_ALPHABETICALLY)

However when this gets serialized as

"registered_at" : 1420324047.000000000

By looking at the code there's no way to serialize this value as epoch seconds using the standard mechanism.

InstantSerializerBase.serialize(T value, JsonGenerator generator, SerializerProvider provider)

        if (useTimestamp(provider)) {
            if (useNanoseconds(provider)) {
                generator.writeNumber(DecimalUtils.toBigDecimal(
                        getEpochSeconds.applyAsLong(value), getNanoseconds.applyAsInt(value)
                ));
                return;
            }
            generator.writeNumber(getEpochMillis.applyAsLong(value));
            return;
        }

The only option is to use :

    @JsonGetter("registered_at")
    public long getRegisteredAtEpoch() {
        return registeredAt.getEpochSecond();
    }

It could be nice if there could be way to tell jackson to avoid serializing the fraction part.

sashikanthr added a commit to sashikanthr/jackson-modules-java8 that referenced this issue Feb 27, 2020
sashikanthr added a commit to sashikanthr/jackson-modules-java8 that referenced this issue Feb 27, 2020
* test: add initial test case

* fix: fix problems in test case

for issue #1

* feat: add test cases for timestamps part and nanoseconds part

related to issue #1

* create a false test cases

#1

* Use flag for diabling fraction

* Use flag for diabling fraction

* Added the flag for DurationSerializer classes

* Removed test cases that are not relevant to the added Serialization Feature

* Only keep requirement specific tests

Some tests were set up to get an understanding of the code. They should
not be kept and included in the pull request

Co-authored-by: Nagisa <[email protected]>
Co-authored-by: Jacob Adlers <[email protected]>
@kupci
Copy link
Member

kupci commented Feb 27, 2020

Thanks for the contribution, taking a look, and also interested in @cowtowncoder 's thoughts on a new flag in SerializationFeature.

The nanoseconds handling is one area where there are a few bugs in the issues list. With that in mind, one initial thought on the following:

/**
     * Feature that determines whether time values are serialized with a
     * fraction part or not.
     *<p>
     * Note: if enabled the feature {@link
     * #WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS} is ignored in {@link
     * com.fasterxml.jackson.module.jackson-modules-java8}.
     *<p>
     * Feature is disabled by default.
     */

Given the comment that the WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS feature is ignored, there seems to be some overlap between these flags. Unavoidable, or is there a documentation / coding issue? The problem with adding another flag is that it gets a little unwieldy after a while, and as here we have the scenario where one flag is now ignored if another is set, which might not be apparent unless one is paying attention, and thus could lead to complaints.

Given that, it would be helpful to have a corresponding note in the WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS about this behavior, and also the project documentation for the Serialization Features should be updated as part of the PR:
https://github.com/FasterXML/jackson-databind/wiki/Serialization-Features

Anyway, will review this further, thanks again.

@cowtowncoder
Copy link
Member

I have some concerns about the new SerializationFeature, but haven't yet been able to formulate exact problem.

@kupci
Copy link
Member

kupci commented Mar 1, 2020

Ok, here are a couple thoughts, I'll add these to the actual code (in the PR) too, but too summarize:

  1. Maybe the SerializationFeature could be WRITE_DATE_TIMESTAMPS_AS_EPOCHSECONDS, so it is a little more consistent with other flag WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS
  2. Instead of dividing the millis by 1000, I think you could just call getEpochSeconds, i.e.:
    generator.writeNumber(getEpochSeconds.applyAsLong(value));

The other thing to still look into is to make sure there aren't any other impacts/changes required, such as in the deserializers? The Travis build ran successfully but that's not always fool-proof. For example, wouldn't there be a corresponding change required in the InstantDeserializer, which is currently assuming nanos or millis? I think a round-trip unit test would validate this, i.e. serialize it out and then deserialize that value..

protected T _fromLong(DeserializationContext context, long timestamp)
    {
        if(context.isEnabled(DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS)){
            return fromNanoseconds.apply(new FromDecimalArguments(
                    timestamp, 0, this.getZone(context)
            ));
        }
        return fromMilliseconds.apply(new FromIntegerArguments(
                timestamp, this.getZone(context)));
    }

@cowtowncoder
Copy link
Member

Other things to note:

  1. In addition to Shape.NUMBER, there are "subtype" choices of Shape.NUMBER_INT and Shape.NUMBER_FLOAT that would allow specifying where to include fractional part
  2. When Integer value is specified without some explicit override, default really should be (if not even MUST be) milliseconds: this is what JDK has traditionally used, and while some other systems (Unix timestamps) use other units, I do not feel comfortable using other default. Even with various Features.
  3. We do not have to limit ourselves only to Features (Deserialization-, Serialization-, Mapper) -- with Builder-style construction, it would be possibly to have something like DateTimeConfig` configuration object that could define richer semantics

On (3), this would obviously be bigger change, and there would be challenges regarding merging those settings with already existing alternatives. But it is an option, if (but only if) we managed to define consistent set of date/time configuration options that could work across all date/time types (that is, "classic" JDK Date, Calendar; Joda; Java 8 date/time).

@kupci
Copy link
Member

kupci commented Mar 2, 2020

Yes, I think the Shape.* has helped in a few other cases like this, so that might be a better route. Also, one thing I was thinking about was the case where there are nanoseconds (non-zero), and in the current proposal they'd be lost? That would likely create questions/issues.

Now for the DateTimeConfig, that is something new, so a bit farther out (3.0, etc)?

@cowtowncoder
Copy link
Member

@kupci DateTimeConfig could be implemented for 2.x, since Builder-style approach (added in 2.10) makes it much easier to define "immutable" configuration (one that can not be changed after constructing ObjectMapper). I was hoping to add NodeConfig or similar in 2.11 (see https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP-3), but I don't seem to have enough time to implement it.

If you had time and interest, one possibility would be to maybe write up similar (well, more extensive ideally :) JSTEP entry explaining overall changes to date/time handling. I mean, if you can think of ... maybe collection of ideas, proposals, either for a later 2.x or 3.0.
I think it is tricky to find out an effective way to hash out such bigger ideas, change plans.
But maybe starting with a small document could be one way; this is what I was hoping to achieve with JSTEP idea/approach, as per:

https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP

@kupci
Copy link
Member

kupci commented Mar 2, 2020

On the Shape solution, I think the Shape.NUMBER_FLOAT would've helped avoid the need for the SR WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS :)

But now with EpochSeconds, I don't see a way it would solve this, as both milliseconds and epoch seconds would be Shape.NUMBER_INT.

And another concerning thing, speaking of defaults and Cowtowncoder's point about the millis default, I just noticed, at least based on the javadoc, the WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS is defaulting to true. Note that this is not an issue with the current PR, it is somehow already in the codebase..

So the DateTimeConfig - I will add that as a JSTEP, hoping I can borrow some ideas or template from the NodeConfig.

However, that looks like a longer effort, I'm working with Jakob on the original changes given these points:

  1. It looks like there is a gap (i.e. there should be support for epoch seconds)
  2. The proposed fix looks fairly straightforward, low-risk fix (at the expense of adding yet another flag)
  3. There is already precedent set with the other SR flags (though this may be falling for the 'sunk costs' fallacy?)

Do you think we should proceed with this route (new SR flag), or fully go with the longer effort towards the DateTimeConfig?

@cowtowncoder
Copy link
Member

Right, that whole Time[stamp] Unit for integers (or floats, come to think of that) is my main concern.

I create JSTEP placeholder at:

https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP-5

and you should be able to edit it (if not, let me know and I'll figure out access).

I added 2 main idea-lets:

  1. Time[stamp] unit (for @JsonFormat?)
  2. DateTimeConfig

I don't think I will accept the PR for 2.11, due to timing and not being convinced it is the way to go -- this gives us some more time to consider good (not perfect) approach, and that can involve longer route.

On DateTimeConfig, in particular: I have no concerns wrt how to plug that in, and can help with that. What I do think is challenging (and the crux) is simple aspects to configure: figuring out things that should vary. And after that, how to merge those with existing settings.

@kupci
Copy link
Member

kupci commented Mar 4, 2020

specifically, while [De]SerializationFeature on/off options are simple, their use does not work well with multiple-options/choices case.

I agree, especially given there's a good (not perfect) workaround for the original issue, where the user is fully in control of determining how to handle the timestamp value (and not getting strange results and maybe writing up issues because they misconfigured a flag):

return registeredAt.getEpochSecond();

Thanks to Jakob et al. for highlighting this and helping to generate some thoughts on improvements to the SerializationFeature.

@kupci
Copy link
Member

kupci commented Mar 4, 2020

There are definitely other fixes/enhancement requests that might be addressed by the DateTimeConfig: here's another enhancement request #117
_

if seconds or nanos are set to 0 in the LocalDateTime object being serialized, they will NOT be included into the resulting JSON.

_

@cowtowncoder cowtowncoder added the date-time-config Work related to possible larger DateTimeConfig feature label Mar 4, 2020
@cowtowncoder
Copy link
Member

Created new label to connect these. There is also a databind issue for adding colon in timezone marker on serialization:

FasterXML/jackson-databind#1624

something I hoped to get in 2.11, but am worried about compatibility aspects.

sashikanthr added a commit to sashikanthr/jackson-modules-java8 that referenced this issue May 1, 2020
@elrob
Copy link

elrob commented Sep 8, 2020

I was hoping there was a better way like those suggested by the original post here. I don't like adding extra getters or annotations to my classes if I can avoid it.

As it may be useful to others, to serialize Instant as epoch seconds (with no fractional part), I did the following:

    private final ObjectWriter objectWriter = new ObjectMapper()
            .registerModule(new JavaTimeModule().addSerializer(new EpochSecondInstantSerializer()))
            .writerFor(MyValueClass.class);
    public class EpochSecondInstantSerializer extends JsonSerializer<Instant> {
        @Override
        public Class<Instant> handledType() {
            return Instant.class;
        }

        @Override
        public void serialize(Instant instant, JsonGenerator js, SerializerProvider sp) throws IOException {
            js.writeNumber(instant.getEpochSecond());
        }
    }

@kupci
Copy link
Member

kupci commented Sep 8, 2020

Thanks for including this. This looks like a clean solution, and yes, avoids the impact of extra annotations or SRs that were preventing this from being a simple fix. My thought is that, while certainly there are drawbacks to extending, it is nice to have the capability to extend for situations like this.

@nniesen
Copy link

nniesen commented Oct 30, 2020

I think it should be epoch millis for this change to be useful. Epoch millis makes the the timestamps portable with regard to both Java and JavaScript; which are the likely producers and consumers on both ends of the request.

Default serializer: Instant.toEpochMilli()
Default deserializer: Instant.ofEpochMilli(long epochMilli)

I currently use this configuration:

jackson:
  serialization: # com.fasterxml.jackson.databind.SerializationFeature
    WRITE_DATES_AS_TIMESTAMPS: true # Instant.toEpochMillis()
    WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS: false
  deserialization: # com.fasterxml.jackson.databind.DeserializationFeature
    READ_DATE_TIMESTAMPS_AS_NANOSECONDS: false

This makes the timestamps easily transferrable between the platforms with consistent APIs.

Java:

long value = 1551461407999L
Instant deserialized = Instant.ofEpochMilli(value)
System.out.println(deserialized)
long serialized = deserialized.toEpochMilli()
System.out.println(serialized)
System.out.println(serialized == value)
System.out.println(Instant.ofEpochMilli(1551461407999L).toEpochMilli() == 1551461407999L)

Javascript:

var value = 1551461407999;
var deserialized = new Date(value);
console.log(deserialized.toISOString());
var serialized = deserialized.valueOf();
console.log(serialized);
console.log(serialized == value);
console.log(new Date(1551461407999).valueOf() == 1551461407999);

Moment.js:

var value = 1551461407999;
var deserialized = new moment(value);
console.log(deserialized.toISOString());
var serialized = deserialized.valueOf();
console.log(serialized);
console.log(serialized == value);
console.log(new moment(1551461407999).valueOf() == 1551461407999);

All 3 produce:

2019-03-01T17:30:07.999Z
1551461407999
true
true

If some needs/wants epoch seconds, they can use the Instant/Date APIs to truncate to seconds when creating the initial value.

@cowtowncoder
Copy link
Member

Couple of notes:

  1. Backwards compatibility is very important, so any change in 2.x that would change existing behavior is typically a big no-no
  2. In addition to existing (De)SerializationFeature choices, there is also @JsonFormat -- with "shape" that can distinguish integer/floating-point representations, as well as text; and "pattern" (plus "config-overrides" allow application not only via annotations but also as per-type defaults) -- this allows for bit better fidelity
  3. It is important to consider serialization/deserialization compatibility (roundtrip)

@obarcelonap @kupci WDYT?

@magixyu
Copy link

magixyu commented Sep 24, 2021

Any update on this topic?

@kupci
Copy link
Member

kupci commented Sep 24, 2021

No updates right now. It looks like there are some good workarounds, and there are folks who don't like adding extra getters or annotations to classes.

But otherwise, I could help with @JsonFormat implementation, suggested by @cowtowncoder:

In addition to Shape.NUMBER, there are "subtype" choices of Shape.NUMBER_INT and Shape.NUMBER_FLOAT that would allow specifying where to include fractional part

While keeping in mind these requirements:

  • backwards compatibility
  • serialization/deserialization (roundtrip)

@tmancill
Copy link

Thanks to all for the helpful thread. I ran into this issue during an update from 2.10.0 -> 2.12.5.

2.10.0: default behavior

final Instant instant = Instant.ofEpochSecond(42, 0);
final byte[] bytes = new ObjectMapper().writeValueAsBytes(Map.of("instant", instant));
assertEquals("{\"instant\":{\"epochSecond\":42,\"nano\":0}}", new String(bytes));

2.12.5: default behavior

final Map<String, Object> data = Map.of("instant", Instant.ofEpochSecond(42, 0));
final bytes[] bytes = JsonMapper.builder()
        .addModule(new JavaTimeModule())
        .build()
        .writeValueAsBytes(data);
assertEquals("{\"instant\":42.000000000}", new String(bytes));
// Default behavior results in a decimal without field names.

2.12.5: after disabling WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS

final Map<String, Object> data = Map.of("instant", Instant.ofEpochSecond(42, 0));
final bytes[] bytes = JsonMapper.builder()
        .addModule(new JavaTimeModule())
        .build()
        .writer()
        .without(WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
        .writeValueAsBytes(data);
assertEquals("{\"instant\":42000}", new String(bytes));
// Behavior after disabling WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS is milliseconds without field names.

2.12.5: using a custom JsonSerializer

Thank you to @elrob for the suggestion. The intent here is to match the default behavior from 2.10.0.

public static class InstantSerializer extends JsonSerializer<Instant> {
    @Override
    public Class<Instant> handledType() {
        return Instant.class;
    }

    @Override
    public void serialize(final Instant instant, final JsonGenerator js, final SerializerProvider sp) throws IOException {
        js.writeStartObject();
        js.writeNumberField("epochSecond", instant.getEpochSecond());
        js.writeNumberField("nano", instant.getNano());
        js.writeEndObject();
    }
} 
...

final Map<String, Object> data = Map.of("instant", Instant.ofEpochSecond(42, 0));
final byte[] bytes = JsonMapper.builder()
                .addModule(new JavaTimeModule().addSerializer(new InstantSerializer()))
                .build()
                .writeValueAsBytes(data);
assertEquals("{\"instant\":{\"epochSecond\":42,\"nano\":0}}", new String(bytes));
// Behavior matches default when using 2.10.0.

@kupci
Copy link
Member

kupci commented Nov 29, 2021

@tmancill Glad you found a workaround. One thing I'm curious about is that it looks like, with your scenario, some information was lost, so we'll have to see if that was intentional or an inadvertent change, due to some other change:
// Default behavior results in a decimal without field names.

@jsnelgro
Copy link

Just wanted to post my solution here, since this was the first thread that came up in a web search and I didn't see an explicit code snippet that solved my problem. I needed to serialize as epoch millisecond without globally configuring the ObjectMapper. This is what solved my use case (I'm working in Kotlin):

data class MyPojo(
  @JsonFormat(without = [JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS])
  val timestamp: Instant
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
date-time-config Work related to possible larger DateTimeConfig feature new-feature
Projects
None yet
8 participants