Skip to content

Add document path equivalent to path to ResolveInfo #183

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

Closed
wants to merge 1 commit into from
Closed

Add document path equivalent to path to ResolveInfo #183

wants to merge 1 commit into from

Conversation

nickbruun
Copy link
Contributor

@nickbruun nickbruun commented May 31, 2018

This extends the behavior introduced in #148 by adding a document path to ResolveInfo, ie. the path as the final response will see it, such that it is possible to refer to a path equivalent to the aliased path. This serves to make it possible to use the path to describe error paths in custom error messages.

Further, this copies the ResolveInfo with the updated paths for list element iteration, to avoid mutating the ResolveInfo of the field as well as to prevent race issues between list elements accessing the path in promises.

@nickbruun
Copy link
Contributor Author

On second thought, there is another way in which we can track the path that makes this a little less use-case specific. I don't know if this is a change that's acceptable given that the path addition has already been in master for a while, but essentially, we could describe the path as a list of AST nodes and list indices, rather than string and list indices. This way, an end user could easily evaluate the path.

@syrusakbary
Copy link
Member

syrusakbary commented Jun 5, 2018

Hi @nickbruun,

Thanks to your PR I realized there was a small bug in the ResolveInfo path for aliased fields.
I've fixed it with this commit: graphql-python/graphql-core@0b3cb44

@ekampf

I'm closing this PR since your changes shouldn't be necessary now.
Feel free to reopen it (or comment) if you see something buggy :)

@syrusakbary syrusakbary closed this Jun 5, 2018
@nickbruun
Copy link
Contributor Author

This does the trick perfectly :) Thanks!

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.

2 participants