DataFetchingEnvironment should always have a way to get field sub-selection #377

kaqqao · 2017-04-16T12:34:33Z

Currently, DataFetchingEnvironment has no way to fetch only the required fields if the query uses fragments.
DataFetchingEnvironment#getFields currently has populated SelectionSet only for Field and InlineFragment, while it is always blank for Fragment. Additionally, fragment definitions are inaccessible as they're only kept in ExecutionContext.

bbakerman · 2017-04-27T00:16:06Z

One other concern I had was around query proxiying.

Imagine you have this query

  query {
         handledByGraphqlProxyServer1 {
                   name
                   address {
                       street
                       postcode
                  }
                  age
        }
       handledByGraphqlProxyServer2 {
                 height
                 weight
      }
}

Now imagine the backing data fetcher of handledByGraphqlProxyServer1 is in fact going to call out to a remote graphql server and pass on the inner query part

eg: effectively

 query {
      name
      address {
             street
             postcode
      }
      age
}

So how could we implement this? I dont think today we can.

Because the selection set (including deeply beyond this objecy) is available to the data fetcher.

bbakerman · 2017-05-02T02:17:45Z

I looked further into this issue and I think this is available now

@apottere added #392

I confirmed this locally using this little wrapper class

 class DataFetcherProxy implements DataFetcher {
    final DataFetcher delegate

    DataFetcherProxy(DataFetcher delegate) {
        this.delegate = delegate
    }

    @Override
    Object get(DataFetchingEnvironment environment) {
        String fields = printFields(environment.getFields())
        String fragments = printFragments(environment.getFragmentsByName())
        System.out.printf("\n\nfields :\n %s",fields)
        System.out.printf("\nfragments :\n %s",fragments)
        return delegate.get(environment)
    }

    String printFragments(Map<String, FragmentDefinition> fragmentDefinitionMap) {
        return fragmentDefinitionMap.values().stream().map({ node -> AstPrinter.printAst(node) }).collect(Collectors.joining("\n"))
    }

    String printFields(List<Field> fields) {
        return fields.stream().map({ f -> AstPrinter.printAst(f) }).collect(Collectors.joining("\n"))
    }
}

I put it over the starwars schema like this :

query UseFragment {
        luke: human(id: "1000") {
            ...HumanFragment
            homePlanet
        }
        leia: human(id: "1003") {
            ...HumanFragment
            appearsIn
        }
    }
    fragment HumanFragment on Human {
        name
        ...FriendsAndFriendsFragment
        
    }
    
    fragment FriendsAndFriendsFragment on Character {
        friends {
            name 
            friends {
                name
            }
       }
    }

The DataFetchingEnvironment now has the selection set via fields and fragments. You would need to combine them to synthesize a pure list of fields.

The system output is given below

ields :
    luke: human(id: "1000") {
    ...HumanFragment
    homePlanet
    }
    fragments :
    fragment HumanFragment on Human {
    name
    ...FriendsAndFriendsFragment
    }
    fragment FriendsAndFriendsFragment on Character {
    friends {
        name
        friends {
        name
        }
    }
    }

    fields :
    friends {
    name
    friends {
        name
    }
    }
    fragments :
    fragment HumanFragment on Human {
    name
    ...FriendsAndFriendsFragment
    }
    fragment FriendsAndFriendsFragment on Character {
    friends {
        name
        friends {
        name
        }
    }
    }

    fields :
    leia: human(id: "1003") {
    ...HumanFragment
    appearsIn
    }
    fragments :
    fragment HumanFragment on Human {
    name
    ...FriendsAndFriendsFragment
    }
    fragment FriendsAndFriendsFragment on Character {
    friends {
        name
        friends {
        name
        }
    }
    }

    fields :
    friends {
    name
    friends {
        name
    }
    }
    fragments :
    fragment HumanFragment on Human {
    name
    ...FriendsAndFriendsFragment
    }
    fragment FriendsAndFriendsFragment on Character {
    friends {
        name
        friends {
        name
        }
    }
    }

Notice how the fields are calculated as we go down but the fragments are global in the query. This is defined in the grammar

document : definition+;

definition:
operationDefinition |
fragmentDefinition |
typeSystemDefinition
;

fragmentDefinition : 'fragment' fragmentName typeCondition directives? selectionSet;

So they will always be "global" and need replacing as your data fetcher executes.

You could nominally use FieldCollector but it requires ExecutionContext which you dont have

        Map<String, List<Field>> subFields = new LinkedHashMap<>();
        List<String> visitedFragments = new ArrayList<>();
        for (Field field : fields) {
              if (field.getSelectionSet() == null) continue;
            fieldCollector.collectFields(executionContext, resolvedType, field.getSelectionSet(), visitedFragments, subFields);
        }

Perhaps we should fix DataFetchingEnvironement so it has the executionContext and hence can call fieldCollector

apottere · 2017-05-02T02:49:01Z

Yeah, fragment definitions are available on the DataFetchingEnvironment now.

apottere · 2017-05-02T02:51:29Z

Oh, missed your last observation. I'm all for adding the ExecutionContext to the DataFetchingEnvironment, but there was a comment on the last PR that tried to fix a similar issue about not expanding the API surface by exposing the ExecutionContext, so that's why I didn't in my PR.

kaqqao · 2017-05-02T08:46:06Z

Ah, perfect, this has actually been solved already! Thanks for the explanations @bbakerman and @apottere !
And this issue is basically just a duplicate of #303 so I'll close it.

kaqqao · 2017-05-02T08:54:41Z

Actually, when I think about it better, maybe you wanted to keep it open until the decision on FieldCollector and the query proxying use-case from above.

Thinking of FieldCollector, perhaps it can be refactored a little to have methods that allow collecting based on fragment definitions only (when directives had already been processed) so that it can be used from within DataFetcher?

… a data fetcher

bbakerman · 2017-05-02T11:16:46Z

Thinking of FieldCollector, perhaps it can be refactored a little to have methods that allow collecting based on fragment definitions only (when directives had already been processed) so that it can be used from within DataFetcher?

Can you give examples of this. Having the specific use cases allows for unit tests to be more easily written

kaqqao · 2017-05-04T11:47:03Z

@bbakerman I was having similar ideas to what you ended up doing in your PR. I was thinking of refactoring FieldCollector in way that would allow it to be created ("initialized") with the variables, and passed to DataFetchingEnvironement, which would then be able to use it without having to pass variables again. It could even hide FieldCollector internally, just exposing the field collection methods itself. Not sure if this is smart, was just thinking aloud.

…tchers #377 - have the ability to know and capture all fields in a data fetcher

bbakerman · 2017-05-10T00:01:23Z

Thanks for the idea. I have implemented it in the way you suggested

* upstream/master: graphql-java#438 - unit test for NonNullableFieldValidator graphql-java#438 now with double checks in null values that might come out of coercion graphql-java#457 - support for implicit schema when types are named `Query` graphql-java#427 TDD driven null support (graphql-java#452) refactoring add some comments fix javadoc tweak build update jdk version for travis build cleanup build javadoc too to ensure that it is all valid javadoc fix javadoc mark as internal cleanup mark internal validation classes as @internal a bit refactoring Added antlr parsing tests as outlined in graphql-java#200 graphql-java#448 parse null pointer make buildRegistry public Introspection parser (graphql-java#463) fix test improve assertion error document current behaviour of failed serialization with test rename test classes to *Test (before it was *Spec) update javadoc for serialize/parseValue fix test add MapEnumValuesProvider for simple map based mappings rename StaticEnumValuesProvider to NaturalEnumProvider enum values provider Scalar changes (graphql-java#455) cleanup: combine catch block Make NoOpInstrumentation.INSTANCE final don't trim comment lines for description bugfix: comments as descriptions this is a small bugfix for enum field descriptions and a change so that an empty line in an comment separate a comment from a description (for the IDL) adding tests for missing arguments to document the current behaviour add test for too large int literal, which is failing currently docs: fix styling issues docs docs docs doc: add relay info cleanup: remove assert statements and author comments remove jacoco: not working currently improve javadoc, mark as public spi docs: typo update version updated readme, delete readme.next initial version of new documentation improve assertion method refactoring: removed `ResolvedTypeInterface` graphql-java#419 - dynamic runtime wiring factory support add index.rst add docs folder Add environment to field instrumentation. Update README.next.md Move related projects to top of readme fix merge fix merge add lincol to exception Download gradle over https add public annotation add public annotation renames SchemaCompiler -> SchemaParser and SchemaDecompiler -> SchemaPrinter IDL: fix subscription support IDL: fix missing subscription support add public/internal annotations fix IDL example add test for SchemaValidator graphql-java#414 added schema validation in subscriptions remove link to google group: ask to open a new issue instead remove javadoc link 425 improve wrong type exceptions (graphql-java#429) phrase better exception messages fix union IDL parsing and add tests for it rebase from master better exceptions for incorrect types fix union type generation (test missing) do not overwrite top level schema definition during type registry merge Fixed tests Test class that had an invalid schema in place object interface is now validated added argument support to graphql schema checking on interfaces - with default value checks added argument support to graphql schema checking on interfaces remove list of related projects in favour of the awesome list add @documented add public/internal/spi annotations graphql-java#409 - renamed schema validator code to exactly that graphql-java#410 - Added interface checking on types at IDL level graphql-java#409 - Added interface checking on object types graphql-java#406 - PR clean code fix ups Made DataFetchingFieldSelectionSet a supplier since it really is Added a selection set interface instead as suggested. This means field collector doesnt NOT have to be API revert changes: will be handled in a PR add public/internal/spi annotations Documentation: add graphql language identifier Documentation: remove outdated java 8 lambdas note Documentation: use correct heading Documentation: minor wording change remove duplicate code remove TypeOrReference, but changing the return type of `getInterfaces` and `getTypes` at the same time. redefine references api Removes the TypeReference class in favour of the already existing GraphQLTypeReference. simplify subscription test a bit subscription documentation subscription documentation add subscription documentation graphql-java#406 readme updates for type extensions graphql-java#406 added extend type XXX {} support ExecutionContext is not longer API but there is a replacement Makes type name specified to builder graphql-java#402 - put a next version README in place graphql-java#377 - have the ability to know and capture all fields in a data fetcher cleanup: replace explicit type arguments with diamond operator <> cleanup: remove redundant type infos cleanup: rename class to match actual file remove BuilderFunction in favour of Java 8 UnaryOperator Merge remote-tracking branch 'upstream/master' into kaqqao-122TypeResolver cleanup: public is not needed on interfaces Added more tests for the bug in schema generator Fixed a bug where schema generator is nor respecting type uniqueness PR feedback - moved classes around and made the IDL spec support call PR feedback and made methods static since they had no state Breaking change: Renaming the map with extra types for the schema from `dictionary` to `additionalTypes` to make it more clear. Revert "graphql-java#383 added extra readme documentation for specific references" Add failing test case for missing fragment name Adding extra data to type resolver graphql-java#379 added extra readme documentation for causing mutations graphql-java#383 added extra readme documentation for specific references Add fragment definitions and execution id to DataFetchingEnvironment (fixes graphql-java#303). Fixed a problem where non unique type names cause class exceptions later down the track. Removes javadoc warnings during build time graphql-java#296 - now with an AST from object support and tests graphql-java#296 -more tests for extra conditions graphql-java#296 - added a AST pretty printer system so I can fix other problems like 296 Allow ExecutionStrategy to specify dataFetchingExceptionHandler graphql-java#381 - Added more tests for merging and change the signature to throw SchemaProblem revert two files graphql-java#381 - Adds the ability to compile and build executable schemas from schema IDL definitions and also method reference use lambda where possible graphql-java#352 - fixed the bug where too many tokens are allowed as valid closes graphql-java#122 Provide more information to TypeResolver fix with java8 type param and misc. Adding support for subscriptions with an implemenation to enable adding a user defined subscriptionType. Subscriptions can be thought of as mutations by a different name and with a different purpose and, as such, the implementation here is the same as that of mutations. graphql-java#268 - now follows spec in regard to null error handling fix an errant newline Check for invalid list indices on client-provided cursor

kaqqao closed this May 2, 2017

kaqqao reopened this May 2, 2017

bbakerman added a commit to bbakerman/graphql-java that referenced this issue May 2, 2017

graphql-java#377 - have the ability to know and capture all fields in…

5d487ea

… a data fetcher

bbakerman mentioned this issue May 2, 2017

#377 - have the ability to know and capture all fields in a data fetcher #401

Merged

bbakerman added a commit that referenced this issue May 10, 2017

Merge pull request #401 from bbakerman/377-field-selection-in-data-fe…

Loading status checks…

c7fee85

…tchers #377 - have the ability to know and capture all fields in a data fetcher

bbakerman closed this May 10, 2017

bbakerman added this to the 3.0.0 milestone May 10, 2017

bbakerman mentioned this issue May 11, 2017

Create a text query from the current DataFetchingEnvironment's field (and Document). #422

Closed

graphql-java / graphql-java

DataFetchingEnvironment should always have a way to get field sub-selection #377

DataFetchingEnvironment should always have a way to get field sub-selection #377

kaqqao commented Apr 16, 2017

bbakerman commented Apr 27, 2017

bbakerman commented May 2, 2017 •

edited

apottere commented May 2, 2017

apottere commented May 2, 2017

kaqqao commented May 2, 2017

kaqqao commented May 2, 2017

bbakerman commented May 2, 2017

kaqqao commented May 4, 2017 •

edited

bbakerman commented May 10, 2017

graphql-java / graphql-java

Join GitHub today

DataFetchingEnvironment should always have a way to get field sub-selection #377

DataFetchingEnvironment should always have a way to get field sub-selection #377

Comments

kaqqao commented Apr 16, 2017

bbakerman commented Apr 27, 2017

bbakerman commented May 2, 2017 • edited

apottere commented May 2, 2017

apottere commented May 2, 2017

kaqqao commented May 2, 2017

kaqqao commented May 2, 2017

bbakerman commented May 2, 2017

kaqqao commented May 4, 2017 • edited

bbakerman commented May 10, 2017

Essential cookies

Always active

Analytics cookies

bbakerman commented May 2, 2017 •

edited

kaqqao commented May 4, 2017 •

edited