-
Notifications
You must be signed in to change notification settings - Fork 1.1k
GH-10058: Add Jackson 3 ObjectMapper and MessageParser #10160
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
base: main
Are you sure you want to change the base?
Conversation
Related to: spring-projects#10058 * Add Jackson3JsonObjectMapper, Jackson3JsonMessageParser to prepare for Jackson 2 to 3 migration. Signed-off-by: Jooyoung Pyoung <[email protected]>
...re/src/main/java/org/springframework/integration/support/json/Jackson3JsonMessageParser.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This is great start!
Any chances to have some tests on the matter?
Especially I'm interested in those loaded by the Jackson 3 modules if we remove our custom collectWellKnownModulesIfAvailable()
.
Re. defaults in v3 VS v2: I have raised concern with the team.
Will come back to ASAP.
...re/src/main/java/org/springframework/integration/support/json/Jackson3JsonMessageParser.java
Outdated
Show resolved
Hide resolved
List<JacksonModule> jacksonModules = collectWellKnownModulesIfAvailable(); | ||
this.objectMapper = JsonMapper.builder() | ||
.addModules(jacksonModules) | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see Spring Framework does this instead:
JsonMapper.builder().findAndAddModules(Jackson3JsonObjectMapper.class.getClassLoader()).build();
Maybe we check if really those well-known modules are picked up properly by Jackson 3 native API?
I mean this our new class might be a bit lighter without those inner classes and collectWellKnownModulesIfAvailable()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By test results, it works as we expected.
But I think JacksonJsonObjectMapper.class.getClassLoader()
would be the ApplicationClassLoader
. This works because our Jackson dependencies only have these two modules, but if a user's application has other Jackson modules beyond Spring Integration... the situation might be different.
I'm a bit worried about this case. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our Jackson dependencies only have these two modules
Those are optional in our case, so end-user has to add even these two in their project explicitly.
I'm not sure what problem you see since in the end any custom Jackson module has to be added into final product classpath to make them available.
Do you see something off with our ClassLoader arrangement?
We probably can use ClassUtils.getDefaultClassLoader()
instead.
I always had a problem with understand how all those class loaders work 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I understood that in the existing Jackson2JsonObjectMapper
, no matter how many Jackson dependencies a user's product has, when created with the default ctor, only wellKnownModules()
would be added.
This time, since it's changed to find modules from the loader, I thought that in such situations, when created with the default ctor, an ObjectMapper
with additional JacksonModules
beyond wellKnownModules()
could be created.
I might be misunderstanding something though - please let me know if I'm thinking about this incorrectly! 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only wellKnownModules() would be added.
Correct. That's the current behavior.
with additional
JacksonModules
beyondwellKnownModules()
could be created.
That's correct, too. Those "wellKnownModules" are just placeholder in our code to simplify class loading.
With this findAndAddModules()
we definitely are going to load everything what would fit Jackson expectations, essentially whatever is there on classpath.
Having this discussion with you I have to go and double check a new code from Jackson.
Might be the case that this findAndAddModules()
is not OK by default and we should comeback to wellKnownModules()
instead.
Again: team consultation 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Looks like it uses a ServiceLoader
:
/**
* Method for locating available methods, using JDK {@link ServiceLoader}
* facility, along with module-provided SPI.
*<p>
* Note that method does not do any caching, so calls should be considered
* potentially expensive.
*/
public static List<JacksonModule> findModules(ClassLoader classLoader)
{
ArrayList<JacksonModule> modules = new ArrayList<>();
ServiceLoader<JacksonModule> loader = secureGetServiceLoader(JacksonModule.class, classLoader);
for (JacksonModule module : loader) {
modules.add(module);
}
return modules;
}
So, we should be good as long as the ClassLoader
is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ! Actually I didn't know that.. Sorry for making you overthink this. 😞
...re/src/main/java/org/springframework/integration/support/json/Jackson3JsonMessageParser.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public String toJson(Object value) throws JacksonException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to support this abstraction contract we have to re-throw an IOException
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've all re-throw as IOException to comply with the abstraction contract !
But I saw Spring Framework plans to remove Jackson 2 in version 7.2. In that case, would we be able to change JsonObjectMapper
's IOException
to RuntimeException
at that time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in the past we had one for Boon in the past, but then it was removed due to target library low maintenance.
There is also a request for GSON, but didn't go anywhere yet.
Well, I think we should really remove that IOException
from the contract and re-throw our own generic RuntimeException
.
But that's different story.
Let's come back to that when we done here in the separate GH issue!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The team decision is to keep Jackson 3 defaults since this is a major version it is better to dictate a modern protocols. Luckily, we expose an API to inject a customized ObjectMapper
.
* @since 7.0 | ||
* | ||
*/ | ||
public class Jackson3JsonObjectMapper extends AbstractJacksonJsonObjectMapper<JsonNode, JsonParser, JavaType> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After team discussion, we prefer to have this as a JacksonJsonObjectMapper
instead.
And try to keep the rest of API without a number as long as we have that Jackson2
variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious - when you say team, do you mean the Spring team? Or is there a separate Spring Integration team internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a separate number of people who work for this specific project, but of course we all here is Spring team and we collaborate with each other on daily basis.
The "team" in this case was a broader scope since we'd like to have consistency between all the Spring projects.
...re/src/main/java/org/springframework/integration/support/json/Jackson3JsonMessageParser.java
Outdated
Show resolved
Hide resolved
* Rename `Jackson3JsonObjectMapper` to `JacksonJsonObjectMapper` * Rename `Jackson3JsonMessageParser` to `JacksonJsonMessageParser` * Wrap `JacksonException` in `IOException` to maintain API contract * Replace manual module collection with `findAndAddModules()` Signed-off-by: Jooyoung Pyoung <[email protected]>
Signed-off-by: Jooyoung Pyoung <[email protected]>
Related to: #10058
Jackson3JsonObjectMapper
,Jackson3JsonMessageParser
to prepare for Jackson 2 to 3 migration (Work3).Jackson 3 Exception Model Changes
Jackson 3 changed its exception hierarchy from checked to unchecked.
Now,
JacksonException
extendsRuntimeException
notIOException
.Jackson 3 default setting changes
Jackson 3.0 has different default settings compared to 2.x that may affect serialization behavior.
For example, these timestamp-related settings have changed:
WRITE_DATES_AS_TIMESTAMPS(false)
// was true in 2.xWRITE_DURATIONS_AS_TIMESTAMPS(false)
// was true in 2.xONE_BASED_MONTHS(true)
// was false in 2.xShould we maintain Jackson 2.x compatibility, or adopt Jackson 3 defaults?