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

TestJacksonWithKotlin#findingFactoryMethod3 doesn't pass on Kotlin 1.5 in 2.14.0-rc2 #587

Closed
Spikhalskiy opened this issue Oct 13, 2022 · 13 comments
Labels
bug pr-welcome Issue for which progress most likely if someone submits a Pull Request

Comments

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented Oct 13, 2022

Describe the bug
An attempt to build 2.14.0-rc2 using Kotlin 1.5 leads to

[ERROR] Tests run: 13, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.308 s <<< FAILURE! - in com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin
[ERROR] findingFactoryMethod3(com.fasterxml.jackson.module.kotlin.test.TestJacksonWithKotlin)  Time elapsed: 0.016 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)

It looks like 2.14.0-rc2 has a regression around

        @JsonProperty("renamed")
        override var wrongName: Boolean = false

functionality.

The test is passing on Jackson 2.13 & Kotlin 1.5.

Expected behavior
Tests should pass.

Versions
Kotlin: 1.5
Jackson-module-kotlin: 2.14.0-rc2
Jackson-databind: 2.14.0-rc2

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Oct 13, 2022

@cowtowncoder I don't think it's safe to promote RC into a Release with this regression.

@cowtowncoder
Copy link
Member

@Spikhalskiy I'd need someone to help with fix. I hope it can be resolved within next week or two as ideally we would fix it for 2.14.0. As long as this is feasible I think it is good to hold up release until this can be resolved.

But one thing I don't understand is why unit tests didn't catch this -- I guess they are only run with Kotlin 1.6 at this point?
I have thought about making CI use matrix option to override for kotlin core (see #565) and that would have caught this issue, as well as maybe #586). Override itself works fine but I haven't had time to change CI yet.

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Oct 13, 2022

But one thing I don't understand is why unit tests didn't catch this -- I guess they are only run with Kotlin 1.6 at this point?

Branch 2.14 has an increment of Kotlin from 1.5 to 1.6. I don't know the background of why it was done, because the code itself is compatible with Kotlin 1.5. So, changes in 2.14 were verified against 1.6 only.

I have thought about making CI use matrix option to override for kotlin core (see #565) and that would have caught this issue, as well as maybe #586).

That's a proper way to do that, yes. Build with 1.5, 1.6 and 1.7. 1.4 can't be used for the build, because classes/methods present in Kotlin 1.5+ only are used. But the final artifact should be possible to use with 1.4 if it's built with 1.5. Kotlin provides a binary compatibility one minor version down.
A simple alternative way is to have a maintainer that verifies the matrix manually sometimes. That's what I do in the project I support. But I think that matrix in CI is a more sustainable way for Jackson.

@Spikhalskiy
Copy link
Contributor Author

I'd need someone to help with fix. I hope it can be resolved within next week or two as ideally we would fix it for 2.14.0. As long as this is feasible I think it is good to hold up release until this can be resolved.

I'm gonna be on vacation in the upcoming weeks, but I will try to take a peek on what happened here.

@cowtowncoder
Copy link
Member

Yeah the real challenge is that there is currently no active maintainers for this project.

As to baseline to use, I am not sure: Kotlin module is bit different from datatype modules, but sort of similar questions relate to Guava and Joda modules too in that in both cases there has to be one default version used for the dependency -- but there is expectation/hope that module would work across wider ranges of versions.

But definitely for Kotlin case the very first thing would be to define which Kotlin-core versions are supported by which Jackson Kotlin module versions.

I think it is fair to say that unless there's a PR for fixing this, issue is unlikely to get resolved.

@cowtowncoder cowtowncoder added the pr-welcome Issue for which progress most likely if someone submits a Pull Request label Oct 13, 2022
@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Oct 13, 2022

@cowtowncoder The module should work correctly for all kotlin versions it is binary compatible with. Otherwise, it creates a landmine for the users. Even if the kotlin version for this project is increased to 1.6 (which removes compatibility with 1.4 without good reason), it's still binary compatible with 1.5 and should work correctly with 1.5.

I think based on what we have in 2.13, the target should be:
Main Kotlin version used in the project: 1.5
Binary compatible with: 1.4+
Tested on 1.5+

And shifting these values up should have clear reasoning why it should be done. For example, Kotlin version in 2.14 branch was raised from 1.5 to 1.6 without much reasoning or context here: 5ec7aad. While the project clearly builds just fine on 1.5.

@cowtowncoder
Copy link
Member

I don't quite agree that module MUST work for all versions possible. That may make sense or might not.
But module maintainers need to use their judgment in what makes sense; it is not about maximizing range of supported platform versions. Perhaps it makes sense to try to support 1.4. Perhaps not. But I do know that eventually support for older platform versions should be removed in controller manner.
This is done for Scala module, which does have an active maintainer.

But this is not my decision as I am not a maintainers. I am not advocating on specific support. But rather whatever support is decided to be, that should be documented and tested.

All of above is pretty academic, too, unless there is someone who steps up to maintain this module.
@Spikhalskiy if you wanted to consider such responsibility, I'd be happy to give you access. But I understand it may not be something you want to do. I just mention this since you seem to be quite knowledgeable of issues and so on.

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Oct 13, 2022

Users of the project that I develop and maintain depend heavily on this module. So I'm transitively invested in keeping things in working order. I honestly don't have much time or energy to actively maintain this module right now. But I guess there is no choice.
@cowtowncoder Shoot me a mail at dmitry@spikhalskiy.com and let's have a short chat maybe in a week or so, so you may give me some insight into your CI, policies, release process, anything else I should know and we kickstart it.

@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 13, 2022

Thank you @Spikhalskiy !!! I will reach out to you via email for more details, discussion.

But I want to emphasize that it being OSS project it is all voluntary and anything you can help with helps. Same goes for everyone else: we do what we want to do, when are able to. And life happens outside of these projects. So I don't want there to be any expectation that one must do certain minimum level of things, shame or guilt. This should go without saying but I want to say it because it is too easy to forget.

With that said, let's figure out how we can keep this module usable and users happy!

@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 14, 2022

Something that may be of interest in #565 that I finally decided to do: add "matrix build" to CI so that we can easily verify compatibility.

Looks like 2.13 fails for Kotlin 1.4 and 1.7, passes for 1.5 and 1.6.
And 2.14 fails with 1.4, 1.5 and 1.7; only passes for Kotlin 1.6. So this issue is reproduced.

@jarekratajski
Copy link
Contributor

jarekratajski commented Oct 16, 2022

What is the root of the problem. Changes in code compiled by kotlin 1.5.31 vs 1.6.21
In 1.5.31 we have:

 @Metadata(
            mv = {1, 5, 1},
            k = 1,
            xi = 48,
            d1 = {"\u0000&\n\u0002\u0018\u0002\n\u0002\u0018\u0002\n\u0000\n\u0002\u0010\u000e\n\u0000\n\u0002\u0010\b\n\u0002\b\u0002\n\u0002\u0010\u000b\n\u0000\n\u0002\u0018\u0002\n\u0002\b\u0010\b\u0002\u0018\u0000 \u00192\u00020\u0001:\u0001\u0019B/\b\u0002\u0012\u0006\u0010\u0002\u001a\u00020\u0003\u0012\u0006\u0010\u0004\u001a\u00020\u0005\u0012\u0006\u0010\u0006\u001a\u00020\u0003\u0012\u0006\u0010\u0007\u001a\u00020\b\u0012\u0006\u0010\t\u001a\u00020\n¢\u0006\u0002\u0010\u000bR\u0014\u0010\u0004\u001a\u00020\u0005X\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\f\u0010\rR\u0014\u0010\t\u001a\u00020\nX\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u000e\u0010\u000fR\u001a\u0010\u0010\u001a\u00020\bX\u0086\u000e¢\u0006\u000e\n\u0000\u001a\u0004\b\u0011\u0010\u0012\"\u0004\b\u0013\u0010\u0014R\u0014\u0010\u0002\u001a\u00020\u0003X\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u0015\u0010\u0016R\u0014\u0010\u0006\u001a\u00020\u0003X\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u0017\u0010\u0016R\u0014\u0010\u0007\u001a\u00020\bX\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u0018\u0010\u0012¨\u0006\u001a"},
            d2 = {"Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion;", "Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$TestFields;", "name", "", "age", "", "primaryAddress", "wrongName", "", "createdDt", "Ljava/util/Date;", "(Ljava/lang/String;ILjava/lang/String;ZLjava/util/Date;)V", "getAge", "()I", "getCreatedDt", "()Ljava/util/Date;", "factoryUsed", "getFactoryUsed", "()Z", "setFactoryUsed", "(Z)V", "getName", "()Ljava/lang/String;", "getPrimaryAddress", "getWrongName", "Named", "jackson-module-kotlin"}
    )
    private static final class StateObjectWithFactoryOnNamedCompanion implements TestFields {
        @NotNull
        private static final Named Named = new Named((DefaultConstructorMarker)null);
        @NotNull
        private final String name;
        private final int age;
        @NotNull
        private final String primaryAddress;
        private final boolean wrongName;
        @NotNull
        private final Date createdDt;
        private boolean factoryUsed;

        private StateObjectWithFactoryOnNamedCompanion(String name, int age, String primaryAddress, boolean wrongName, Date createdDt) {
            this.name = name;
            this.age = age;
            this.primaryAddress = primaryAddress;
            this.wrongName = wrongName;
            this.createdDt = createdDt;
        }

        @NotNull
        public String getName() {
            return this.name;
        }

        public int getAge() {
            return this.age;
        }

        @NotNull
        public String getPrimaryAddress() {
            return this.primaryAddress;
        }

        public boolean getWrongName() {
            return this.wrongName;
        }

        @NotNull
        public Date getCreatedDt() {
            return this.createdDt;
        }

        public final boolean getFactoryUsed() {
            return this.factoryUsed;
        }

        public final void setFactoryUsed(boolean var1) {
            this.factoryUsed = var1;
        }

        public void validate(@NotNull String nameField, int ageField, @NotNull String addressField, boolean wrongNameField, @NotNull Date createDtField) {
            TestJacksonWithKotlin.TestFields.DefaultImpls.validate(this, nameField, ageField, addressField, wrongNameField, createDtField);
        }

        // $FF: synthetic method
        public StateObjectWithFactoryOnNamedCompanion(String name, int age, String primaryAddress, boolean wrongName, Date createdDt, DefaultConstructorMarker $constructor_marker) {
            this(name, age, primaryAddress, wrongName, createdDt);
        }

        @Metadata(
                mv = {1, 5, 1},
                k = 1,
                xi = 48,
                d1 = {"\u0000,\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0002\b\u0002\n\u0002\u0018\u0002\n\u0000\n\u0002\u0010\u000e\n\u0000\n\u0002\u0010\b\n\u0002\b\u0002\n\u0002\u0010\u000b\n\u0000\n\u0002\u0018\u0002\n\u0000\b\u0082\u0003\u0018\u00002\u00020\u0001B\u0007\b\u0002¢\u0006\u0002\u0010\u0002J:\u0010\u0003\u001a\u00020\u00042\b\b\u0001\u0010\u0005\u001a\u00020\u00062\b\b\u0001\u0010\u0007\u001a\u00020\b2\b\b\u0001\u0010\t\u001a\u00020\u00062\b\b\u0001\u0010\n\u001a\u00020\u000b2\b\b\u0001\u0010\f\u001a\u00020\rH\u0003¨\u0006\u000e"},
                d2 = {"Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion$Named;", "", "()V", "create", "Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion;", "nameThing", "", "age", "", "primaryAddress", "wrongName", "", "createdDt", "Ljava/util/Date;", "jackson-module-kotlin"}
        )
        private static final class Named {
            private Named() {
            }

            @JvmStatic
            @JsonCreator
            private final StateObjectWithFactoryOnNamedCompanion create(@JsonProperty("name") String nameThing, @JsonProperty("age") int age, @JsonProperty("primaryAddress") String primaryAddress, @JsonProperty("renamed") boolean wrongName, @JsonProperty("createdDt") Date createdDt) {
                StateObjectWithFactoryOnNamedCompanion obj = new StateObjectWithFactoryOnNamedCompanion(nameThing, age, primaryAddress, wrongName, createdDt, (DefaultConstructorMarker)null);
                obj.setFactoryUsed(true);
                return obj;
            }

            // $FF: synthetic method
            public Named(DefaultConstructorMarker $constructor_marker) {
                this();
            }
        }
    }

in 1.6.21 we have

@Metadata(
            mv = {1, 6, 0},
            k = 1,
            xi = 48,
            d1 = {"\u0000&\n\u0002\u0018\u0002\n\u0002\u0018\u0002\n\u0000\n\u0002\u0010\u000e\n\u0000\n\u0002\u0010\b\n\u0002\b\u0002\n\u0002\u0010\u000b\n\u0000\n\u0002\u0018\u0002\n\u0002\b\u0010\b\u0002\u0018\u0000 \u00192\u00020\u0001:\u0001\u0019B/\b\u0002\u0012\u0006\u0010\u0002\u001a\u00020\u0003\u0012\u0006\u0010\u0004\u001a\u00020\u0005\u0012\u0006\u0010\u0006\u001a\u00020\u0003\u0012\u0006\u0010\u0007\u001a\u00020\b\u0012\u0006\u0010\t\u001a\u00020\n¢\u0006\u0002\u0010\u000bR\u0014\u0010\u0004\u001a\u00020\u0005X\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\f\u0010\rR\u0014\u0010\t\u001a\u00020\nX\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u000e\u0010\u000fR\u001a\u0010\u0010\u001a\u00020\bX\u0086\u000e¢\u0006\u000e\n\u0000\u001a\u0004\b\u0011\u0010\u0012\"\u0004\b\u0013\u0010\u0014R\u0014\u0010\u0002\u001a\u00020\u0003X\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u0015\u0010\u0016R\u0014\u0010\u0006\u001a\u00020\u0003X\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u0017\u0010\u0016R\u0014\u0010\u0007\u001a\u00020\bX\u0096\u0004¢\u0006\b\n\u0000\u001a\u0004\b\u0018\u0010\u0012¨\u0006\u001a"},
            d2 = {"Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion;", "Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$TestFields;", "name", "", "age", "", "primaryAddress", "wrongName", "", "createdDt", "Ljava/util/Date;", "(Ljava/lang/String;ILjava/lang/String;ZLjava/util/Date;)V", "getAge", "()I", "getCreatedDt", "()Ljava/util/Date;", "factoryUsed", "getFactoryUsed", "()Z", "setFactoryUsed", "(Z)V", "getName", "()Ljava/lang/String;", "getPrimaryAddress", "getWrongName", "Named", "jackson-module-kotlin"}
    )
    private static final class StateObjectWithFactoryOnNamedCompanion implements TestFields {
        @NotNull
        private static final Named Named = new Named((DefaultConstructorMarker)null);
        @NotNull
        private final String name;
        private final int age;
        @NotNull
        private final String primaryAddress;
        private final boolean wrongName;
        @NotNull
        private final Date createdDt;
        private boolean factoryUsed;

        private StateObjectWithFactoryOnNamedCompanion(String name, int age, String primaryAddress, boolean wrongName, Date createdDt) {
            this.name = name;
            this.age = age;
            this.primaryAddress = primaryAddress;
            this.wrongName = wrongName;
            this.createdDt = createdDt;
        }

        @NotNull
        public String getName() {
            return this.name;
        }

        public int getAge() {
            return this.age;
        }

        @NotNull
        public String getPrimaryAddress() {
            return this.primaryAddress;
        }

        public boolean getWrongName() {
            return this.wrongName;
        }

        @NotNull
        public Date getCreatedDt() {
            return this.createdDt;
        }

        public final boolean getFactoryUsed() {
            return this.factoryUsed;
        }

        public final void setFactoryUsed(boolean var1) {
            this.factoryUsed = var1;
        }

        public void validate(@NotNull String nameField, int ageField, @NotNull String addressField, boolean wrongNameField, @NotNull Date createDtField) {
            TestJacksonWithKotlin.TestFields.DefaultImpls.validate(this, nameField, ageField, addressField, wrongNameField, createDtField);
        }

        @JvmStatic
        @JsonCreator
        private static final StateObjectWithFactoryOnNamedCompanion create(@JsonProperty("name") String nameThing, @JsonProperty("age") int age, @JsonProperty("primaryAddress") String primaryAddress, @JsonProperty("renamed") boolean wrongName, @JsonProperty("createdDt") Date createdDt) {
            return Named.create(nameThing, age, primaryAddress, wrongName, createdDt);
        }

        // $FF: synthetic method
        public StateObjectWithFactoryOnNamedCompanion(String name, int age, String primaryAddress, boolean wrongName, Date createdDt, DefaultConstructorMarker $constructor_marker) {
            this(name, age, primaryAddress, wrongName, createdDt);
        }

        @Metadata(
                mv = {1, 6, 0},
                k = 1,
                xi = 48,
                d1 = {"\u0000,\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0002\b\u0002\n\u0002\u0018\u0002\n\u0000\n\u0002\u0010\u000e\n\u0000\n\u0002\u0010\b\n\u0002\b\u0002\n\u0002\u0010\u000b\n\u0000\n\u0002\u0018\u0002\n\u0000\b\u0082\u0003\u0018\u00002\u00020\u0001B\u0007\b\u0002¢\u0006\u0002\u0010\u0002J:\u0010\u0003\u001a\u00020\u00042\b\b\u0001\u0010\u0005\u001a\u00020\u00062\b\b\u0001\u0010\u0007\u001a\u00020\b2\b\b\u0001\u0010\t\u001a\u00020\u00062\b\b\u0001\u0010\n\u001a\u00020\u000b2\b\b\u0001\u0010\f\u001a\u00020\rH\u0003¨\u0006\u000e"},
                d2 = {"Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion$Named;", "", "()V", "create", "Lcom/fasterxml/jackson/module/kotlin/test/TestJacksonWithKotlin$StateObjectWithFactoryOnNamedCompanion;", "nameThing", "", "age", "", "primaryAddress", "wrongName", "", "createdDt", "Ljava/util/Date;", "jackson-module-kotlin"}
        )
        private static final class Named {
            private Named() {
            }

            @JvmStatic
            @JsonCreator
            private final StateObjectWithFactoryOnNamedCompanion create(@JsonProperty("name") String nameThing, @JsonProperty("age") int age, @JsonProperty("primaryAddress") String primaryAddress, @JsonProperty("renamed") boolean wrongName, @JsonProperty("createdDt") Date createdDt) {
                StateObjectWithFactoryOnNamedCompanion obj = new StateObjectWithFactoryOnNamedCompanion(nameThing, age, primaryAddress, wrongName, createdDt, (DefaultConstructorMarker)null);
                obj.setFactoryUsed(true);
                return obj;
            }

            // $FF: synthetic method
            public Named(DefaultConstructorMarker $constructor_marker) {
                this();
            }
        }
    }

The problem is missing method in 1.5.31

  @JvmStatic
        @JsonCreator
        private static final StateObjectWithFactoryOnNamedCompanion create(@JsonProperty("name") String nameThing, @JsonProperty("age") int age, @JsonProperty("primaryAddress") String primaryAddress, @JsonProperty("renamed") boolean wrongName, @JsonProperty("createdDt") Date createdDt) {
            return Named.create(nameThing, age, primaryAddress, wrongName, createdDt);
        }

So if we want to support 1.5 we need to scan private inner classes with factory.

@jarekratajski
Copy link
Contributor

I proposed workaround for kotlin 1.5 in #590

@cowtowncoder
Copy link
Member

@jarekratajski I merged that work-around for now, assuming it makes sense to get tests back to green; presuming that use of private was not the main point of test.

Going forward our multi-kotlin-core CI should better catch compatibility issue. I'm sure there's room for improvement if anyone wants to take a stab -- f.ex actually compile with different Kotlin version -- but I hope even this small improvement helps with multi-version compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pr-welcome Issue for which progress most likely if someone submits a Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants