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 support #72

Closed
szantogab opened this issue Feb 22, 2018 · 22 comments
Closed

Kotlin support #72

szantogab opened this issue Feb 22, 2018 · 22 comments

Comments

@szantogab
Copy link

szantogab commented Feb 22, 2018

Hi!

Awesome library I love it, I even dropped graphql-java-tools and graphql-spring-boot-started for it.

However, it would be awesome to have Kotlin support in the library. For example, it could automatically pick up annotations like: org.jetbrains.annotations.NotNull, which tells spqr which field is nullable and which is not.

Right now, I can't even get the @GraphQLNonNull annotation to work in Kotlin:

@GraphQLQuery(description = "Logs a user in.")
    fun login(@GraphQLArgument(name = "username") username: @GraphQLNonNull String, @GraphQLArgument(name = "password") password: @GraphQLNonNull String): LoginResponse = loginService.login(username, password)

What can go wrong here? Also do you plan to support Kotlin?

My final question would be that, is it possible with this library that I don't need to specify @GraphQLArgument(name = "username") in the example mentioned above?
It could automatically infer the name from the name of the variable with the -parameter javac option enabled.

Thanks,
Gabor

@kaqqao
Copy link
Member

kaqqao commented Feb 22, 2018

Thanks for the kind words!

@GraphQLArgument is already optional, and with the -parameters javac option, it already works as you explained.

I'll add the support for org.jetbrains.annotations.NotNull in the next release as it's a tiny change.
@GraphQLNonNull can also be replaced with @javax.annotation.Nonnull. But I have no idea why it wouldn't work in Kotlin... I'll try to figure it out.

While I would certainly love for SPQR to work with Kotlin, I have no capacity to support it... I do not know enough about it nor have the time to go into it without the rest of the project suffering, as I'm currently the only active developer. Are there any other problems in Kotlin currently?

It's important to note though that almost everything about SPQR is customizable and extensible externally, so you could actually add support for org.jetbrains.annotations.NotNull yourself pretty easily, by implementing a custom TypeMapper. (No need as I'll extend the existing one, but just as an example).

Also, there's a SPQR-powered Spring Boot starter in the works that will be much more convenient to use than anything else out there :)

@szantogab
Copy link
Author

Thanks for your fast response!

I checked the bytecode that Kotlin generates, and I can clearly see that it implicitly annotates non-null types with the org/jetbrains/annotations/NotNull , and nullable types with org/jetbrains/annotations/Nullable annotation.

If you could add these annotation to SPQR's schema-gen it would be fantastic.

As for the SPQR-powered Spring Boot starter, it sounds awesome, can't wait for it.
Do you have a release plan for this, and also for this Kotlin modification?

Thanks!

@kaqqao
Copy link
Member

kaqqao commented Feb 23, 2018

I'm planning the next release by the end of the week.
As for Spring Boot starter, the first beta release will probably come out somewhere in late March.

Btw, if you're versatile with Kotlin, maybe you can help troubleshoot the issue with @GraphQLNonNull? The first place to put a breakpoint would likely be inside AnnotatedArgumentBuilder#38 and then in NonNullMapper.

@szantogab
Copy link
Author

I've put a breakpoint in both classes, and I guess the problem can came from the fact that the @GraphQLNonNull annotation needs to be placed on a type and not the field?

I'll try to explain, please have a look at the following code:

class User(
        id: Long? = null,

        @Column(nullable = false)
        var firstName: @GraphQLNonNull String = "")

This is a Kotlin class that has a firstName property, so a getter and a setter method will be created by Kotlin. The @GraphQLNonNull annotation needs to be placed on the String itself, not the getter and setter methods. I could place the annotation on the methods like this

        @Column(nullable = false)
        @get:GraphQLNonNull
        var firstName: String = ""

or on the field directly like this:

        @Column(nullable = false)
        @GraphQLNonNull
        var firstName: String = ""

But I can't do it because the annotation's target is set to @Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})

What I can see from NonNullMapper's code is that it only finds the getter and setter methods, and it looks for the annotation on the method itself.

Is this possible or I am completely wrong here?

@szantogab
Copy link
Author

As far as the parameter names are concerned, now I've got it to work without the @GraphQLArgument annotation by passing the -java-parameters flag to the Kotlin compiler.

In Gradle you can do it like this:

compileKotlin {
	kotlinOptions {
		freeCompilerArgs = ["-Xjsr305=strict", "-java-parameters"]
		jvmTarget = "1.8"
	}
}

So one mystery solved :)

@kaqqao
Copy link
Member

kaqqao commented Feb 23, 2018

Aaah, nice analysis! I think you're on the spot with the annotation target mismatch. Yes, @GraphQLNonNull is TYPE_USE meaning it needs to be on the type itself and not the field/method. I guess I can add annotation targets to it... but I wonder if that's the best approach.
I see Kotlin has TYPE target signifying type usage. So I'd expect there to be a way to place an annotation on the type for fields.
I know next to nothing about Kotlin, so this is very likely wrong, but can you do something like:

var firstName: @GraphQLNonNull String = ""? Or is that just nonsense?

The reason it's a TYPE_USE annotation is that you should be able to do things like List<@GraphQLNonNull String> or more complicated constructs...

Also, thanks for the help :)

@szantogab
Copy link
Author

Well the syntax you proposed is syntactically correct, but unfortunately NonNullMapper does not recognize it.

@kaqqao
Copy link
Member

kaqqao commented Feb 23, 2018

I experimented a bit... The annotation is nowhere to be found with Java reflection. Not on the field return type, not on the getter return type and not even on the constructor parameter. It's only available in KMutableProperty and KType via Kotlin reflection... So I unfortunately don't think there's anything I can do :(

@szantogab
Copy link
Author

szantogab commented Feb 23, 2018

So the only possible solution is to write a Kotlin extension to SPQR and provide a Kotlin implementation of NonNullMapper ? If so, or you can point me in the right direction where to go with this, I would gladly participate in this project :)

@kaqqao
Copy link
Member

kaqqao commented Feb 23, 2018

That's good to hear :) Actually, I think it's the AnnotatedArgumentBuilder that needs a Kotlin specific replacement, because that's where the reflection is performed to determine the type. But... even if this is possible, it will be very difficult, as SPQR works with AnnotatedTypes all over the place and there seems to be no way to obtain that from Kotlin's KType nor vice versa. It might be possible to manually reconstruct AnnotatedType (using GeAnTyRef's TypeFactory) and use JvmClassMappingKt or KotlinTypeFactory (if this is even public API) to go the other direction, but it will not be easy :(

@kaqqao
Copy link
Member

kaqqao commented Feb 23, 2018

I opened a question on SO and it got a very interesting answer... At least partially, this seems to be a bug in Kotlin :( And if you dig through the related issues, it just gets scarier...

@szantogab
Copy link
Author

Wow, that's weird. So once they fix this in Kotlin it is going to work as intended?

@kaqqao
Copy link
Member

kaqqao commented Feb 27, 2018

If KT-13228 would get fixed, yes. But I wouldn't hold my breath as it's been ignored for 2 years now... It doesn't seem like JetBrains actually cares for Kotlin outside of Android.

I'll think some more if anything can be done in SPQR to alleviate the problem.

@kaqqao
Copy link
Member

kaqqao commented Mar 4, 2018

I added support for org.jetbrains.annotations.NotNull to NonNullMapper, but I don't think it will do anything until the issues above are dealt with...
For now, I'm afraid you'll have to write out the getters and setters with the annotated types... I'm sorry for such an unsatisfactory conclusion, but I can't do anything further on my end :(

@williamboman
Copy link

I've published a POC for Kotlin support (it leverages runtime reflection) https://github.com/williamboman/graphql-spqr-kotlin. (I don't recommend using it for any production code though, but great fit for hobby projects if you want to use Kotlin + graphql-spqr).

@sompornp
Copy link

I've published a POC for Kotlin support (it leverages runtime reflection) https://github.com/williamboman/graphql-spqr-kotlin. (I don't recommend using it for any production code though, but great fit for hobby projects if you want to use Kotlin + graphql-spqr).

Why not recommended to use this on production?

@williamboman
Copy link

Why not recommended to use this on production?

It's more a POC which I've only tested with basic implementations. I also don't recommend doing it because of:

Due to a 3 year old bug, Kotlin properties produce incorrect AnnotatedTypes on which most of SPQR is based.

That's just my advice, feel free to do with it what you will!

@kaqqao
Copy link
Member

kaqqao commented Dec 10, 2019

I don't understand what @williamboman 's code is doing, but with this bug in Kotlin fixed it should be possible to implement a TypeMapper in Kotlin that uses Kotlin reflection to discover non-nullness of properties. I believe that's all that's needed.

@kaqqao
Copy link
Member

kaqqao commented Dec 10, 2019

Actually... now that I think about it, it should probably be a custom ResolverBuilder and ArgumentBuilder as well, but all can be simple. If one of you decides to experiment, contact me and I'll support you the best I can, and maybe we formalize the result into as Kotlin-compatibility module.

@williamboman
Copy link

Actually... now that I think about it, it should probably be a custom ResolverBuilder and ArgumentBuilder as well, but all can be simple. If one of you decides to experiment, contact me and I'll support you the best I can, and maybe we formalize the result into as Kotlin-compatibility module.

Would the SchemaTransformer be an incorrect API to use for these purposes?

@stengvac
Copy link

stengvac commented Jul 24, 2020

For Kotlin 1.3.70+ as mentioned in KT-13228 when added the compiler argument -Xemit-jvm-type-annotations then @GraphQLNonNull works fine.

@ThallesP
Copy link

is @NotNull working? When using the flag mentioned above with @GraphQLNonNull it works, but @NotNull doesnt. @kaqqao

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

6 participants