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

Fixed overriding findCreatorAnnotation instead of hasCreatorAnnotation #727

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Fixed overriding findCreatorAnnotation instead of hasCreatorAnnotation #727

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
e22bc28
Fixed overriding findCreatorAnnotation instead of hasCreatorAnnotation
k163377 Nov 19, 2023
3b6d46a
Update release notes wrt #727
k163377 Nov 19, 2023
23e8185
Move TODO
k163377 Nov 20, 2023
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
1 change: 1 addition & 0 deletions release-notes/CREDITS-2.x
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Authors:
Contributors:

# 2.17.0 (not yet released)
* #727: Fixed overriding findCreatorAnnotation instead of hasCreatorAnnotation

# 2.16.0

Expand Down
2 changes: 1 addition & 1 deletion release-notes/VERSION-2.x
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Co-maintainers:

2.17.0 (not yet released)

-
#727: Fixed overriding findCreatorAnnotation instead of hasCreatorAnnotation.

2.16.0 (15-Nov-2023)

Expand Down
11 changes: 0 additions & 11 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand much about this TODO comment, but I moved it to KotrinNamesAnnotationIntrospector just in case.

Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
package com.fasterxml.jackson.module.kotlin

import com.fasterxml.jackson.annotation.JsonCreator
import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.databind.DeserializationFeature
import com.fasterxml.jackson.databind.JsonSerializer
import com.fasterxml.jackson.databind.Module
import com.fasterxml.jackson.databind.cfg.MapperConfig
import com.fasterxml.jackson.databind.introspect.*
import com.fasterxml.jackson.databind.jsontype.NamedType
import com.fasterxml.jackson.databind.util.Converter
Expand Down Expand Up @@ -59,15 +57,6 @@ internal class KotlinAnnotationIntrospector(
return hasRequired
}

override fun findCreatorAnnotation(config: MapperConfig<*>, a: Annotated): JsonCreator.Mode? {

// TODO: possible work around for JsonValue class that requires the class constructor to have the JsonCreator(Mode.DELEGATED) set?
// since we infer the creator at times for these methods, the wrong mode could be implied.

// findCreatorBinding used to be a clearer way to set this, but we need to set the mode here to disambugiate the intent of the constructor
return super.findCreatorAnnotation(config, a)
}

override fun findSerializationConverter(a: Annotated): Converter<*, *>? = when (a) {
// Find a converter to handle the case where the getter returns an unboxed value from the value class.
is AnnotatedMethod -> a.findValueClassReturnType()?.let {
Expand Down
15 changes: 10 additions & 5 deletions 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 @@ -2,6 +2,7 @@ package com.fasterxml.jackson.module.kotlin

import com.fasterxml.jackson.annotation.JsonCreator
import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.databind.cfg.MapperConfig
import com.fasterxml.jackson.databind.introspect.Annotated
import com.fasterxml.jackson.databind.introspect.AnnotatedConstructor
import com.fasterxml.jackson.databind.introspect.AnnotatedMember
Expand Down Expand Up @@ -111,11 +112,15 @@ internal class KotlinNamesAnnotationIntrospector(
}
}

override fun hasCreatorAnnotation(member: Annotated): Boolean =
if (member is AnnotatedConstructor && member.isKotlinConstructorWithParameters())
cache.checkConstructorIsCreatorAnnotated(member) { hasCreatorAnnotation(it) }
else
false
// TODO: possible work around for JsonValue class that requires the class constructor to have the JsonCreator(DELEGATED) set?
// since we infer the creator at times for these methods, the wrong mode could be implied.
override fun findCreatorAnnotation(config: MapperConfig<*>, ann: Annotated): JsonCreator.Mode? {
if (ann !is AnnotatedConstructor || !ann.isKotlinConstructorWithParameters()) return null
Copy link
Member

Choose a reason for hiding this comment

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

So Kotlin does not add factory methods as creators, just constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the role of this function is to consider the primary constructor as a JsonCreator when there is no function with JsonCreator assigned.
Factory methods are not processed here because they are explicitly granted JsonCreator.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Going forward (2.17?) I hope to add a new type of Creator level -- "preferred" (or "default" or something -- which is higher than implicit (auto-detected) but lower than explicit annotation.
The idea is that it would be used as THE Creator if no annotations used, but actual @JsonCreator annotation could be used to override. But the two would not conflict -- unlike the case of 2 annotated constructors, for example, there would be clear precedence (explicit annotation wins). Similarly "regular" auto-detection could find other candidates (from Java side), but modules (Kotlin, Scala) could indicate something that is automatically preferred.
This would also work better for Java Records which have the "canonical constructor" which should be indicated as "preferred" over alternatives.


return JsonCreator.Mode.DEFAULT.takeIf {
cache.checkConstructorIsCreatorAnnotated(ann) { hasCreatorAnnotation(it) }
}
}

@Suppress("UNCHECKED_CAST")
private fun findKotlinParameterName(param: AnnotatedParameter): String? {
Expand Down