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

Kotlin boolean property setters are skipped if isX prefixed #495

Open
IceBlizz6 opened this issue May 9, 2024 · 11 comments
Open

Kotlin boolean property setters are skipped if isX prefixed #495

IceBlizz6 opened this issue May 9, 2024 · 11 comments

Comments

@IceBlizz6
Copy link

Hello

Another interesting Kotlin problem here.
I'm not sure if this issue should be reported to graphql-java or here.
I will try here first, let me know if it belongs elsewhere.

Look at the following code:

class MyTest {
    var isAlive: Boolean? = null
        set(value) {
            field = value
            println("Hello world")
        }
}

@GraphQLQuery
fun makeTest(input: MyTest): Int {
    return 42
}

This is comparable to a backing field with a setter and a getter.
javap displays it like this:

public final class project.MyTest {
  private java.lang.Boolean isAlive;
  public project.MyTest();
  public final java.lang.Boolean isAlive();
  public final void setAlive(java.lang.Boolean);
}

Making a call to makeTest like this
query { makeTest(input: { isAlive: true }) }

I expect it to print "Hello world", but it does not.
I believe this is because it may assign the value through the backing field directly instead.

Now i rename the field to 'alive'

class MyTest {
    var alive: Boolean? = null
	set(value) {
		field = value
		println("Hello world")
	}
}

javap displays it like this:

public final class project.MyTest {
  private java.lang.Boolean alive;
  public project.MyTest();
  public final java.lang.Boolean getAlive();
  public final void setAlive(java.lang.Boolean);
}

If i call it from GraphQL now then it will print as expected.
So it seems like boolean fields prefixed with isX will be set through backing field directly.
But boolean fields without the prefix will be set through the setter method.

I expected the setter to be used regardless of property name.

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

EDIT: I got things mixed up. The issue here is with Jackson, not SPQR. See my comment below.

In a way, this is expected... Because SPQR expects the JavaBean naming convention. At least in Java, there is no relationship between the field and the setter beyond the naming convention and a matching type. So if the naming scheme is violated, there's no clear way of knowing the relationship exists. In Kotlin, the relationship is likely maintained by different means (and I expect Kotlin reflection to be aware of it), but SPQR isn't really aware of Kotlin, so the issue described above occurs.

What I had always been advocating for is a Kotlin aware module, (that would ideally be written in Kotlin, although Java works fine too) that takes care of all Kotlin specific behaviors. It would likely be a small module, without much complications. But. It would ideally be community maintained, as I'm not all that familiar with Kotlin. If you'd care to contribute such a thing, just with this one fix, I'd happily take it as a start.

@IceBlizz6
Copy link
Author

Would such a module for SPQR solve this specific case?
or is this a problem with graphql-java?

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

Ignore (almost) everything I said. I got confused 🤦‍♂️
This is about Jackson, nothing else. You just have to add jackson-module-kotlin. That makes Jackson understand Kotlin properties correctly. The root issue is the naming convention, as I described above, but in Jackson, not in SPQR.

@IceBlizz6
Copy link
Author

jackson-module-kotlin has already been added to my project.
How do i apply it to graphql-spqr?

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

Huh. SPQR normally calls objectMapper.findAndRegisterModules() so it should find it automatically.
But I don't know how or what is customized etc. Try:

generator.withValueMapperFactory(JacksonValueMapperFactory.builder()
             .withPrototype(new ObjectMapper()) //customize ObjectMapper as you please, e.g. add a module
             .build())

If you're using SPQR with Spring, you might want to use the Spring-provided ObjectMapper instance (I don't actually advise this, but it might be a good test).

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

If that doesn't help... then Jackson is weird about Kotlin?
The thing to note here is that SPQR delegates (almost) all input deserialization to Jackson. So Jackson decides how to instantiate the object and populate its fields. No clue why it would decide against using the setter though 🤷‍♂️ Can you try messing with your class and Jackson directly, and see how it behaves?

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

I tired this quickly* and I indeed do not get "Hello world" printed:

class MyTest {
    var isAlive: Boolean? = null
        set(value) {
            field = value
            println("Hello world")
        }
}

fun main(args:Array<String>) {
    val clazz: Class<MyTest> = MyTest::class.java
    ObjectMapper().registerModules(KotlinModule.Builder()
                   .enable(KotlinFeature.KotlinPropertyNameAsImplicitName).build())
        .readValue("{ \"isAlive\": true }", clazz)
}

So either I did something wrong (did I?) or Jackson did...

*not actually quickly 😶

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

Actually... the more I look at this the more sense it makes.
Jackson isn't doing anything wrong. Without KotlinPropertyNameAsImplicitName, it sees isAlive and decides the property is called alive, as per JavaBeans convention. With KotlinPropertyNameAsImplicitName the property is called isAlive but such a property doesn't have a setter. Jackson is simply following the spec, just like SPQR. So you'd have to do some further customizations if to achieve what you want. I can help you do that, but... I first have to ask: why go against the JavaBeans spec in the first place? It exists for a reason and tools and libraries are expecting it to be respected. Why go against the grain here?

@kaqqao
Copy link
Member

kaqqao commented May 9, 2024

The easiest you can do is:

class MyTest {

    var isAlive: Boolean? = null
        @JsonSetter("isAlive")
        set(value) {
            field = value
            println("Hello world")
        }
}

fun main(args:Array<String>) {
    val clazz: Class<MyTest> = MyTest::class.java
    ObjectMapper() //No KotlinPropertyNameAsImplicitName
        .readValue("{ \"isAlive\": true }", clazz)
}

It prints "Hello world" as expected. You could probably achieve the same with @GraphQLInputField(name = "isAlive").
There are also a couple of ways to ensure this is applied everywhere. I can whip something up if you're sure that's what you need.

@IceBlizz6
Copy link
Author

I actually tried this myself some time ago and reached the same conclusion.
You will get a "Hello world" if you rename it to 'alive' by dropping the isX prefix.
Which seems to confirm that the bug is indeed in jackson.
Thank you for your assistance.
I will bring the issue to them.

@IceBlizz6
Copy link
Author

Actually didn't see your last 2 messages.

So i think the JavaBean convention probably makes sense for Java.
But it doesn't seem to play well with Kotlin here.
It makes more sense for me to write a property in Kotlin named 'isAlive' rather than 'alive'.
Kotlin doesn't need to specify explicit getter and setter methods as that is handled by Kotlin compiler.
I normally don't think about or care about how this is translated into java's getters and setters.
If anything maybe it is the Kotlin language designers who should have tried to make Kotlin compatible with JavaBeans convention.

Your @JsonSetter solution is interesting, i may use that as a fallback.
But before that i will try to contact jackson maintainers to see what they have to say.

@IceBlizz6 IceBlizz6 reopened this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants