FasterXML / FasterXML/jackson-module-kotlin

There are some problems with KNAI.findDefaultCreator

Open
#547 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Kotlin
Stars
1.2k
Forks
187
Avg merge
9h 48m
Merged PRs (30d)
16

Description

I found several processes for `KNAI.findDefaultCreator` that are not working as intended, or the test results do not change even if they are removed.

## 1. `JsonProperty.Access` is ignored in `KFunction<*>.isPossibleSingleString`.
This function is decraled as follows.

```kotlin
private fun KFunction<*>.isPossibleSingleString(propertyNames: Set): Boolean = parameters.singleOrNull()?.let {
it.name !in propertyNames
&& it.type.javaType == String::class.java
&& !it.hasAnnotation()
} == true
```

From reading the `Javadoc` of `JsonProperty.Access`, it looks like the correct behavior is to ignore it during deserialization if the `JsonProperty.access` property is `READ_ONLY`.
https://javadoc.io/static/com.fasterxml.jackson.core/jackson-annotations/1.13.1/com/fasterxml/jackson/annotation/JsonProperty.Access.html

Giving `JsonProperty` to parameters with `READ_ONLY` looks like a strange edge case conceptually, but I think it should be fixed if we want to match `Javadoc`.

## 2. `kConstructor.parameters.any { it.name == null }` will not be `true`
Since `KParameter.name` is `nullable`, this decision seems reasonable, but it does not seem to be `true` in practice.

In the constructor, `KParameter.name` is `null` only if get the constructor of `inner class` as follows.
In this case, `outer class` is requested as an instance parameter, and its `KParameter.name` is `null`.

```kotlin
val anyConstructorHasJsonCreator = kClass.constructors
.filterOutSingleStringCallables(propertyNames)
.any { it.hasAnnotation() }

val anyCompanionMethodIsJsonCreator = member.type.rawClass.kotlin.companionObject?.declaredFunctions
?.filterOutSingleStringCallables(propertyNames)
?.any { it.hasAnnotation() && it.hasAnnotation() }
?: false
```

On the other hand, `Jackson` does not support deserialization to `non-static` classes, so this pattern can be ignored.

Also, if `KParameter.name` is `null`, it seems to fail with `findKotlinParameterName` even if this process does not fail.
In this case, the error message will be `has no property name` instead of `no Creators`.

I think it would be better to remove this decision, as it eliminates unnecessary processing and appears to make error messages easier to understand.

---

The content has been rewritten because the modification of #818 has changed the search process for creators.

Original

I found several processes for `KNAI.hasCreatorAnnotation` that are not working as intended, or the test results do not change even if they are removed.

## 1. There are some problems with `KFunction<*>.isPossibleSingleString`.
This function is decraled as follows.

```kotlin
private fun KFunction<*>.isPossibleSingleString(propertyNames: Set): Boolean = parameters.size == 1 &&
parameters[0].name !in propertyNames &&
parameters[0].type.javaType == String::class.java &&
!parameters[0].hasAnnotation()
```

### 1-1. Unable to get the intended result from functions retrieved with `companionObject.declaredFunctions`.
The `parameters` of functions retrieved with `companionObject.declaredFunctions` include the `instance parameter`.
In other words, `parameters.size == 1` will be `true` only for functions with no parameters.

Therefore, this decision needs to be modified to `valueParameters.size == 1`.

### 1-2.
From reading the `Javadoc` of `JsonProperty.Access`, it looks like the correct behavior is to ignore it during deserialization if the `JsonProperty.access` property is `READ_ONLY`.
https://javadoc.io/static/com.fasterxml.jackson.core/jackson-annotations/1.13.1/com/fasterxml/jackson/annotation/JsonProperty.Access.html

Giving `JsonProperty` to parameters with `READ_ONLY` looks like a strange edge case conceptually, but I think it should be fixed if we want to match `Javadoc`.

## 2. Removing `kConstructor.isPossibleSingleString` does not change the test results.
The process I am referring to here is the following part.
Removing `kConstructor.isPossible...` from the branch of the `when` expression does not seem to change the test results.

```kotlin
val propertyNames = kClass.memberProperties.map { it.name }.toSet()

return when {
kConstructor.isPossibleSingleString(propertyNames) -> false

/* omission */

}
```

To begin with, what is the purpose of the `isPossibleSingleString` decision?
I did not understand why it specifically excludes functions that match this condition.
In addition, I think that this decision should be removed from the `KotlinModule` process, since there seems to be no bug reports despite the problems mentioned in `1-1`.

## 3. `kConstructor.parameters.any { it.name == null }` will not be `true`
Since `KParameter.name` is `nullable`, this decision seems reasonable, but it does not seem to be `true` in practice.

In the constructor, `KParameter.name` is `null` only if get the constructor of `inner class` as follows.
In this case, `outer class` is requested as an instance parameter, and its `KParameter.name` is `null`.

```kotlin
val anyConstructorHasJsonCreator = kClass.constructors
.filterOutSingleStringCallables(propertyNames)
.any { it.hasAnnotation() }

val anyCompanionMethodIsJsonCreator = member.type.rawClass.kotlin.companionObject?.declaredFunctions
?.filterOutSingleStringCallables(propertyNames)
?.any { it.hasAnnotation() && it.hasAnnotation() }
?: false
```

On the other hand, `Jackson` does not support deserialization to `non-static` classes, so this pattern can be ignored.

Also, if `KParameter.name` is `null`, it seems to fail with `findKotlinParameterName` even if this process does not fail.
In this case, the error message will be `has no property name` instead of `no Creators`.

I think it would be better to remove this decision, as it eliminates unnecessary processing and appears to make error messages easier to understand.

## 4. `JsonCreator.mode` is being ignored.
In the following process, all cases where `JsonCreator` is included are handled in the same way.

```kotlin
val anyConstructorHasJsonCreator = kClass.constructors
.filterOutSingleStringCallables(propertyNames)
.any { it.hasAnnotation() }

val anyCompanionMethodIsJsonCreator = member.type.rawClass.kotlin.companionObject?.declaredFunctions
?.filterOutSingleStringCallables(propertyNames)
?.any { it.hasAnnotation() && it.hasAnnotation() }
?: false
```

On the other hand, if `JsonCreator.mode` is `DISABLED`, returning `false` seems to be correct.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.