Fix serialization of ZonedDateTime #1135
Conversation
| @@ -157,7 +158,8 @@ private JinjavaConfig(Builder builder) { | |||
|
|
|||
| private ObjectMapper setupObjectMapper(@Nullable ObjectMapper objectMapper) { | |||
| if (objectMapper == null) { | |||
| objectMapper = new ObjectMapper(); | |||
| objectMapper = new ObjectMapper().registerModule(new JavaTimeModule()); | |||
There was a problem hiding this comment.
What if ObjectMapper is not null? Shouldn't you register JavaTimeModule in that case?
There was a problem hiding this comment.
Yes, that's a good point. This will be addressed by the change Julia suggested.
| interpreter | ||
| .getConfig() | ||
| .getObjectMapper() |
There was a problem hiding this comment.
Thoughts on registering the module here so we scope the change to the object mapper to the pretty print filter? I think my concern here is if changing the mapper on the config level will inadvertently change something else
There was a problem hiding this comment.
Good idea. I'll update.
There was a problem hiding this comment.
I don't think registering here is the right move. This will register the module on every call to the PrettyPrintFilter.
I need to look through the PR to figure out where the right place to do it would be.
There was a problem hiding this comment.
Updated to remove module registration, but the remaining changes are still needed to support using the interpreter's config's ObjectMapper.
| public ZonedDateTime getDate() { | ||
| return ZonedDateTime.of(2023, 12, 11, 0, 0, 0, 0, ZonedDateTime.now().getZone()); | ||
| } |
There was a problem hiding this comment.
Yes, this tests that ZonedDateTime (the problem type) is being serialized successfully.
| interpreter | ||
| .getConfig() | ||
| .getObjectMapper() | ||
| .registerModule(new JavaTimeModule()) |
There was a problem hiding this comment.
Does this mean we register the module every time the function is called? Is this an expensive operation?
There was a problem hiding this comment.
I raised the same question on an above thread.
|
I'm thinking it shouldn't be Jinjava's job to configure the If no TL;DR: I think the right place to add this module is where we're configuring our own |
| interpreter | ||
| .getConfig() | ||
| .getObjectMapper() |
pom.xml
Outdated
| @@ -83,6 +83,11 @@ | |||
| <artifactId>jackson-dataformat-yaml</artifactId> | |||
| <version>2.14.0</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>com.fasterxml.jackson.datatype</groupId> | |||
There was a problem hiding this comment.
Do you still need those POM update?
There was a problem hiding this comment.
Only for the test, but I'm going to move it into my upcoming cos-renderer PR
Serializing
ZonedDateTimeis not supported by Jackson by default, so this PR registersJavaTimeModuleto add that support.