Skip to content

Conversation

@tinnou
Copy link
Contributor

@tinnou tinnou commented Jun 9, 2020

[DO NOT MERGE]

Description

This PR is more of a question than a PR, the code changes consist of a single unit test to reproduce the use case.

Say I have the following shortened SWAPI schema:

type QueryType {
    humanCharacter(id : String) : Character
}
interface Character {
    id: ID!
}
type Human implements Character {
    id: ID!
    homePlanet: String
}

and the following query:

{ 
   humanCharacter(id: "1003") {    --> returns Leia who is a Human
        id
        __typename
        ... on Human {          
            homePlanet
        }
    }
}

The selection set returned by environment.getSelectionSet().get().keySet() when ran from the Query.humanCharacter data fetcher contains all the fields but homePlanet. That is:

["id",
"__typename"]

I would like to understand if this omission is by design or it is a defect.

If it is by design:
We are returning only the fields in the selection set which we are sure are of the same type as the field output type. In our example, we would only return the fields from the selection set that are on the type Character.
Assuming it is by design, does it kind of defeats the purpose of looking ahead?
If say, we were making a request to a SQL system (as per the doc) using the DataDetchingSelectionSet fields as projection. Since we only get back the selected fields on Character, i.e id and __typename, wouldn't this create a hurdle as homePlanet would be missing from the SQL statement?

If it is a defect
I can update the PR to include fragments in the selection set.

The test added in this PR fails because it expected homePlanet to be included:

Condition not satisfied:

captureMap.keySet() == [ "id", "__typename", "friends", "friends/id", "friends/otherId", "otherId", "homePlanet" ].toSet()
|          |        |                                                                                              |
|          |        false                                                                                          [otherId, __typename, id, friends/otherId, friends/id, homePlanet, friends]
|          [otherId, __typename, id, friends/otherId, friends/id, friends]
[otherId:otherId: id, __typename:__typename, id:id, friends/otherId:otherId: id, friends/id:id, friends:friends {
  id
  otherId: id
}]

@tinnou
Copy link
Contributor Author

tinnou commented Jun 9, 2020

Now thinking about it, I think it must by design since if we would include the inline fragments there is a risk they would conflict:
Example:

type QueryType {
    humanCharacter(id : String) : Character
}
interface Character {
    id: ID!
}
type Human implements Character {
    id: ID!
    homePlanet: String
}
type Droid implements Character {
    id: ID!
    homePlanet: String
    make: String
}

and query:

{ 
   humanCharacter(id: "1003") {    --> returns Leia who is a Human
        id
        __typename
        ... on Human {          
            homePlanet
        }
        ... on Droid {
            homePlanet
            make
        }
    }
}

the result of environment.getSelectionSet().get().keySet() would be:

["id",
"__typename",
"homePlanet",
"make"]

and this would be ambiguous? (as we would lose information about one of the two homePlanet fields)

@andimarek
Copy link
Member

@tinnou you are right: this is intentionally as we don't have a good way at the moment to really analyze the query statically (without executing it) to the desired level you describe.

The challenge are really Interfaces and Union as they are only "realized" when executed.
We are planning to add a new feature we call "normalized Query" (see #1731 for an outdated first version).

This normalized Query would tell you that:
(QueryType.humanCharacter) has two different children (Human.homePlanet) and (Droid.homePlanet) which are both "conditional" meaning it depends on the resolved value of "humanCharacter" if they are getting executed (or mabye even both if it would be inside a List)

@bbakerman
Copy link
Member

And remember that now we have "branching" in the fields. That is you code would have to cope with "if its a Human it could be these fields and if its a Droid it could be these fields".

This is a much more complicated programming model than "these are the concrete fields" you can access.

@andimarek
Copy link
Member

@tinnou Please have a look at this test: https://github.com/graphql-java/graphql-java/pull/1963/files#diff-d9e663cf35993c9d4de118e0353dda5f to see how it will probably look like with NormalizedField:

@tinnou
Copy link
Contributor Author

tinnou commented Jun 23, 2020

@tinnou Please have a look at this test: https://github.com/graphql-java/graphql-java/pull/1963/files#diff-d9e663cf35993c9d4de118e0353dda5f to see how it will probably look like with NormalizedField:

Ok looking at the test you linked, that makes sense. So a NormalizedField from what I understand, represents a fused version of the language Field with schema aware properties (like GraphQLFieldDefinition, level in the selection set, and output type)

@andimarek
Copy link
Member

@tinnou we talked about it a bit more and we will probably incorporate the NormalizedFields into the DataFetchingFieldSelectionSet in a way. NormalizedFields will probably be more like a fundamental concept "wrapped" by DataFetchingFieldSelectionSet

@andimarek
Copy link
Member

This PR will be superseded by #2079
@bbakerman please have a look if you think copying this test here is worth it.

@andimarek andimarek closed this Oct 27, 2020
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