Support in DataFetchingFieldSelectionSet for Unions and Interfaces #2079
Conversation
…d-to-dfe # Conflicts: # src/main/java/graphql/util/WeakMemoizedSupplier.java
…ield-support # Conflicts: # src/main/java/graphql/util/FpKit.java
|
graphql.schema.DataFetcherSelectionTest still needs to be fixed |
…- this is another one that should die
bbakerman
left a comment
bbakerman
left a comment
There was a problem hiding this comment.
I think this is now ready for review
| protected Supplier<NormalizedField> getNormalizedField(ExecutionContext executionContext, ExecutionStrategyParameters parameters, Supplier<ExecutionStepInfo> executionStepInfo) { | ||
| Supplier<NormalizedQueryTree> normalizedQuery = executionContext.getNormalizedQuery(); | ||
| return () -> normalizedQuery.get().getNormalizedField(parameters.getField(), executionStepInfo.get().getFieldContainer(), executionStepInfo.get().getPath()); | ||
| } |
There was a problem hiding this comment.
All made as Supplier to ensure its super lazy - lets not pay any cost unless some one asks for this.
| /** | ||
| * @return a map of the {@link graphql.schema.GraphQLFieldDefinition}s for each field in the selection set | ||
| */ | ||
| Map<String, GraphQLFieldDefinition> getDefinitions(); |
There was a problem hiding this comment.
This is part of the breaking changes - these no longer make sense because we have multiple SelectedFields to a named path
| name | ||
| appearsIn | ||
| friends(separationCount : 5) { | ||
| friends { |
There was a problem hiding this comment.
This was actually illegal query text - now that we run the whole thing, it needed to be fixed
| DataFetchingFieldSelectionSet selectionSet = null | ||
| DataFetcher humanDF = { DataFetchingEnvironment env -> | ||
| selectionSet = env.getSelectionSet() | ||
| return StarWarsData.humanDataFetcher.get(env) |
There was a problem hiding this comment.
We use side effect to get the inner selection set.
|
|
||
| (selectionSet.definitions['name'].getType() as GraphQLNonNull).getWrappedType() == Scalars.GraphQLString | ||
| (selectionSet.definitions['friends'].getType() as GraphQLList).getWrappedType() == starWarsSchema.getType('Character') | ||
| (selectionSet.definitions['friends/friends'].getType() as GraphQLList).getWrappedType() == starWarsSchema.getType('Character') |
There was a problem hiding this comment.
These have been removed and hence so have the tests
|
@bbakerman thanks for improving the PR. Here are some things that are problematic. Overall this has reduced the custom code we need thanks to the ability to find fields with We noticed that the transitive nature of type User {
name: String
address: String
friend: User
}
{
user(id: "steve") {
name
friend {
name
address
}
}
}The selection set for the user query would contain all fields (also the nested ones) and if we try to use the type to tell which fields to take, we end up with [name, friend, address] when what we really want is to be able to tell the two “users” apart and have [name, friend] and later [name, address]. |
|
@bbakerman We've switched to using |
|
@bbakerman @andimarek another friendly ping about this PR. Are there any leftovers or things we can do or help out with in order to get this PR merged? We really need union support in graphql-java 🙏 |
This improves the DataFetchingFieldSelectionSet support so that conditional composite types such as Unions can now be supported.
Its a breaking change because we can now get multiple
SelectedFields in for a given addressing name. ThereforegetFieldno longer makes sense nor do the maps to 1:1 arguments and field definitiionsThis supersedes the PR #1963