-
Notifications
You must be signed in to change notification settings - Fork 1.7k
GH-3930: Add Jackson 3 support; deprecate Jackson 2 #3996
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
Conversation
*/ | ||
public DefaultJacksonKafkaHeaderMapper(String... patterns) { | ||
this(JsonMapper.builder() | ||
.findAndAddModules(JacksonJsonMessageConverter.class.getClassLoader()) |
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 class loader from this class exactly, not something else to avoid class tangle and so.
* Create an instance for inbound mapping only with pattern matching. | ||
* @param patterns the patterns to match. | ||
* @return the header mapper. | ||
* @since 2.8.8 |
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.
No @since
on methods in all new classes, please.
Object value = decodeValue(header, type); | ||
headers.put(header.key(), value); | ||
} | ||
catch (IOException e) { |
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.
This should be revised since Jackson 3 does not throw IOException
any more.
May just catch (Exception e) {
instead?
@@ -101,6 +103,9 @@ public BatchMessagingMessageConverter(@Nullable RecordMessageConverter recordCon | |||
if (JacksonPresent.isJackson2Present()) { | |||
this.headerMapper = new DefaultKafkaHeaderMapper(); | |||
} | |||
else if (JacksonPresent.isJackson3Present()) { |
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.
Let's prefer Jackson 3!
If they have both in classpath, so Jackson 3 choice would win.
...a/src/main/java/org/springframework/kafka/support/converter/JacksonJsonMessageConverter.java
Show resolved
Hide resolved
|
||
private JacksonJavaTypeMapper typeMapper = new DefaultJacksonJavaTypeMapper(); | ||
|
||
private final TypeFactory typeFactory = TypeFactory.createDefaultInstance(); |
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.
Let's use ObjectMapper.getTypeFactory()
instead in the ctor!
import org.springframework.util.Assert; | ||
|
||
/** | ||
* A {@link MessageConverter} implementation based on Jacthat uses a Spring Data |
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.
You probably meant based on Jackson 3
😄
build.gradle
Outdated
@@ -263,6 +265,13 @@ project ('spring-kafka') { | |||
exclude group: 'org.jetbrains.kotlin' | |||
} | |||
|
|||
optionalApi 'tools.jackson.core:jackson-databind' | |||
optionalApi 'tools.jackson.datatype:jackson-datatype-joda' | |||
optionalApi 'tools.jackson.dataformat:jackson-dataformat-xml' |
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.
We don't do XML here in Spring Kafka, so let's just remove this redundant dependency!
Fixes spring-projects#3930 Issue link: spring-projects#3930 This commit introduces comprehensive Jackson 3 support across the Spring for Apache Kafka framework while maintaining backward compatibility by deprecating Jackson 2 components. BREAKING CHANGES: - All Jackson 2-based classes are now deprecated and will be removed in a future version - Default Jackson version updated from 2.19.2 to 2.19.1 for Jackson 2 - Added Jackson 3 BOM dependency (3.0.0-rc5) NEW FEATURES: - Added complete Jackson 3 counterparts for all existing Jackson 2 classes: * JsonKafkaHeaderMapper (replaces DefaultKafkaHeaderMapper) * JacksonJsonSerializer/Deserializer (replaces JsonSerializer/Deserializer) * JacksonJsonSerde (replaces JsonSerde) * JacksonJsonMessageConverter and subclasses (replaces JsonMessageConverter family) * JacksonProjectingMessageConverter (replaces ProjectingMessageConverter) * DefaultJacksonJavaTypeMapper (replaces DefaultJackson2JavaTypeMapper) * Jackson3Utils and Jackson3MimeTypeModule utilities INFRASTRUCTURE: - Enhanced JacksonPresent utility to detect Jackson 3 availability - Updated MessagingMessageConverter and BatchMessagingMessageConverter to prefer Jackson 3 - Modified RecordMessagingMessageListenerAdapter to handle new converter types - Updated all serialization/deserialization logic to use Jackson 3 APIs DOCUMENTATION: - Updated all documentation references from Jackson 2 to Jackson 3 classes - Corrected sample code and configuration examples - Updated method signatures and class references throughout documentation TESTING: - Migrated all test cases to use Jackson 3 equivalents - Updated test configurations and mock setups - Maintained test coverage for both Jackson 2 (deprecated) and Jackson 3 paths The framework now automatically detects and prefers Jackson 3 when available, falling back to Jackson 2 for backward compatibility. All existing applications will continue to work with Jackson 2, but new development should migrate to Jackson 3 classes. Signed-off-by: Soby Chacko <[email protected]>
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.
FYI: spring-projects/spring-framework#35282.
Might be the case that we can do that already in your current incarnation.
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 whats-new
deserves a dedicated bullet for Jackson 3.
Thanks
build.gradle
Outdated
@@ -55,7 +55,8 @@ ext { | |||
awaitilityVersion = '4.3.0' | |||
hamcrestVersion = '3.0' | |||
hibernateValidationVersion = '9.0.1.Final' | |||
jacksonBomVersion = '2.19.2' | |||
jacksonBomVersion = '2.19.1' |
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.
Why downgrade?
.enable(SerializationFeature.FAIL_ON_EMPTY_BEANS) | ||
.addModule(new Jackson3MimeTypeModule()) | ||
.build(); | ||
return objectMapper; |
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.
Why do we need local variable?
Why cannot we just use return
forthe builder result?
* {@link DeserializationFeature#FAIL_ON_UNKNOWN_PROPERTIES} features. | ||
* @return the {@link ObjectMapper} instance. | ||
*/ | ||
public static ObjectMapper enhancedObjectMapper() { |
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.
See my general comment.
Apparently we have to be as specific as possible.
And make this as JsonMapper
instead.
@@ -29,7 +29,10 @@ | |||
* @author Artem Bilan | |||
* | |||
* @since 2.3 | |||
* | |||
* @deprecated since 4.0 in favor of native Jackson 3 {@link tools.jackson.databind.json.JsonMapper#builder()} API. |
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.
And now this is not correct since you have just introduced Jackson3Utils
.
* | ||
* @since 4.0 | ||
*/ | ||
public final class Jackson3Utils { |
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 name is not what is required from the framework perspective.
The goal is to avoid numbers as much as possible.
How about to name this class as JacksonMapperUtils
instead?
@@ -31,6 +31,8 @@ | |||
* @author Artem Bilan | |||
* | |||
* @since 2.3 | |||
* | |||
* @deprecated since 4.0 in favor of {@link Jackson3Utils}. |
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.
Not correct link.
And class is not marked with @Deprecated
.
* | ||
* @since 4.0 | ||
*/ | ||
public final class Jackson3MimeTypeModule extends SimpleModule { |
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.
How about MimeTypeJacksonModule
instead?
Just to avoid that number!
@@ -94,6 +96,9 @@ public MessagingMessageConverter(Function<Message<?>, @Nullable Integer> partiti | |||
if (JacksonPresent.isJackson2Present()) { | |||
this.headerMapper = new DefaultKafkaHeaderMapper(); | |||
} | |||
else if (JacksonPresent.isJackson3Present()) { |
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 Jackson 3 has to be in priority if both are on classpath.
@@ -0,0 +1,485 @@ | |||
/* | |||
* Copyright 2017-present the original author or authors. |
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.
And no this is not right.
This class was not started back in that year.
Just nit-picking thought, since I know that Spring Boot uses a year of the project beginning for everything 🤷
FYI, the refinement has already happened in Spring AMQP: spring-projects/spring-amqp#3151. The fix might be useful to follow here as well. |
This commit refines the Jackson 3 implementation introduced in the previous commit to improve API consistency and naming conventions. CHANGES: - Rename Jackson3Utils to JacksonMapperUtils for better naming consistency - Rename Jackson3MimeTypeModule to MimeTypeJacksonModule to follow Spring conventions - Update all Jackson 3 classes to use JsonMapper instead of ObjectMapper for type safety - Restore Jackson 2 BOM version from 2.19.1 to 2.19.2 - Prioritize Jackson 3 over Jackson 2 in MessagingMessageConverter detection - Add a section in whats-new for Jackson 3 changes TECHNICAL IMPROVEMENTS: - Enhanced type safety by using JsonMapper (Jackson 3) instead of generic ObjectMapper - Consistent naming patterns across all Jackson 3 components - Updated method names: getObjectMapper() → getJsonMapper() - Updated parameter names: objectMapper → jsonMapper throughout codebase - Updated factory method: enhancedObjectMapper() → jsonMapper() API CHANGES: - All Jackson 3 constructors now accept JsonMapper instead of ObjectMapper - JsonKafkaHeaderMapper uses JsonMapper for better type consistency - JacksonProjectingMessageConverter updated to use JsonMapper API - All serializer/deserializer classes consistently use JsonMapper DEPRECATION UPDATES: - Updated deprecation messages to reference correct replacement classes - Added missing @deprecated annotation to JacksonMimeTypeModule This refinement aligns with Spring Framework's approach to Jackson 3 integration and ensures a clean, consistent API surface for the new Jackson 3 support. Related: spring-projects/spring-framework#35282 Signed-off-by: Soby Chacko <[email protected]>
* {@link DeserializationFeature#FAIL_ON_UNKNOWN_PROPERTIES} features. | ||
* @return the {@link JsonMapper} instance. | ||
*/ | ||
public static JsonMapper jsonMapper() { |
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.
Why cannot we call enhanced
any more?
Signed-off-by: Soby Chacko <[email protected]>
Fixes: #3930