Implement Jackson3 support and maintain Jackson2 support#117
Conversation
There was a problem hiding this comment.
1 issue found across 36 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="pom.xml">
<violation number="1" location="pom.xml:40">
P2: Jackson 3 requires Java 17+, but the build is pinned to Java 8. Adding the Jackson 3 BOM/tools.jackson dependencies will make builds fail with class file version incompatibility unless the toolchain is upgraded or Jackson 3 is isolated.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Some extra context:
|
|
I'm waiting this PR to be released because I can't migrate to spring 4 without breaking my app behavior... 💯 |
…upport' into implement-jackson3-with-jackson2-support # Conflicts: # .github/workflows/maven_release.yml # .github/workflows/maven_test.yml
|
@wing328 is there any chance that this PR gets merged? This could be useful to add it once |
|
I'm waiting :) |
|
Is there an ETA for releasing this PR? That will be very helpful for the migration to Spring Boot 4. Thanks! |
|
I'll try to get it merged this week and cut a release |
|
please review the build failure when you've time |
|
I just tested the PR and I found a regression. The following code doesn't work - name is always undefined even if a The following code does work: |
…with-jackson2-support # Conflicts: # pom.xml
|
I updated the PR to fix the integration tests (working together with @smals-mavh). The jackson dependencies aren't transitively added anymore, either jackson 2 or 3 has to be added explicitly. We still have to investigate the regression reported by @mikomarrache . For projects using a module path, there's an issue: the SPI mechanism with We could support SPI from Java 25 onwards using multi-release jar, but I don't see a way to support SPI for Java 17 to 24 without making jackson 2 and 3 a mandatory dependency. Any preference on how to go forward with this or other ideas? |
|
@mikomarrache, could you tell a bit more about your setup. I was unable to reproduce. This is also more or less tested here: https://github.com/smals-mavh/jackson-databind-nullable/blob/implement-jackson3-with-jackson2-support/src/test/java/org/openapitools/jackson/nullable/JsonNullableSimpleTest.java#L134 Could you tell us more about the setup / share a reproducer? |
|
Sorry, my bad, I didn't explain the regression correctly. You can use this test: |
|
@mikomarrache , good catch! I don't think this is really a regression in the json-nullable lib, but more a behavioral change in Jackson 3. Note that the jackson 2 test you provided still passes, but to let it work on jackson 3 I suppose we have two options. But to be seen how future proof this will be.
I don't think this is an issue for this library, but might be an annoying behavioral change in Jackson 3. |
|
@wing328 , the PR should be fine now and build works. There's only a minor regression with JDK >= 17 using module-path, that the ServiceLoader mechanism doesn't work. I can't find a workaround for that, bc optional serviceloader dependencies aren't supported until JDK25. |
|
thanks for the PR, which has been merged for those looking for this feature, please help test the SNAPSHOT version to ensure it works for your use cases. |
Just try snapshot version Still working with Springboot v3 and I'm using Java 17 / Springboot 4.0.2. |
|
Could you share a reproducer? My first guess would be a Jackson 2 / 3 behavioral change, but I can help verify if you wish. |
I investigate a little bit, it seems that the problem occurs when it exists a constructor with attribute provided. Do not have an example project to provide since I'm testing on a real project. I tested with an There are two "regressions" on an attribute (I supposed linked due to object instance creation) when having a constructor with this attribute provided:
I hope it's usefull, I'll try to investigate further... Here is a simple test file to illustrate this behavior (only the Test package xxxx;
import org.junit.jupiter.api.MethodOrderer;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestMethodOrder;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.json.JsonTest;
import tools.jackson.databind.ObjectMapper;
import java.util.Optional;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
@JsonTest
@TestMethodOrder(MethodOrderer.MethodName.class)
class ModelNoArgsJsonTest {
@Autowired
private ObjectMapper objectMapper;
@Test
void t001_no_body() { // OK
final var model = objectMapper.readValue("{}", ModelNoArgs.class);
assertNull(model.getTest());
}
@Test
void t001_body_with_null() { // OK
final var model = objectMapper.readValue("{\"test\":null}", ModelNoArgs.class);
assertEquals(Optional.empty(), model.getTest());
}
@Test
void t002_body_with_value() { // OK
final var model = objectMapper.readValue("{\"test\":\"aze\"}", ModelNoArgs.class);
assertEquals(Optional.of("aze"), model.getTest());
}
@Test
void t100_no_body() { // KO = should be same result as t000
final var model = objectMapper.readValue("{}", ModelArgs.class);
assertNull(model.getTest());
}
@Test
void t101_body_with_null() { // OK
final var model = objectMapper.readValue("{\"test\":null}", ModelArgs.class);
assertEquals(Optional.empty(), model.getTest());
}
@Test
void t102_body_with_value() { // OK
final var model = objectMapper.readValue("{\"test\":\"aze\"}", ModelArgs.class);
assertEquals(Optional.of("aze"), model.getTest());
}
@Test
void t200_no_body() { // KO = should be same result as t100 and it is ok for test2 value
final var model = objectMapper.readValue("{}", ModelOneArg.class);
assertNull(model.getTest());
assertNull(model.getTest2());
}
@Test
void t201_body_with_null() { // OK
final var model = objectMapper.readValue("{\"test\":null}", ModelOneArg.class);
assertEquals(Optional.empty(), model.getTest());
assertNull(model.getTest2());
}
@Test
void t202_body_with_value() { // OK
final var model = objectMapper.readValue("{\"test\":\"aze\"}", ModelOneArg.class);
assertEquals(Optional.of("aze"), model.getTest());
assertNull(model.getTest2());
}
}Here are models used : public class ModelNoArgs {
protected Optional<String> test;
public void setTest(Optional<String> test) {
this.test = test;
}
public Optional<String> getTest() {
return test;
}
}
public class ModelArgs {
protected Optional<String> test;
public ModelArgs(Optional<String> test) {
this.test = test;
}
public void setTest(Optional<String> test) {
this.test = test;
}
public Optional<String> getTest() {
return test;
}
}
public class ModelOneArg {
protected Optional<String> test;
protected Optional<String> test2;
public ModelOneArg(Optional<String> test) {
this.test = test;
}
public void setTest(Optional<String> test) {
this.test = test;
}
public Optional<String> getTest() {
return test;
}
public Optional<String> getTest2() {
return test2;
}
public void setTest2(Optional<String> test2) {
this.test2 = test2;
}
} |
|
@SirBigoo , this seems related to the Jackson 3 upgrade or Spring Boot 4 rather than this module, seeing your examples don't use the On the first issue, Jackson 3.1 adds a configuration flag to map a missing property to a |
Summary by cubic
Adds Jackson 3 support alongside Jackson 2 so JsonNullable works on both runtimes. Also adds a Java 17 module and updates CI to JDK 17.
New Features
Refactors
Written for commit 460051d. Summary will update on new commits.