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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

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

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.

5 participants