FasterXML / jackson-modules-java8

Set of support modules for Java 8 datatypes (Optionals, date/time) and features (parameter names)
Apache License 2.0
401 stars 117 forks source link

Why is there no concrete `OffsetDateTimeDeserializer` class to use via annotations #130

Closed uqmat closed 2 years ago

uqmat commented 5 years ago

I am wondering why there is com.fasterxml.jackson.datatype.jsr310.ser.OffsetDateTimeSerializer but no com.fasterxml.jackson.datatype.jsr310.deser.OffsetDateTimeDeserializer

Without that it seems it's not possible to configure serialization behaviour only via @JsonSerialize and @JsonDeserialize annotations.

I have classes with fields of type OffsetDateTime. Annotating these with the respective annotation works like a charm for serialization, but for deserialization I'm stuck with explicitly adding the JavaTimeModule to the object mapper, which I'd prefer to do without.

As the annotations need objects of type Class<? extends JsonDeserializer> to be passed as values to the using parameters, there currently seems to be no way of using these for deserialization.

Is there any specific reason to only have a smaller subset of explicit *Deserializer classes? Or is there some other way to use these annotations which I'm just missing here?

cowtowncoder commented 5 years ago

There is a deserializer for OffsetDateTime, registered by the date-time module, implement as variation of InstantDeserializer. I do not know specific details of that choice, but I assume it was just more compact to handle multiple types with a single class; @beamerblvd (original author) may have more info on that.

But I am not sure why you try to use @JsonDeserialize when date/time module provides for bindings already. What exactly is the benefit of trying to add handlers with annotations, instead of by-type registration?

uqmat commented 5 years ago

Well, indeed there are variations of InstantDeserializer (those are actually used in the JavTimeModule). Unfortunatly these are only concrete instances, and no Class<? extends JsonDeserializer> exists. On the other hand, specific deserializer classes for the following indeed do exist: LocalDate, LocalDateTime, LocalTime, OffsetTime Seems a bit strange to leave out OffsetDateTime...

As for the reason: firstly it breaks compatibility with the Jackson databind annotations, if those can't be used. This should at least be very well documented. Secondly, more to the point, we work with a model driven development cycle where the model classes are being used in different "serialization domains", e.g. REST services, Kafka, etc. Generating the model classes with the correct annotations just makes these classes usable for everyone without further ado, instead of everyone having to somehow register the module (which works differently in different domains).

cowtowncoder commented 5 years ago

No, I don't see how this breaks Jackson annotations: specific property of @JsonSerialize / @JsonDeserialize is meant for adding custom (de)serializers. There is no implied requirement for all (de)serializers to be usable that way -- many core (de)serializers can not be used this way, including anything to do with generic types.

I also do not understand your second part: adding @JsonSerialize / @JsonDeserialize adds more invasive dependency. Inability to register handler module for Java 8 date/time types sounds like a problem with platform to me -- this is the way Jackson is designed to be extended to support new types.

With that, I think one thing I could add might be a note on README to say that serializers/deserializers in this module are not meant to be attached using @JsonSerialize / @JsonDeserialize, and that even for types where it works, this is not a recommended practice.

beamerblvd commented 4 years ago

Catching up on some stuff at long last. I agree with @cowtowncoder's analysis of this. The serializers and deserializers in this module are not designed to be used with the @Json[De]Serialize annotations. Some of them might work unintentionally, but that is not the design. I think an update to the README to this effect is the appropriate remedy.

Arkhypov commented 2 years ago

So we would need to declare this DeSerializer manually instead of using jsr310? @beamerblvd are you that 'genius' who designed to remove OffsetDateTimeDeserializer ?

cowtowncoder commented 2 years ago

@Arkhypov hmmh? No, he is saying that you would register the module to get serializers and deserializers added. And not using annotations on properties. The whole point is to reduce amount of work, not to add.

Arkhypov commented 2 years ago

stackTrace:

java.lang.IllegalArgumentException: Cannot deserialize value of type `java.time.OffsetDateTime` from String "2017-11-29T19:36:35": Failed to deserialize java.time.OffsetDateTime: (java.time.format.DateTimeParseException) Text '2017-11-29T19:36:35' could not be parsed at index 19
 at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: com.stitcher.indexer.model.es.ShowESModel["created_at"])

where mapper is:

@Bean(name = ["mapper"])
    fun customObjectMapperSupplier(): ObjectMapper {
        val objectMapper = ObjectMapper().registerKotlinModule()
            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
            .configure(DeserializationFeature.ACCEPT_SINGLE_VALUE_AS_ARRAY, true)
        .configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true)
        .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
        objectMapper.enable(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES)
        objectMapper.registerModule(JavaTimeModule())
        return objectMapper
    }

where parsing is:

val converter: MappingElasticsearchConverter = object : MappingElasticsearchConverter(mappingContext!!, cs) {
            override fun <R> read(type: Class<R>, source: Document): R {
                return mapper.convertValue(source, type)
            }
        }

where jackson-datatype-jsr310: 2.11.4 Due to JavaTimeModule could not recognize pattern yyyy-MM-dd'T'HH:mm:ss jsr310 LocalDateTimeDeserializer is capable to recognize this pattern

Now I need to create my own deserializer due to difference in return type.

@cowtowncoder , @beamerblvd please reopen this ticket.

cowtowncoder commented 2 years ago

Could you not use `@JsonFormat(pattern = "....") annotation?

Or configuration overrides for type in question, like:

        Value format = JsonFormat.Value
            .forPattern("yyyy-MM-dd'T'HH:mm:ss");
        mapper.configOverride(OffsetDateTime.class).setFormat(format);

(which should work but I do not see a unit test verifying this, fwtw)

The goal is that users should not have to manually register specific (de)serializer implementations.

GedMarc commented 2 years ago

Shew I went a whole different route but looking at that might actually be a good option, is the option available on a Writer/Reader so we don't have to construct the entire object mapper? :)

I went the DateTimeFormatBuilder route, to allow for various combinations and locales, and then centralized the entire time package to read from the single point (duration got its own though)

You can't really specify a generic "all" for date time formatting, Annotating won't work really especially on say rest responses to a web / It is always locale (and locales themselves have two formats), language and timezone driven - A person in America won't be using the same date time format as a person in Russia for example, And if you are using Cantonese it's not even the same structure to represent date and time so, not really an option - There is simply no one size fits all with this, so the root question is actually a little invalid from that point of view

Like booleans, there are simply too many formats and structures that are allowed to create a base module that can do it all or a specific annotation to set a format, I really thought that was actually why jsr310 was removed - it's just simply not viable to the requirement of date/time formatting?

I do like the DateTimeFormatBuilder structure to be able to do these and build up the formats dynamically, but if the config override is available for the reader/writer instead of the base mapper object I think I would definitely use that suggestion @cowtowncoder - Don't want to inject an instance of object mapper that's fixed to a certain format ;)

https://github.com/GedMarc/GuiceInjection/blob/7dfb92f05a38e56dfe89e5de4316ad9d38ca6ed0/src/main/java/com/guicedee/guicedinjection/json/LocalDateTimeDeserializer.java

https://github.com/GedMarc/GuiceInjection/tree/7dfb92f05a38e56dfe89e5de4316ad9d38ca6ed0/src/main/java/com/guicedee/guicedinjection/json

cowtowncoder commented 2 years ago

@GedMarc Ok so, no, configuration cannot be one on ObjectReader/ObjectWriter since formatting settings cannot be changed on per-call basis unfortunately -- they are part of somewhat static configuration of serializers/deserializers.

On global format, I agree: there is no way to have a single global default format. This is why format strings are on per-Java-type basis.

I am not sure what

 I really thought that was actually why jsr310 was removed 

means here? Module is not removed by any means.

And finally on Locale-specific formatting: I think I disagree here; or perhaps we are talking about different things.

For UIs this is definitely true: Date/Time (and Number etc) formatting varies a lot.

But for back end data handling the opposite is true: I think format used between systems should be well-defined and not vary across requests at all (with the possible exception of needing to pass User Input as-is -- if so, passed as String nominal type). So regardless of end user's or system's Locale, Date/Time values should use just one particular ISO-8601 format choice, I think.

It sounds like you are thinking of what I think of as UI part, where representations do vary. But I don't think this is what Jackson's date/time modules (or JSON, more generally) is aimed at solving. Other date/time conversion libraries can handle those needs,

GedMarc commented 2 years ago

Hmm I hear you -

I do it like this on a backend as well though, not just UI driven (although a rest service usually supports more than one consumer?)

A lot of the integrations in my experience definitely use many many many date/time formats, not sure if it's a "every company i've been at" special but I have always seen custom dates on any message to any integrated provider, to the point where even in a single message there will be like 4/5 different formats to apply xD perfect world and real world?

I know a lot of people just like making them strings and set them manually but I do always try to stay away from that - So I always use a generic,

I did try mixins with different annotations, a few work arounds, but in the end, that is the solution that's been going good now for like over 2 years or so over 70 countries and all their formats

Oh sorry bad wording! Not removed, merged into the jdk8 guess :)

But yeah, I can't really use the annotations or the jsr310 module for this, not on large-scale enterprise applications/micro services hosted in different parts of the world,

But the deserializers/serializer plugin implementation is perfect for my needs and the performance is top notch,

We managed to get rid of all (like 8) of the json parsers and use only Jackson now for everything across the board, so big BIG thank you!

cowtowncoder commented 2 years ago

@GedMarc cool -- this is definitely an interesting use case and something I hadn't considered.

Wrt stand-alone serializers/deserializers: when all is said and done, I am not dead against allowing (re)use of provided (de)serializers, and if someone wants to refactor things to make that easier, PR would likely be accepted.

And one last thing: while it may not work anyway, note that instead of annotations (direct or mix-ins), use of "Config Overrides" (with JsonFormat.Value) can often work better. Since it is per-Mapper, probably not something that works for your case. But I thought it worth mentioning for others who might benefit.