Fixed a bug in #512. #533

k163377 · 2022-01-03T16:42:16Z

In the #512 changing, the rewrite of the isAccessible property of the factory function was missing, which could cause the call to fail, so it has been fixed.
Also, I found a new problem where the call fails if the companion object is private, so I fixed it at the same time (this problem existed before #438).

I have also added a test pattern to prevent these omissions.

dinomite · 2022-01-04T11:16:50Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ConstructorValueCreator.kt

@@ -7,6 +7,6 @@ internal class ConstructorValueCreator<T>(override val callable: KFunction<T>) :
    override val accessible: Boolean = callable.isAccessible

    init {
-        callable.isAccessible = true
+        if (!accessible) callable.isAccessible = true


Would you add a note here or in the class comment as to why isAccessible is being forced to true?

Fixed.
5eca9cd

dinomite · 2022-01-04T11:18:28Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/MethodValueCreator.kt

@@ -23,14 +23,18 @@ internal class MethodValueCreator<T> private constructor(
            // abort, we have some unknown case here
            if (!possibleCompanion.isCompanion) return null

+            val initialCallableAccessible = callable.isAccessible


Good name 👍

dinomite · 2022-01-04T11:20:38Z

src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/ParameterNameTests.kt

-        companion object Named {
-            @JvmStatic @JsonCreator fun create(@JsonProperty("name") nameThing: String, @JsonProperty("age") age: Int, @JsonProperty("primaryAddress") primaryAddress: String, @JsonProperty("renamed") wrongName: Boolean, @JsonProperty("createdDt") createdDt: Date): StateObjectWithFactoryOnNamedCompanion {
+        private companion object Named {
+            @JvmStatic @JsonCreator private fun create(@JsonProperty("name") nameThing: String, @JsonProperty("age") age: Int, @JsonProperty("primaryAddress") primaryAddress: String, @JsonProperty("renamed") wrongName: Boolean, @JsonProperty("createdDt") createdDt: Date): StateObjectWithFactoryOnNamedCompanion {


Would you split this over lines (if you're using IntelliJ, ⌥⏎ then select Put parameters on separate lines):

All lines that were exceeding 120 characters in length depending on the parameter were modified to break lines.
b19c44f

dinomite · 2022-01-04T14:01:46Z

@k163377 I'm seeing a failure in this test https://github.com/FasterXML/jackson-module-kotlin/blob/2.13/src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/ParameterNameTests.kt#L211

k163377 · 2022-01-04T23:10:38Z

@dinomite
I was not able to reproduce the failure in my environment.
Could you please share how to run it and the error message?

dinomite · 2022-01-05T19:12:29Z

@k163377 The error I get when I run via the command line (mvn verify):

[INFO] Running com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin
[ERROR] Tests run: 13, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.399 s <<< FAILURE! - in com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin
[ERROR] findingFactoryMethod3(com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin)  Time elapsed: 0.023 s  <<< ERROR!
com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException:
Unrecognized field "renamed" (class com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion), not marked as ignorable (6 known properties: "primaryAddress", "factoryUsed", "wrongName", "name", "age", "createdDt"])
 at [Source: (String)"{"name":"Frank","age":30,"primaryAddress":"something here","renamed":true,"createdDt":"2016-10-25T18:25:48.000+00:00"}"; line: 1, column: 119] (through reference chain: com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion["renamed"])
	at com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin.findingFactoryMethod3(ParameterNameTests.kt:288)

My system setup:

14:07:37 Caracalla:~/code/misc/jackson-module-kotlin$ java -version
openjdk version "11.0.12" 2021-07-20 LTS
OpenJDK Runtime Environment Zulu11.50+19-CA (build 11.0.12+7-LTS)
OpenJDK 64-Bit Server VM Zulu11.50+19-CA (build 11.0.12+7-LTS, mixed mode)
14:07:39 Caracalla:~/code/misc/jackson-module-kotlin$ mvn -version
Apache Maven 3.6.2 (40f52333136460af0dc0d7232c0dc0bcf0d9e117; 2019-08-27T11:06:16-04:00)
Maven home: /usr/local/Cellar/maven/3.6.2/libexec
Java version: 11.0.12, vendor: Azul Systems, Inc., runtime: /Users/dinomite/.sdkman/candidates/java/11.0.12-zulu/zulu-11.jdk/Contents/Home
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "11.5", arch: "x86_64", family: "mac"

I don't see this error when running in IntelliJ :-/

k163377 · 2022-01-05T20:23:40Z

@dinomite
I was able to reproduce the problem on mac (and also reproduced the point that Intellij did not).
It looks like the primary constructor is being called instead of the JsonCreator when run with ./mvnw verify.

The same problem occurred when I checked eb7a12a with the factory function set to private.
So this seems to be a problem that has existed for some time.

This may be related to #514.

dinomite · 2022-01-06T13:06:27Z

Hrm, yay for platform-independent languages. I'm not sure what to do about this, I'd like to avoid ignoring tests if at all possible.

k163377 · 2022-01-07T11:31:16Z

@dinomite
I found the real cause of the problem.
https://youtrack.jetbrains.com/issue/KT-46181

This test does not seem to succeed when running on Kotlin 1.5.x.
In fact, if I change the version of Kotlin to 1.6.0, it does not fail anymore.
The reason why this test doesn't fail with Intellij is probably because the compilation results vary depending on the plugin version.

For this reason, I think it would be better to merge this PR with creator set to public.
Also, I think it would be better to create a test for private factory functions that are executed under specific conditions separately from this one.
However, I don't know how to create such a test myself, so I think it will take some time to work on it.
I'd like to prioritize #439 related tasks and value class related improvements, so I'd like to record this in an issue and put it off.

A `private` and `JvmStatic` function was broken in `Kotlin 1.5.x`. See GitHub Comment below for details. FasterXML#533 (comment)

dinomite · 2022-01-07T15:00:56Z

Great digging, @k163377! I think your change to public makes sense, I'll re-private the creator on the 2.14 branch (which is using Kotlin 1.6.10).

Focusing on #439 sounds good to me, would you make that issue for private factory functions and link it here? In the meantime, I'll get this PR merged.

#533 (comment)

k163377 · 2022-01-08T10:16:38Z

@dinomite I created an issue as #535.

k163377 added 4 commits January 4, 2022 01:14

In order to reduce processing costs, not rewrite if not necessary.

844ce35

Fix the call would fail if callable.isAccessible was false.

b01c4bd

Fix the call would fail if the companion object was private.

c0a58ab

Fix test case

3428cd3

dinomite approved these changes Jan 4, 2022

View reviewed changes

dinomite self-assigned this Jan 4, 2022

k163377 added 2 commits January 4, 2022 21:01

add comments

5eca9cd

formatting

b19c44f

Merge remote-tracking branch 'origin/2.13' into github512/fix

c9df362

Fixed to public

9aa88e5

A `private` and `JvmStatic` function was broken in `Kotlin 1.5.x`. See GitHub Comment below for details. FasterXML#533 (comment)

dinomite merged commit 99389f3 into FasterXML:2.13 Jan 7, 2022

dinomite added a commit that referenced this pull request Jan 7, 2022

Re-privatize method

5e5773d

#533 (comment)

k163377 mentioned this pull request Jan 8, 2022

Unable to discover private factory functions in Kotlin 1.5.x/jackson-module-kotlin 2.13.x #535

Closed

k163377 deleted the github512/fix branch January 9, 2022 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed a bug in #512. #533

Fixed a bug in #512. #533

k163377 commented Jan 3, 2022 •

edited

Loading

dinomite Jan 4, 2022

k163377 Jan 4, 2022

dinomite Jan 4, 2022

dinomite Jan 4, 2022

k163377 Jan 4, 2022

dinomite commented Jan 4, 2022

k163377 commented Jan 4, 2022

dinomite commented Jan 5, 2022

k163377 commented Jan 5, 2022

dinomite commented Jan 6, 2022

k163377 commented Jan 7, 2022

dinomite commented Jan 7, 2022

k163377 commented Jan 8, 2022

Fixed a bug in #512. #533

Fixed a bug in #512. #533

Conversation

k163377 commented Jan 3, 2022 • edited Loading

dinomite Jan 4, 2022

Choose a reason for hiding this comment

k163377 Jan 4, 2022

Choose a reason for hiding this comment

dinomite Jan 4, 2022

Choose a reason for hiding this comment

dinomite Jan 4, 2022

Choose a reason for hiding this comment

k163377 Jan 4, 2022

Choose a reason for hiding this comment

dinomite commented Jan 4, 2022

k163377 commented Jan 4, 2022

dinomite commented Jan 5, 2022

k163377 commented Jan 5, 2022

dinomite commented Jan 6, 2022

k163377 commented Jan 7, 2022

dinomite commented Jan 7, 2022

k163377 commented Jan 8, 2022

k163377 commented Jan 3, 2022 •

edited

Loading