Open bric3 opened 5 years ago
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.
I have some concerns about the new SerializationFeature, but haven't yet been able to formulate exact problem.
Ok, here are a couple thoughts, I'll add these to the actual code (in the PR) too, but too summarize:
WRITE_DATE_TIMESTAMPS_AS_EPOCHSECONDS
, so it is a little more consistent with other flag WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS
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)));
}
Other things to note:
Shape.NUMBER
, there are "subtype" choices of Shape.NUMBER_INT
and Shape.NUMBER_FLOAT
that would allow specifying where to include fractional partFeature
s (Deserialization-, Serialization-, Mapper) -- with Builder-style construction, it would be possibly to have something like
DateTimeConfig` configuration object that could define richer semanticsOn (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).
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)?
@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
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:
Do you think we should proceed with this route (new SR flag), or fully go with the longer effort towards the DateTimeConfig
?
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:
@JsonFormat
?)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.
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.
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.
_
Created new label to connect these. There is also a databind issue for adding colon in timezone marker on serialization:
https://github.com/FasterXML/jackson-databind/issues/1624
something I hoped to get in 2.11, but am worried about compatibility aspects.
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());
}
}
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.
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.
Couple of notes:
@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@obarcelonap @kupci WDYT?
Any update on this topic?
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:
Thanks to all for the helpful thread. I ran into this issue during an update from 2.10.0 -> 2.12.5.
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));
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.
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.
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.
@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.
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
)
I have an
Instant
field that represents an epoch timestamp in seconds.the mapper is configured this way
However when this gets serialized as
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)
The only option is to use :
It could be nice if there could be way to tell jackson to avoid serializing the fraction part.