1479 client union

[t1]

Support @Union in typesafe client.

[jmartisk]

This adds jandex into the runtime dependencies, which is a big no-no (it would bloat the resulting artifact and also probably not work at all in native mode). Could this be instead integrated with the newer approach that avoids runtime reflection (the client/model module which builds a complete representation of the client API at build time from Jandex)? @mskacelik can provide some explanations around it I assume. The original approach where the client API is scanned using reflection at runtime is to be deprecated.

[t1]

I've seen that, but it currently just looks like a PoC. Is there any documentation? I'd be very interested in understanding where we're going here!

[jmartisk]

I've seen that, but it currently just looks like a PoC. Is there any documentation? I'd be very interested in understanding where we're going here!

It's not quite PoC, it's already the default behavior with Quarkus 3.9, but @mskacelik is working on "documenting" it as his university thesis. He's going to add some notes here in a bit. Or you can connect on zulip too

[t1]

Or you can connect on zulip too

I'd rather not use too many communication channels... let's keep it here 😁

[mskacelik]

Hi @t1, Two new modules have been added to the Type-Safe client (Quarkus): smallrye-graphql-client-model and smallrye-graphql-client-model-builder. 
The schema and schema-builder on the server side heavily inspired this approach.

The model module works both in the build-time and runtime. In this module there is the ClientModel which represents the specific scanned API @GraphQLClientAPI. It contains a map of all the operations, with the key being the method signature and the value being the query generated by Jandex scanning.
There is also the class ClientModels, which are all the scanned APIs. (Map of configKeys and ClientModel instances) The bytecode recorder then stores this ClientModels class, and its instances are then used in the Proxy class during the GraphQL request building.


For the model-builder, this module works only in build-time and builds the undermentioned ClientModel(s).
 ClientModelBuilder scans all the client APIs and generates their operation queries. 
The scanning technology used here is Jandex, and it should only be used in this module during build-time.
 The code in the model module is very similar to the reflect approach in the runtime, as the QueryBuilder class builds the queries. It also contains model classes representing fields, parameters, operations, and types (as in the operation's return type). But unlike the reflect approach, it uses Jandex scanning. 
 


With that in mind, this code is only responsible for doing the “serialization” and creating the GraphQL request for the proxy. “Deserialization” and handling of the GraphQL response stays the same (using the java.lang.reflect APIs).

You can try debug things in Quarkus version 3.9.4.

I hope that makes it more clear :D If you have any other questions let me know.

[jmartisk]

I have to add that it would be good to keep the current implementation (extending the original reflection-based approach) too for now, but we have to get rid of the Jandex dependency. I'm not yet completely sure how best to do that, but it sounds like it will have to be implemented in one of the build-time modules (client/model) and recorded via the SmallRyeGraphQLClientRecorder (or set as some sort of static field for non-Quarkus cases)

[t1]

That's a really cool idea. I love it. But it's still quite a way to go. As I need the implementations of a super type for the Union, I created a TypeModels class to hold that data.

But that's really just an intermediary step. In the long run, I think the TypeModel, etc. should become interfaces in the model module, and be merged with old reflection-based TypeInfo etc. Having those unified would make things much easier to understand.

[mskacelik]

I would add a small note.


The type-safe client currently has two implementations, one especially for Quarkus and the other for different runtimes (WF,…).
 
 
1) New Quarkus implementation: This implementation uses the Client Model — using the Jandex API for scanning — which is then recorded as a bytecode by the SmallRyeGraphQLClientRecorder.
 This makes it so that the client models — and, most importantly, queries — are only created during the build-time, making it very efficient.

 
 
2) The former implementation uses the java.lang.reflect API. This approach is only there for other non-Quarkus (unless config enables it) runtimes and was left as it is. 
 


So, if there is a need to add changes to the QueryBuilder, there need to be changes in both implementations of query building… (at least for now). 


 




If you want, I can handle the Quarkus approach, and you can focus on the classic reflect one. I would gladly do so :D 


 




Also, regarding the testing. Since the ClientModel approach needs to have all the model classes indexed. But we did not want to duplicate all the TCK tests, so we just created a new TypesafeTckClientModelSuite class which is configured to execute the tests with the new implementation (while the TypesafeTckSuite tests with the older implementation). So you can create the TCK tests using the same patterns as you are used to.

[t1]

@mskacelik: would it be possible to have only a single query string generator, but two meta data providers, one for Quarkus and one via reflection? The former would run at build time, while the latter at startup (or maybe on-demand). And when a request has to be built, both would use the same static query storage with the method as a key.

[t1]

I have another problem with this approach: If I have a Java SE client, e.g. in a test, the Jandex has not been scanned. I would love to use sealed classes for this instead, but they are JDK 17, not 11 😿

[jmartisk]

I have another problem with this approach: If I have a Java SE client, e.g. in a test, the Jandex has not been scanned. I would love to use sealed classes for this instead, but they are JDK 17, not 11 😿

Yeah I fear we will have to, for now, just keep supporting both approaches (ClientModels and reflection) until we find a reasonable way way to create a ClientModel in pure Java SE apps, but without making Jandex a required dependency - so, probably a separate artifact that serves as the tool to create it...

[t1]

What do you think of this idea:

1. We create an interface for the VertxTypesafeGraphQLClientProxy#queryCache named OperationStore. It provides the query string for a MethodKey (which should BTW really be immutable and become a record in the future). MethodInvocation#getKey would become obsolete. We pass that interface as a constructor parameter instead of the ClientModel.
2. The VertxTypesafeGraphQLClientBuilder either passes an adapter for the ClientModel that fetches the query string from the map, or a reflection-based adapter that calculates and caches the operation string dynamically. I will see how we can do this in a good way.
3. Now comes the interesting part: we move all the classes that are used to build operation strings via reflection into a new module model-reflection (or so). This would be excluded in Quarkus. There is no standard for the typesafe client, so we we are free to go ahead and compile this module with Java 17, so we can use reflection on sealed classes to provide the subclasses of a Union instead of Jandex.

Can you already see any major flaws or should I give it a try, so we can talk about real code?

[jmartisk]

Hm WildFly still compiles with Java 11 and supports applications running on it, so by switching something to 17 we would break it :(

Also regarding the sealed interfaces: Do I understand it that you would require the union interface to be sealed? What if it's not? Or, what if it has any implementations that the user didn't intend to be part of the union? Maybe things would be much easier if we required the user to explicitly specify the list of classes that should be part of the union. Then you wouldn't need jandex to find the implementations either.

By the way, we shouldn't exclude the reflection-based approach in Quarkus (at least not yet). Firstly, we've just added a config property that reverts to the reflection-based behavior, so until we've had enough bake time and we're sure that the ClientModel approach works well, we should keep it.. Secondly, the reflection approach is still needed in case that somebody creates clients dynamically using the builder at runtime - we don't have the pre-built ClientModel available in that case.

[t1]

Current WildFly versions still support Java 11, true. But they've also been supporting Java 17 for a while; I can't recall since when exactly... do you happen to know? I personally could live with a new version breaking on Java 11. An alternative would be to extract that part to another, optional module, and load it, e.g., with a ServiceLoader.

I think a very elegant way to specify the allowed subclasses is sealed interfaces. We could use a different mechanism, but that would be cumbersome, add additional APIs or configs outside of the code. Sealed interfaces would also make switching on the different subtypes super nice. So I think this would be easier and more aligned to where Java is going.

we shouldn't exclude the reflection-based approach in Quarkus

Okay. And it also still supports Java 11. So we have the same issue as we have with WildFly. So I guess we really need that optional Java 17 module.

[jmartisk]

The whole SmallRye org uses one common strategy and build+release system that runs with JDK 11 (and it will stay like this until WildFly moves to 17+), I don't see it worthwhile to break it for one feature that needs JDK 17 :/ Also the need to use sealed interfaces seems to put quite a burden on application developers... I'd go with the Jandex approach that scans it within the model-builder module, or have the user explicitly list the classes that should be part of the union...

[t1]

Okay, so we'll stick to 11 for now. I still think that sealed classes would not put a burden on developers, but if we support both options (because it will be hard to remove an option later), we'll see what people actually use.

I've just pushed step one and two of my suggestion: I've introduced the OperationRepository. Would you like to take a look before I continue with extracting the reflection-based operation building?

[t1]

I'd go with the Jandex approach that scans it within the model-builder module

IIUC, the model-builder is only used in Quarkus, and it has a dependency on Jandex, so we can't use it, can we?

Or, to put it in other words: even if we introduce a new module, as long as it is not optional, it can't depend on Jandex. So we'll have to introduce some sort of API, maybe an annotation like @UnionTypes({Foo.class, Bar.class})?

[jmartisk]

IIUC, the model-builder is only used in Quarkus, and it has a dependency on Jandex, so we can't use it, can we?

Currently, it is only in Quarkus, but we'd like to integrate it in WildFly too, same as the server-side schema-builder is used there too. I'm just not yet sure when that will be (unless you want to do it). It should, at deployment time, build a ClientModels instance and register it (via a CDI extension most likely) to be available to the app, just like Quarkus does it.

I do think that @UnionTypes({Foo.class, Bar.class}) with an explicit list of implementations would be a good way, avoiding the complexity of scanning the app (including dependencies, to make it worse) and the risk that there will be some implementations that are not meant to be members of the union.

[t1]

The WildFly Feature Pack doesn't find the MethodKey class. How can we fix that?

[t1]

BTW: probably WF 26 will (probably) drop support for Java 11, because it will (probably) support Jakarta 11, which requires Java 17. Maybe we should wait with the Unions for that to happen?