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.

Sign up for GitHub

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

Jump to bottom

Preserve read after write #406

Open
wants to merge 3 commits into
base: 2.12
Choose a base branch
from
Open

Preserve read after write #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0f9ab6
Mangle constructor parameter names to preserve read-after-write
Jul 3, 2020
dda9c82
Merge branch '2.12' into preserve-read-after-write
dinomite Jan 1, 2021
74c6cb9
Credit popiel
dinomite Jan 1, 2021
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
4 changes: 4 additions & 0 deletions release-notes/CREDITS-2.x
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ Authors:

Contributors:

T. Alexander Popiel (popiel@github)
* Fixed roundtrip serialization when using PascalCase
(2.12.1)

Wolfgang Jung (elektro-wolle@github)
* Fixed inline class serialization
(2.12.1)
Expand Down
11 changes: 3 additions & 8 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/Extensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,9 @@ package com.fasterxml.jackson.module.kotlin
import com.fasterxml.jackson.core.JsonParser
import com.fasterxml.jackson.core.TreeNode
import com.fasterxml.jackson.core.type.TypeReference
import com.fasterxml.jackson.databind.JsonMappingException
import com.fasterxml.jackson.databind.MappingIterator
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.ObjectReader
import com.fasterxml.jackson.databind.module.SimpleModule
import com.fasterxml.jackson.databind.JsonDeserializer
import com.fasterxml.jackson.databind.JsonSerializer
import com.fasterxml.jackson.databind.*
import com.fasterxml.jackson.databind.json.JsonMapper
import com.fasterxml.jackson.databind.module.SimpleModule
import java.io.File
import java.io.InputStream
import java.io.Reader
Expand Down Expand Up @@ -64,4 +59,4 @@ inline fun <reified T : Any> SimpleModule.addSerializer(kClass: KClass<T>, seria
inline fun <reified T : Any> SimpleModule.addDeserializer(kClass: KClass<T>, deserializer: JsonDeserializer<T>) = this.apply {
addDeserializer(kClass.java, deserializer)
addDeserializer(kClass.javaObjectType, deserializer)
}
}
3 changes: 2 additions & 1 deletion src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinNamesAnnotationIntrospector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ internal class KotlinNamesAnnotationIntrospector(val module: KotlinModule, val c
return member.name.substringAfter("is").decapitalize().substringBefore('-')
}
} else if (member is AnnotatedParameter) {
return findKotlinParameterName(member)
val simpleName = findKotlinParameterName(member)
return if (simpleName == null) null else BeanUtil.stdManglePropertyName(simpleName, 0)
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem right to me -- more like an arbitrary change that makes specific case not fail the way it did.
And it is not a good place to make the change either, as name mangling aspects are not expected to be handled by AnnotationIntrospector.

I realize that there might not be a better way to fix it at the moment (2.12 or earlier), but, FWTW, I plan on revamping property introspection for 2.13. And this time try to keep module owners better in the loop regarding changes too.

Anyway: If change is to be made, I would recommend adding a comment with link to this PR and/or issue (yes, I know, git history can show it if one knows where to look) to explain the logic of making a change here.

Copy link

@popiel popiel Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If name mangling isn't expected in the introspector, why is it mangling (decapitalizing, etc) the names that start with 'get' and 'is'? Also, mangling is explicitly managed by the introspector in the next method (findRenameByField), so if this isn't the right place for mangling, a lot more changes are needed to move it to the right place...

Copy link
Member

Choose a reason for hiding this comment

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

It is possible that some other (relatively recent) changes are similarly sub-optimal (especially code right before your change in same method) but adding more of the same does not help.

But specifically, I think name-mangling in "findImplicitPropertyName()" seems wrong just due to its place in call sequences; it is a low-level accessor, whereas "findRenameByField()" has slightly different function.
I agree that "findRenameByField()" is not the best design choice either (and it is something I added).

I think my main concern here is just that I am not sure I understand intended logic, and strongly suspect that this is working around a real problem, but one that is not due to missing name-mangling but due to missing linkage between constructors and fields -- something that I know is an existing problem wrt auto-detected (non-annotated) constructors.

Still, if you could explain the intended logic in a comment, that would be helpful.

Copy link

Choose a reason for hiding this comment

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

I shall add commentary in the morning, after finding references to where the standard getXxx and setXxx names get mangled, too (which is where I learned of the mangling routines in the first place).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Jackson 2.12 changed this handling a bit, trying to make it easier to replace pieces, moving away (ideally) from static helper methods.

I will also try to keep everyone updated on my plans for jackson-databind 2.13, via "jackson-dev" mailing list (and related Github issue for jackson-databind, once I get little bit further).
So even if there is a work-around here for 2.12, perhaps it could be further refactored into nicer packaging, once there is a better way (there might not be in 2.12).

}

return null
Expand Down
18 changes: 17 additions & 1 deletion src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/ParameterNameTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import com.fasterxml.jackson.annotation.JsonIgnore
import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.databind.PropertyNamingStrategies
import com.fasterxml.jackson.databind.SerializationFeature
import com.fasterxml.jackson.module.kotlin.*
import com.fasterxml.jackson.module.kotlin.isKotlinClass
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import com.fasterxml.jackson.module.kotlin.readValue
import org.hamcrest.CoreMatchers.equalTo
import org.hamcrest.MatcherAssert.assertThat
import org.junit.Test
Expand Down Expand Up @@ -160,6 +162,20 @@ class TestJacksonWithKotlin {
}


// ==================

private data class NonIdiomaticPascalCasedClass(val Some_Number: Int, val Email_Address: String)

@Test fun readAfterWriteWithPascalCaseProperties() {
val obj = NonIdiomaticPascalCasedClass(6, "nobody@test.com")

val serialized = normalCasedMapper.writeValueAsString(obj)
val deserialized = normalCasedMapper.readValue<NonIdiomaticPascalCasedClass>(serialized)

assertThat(deserialized, equalTo(obj))
}


// ==================

private class StateObjectWithFactory private constructor (override val name: String, override val age: Int, override val primaryAddress: String, override val wrongName: Boolean, override val createdDt: Date) : TestFields {
Expand Down