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

Include fields for GraphQLError #273

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@CatTail
Copy link

commented Jan 7, 2016

Possible fix #192

@CatTail CatTail force-pushed the CatTail:master branch from e891d9c to eeb218c Jan 7, 2016

@@ -353,6 +354,7 @@ function collectFields(
if (!fields[name]) {
fields[name] = [];
}
selection.parentField = parentField;

This comment has been minimized.

Copy link
@leebyron

leebyron Jan 9, 2016

Collaborator

We want to ensure that we're not mutating the provided AST, as it may be used elsewhere.

@leebyron

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2016

Thanks so much for investigating this!

We should be careful not to mutate inputs but also careful to ensure the performance of query execution is not negatively impacted. One case I imagine would not work correctly here is if a query includes the same fragment in two locations, an error occurring in both locations. The parentField property would have been overridden by the second call, and the first error would have an incorrect execution path.

@CatTail

This comment has been minimized.

Copy link
Author

commented Jan 11, 2016

Add unittest for

One case I imagine would not work correctly here is if a query includes the same fragment in two locations, an error occurring in both locations.

Maybe I have misunderstanding?

And the problem of add parentField for AST, can you provide a better way to retain parent field information without modification of AST?

@leebyron leebyron force-pushed the graphql:master branch from 2f58173 to 6dba6ce Jan 21, 2016

@CatTail

This comment has been minimized.

Copy link
Author

commented Feb 1, 2016

ping @leebyron

@leebyron

This comment has been minimized.

Copy link
Collaborator

commented Feb 2, 2016

For example:

fragment X on T {
  x
}

query {
  a { ...X }
  b { ...X }
}

Where the field x throws an error.

Currently by my understanding of your code, this will result in printing out two errors, where both errors will report b.x even though the first should report a.x - this occurs because of the mutation of the field nodes, and the fragment being used in more than one location.

At the very least, this test should be included so that this is known not to occur.

My opinion on how to maintain the execution path is to maintain this as an additional "Stack" variable provided during execution, rather than mutating the AST provided to the execution.

@leebyron leebyron force-pushed the graphql:master branch from 1983079 to a781b55 Mar 24, 2016

@CatTail CatTail closed this May 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.