Try builder first, then fallback to deprecated constructor #2072
Conversation
|
|
|
The upstream issue has a fix here FasterXML/jackson-module-kotlin#799 but using the builder would have avoided this issue. |
| val km = KotlinModule() | ||
| val km = try { | ||
| KotlinModule.Builder() | ||
| .build() |
There was a problem hiding this comment.
Have we confirmed this works with jackson v2.9.0?
There was a problem hiding this comment.
The existing code doesn't work with jackson 2.9.0
There was a problem hiding this comment.
Not sure I understand, the Java SDK currently does build and all test pass with jackson 2.9.0
There was a problem hiding this comment.
It currently builds with jackson 2.14.2
Line 34 in 5ccb859
There was a problem hiding this comment.
It fails with jackson 2.9.0
'void com.fasterxml.jackson.module.kotlin.KotlinModule.<init>(int, boolean, boolean, boolean, com.fasterxml.jackson.module.kotlin.SingletonSupport, boolean, int, kotlin.jvm.internal.DefaultConstructorMarker)'
java.lang.NoSuchMethodError: 'void com.fasterxml.jackson.module.kotlin.KotlinModule.<init>(int, boolean, boolean, boolean, com.fasterxml.jackson.module.kotlin.SingletonSupport, boolean, int, kotlin.jvm.internal.DefaultConstructorMarker)'
There was a problem hiding this comment.
Note the comment
// [2.9.0,)
indicating our support for 2.9.0
There was a problem hiding this comment.
Unless I'm doing something wrong, the code as-is won't work with jackson 2.9.0 master...Sineaggi:sdk-java:jackson-version-test
If you run ./gradlew :temporal-kotlin:test, it should fail with the stacktrace above.
There was a problem hiding this comment.
If we need to drop support to maintain forward compatibility I would 100% be in favor, but if we can support older versions without harming forward compatibility that would be the best for all SDK users.
There was a problem hiding this comment.
I've gone the registerKotlinModule route, which should allow for maximum backwards and forwards compatibility.
Sineaggi
force-pushed
the
fix-temporal-forwards-compat
branch
from
6f6cfd9 to
897788e
Compare
|
There is another way to simplify this further, and that is to call the |
|
Ah CI is failing because your branch does not have tags , which our CI unfortunately makes use of to detect the version of the SDK, you can use |
Right, that happened during local testing as well. I've pushed tags now. |
|
|
||
| testing { | ||
| suites { | ||
| jacksonVersions.forEach { jacksonVersion -> |
There was a problem hiding this comment.
These tests are not needed since we already have a setup to test multiple versions of dependencies
There was a problem hiding this comment.
How are these tests compiled/run? As it stands, it is impossible to call KotlinObjectMapperFactory.new() in a project that forcefully downgrades to jackson 2.9.0. This test shows that that is now possible.
There was a problem hiding this comment.
Hm yes looking at more carefully your test is doing something slightly different, we test building and running with different version of our dependencies, but here your testing building with one version, but running with another. Finding some breaking ABI change between versions most likely.
There was a problem hiding this comment.
This test, or the inclusion of org.gradle.api.plugins.jvm.JvmTestSuite appears to be causing some issues with our feature test I haven't had a chance to deep dive as to why. I am fine merging this without this test for now and I'll open an issue to create more thorough testing strategy for breaking catching breaking ABI changes. We should probably do tests like this for more then just jackson versions
Sineaggi
force-pushed
the
fix-temporal-forwards-compat
branch
from
7cc7df4 to
f1ca36a
Compare
|
@Quinn-With-Two-Ns can you rerun the checks? It seems as though only maintainers can do so. I think the |
|
(since @Quinn-With-Two-Ns is out, after a quick peek at the PR, I went ahead and triggered CI again) |
What was changed
Tries the new kotlin jackson module builder first, then falls back to the deprecated constructor.
Why?
Newest jackson (2.17.0) broke abi.
Checklist
Closes KotlinObjectMapperFactory is not forwards compatible #2071
How was this tested:
Tested the code manually against jackson 2.11, 2.14 and 2.17.
Any docs updates needed?