search

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 are some serializers missing the public constructor? #103

Closed maraswrona closed 5 years ago

maraswrona commented 5 years ago

Some serializers in this module provide a public constructur which accepts DateTimeFormatter (e.g. ZonedDateTimeSerializer, LocalDateSerializer, LocalDateTimeSerializer etc), while others don't (e.g. InstantSerializer)

Why is that?

I wanted to do a trivial change to how my Instant values are serialized. I expected there would be a way to provide just my DateTimeFormatter instance, but sadly, there isn't. I have to provide whole new implementation of JsonSerializer<Instant> and register it with JavaTimeModule effectively replacing the implementation provided by this module.

cowtowncoder commented 5 years ago

As to accepting DateTimeFormatter or not, I think that is due to formatter not being originally used at all, and support having been added incrementally on as-needed basis (where needed and/or easy to add). So we are happy to accept contributions that are such configurability.

In general this mechanism was not meant as mechanism for sub-classing or constructing (de)serializers by anything other than module itself, although it may be found useful by those who do.

maraswrona commented 5 years ago

I see. Well, I may be wrong, but it seems to me it should be possible to just mark those few protected constructors as public, and it should just work. Unless this change is somehow against the general design, or some other mechanism that I'm not aware of.

I'll try myself and will publish a PR if it works.

cowtowncoder commented 5 years ago

Going back to original question, I am still not quite sure why no-arguments constructor would be needed, so I'll close this issue. Specific one for specific type, along with explanation, could be filed as new issue -- there may be valid need, I just want to verify this is not something where other mechanisms (use of @JsonFormat f.ex) would work.

maraswrona commented 4 years ago

@cowtowncoder Sorry for late response. The question was not about no-arguments constructor, but the constructor that accepts DateTimeFormatter that is also public - some serializer classes provide one, some don't (examples in first post).

@JsonFormat approach has two downsides: 1) It cannot be used once for all, e.g. if I want common shared format for all my Instant fields, I have to put proper @JsonFormat on all of them. This can be inconvenient and error prone. 2) I'm not sure but I think not all serialization / deserialization features can be expressed as format string. E.g. DateTimeFormatterBuilder provides methods like parseDefaulting() which control how the parser should fill in fields missing in input data. I don't think this can be expressed by a string format and achieved by @JsonFormat

Overall I believe my request is really simple and I only ask for it because other classes from this library already provide such a feature. I think adding this for InstantSerializer would make the codebase more consistent.

As I suggested before, I'm happy to provide a PR with this change. Is that ok?

cowtowncoder commented 4 years ago

@maraswrona Ok. On (1), no, you can actually config it with "config overrides" system added in 2.8, see:

https://medium.com/@cowtowncoder/jackson-2-8-features-5fc692887944

so you can associate all format settings to specific types. Ideally other aspects would also be available: lenient is, as an example, as are some timezone aspects. So I would be interested in providing more configuration options that do not require "manual" instantiation of module's (de)serializers.

Having said that, I am open to specific change to, say, InstantSerializer, given that some serializers do expose manual construction.