search

FasterXML / jackson-modules-java8

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

Fix #175 ObjectMapper#setTimeZone ignored by JSR-310/datetime types during serialization #185

Closed ferenc-csaky closed 3 years ago

ferenc-csaky commented 3 years ago

Probably will fix #180 as well.

As the discussion at #175 pointed out this logic already exists in jackson-datatype-joda, I checked what is happening there as a starting point. I felt adopting something like JacksonJodaDateFormat to here would be a pretty large and complex change, I just added the check for the mapper time zone to the JSR310FormattedSerializerBase#serialize(...) method.

This changes the serialization behavior a bit. If a zoned date-time value is provided for serialization and there is no explicit time zone setting for the ObjectMapper (which means UTC) or any other format (e.g. POJO annotation), then the serialized string will be converted to UTC. Since jackson-datatype-joda works the same way, I felt okay with that.

I adapted every related unit test and added new ones to cover new cases.

cowtowncoder commented 3 years ago

@ferenc-csaky Excellent! Thank you for submitting this. I'll have to go over the patch once more, but with quick glance it makes sense. I hope to do the review soon, although I have some backlog so it might take a day or two -- I will add it on my todo list.

Before merging, the only remaining thing is CLA, found from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

If I have not asked for one before (apologies if I have and just forgot; there are a few filed over the years :) ), I would need one filled before merging (after which CLA is valid for any and all contributions for all Jackson projects). The usual way is to print it, fill & sign, scan/take photo, email to info at fasterxml dot com address.

Looking forward to merging this fix!

cowtowncoder commented 3 years ago

Actually, based on @kupci's comments, yes, I think this really needs to be 2.12 change, and not 2.11 patch. There is behavioral change that will likely cause some surprises to some users.

@ferenc-csaky would it be possible to rebase or recreate the PR against 2.12 branch?

ferenc-csaky commented 3 years ago

@ferenc-csaky would it be possible to rebase or recreate the PR against 2.12 branch?

Sure, I'll update the PR and point it to 2.12.

cowtowncoder commented 3 years ago

@ferenc-csaky thank you!

ferenc-csaky commented 3 years ago

@ferenc-csaky thank you!

No problem! Will send the signed CLA tomorrow. :)