Fix bookkeeping of Resolution path when resolving #1208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Addresses issue absinthe-graphql#1149. module `Absinthe.Phase.Document.Execution.Resolution` walks the result tree and tracks `path` using a function parameter. At the same time, a `Resolution` struct is passed, which `path` field is not consistently tracked. The problem manifests itself in bogus Resolution path passed onto `resolve_type` function as described in function absinthe-graphql#1149. The reason for not updating the resolution.path in the walk process can be explained by comment of @benwilson512: `Traditionally the reason we pass in the env has more to do with making the schema available and the context available, I don't know that the other values in that struct have been validated in this situation.` Perhaps path parameter was used in the beginning and later resolution paramter was added, but not used to track current tree walk in it's path field. Nevertheless, the resolution is passed on to user code - into `resolve_type(value, resolution)` callback, which might use path to understand the context of value (eg. what is the type of parent?). This PR fixes the bookkeeping of `resolution.path` field. I have not inspected the use of other field, I am not sure which ones would change during the value walk (`parent_type` maybe?). One could consider to just use `resolution.path` instead of `path` parameter - so there is not double bookkeeping of this value. This is up to maintainers to decide, I did not make such a change.
|
Hi @marcinkoziej thanks for taking a crack at this. I'm open to moving forward with this but it definitely needs a test case or two. I'll also need some time to get my head back into this module and make sure there aren't any unintended consequences of this change. Thanks for taking a step towards fixing it! |
|
Should I add it to the Interface test or into a separate file ? |
|
I think an interface_test would be good, and then any other more general resolution test that could demo the behavior could be interesting. Please also run |
|
Ran mix format and added a test. |
benwilson512
approved these changes
Nov 28, 2022
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses issue #1149
module
Absinthe.Phase.Document.Execution.Resolutionwalks the result tree and trackspathusing a function parameter. At the same time, aResolutionstruct is passed, whichpathfield is not consistently tracked.The problem manifests itself in bogus Resolution path passed onto
resolve_typefunction as described in function #1149.The reason for not updating the resolution.path in the walk process can be explained by comment of @benwilson512:
Traditionally the reason we pass in the env has more to do with making the schema available and the context available, I don't know that the other values in that struct have been validated in this situation.Perhaps path parameter was used in the beginning and later resolution paramter was added, but not used to track current tree walk in it's path field.Nevertheless, the resolution is passed on to user code - into
resolve_type(value, resolution)callback, which might use path to understand the context of value (eg. what is the type of parent?).This PR fixes the bookkeeping of
resolution.pathfield. I have not inspected the use of other field, I am not sure which ones would change during the value walk (parent_typemaybe?).One could consider to just use
resolution.pathinstead ofpathparameter - so there is not double bookkeeping of this value. This is up to maintainers to decide, I did not make such a change.