smallrye-graphql
smallrye-graphql copied to clipboard
smallrye
Client support Kotlin annotation for NotNull
The server supports the Kotlin annotation for NotNull, the client does not.
Maybe it is similar simple to the server side https://github.com/smallrye/smallrye-graphql/issues/1109
I think we would need to do something like this, I not too familiar with the client code.
io.smallrye.graphql.client.impl.typesafe.reflection.ParameterInfo
private String optionalExclamationMark(TypeInfo itemType) {
if (itemType == this.type) {
// Work around a weird GraalVM native mode behavior in which the object returned by
// parameter.getAnnotatedType() does not include annotation metadata. So if we're
// determining whether this Parameter itself is nullable, introspect the Parameter
// instance itself rather than its AnnotatedType (which is what the
// `itemType.isNonNull` method would do).
return this.type.isPrimitive() || parameter.isAnnotationPresent(NonNull.class) || parameter.isAnnotationPresent(NotNull.class) ? "!" : ""; <-------------------
} else {
return itemType.isNonNull() ? "!" : "";
}
}
Furthermore, we would need the jetbrains annotations as a new dependency. @jmartisk @t1 are there any more places where non-null is handled?
I did not follow this closely, but please use Jandex and do not include the Jetbrains dependency (if you use Jandex you can use String values to reference the class)
On the server side we did it with jandex, but either I overlooked it or there is no jandex on the client side.
Yeah we don't have Jandex on the client side (at least not yet). But I assume you can still try to call Class.forName() to obtain a class reference, and swallow the exception it might throw if the annotation's class isn't there.
Would be ok for me. If this is the only place, I would do a pr on the weekend, probably.
OK, I though we are using Jandex on the client. We really need to do that. Is there an issue for that ?
We have https://github.com/smallrye/smallrye-graphql/issues/873 for build-time scanning in the client
Would be nice if client and server could use the same implementation, always a bit ugly if they behave differently.
Should I still do a PR for this issue?
Should I still do a PR for this issue?
Of course!
Some parts of the schema-builder will be reusable by the client, but not that much. The client doesn't build a full GraphQL schema (it doesn't even depend on graphql-java), it just converts interfaces and classes directly into GraphQL queries. So "using the same implementation" is only possible to some extent. And the schema builder will have to be changed quite heavily if we want to reuse it, because right now it's really built for creating a server-side GraphQL schema, which is not what the client wants. I vaguely remember trying to implement this change in the past, but I gave up, it was harder than initially expected.
Hm, I missed that the Jetbrains Annotations are RetentionPolicy.CLASS, this will have to wait until the client uses jandex.