search

smallrye / smallrye-graphql

Implementation for MicroProfile GraphQL
Apache License 2.0
160 stars 92 forks source link

Fix directives building #2188

Closed RoMiRoSSaN closed 2 months ago

RoMiRoSSaN commented 2 months ago

See #2154

In wundergraph cosmo @link is optional. Apollo requires it. Quarkus does not pass a complete index containing all directives, resulting in the @linkand some others being ignored.

RoMiRoSSaN commented 2 months ago

Fell due to the fact that the Deprecated and OneOf directives were added. You can remove them, or simply remove 2 checks in SchemaBuilderTest testConcurrentSchemaBuilding

jmartisk commented 2 months ago

@RoMiRoSSaN, this change should be done on the Quarkus side, see https://github.com/quarkusio/quarkus/blob/3.15.0.CR1/extensions/smallrye-graphql/deployment/src/main/java/io/quarkus/smallrye/graphql/deployment/SmallRyeGraphQLProcessor.java#L286 - here we add any additional classes that need to be indexed.

The Quarkus integration handles the indexing, we don't do any indexing on the SmallRye side when using Quarkus. When using WildFly, we do this in https://github.com/smallrye/smallrye-graphql/blob/main/server/implementation-servlet/src/main/java/io/smallrye/graphql/entry/http/IndexInitializer.java#L98

so I think the best solution would be to

RoMiRoSSaN commented 2 months ago

OK. All directives have been added for WildFly. I will do PR for quarkus. Just a question. Is this really how it should be? After all, this is necessary for the correct construction of a graphql schema. And it turns out that this code is duplicated in two different places. Although partially the schema builder should focus on its own api (annotations, directives, etc.), and not wait for information about it from the outside. Or did I understand something wrong?

jmartisk commented 2 months ago

Quarkus works in the way that it automatically computes an index and makes it available to extensions, so SmallRye just takes the index produced by Quarkus. As for the duplication, yes we could potentially unify it into a single place (the Quarkus processor would call something inside SmallRye GraphQL) but it hasn't been considered enough of a priority :)

RoMiRoSSaN commented 2 months ago

Okay, I get it. Then I'm closing this PR. And I’ll open it in Quarkus