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

Binary compatibility broken in 2.9.x in DateTimeSerializer #99

Closed
andrewl102 opened this issue Aug 21, 2018 · 6 comments
Closed

Binary compatibility broken in 2.9.x in DateTimeSerializer #99

andrewl102 opened this issue Aug 21, 2018 · 6 comments
Milestone

Comments

@andrewl102
Copy link

This is due to commit df3f16c.

Currently, we make use heavy usage of Jackson in various project, often resulting in heterogenous versions of Jackson and jackson-datatype-joda being present in the classpath.
As a result of this commit, binary compatibility was broken which presents a compatibility issue for us.
A previously valid construct call

public DateTimeSerializer(JacksonJodaDateFormat format) is now invalid, due to the refactoring.

There is a very trivial fix that could be applied, by simply reintroducing this construct and calling the new constructor as follows:
DateTimeSerializer(format,0).

I'm happy to prepare a pull request if desired, though at only 1 line this appears a little redundant.
Is there any possibility of making this change?
Typically breaking binary compatibility in a library happens after first issuing a deprecation of the old constructor/method and/or in a major version bump, so it would be extremely helpful to maintain binary compatibility for now if possible.

@cowtowncoder
Copy link
Member

I am not against adding back a (deprecated) constructor, if needed for backwards compatibility.
And ideally change itself would not have removed it in patch release, to reduce likeilhood of breakage; typically obsolete constructor should have been labelled @deprecated and removed on a later minor version.

But I am curious as to how this would cause breakage? Serializers and deserializers included are not meant to be part of public API, but only registered by module itself. But it sounds like something is directly instantiating (de)serializers?

@andrewl102
Copy link
Author

andrewl102 commented Aug 21, 2018

The code in question is basically as follows (or similar, the specific format and configuration varies somewhat):

       JodaModule jodaModule = new JodaModule();
        mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
        mapper.registerModule(jodaModule);
        SimpleDateFormat simpleDateFormat = new SimpleDateFormat("yy-MM-dd'T'HH:mm:ss");
        mapper.setDateFormat(simpleDateFormat);
        JacksonJodaDateFormat jacksonJodaDateFormat = new JacksonJodaDateFormat(DateTimeFormat.forPattern("yy-MM-dd  'T'  HH:mm:ss "));
//        jodaModule.addSerializer(new DateTimeSerializer(jacksonJodaDateFormat,0));
        System.out.println(mapper.writeValueAsString(new DateTime()));

There's similar code for deserialization in place as well.

The intent here was to globally configure the default date format of the JSON for everything that is read/written using that ObjectMapper.
The full flexibility of JacksonJodaDateFormat was generally required, due to some custom requirements.
Unless I uncomment the explicit adding of the serializer, I was not able to see a way of configuring the date format that would successfully work : the call to setDateFormat appears to do nothing.
This may have been done erroneously, however it wasn't easy to locate documentation about what the correct way to do this would have been.
Do you have any suggestions?
Given that the serializer is in public scope, it wasn't obvious that this was intended to be an internal class.
However it sounds as if there might have been a better way to do this?

One other issue is that if backwards compatibility is maintained, I would not have to go through all the libraries in our codebase that have constructed the date formats as above.
It's likely that there are quite a few other people who are (perhaps unintentionally) making use of this public class so anything that could minimise the risk of breaking binary compatibility could be useful.

Thanks for the explanation so far.

@cowtowncoder
Copy link
Member

@andrewl102 Just to make sure: I do not blame you or anyone for using publicly available classes. Intent is difficult to know, and it is not always easy to use limited mechanisms Java has to limit access.
So it is more explaining my original intent as background. Regardless, change itself was a bug given it broke things in a patch release, without deprecation.

Now: one new mechanism that I think will and should work is "config overrides" , added in 2.8:

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

and these will allow basically associating information equivalent to @JsonFormat for specific classes.
So if you can rely on Jackson 2.8 or later, this is hopefully bit more robust mechanism.

I'll add the missing constructor as discussed.

@cowtowncoder cowtowncoder added 2.9 active Issue being actively investigated and/or worked on labels Aug 22, 2018
@andrewl102
Copy link
Author

Hi,

As someone who has primarily shifted from Java to Scala I understand entirely how difficult package restriction mechanisms can be.
Thanks looking in to this so promptly.

Unless I'm missing something though, I don't think I can use JsonFormat on a global basis can I?
I can easily due it on a per field basis but essentially we have some globally unified standards on typical date formats that could be cumbersome to annotate every time.
From what I've seen of the code so far, I couldn't see any way of doing this without explicit registration of a custom serializer.

@cowtowncoder
Copy link
Member

@andrewl102 You can use it on per-type (class) basis via above-mentioned Config Override mechanism. I would not expect anyone to (have to) use actual per-property annotations.

So something like:

mapper.configOverride(DateTime.class)
    .setFormat(JsonFormat.Value.forPattern("yyyy.dd.MM"));

For Jackson 3.0 I have the idea of allowing further "type groups" (or whatever term it'd be), in which there would be additional level to allow type group of "date/time". But until then per-type configuration should suffice.

@andrewl102
Copy link
Author

@cowtowncoder Thanks!
I'll update all post 2.8 dependent code to use this mechanism instead.

andrewl102 pushed a commit to andrewl102/jackson-datatype-joda that referenced this issue Aug 23, 2018
cowtowncoder added a commit that referenced this issue Aug 28, 2018
Fixes #99 : binary compatibility issue
@cowtowncoder cowtowncoder added this to the 2.9.7 milestone Aug 28, 2018
@cowtowncoder cowtowncoder removed the active Issue being actively investigated and/or worked on label Aug 28, 2018
@cowtowncoder cowtowncoder changed the title Binary compatibility broken in 2.9.x in DateTimeSerializer Binary compatibility broken in 2.9.x in DateTimeSerializer Aug 28, 2018
cowtowncoder added a commit that referenced this issue Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants