GH-10058: Add Jackson 3 (de)serializer support #10193
Conversation
Related to: spring-projects#10058 * Add Jackson3-compatible messaging (de)serializers * Add Jackson 3 `messagingAwareMapper()` support * Maintain existing Jackson 2 functionality Signed-off-by: Jooyoung Pyoung <[email protected]>
There was a problem hiding this comment.
*JacksonDeserializer → *Jackson2Deserializer
thank you for the effort !
I’ll take a look tommorow in details, but I think this is a wrong direction.
we develop here a library for target project to rely on the API. We cannot break anything just because. It could be possible in target product where no one depend on your public API. With frameworks and libraries we don’t have such a liberty.
Therefore renaming existing classes, or moving them to different packages is wrong direction for us.
my apologies that my comment about naming was not so clear. Let me try again!
We only deprecate existing classes. We name new classes without a number, as much as possible. No need in a new package since deprecated classes will be removed in next version.
Again: we need to keep in mind that our code is used to compile other projects: breaking an API so dramatically is not acceptable.
Please, revise your solution.
|
Thank you for taking the time to review this on the weekend ! I apologize for not fully considering that I'm contributing to a widely-used library. At my job, I work on middleware primarily focusing on backward compatibility for end-user functionality, but I hadn't deeply thought about the compatibility of public APIs that other projects depend on. I'll be much more careful to consider API dependencies and backward compatibility in my contributions. For easier review, I'll revert |
* Keep existing `*JacksonDeserializer` classes unchanged * Add new `*Jackson3Deserializer` classes for Jackson 3 support Signed-off-by: Jooyoung Pyoung <[email protected]>
| JavaType baseType, | ||
| PolymorphicTypeValidator subtypeValidator, | ||
| Collection<NamedType> subtypes, boolean forSer, boolean forDeser) { | ||
| JavaType baseType, |
There was a problem hiding this comment.
I'll fix it(indentation) when applying the review feedback.
| @@ -168,7 +168,7 @@ private static final class AllowListTypeIdResolver implements TypeIdResolver { | |||
|
|
|||
| private final TypeIdResolver delegate; | |||
|
|
|||
| private final Set<String> trustedPackages = new LinkedHashSet<>(DEFAULT_TRUSTED_PACKAGES); | |||
| private final Set<String> trustedPackages = new LinkedHashSet<>(JacksonJsonUtils.DEFAULT_TRUSTED_PACKAGES); | |||
| @@ -0,0 +1,48 @@ | |||
| /* | |||
| * Copyright 2017-present the original author or authors. | |||
There was a problem hiding this comment.
I'll fix these years when applying the review feedback 😂
Related to: #10058
messagingAwareMapper()supportI thought about splitting this into separate PRs (
(de)serializers+messagingAwareMapper()support), but figured it'd be easier to review as one. It's a bit large though - apologies for the extra work! 😅Now, I have a few questions..
Package Structure
With many Jackson 2 / Jackson 3 classes in the same package, how about reorganizing Jackson 2 into sub-package?
Copyright and Authors
When refactoring the classes, I moved existing impls and created new ones, following the team's previous decision:
E.g., :
*JacksonDeserializer→*Jackson2Deserializer(Jackson 2 support)*JacksonDeserializerfor Jackson 3 supportI've attempted to update
@authorand copyright year appropriately, but please review if they're handled correctly.Docs
Currently, Many docs (e.g., redis.adoc) references the default class names:
With this PR, we now have version-specific implementations:
MessageHeadersJackson2SerializerMessageHeadersJacksonSerializerShould we update docs to reflect current impl(Jackson 2) or keep it for 7.0 release since Jackson 3 will be the default?