Skip to content
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

Try builder first, then fallback to deprecated constructor #2072

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions temporal-kotlin/build.gradle
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile
import org.gradle.api.plugins.jvm.JvmTestSuite

plugins {
id 'org.jlleitschuh.gradle.ktlint' version '11.3.1'
Expand Down Expand Up @@ -43,3 +44,27 @@ task registerNamespace(type: JavaExec) {

test.dependsOn 'registerNamespace'

def jacksonVersions = ['2.9.0', "$jacksonVersion".toString()]

testing {
suites {
jacksonVersions.forEach { jacksonVersion ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are not needed since we already have a setup to test multiple versions of dependencies

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"jackson${jacksonVersion}Test"(JvmTestSuite) {
useJUnit(junitVersion)
dependencies {
implementation project()
implementation "com.fasterxml.jackson.datatype:jackson-datatype-jsr310:$jacksonVersion!!"
implementation "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:$jacksonVersion!!"
implementation("com.fasterxml.jackson.module:jackson-module-kotlin:$jacksonVersion!!") {
exclude group: 'org.jetbrains.kotlin', module: 'kotlin-reflect'
}

implementation project(':temporal-testing')
implementation "junit:junit:${junitVersion}"
}
}
}
}
}

test.dependsOn("jackson2.9.0Test", "jackson2.14.2Test")
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright (C) 2022 Temporal Technologies, Inc. All Rights Reserved.
*
* Copyright (C) 2012-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Modifications copyright (C) 2017 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this material except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.temporal.common.converter

import com.fasterxml.jackson.module.kotlin.PackageVersion
import org.junit.Assert.assertEquals
import org.junit.Test

class KotlinObjectMapperFactoryTest {
@Test
fun `test jackson 2 14 2`() {
assertEquals(PackageVersion.VERSION.toString(), "2.14.2")
KotlinObjectMapperFactory.new()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright (C) 2022 Temporal Technologies, Inc. All Rights Reserved.
*
* Copyright (C) 2012-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Modifications copyright (C) 2017 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this material except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.temporal.common.converter

import com.fasterxml.jackson.module.kotlin.PackageVersion
import org.junit.Assert.assertEquals
import org.junit.Test

class KotlinObjectMapperFactoryTest {
@Test
fun `test jackson 2 9 0`() {
assertEquals(PackageVersion.VERSION.toString(), "2.9.0")
KotlinObjectMapperFactory.new()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,15 @@
package io.temporal.common.converter

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.kotlin.KotlinModule
import com.fasterxml.jackson.module.kotlin.registerKotlinModule

class KotlinObjectMapperFactory {
companion object {
@JvmStatic
fun new(): ObjectMapper {
val mapper = JacksonJsonPayloadConverter.newDefaultObjectMapper()

// use deprecated constructor instead of builder to maintain compatibility with old jackson versions
@Suppress("deprecation")
val km = KotlinModule()
mapper.registerModule(km)
return mapper
return mapper.registerKotlinModule()
}
}
}
Loading