Fragment variables: enforce fragment variable validation #1422

mjmahone · 2018-07-16T23:36:18Z

Treats experimental fragments with variable definitions as their own top-level
"executable definition": this means a query cannot include a variable
definition found solely beneath a fragment-variable definition, and
a fragment must include all variable definitions that exist under it,
excluding other variables defined by other fragments.

Has no repercussions outside of fragment variable definitions, besides allowing recursive fragment lookups. One note: I am not sure how the NoUnusedFragments validation is supposed to work: internally we don't use it, as we have many use cases for unused fragments. I suspect fragments with variable definitions are a different definition type, and should always be treated as their own self-contained definition-trees (so we should be doing the same thing as below inside NoUnusedFragments). But I'm not positive about that behavior, whereas I am positive about the behavior of unused/undefined variables.

Treats fragments with variable definitions as their own top-level "executable definition": this means a query cannot include a variable definition found solely beneath a fragment-variable definition, and a fragment must include all variable definitions that exist under it, excluding other variables defined by other fragments.

IvanGoncharov · 2018-07-17T13:58:25Z

src/validation/rules/NoUndefinedVariables.js

+ * NOTE: if experimentalFragmentVariables are used, then fragments with
+ * variables defined are considered independent "executable definitions".
+ * If a fragment defines at least 1 variable, it must define all recursively
+ * vused ariables, excluding other fragments with variables defined.


vused typo

IvanGoncharov · 2018-07-17T14:05:09Z

src/validation/ValidationContext.js

+ // All OperationDefinitions, or FragmentDefinitions with variable definitions
+ isExecutableDefinitionWithVariables(
+ definition: ExecutableDefinitionNode,
+ ): boolean {


This function doesn't use this so it should be a standalone function, not a class method.

Also maybe it worth to add %checks so Flow will have better type coverage.

IvanGoncharov · 2018-07-17T16:02:58Z

src/validation/rules/NoUnusedVariables.js

 },
+ leave(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {


just check that variableDefs is not empty since if it's empty it couldn't be any unused variables.

IvanGoncharov · 2018-07-17T16:03:15Z

src/validation/rules/NoUnusedVariables.js

- },
+ const executableDefinitionVisitor = {
+ enter(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {


not needed see leave.

IvanGoncharov · 2018-07-17T16:09:40Z

src/validation/rules/NoUndefinedVariables.js

+ enter(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {
+ return;
+ }


not needed since there is nothing bad in resetting value of variableNameDefined even on fragments without arguments.

Ah right, because this function only cares about finding undefined variables, and if none are defined, it doesn't matter (for fragments). I think there might be a world in which FragmentWithVariableDefinitionNode is a different AST node than FragmentDefinitionNode, but even in that world this wouldn't be needed :)

IvanGoncharov · 2018-07-17T16:15:01Z

src/validation/rules/NoUndefinedVariables.js

+ variableNameDefined = Object.create(null);
+ },
+ leave(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {


As I understand, variableNameDefined was added to not depend on variableDefinitions property at the same time isExecutableDefinitionWithVariables introduces such dependency.
Would be better to replace with:

if (definition.kind === Kind.FRAGMENT && Object.keys(variableNameDefined).length === 0)

Interesting, that would make sense. Will make the changes

IvanGoncharov · 2018-07-17T16:26:44Z

src/validation/ValidationContext.js

- if (fragment) {
+ if (
+ fragment &&
+ !this.isExecutableDefinitionWithVariables(fragment)


we already know that it's fragment so it better to create a function that
check only fragments (isExperimentalFragment) or just add check inplace:

!fragment.variableDefinitions || fragment.variableDefinitions.length === 0

IvanGoncharov · 2018-07-17T17:02:01Z

src/validation/__tests__/harness.js

- const errors = validate(schema, parse(queryString), rules);
+ const errors = validate(
+ schema,
+ parse(queryString, { experimentalFragmentVariables: true }),


There are maintainers of GraphQL 3rd-party implementations that use graphql-js test suite as the refence so I don't think we should enable the experimental feature by default for all tests.
It's better to just add options as 4th argument, it would make it more explicit and also allow to easily grep for all tests cases with the particular experimental feature.

IvanGoncharov · 2018-07-17T17:29:12Z

src/validation/ValidationContext.js

+ * NOTE: if experimentalFragmentVariables are being used, it excludes all
+ * fragments with their own variable definitions: these are considered their
+ * own "root" executable definition.
+ */
 getRecursivelyReferencedFragments(


I'm very concern about changes in this function since it's called getRecursivelyReferencedFragments but not return all recursive fragments and is part of public API.
+ it breaks NoUnusedFragments

An alternative solution would be to extract visitReferencedFragmentsRecursively that accept visitor function and if you return false it doesn't look inside the current fragment.

It only breaks NoUnusedFragments if you're using fragments with variable definitions. I think this is OK, as fragments with variable definitions are their own independent "unit": they have semantic meaning that is very different from normal fragments without variable definitions. An unused fragment that has variable definitions I see as being "stand-alone" in the same way a query is "stand-alone": you could reasonably figure out a way to persist these to the server and execute them, provided you started on the correct type.

fragment A($id: ID!) on Query { node(id: $id) { name } }

is, and in my mind should be, identical to

query A($id: ID!) { node(id: $id) { name } }

I'm very OK with breaking people who are actually using fragments with variable definitions.

I can see the appeal to extracting it out, but I'd much prefer taking an opinionated stance about what a fragment with variable definitions is. It will also make it easier to make an RFC in the spec for them.

Ok, in that case, it's outside of scope for graphql-js and would make sense to discuss it in PR/RFC.

Just a quick note:

and execute them, provided you started on the correct type

There is no one to one matching between root types and operation types so you can write:

schema { query: Type mutation: Type subscription: Type }

Yeah after thinking this through I think it does make more sense to add a second function specifically for excluding fragments with variable definitions. I'll make that change.

…ns + comments

IvanGoncharov · 2018-07-17T17:55:09Z

I suspect fragments with variable definitions are a different definition type, and should always be treated as their own self-contained definition-trees (so we should be doing the same thing as below inside NoUnusedFragments). But I'm not positive about that behavior, whereas I am positive about the behavior of u

@mjmahone As I understand from #1141 experimental fragments are not executed unless they are referenced inside at least one query so I don't see any difference with normal fragments in that sense.

I also concern with lack of PR/RFC in specification repo because maintainer of 3rd-party libraries use this repo as refence and they also implemented this feature:
webonyx/graphql-php@949b853
sangria-graphql/sangria@8992744
and probably others.

So I'm worry that we are repeating IDL/SDL mistake then solution that everyone adopted significantly differ from what was added into the spec and entire comunity now goes through painful migration process. This feature could have even bigger effect if people would start sending queries with such fragments to the server. I think it's very important to comminicate the purpose of this feature and it's current status to community members outside of Facebook by openning minimal PR on a spec repo.

mjmahone · 2018-07-17T20:03:16Z

Fragment variables, and to some extent the SDL/IDL, fall into an interesting problem where we can't know what the right solution is, or even what everything means, until we use it. GraphQL itself was developed before it was formalized. The issue here is that at Facebook we have internal dependencies that would make it impossible for us to experiment with fragment variables (so that we can then drive the future spec RFC explaining what they mean) unless we have support for them within graphql-js. Things like Relay have a lot of adoption within Facebook, and we're not going to fork the open source Relay internally just so we can have one version that supports the spec and one that has experimental fragment variable support.

Fragment variables do not belong in the spec. If there was a clean way to depend on a forked-but-constantly-moving-with-open-source version of graphql-js in all of our GraphQL repos (including things like Relay), we would have done it.

In terms of fragments with variables being executable: I only think that because I'm seeing how they're actually used, and I think we need a way to describe a collection of executable definitions that is stand-alone, but not a "root" definition. This PR is not for that prupose, it is not changing the nature of fragments with variables, but I think it allows us to move in that direction by making sure people don't implement them without enforcing they define exactly those variables the fragments require. Which I'm hoping to put up as a separate GraphQL-spec RFC, but I don't want to get that wrong. We'll probably have to iterate on the various validation rules internally and through graphql-js before opening that up.

…ragment-vars-validate

IvanGoncharov · 2018-07-17T20:26:25Z

src/validation/ValidationContext.js

- usages = this.getVariableUsages(operation);
- const fragments = this.getRecursivelyReferencedFragments(operation);
+ usages = this.getVariableUsages(definition);
+ const fragments = this.getRecursivelyReferencedFragments(definition);
 for (let i = 0; i < fragments.length; i++) {
 Array.prototype.push.apply(
 usages,
 this.getVariableUsages(fragments[i]),
 );
 }


Yeah after thinking this through I think it does make more sense to add a second function specifically for excluding fragments with variable definitions. I'll make that change.

@mjmahone Idea mode 💡: I think we can have one function to do both

visitReferencedFragmentsRecursively(definition, fragment => { if (isExperimentalFragmentWithVariableDefinitions(fragment)) { return false; } Array.prototype.push.apply( usages, this.getVariableUsages(fragments[i]), ); });

And it will also simplify this code:

graphql-js/src/validation/rules/NoUnusedFragments.js

Lines 41 to 45 in e6c36e0

for (const fragment of context.getRecursivelyReferencedFragments(

operation,

)) {

fragmentNameUsed[fragment.name.value] = true;

}

Plus you don't need to create intermidiate array in a process.

@mjmahone Stupid idea 🤦‍♂️ because it will break caching.

Hmmm would it make more sense to create the ValidationContext with a flag indicating we should use fragment variables as a "validation cut" point? Then you're either opting in to it or not, and we could for instance validate "human-written" graphql with fragment variables as well as "transformed" (whatever that means) graphql in two different validation steps, just by creating a new validation context without the flag.

Basically this means people using fragment variables might consume more memory/make two passes, but I'd rather have people on the experimental version have some pain than make things confusing for those on mainline GraphQL.

…or fragment variables

mjmahone · 2018-07-18T15:15:36Z

I'm going to put this PR on ice for a bit: fragment variables need to go over at least 1-2 more iterations of "what they mean" design: I'm no longer confident this is how fragment variable validation should work, so there's no use thrashing more here.

IvanGoncharov · 2018-07-18T18:29:30Z

@mjmahone Thanks for the detailed explanation about the intention of this feature, now I better understand what's happing here and how Facebook depends both on Relay and graphql-js.

Sorry for being too meticulous about this change, I just worried that query syntax is the most critical part of GraphQL since we can never do any breaking change to the existing clients.

Ping me if you need any help with this feature or a devil's advocate assistance 👿

facebook-github-bot added the CLA Signed label Jul 16, 2018

mjmahone requested review from kassens and leebyron July 16, 2018 23:37

mjmahone added 2 commits July 16, 2018 19:55

Modify comments to be a bit more accurate

cbd3abf

More comment cleanup

e9a7991

IvanGoncharov reviewed Jul 17, 2018

View reviewed changes

mjmahone added 2 commits July 17, 2018 13:36

Remove dependencies outside of ValidationContext on variableDefinitio…

0e1c6de

…ns + comments

typo

618349e

Merge branch 'master' of https://github.com/graphql/graphql-js into f…

90408dc

…ragment-vars-validate

IvanGoncharov reviewed Jul 17, 2018

View reviewed changes

Add an optional param for validation: whether to validate specially f…

f8be135

…or fragment variables

mjmahone closed this Jul 18, 2018

mjmahone deleted the fragment-vars-validate branch July 18, 2018 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fragment variables: enforce fragment variable validation #1422

Fragment variables: enforce fragment variable validation #1422

mjmahone commented Jul 16, 2018

IvanGoncharov Jul 17, 2018

IvanGoncharov Jul 17, 2018 •

edited

Loading

IvanGoncharov Jul 17, 2018

IvanGoncharov Jul 17, 2018

IvanGoncharov Jul 17, 2018

IvanGoncharov Jul 17, 2018

mjmahone Jul 17, 2018

IvanGoncharov Jul 17, 2018

mjmahone Jul 17, 2018

IvanGoncharov Jul 17, 2018 •

edited

Loading

IvanGoncharov Jul 17, 2018

IvanGoncharov Jul 17, 2018 •

edited

Loading

mjmahone Jul 17, 2018

IvanGoncharov Jul 17, 2018

mjmahone Jul 17, 2018

IvanGoncharov commented Jul 17, 2018

mjmahone commented Jul 17, 2018

IvanGoncharov Jul 17, 2018

IvanGoncharov Jul 17, 2018

mjmahone Jul 17, 2018

mjmahone commented Jul 18, 2018

IvanGoncharov commented Jul 18, 2018 •

edited

Loading

	for (const fragment of context.getRecursivelyReferencedFragments(
	operation,
	)) {
	fragmentNameUsed[fragment.name.value] = true;
	}

Fragment variables: enforce fragment variable validation #1422

Fragment variables: enforce fragment variable validation #1422

Conversation

mjmahone commented Jul 16, 2018

Choose a reason for hiding this comment

IvanGoncharov Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Jul 17, 2018

mjmahone commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjmahone commented Jul 18, 2018

IvanGoncharov commented Jul 18, 2018 • edited Loading

IvanGoncharov Jul 17, 2018 •

edited

Loading

IvanGoncharov Jul 17, 2018 •

edited

Loading

IvanGoncharov Jul 17, 2018 •

edited

Loading

IvanGoncharov commented Jul 18, 2018 •

edited

Loading