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

Handling federation _entities queries without creating @Query #2180

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

RoMiRoSSaN
Copy link
Contributor

See #2110

Copy link
Collaborator

@t1 t1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. I just don‘t quite get, what the resolvers are needed for. I’m happy to hear that you want to add documentation, Javadoc and tests.

addQueries(schemaBuilder);
GraphQLObjectType resolversType = Config.get().isFederationEnabled()
? buildResolvers()
: GraphQLObjectType.newObject().name("Resolver").build();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I get this right, the resolversType is only used in the if block below, so we could move it there and simplify the trinary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to federation builder block

@@ -124,7 +125,7 @@ public static GraphQLSchema bootstrap(Schema schema) {
}

public static GraphQLSchema bootstrap(Schema schema, boolean skipInjectionValidation) {
if (schema != null && (schema.hasOperations())) {
if (schema != null && (schema.hasOperations()) || Config.get().isFederationEnabled()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (schema != null && (schema.hasOperations()) || Config.get().isFederationEnabled()) {
if (schema != null && schema .hasOperations() || Config.get().isFederationEnabled()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong here (...). I fixed it

Comment on lines 228 to 230
// hack! For schema build success if queries are empty.
// It will be overrides in Federation transformation
addSdlQueryToSchema(schemaBuilder, queryRootType);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I‘m not sure, if I got this right: graphql-java federation transformation is going to overwrite the _service query that we add here? It‘s just a dummy that is required to make the schema builder happy? Maybe it would make the code flow more understandable, if the method was named addDummyQuery. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you right, i rename this

@RoMiRoSSaN
Copy link
Contributor Author

RoMiRoSSaN commented Sep 9, 2024

I found this #1585, #521
Here how I understand was annotation FederationSource, but now smallrye graphql not contains it. The logic there intersects with my PR. For example, in NodeJS federation router itself sends requests about what and what fields it needs. And there it can be any object (list of parameters).
At the moment I have not yet come up with how creeate a universal wrapper object (method must have all required 'primitive' params. see test). With these changes, the router can obtain the required data. And most importantly, queries are not published to the schema. Exmaple, service extends external type other service

extend type Type key('id') {
  id: String external
  name: String external
  value: String requries('name')
}

If you request a field value will be sended request like

_entities(representations: { id: "id", name: "name", __typename: "Type" }) {
  __typename
  ... on ExtendedType {
    value
  }
}

value extended field. It turns out you need to make 2 requests, by id (default and may be query), and id and name (value requires field name).
And if you publish so many queries to the schema, it can become very large. And it may not be necessary for requests to be available exclusively to the federation router.
It can be useful only for federation

@RoMiRoSSaN RoMiRoSSaN requested a review from a team as a code owner September 9, 2024 10:29
@RoMiRoSSaN RoMiRoSSaN marked this pull request as draft September 9, 2024 10:29
@RoMiRoSSaN
Copy link
Contributor Author

RoMiRoSSaN commented Sep 9, 2024

It would probably be nice to have such an option.

record Vars(String id, String name){}
@Resolver
Product find(Vars vars){
   ...
}

and allow pass queries like

_entities(representations: { id: "id", name: "name", __typename: "Product " }) { __typename  ... on ExtendedType { id name value } }

_entities(representations: { id: "id", __typename: "Product " }) { __typename  ... on ExtendedType { id} }

in one method. or maybe it is wrong idea.

@RoMiRoSSaN
Copy link
Contributor Author

Unfortunately, I couldn’t figure out how to make one general type. Too much magic

Needs simple changes in quarkus (add resolvers operations here)

@RoMiRoSSaN RoMiRoSSaN marked this pull request as ready for review September 12, 2024 12:29
@RoMiRoSSaN RoMiRoSSaN marked this pull request as draft September 12, 2024 12:30
@RoMiRoSSaN
Copy link
Contributor Author

Hi @jmartisk, @t1. What do you think about it?
For example, This example in JS. And in Spring

@jmartisk
Copy link
Member

Hi, sorry a bit busy, but I'll try to review at the beginning of next week...

@RoMiRoSSaN RoMiRoSSaN changed the title Handling federation queries without creating @Query Handling federation _entities queries without creating @Query Sep 12, 2024
@RoMiRoSSaN RoMiRoSSaN marked this pull request as ready for review September 12, 2024 12:48
@RoMiRoSSaN RoMiRoSSaN marked this pull request as draft September 19, 2024 17:08
@RoMiRoSSaN
Copy link
Contributor Author

This will require changes that are present in #2184

@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk Hi. I’ve probably already tired you, but please look at this PR too.
In short, this will allow to avoid creating a bunch of @Query for federations, while calmly expanding the types.
For example, there is a service users that implements the User type. There is a status-user service that extends the User type with the blocked field. With the current approach, in the second service requires creating @Query which will be displayed in the general schema, although such a request may not carry any logic separately.

For this PR, it also wouldn’t hurt to add quarkus to the @Resolver index (in the same branch that you did for 2.11. I can do it later when we discuss this PR)

@jmartisk
Copy link
Member

Ok, would you also please create a PR to my Quarkus branch (https://github.com/jmartisk/quarkus/tree/smallrye-graphql-2.11) with the Quarkus-side change and preferably also a simple test that runs with Quarkus?

@RoMiRoSSaN
Copy link
Contributor Author

RoMiRoSSaN commented Sep 27, 2024

Added changes and a couple of simple tests to your fork and create PR. After accepting the PR for quarkus, need restart the pipeline here, to be sure everything is ok.

@jmartisk
Copy link
Member

Ok, merged the Quarkus part, now re-running the CI here... Thanks!

@RoMiRoSSaN RoMiRoSSaN marked this pull request as ready for review September 30, 2024 10:18
@RoMiRoSSaN RoMiRoSSaN marked this pull request as draft September 30, 2024 10:46
@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk It seems like I did everything, made all the necessary corrections.

@RoMiRoSSaN RoMiRoSSaN marked this pull request as ready for review September 30, 2024 13:53
@jmartisk jmartisk merged commit a076d7d into smallrye:main Oct 1, 2024
5 checks passed
@jmartisk
Copy link
Member

jmartisk commented Oct 1, 2024

Thanks!

@RoMiRoSSaN RoMiRoSSaN deleted the federation-resolvers branch October 1, 2024 11:12
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

Successfully merging this pull request may close these issues.

3 participants