Demonstrate poor handling of cyclic datastructures#111
Open
lelandbatey wants to merge 1 commit intomainfrom
Open
Demonstrate poor handling of cyclic datastructures#111lelandbatey wants to merge 1 commit intomainfrom
lelandbatey wants to merge 1 commit intomainfrom
Conversation
This PR is not meant to ever be merged. This is a demonstration of some broken behavior around handling datastructures with cyclic dependencies. The problem is if we provide a struct to `MarshalQueryWithOptions()` and that struct has cyclic fields, even if not populated, and even if we provide a Field that limits the output to a set depth, `MarshalQueryWithOptions()` will still recurse infinitely and cause a panic due to exhausting the stack. This is a big problem because goql is a GraphQL library, and GraphQL is for querying graph datastructures, and datastructures in graphs will very commonly have cyclic relationships because graphs have cycles :) The correct behavior would be to: 1. If no Fields are provided, do not recursively walk into fields of structs that've already been walked. This is safe to do because GraphQL already doesn't allow infinite expansion; if you want relationships, even recursive ones, to a particular depth then GQL will force you to specify that in your query. Thus we can assume that you'll have to specify that to `goql` via a `Fields` param. 2. If `Fields` *are* provided, we can recursively walk already walked fields, but only to the depth specified in those `Fields`. Future work will have to be done to get this behavior to work.
lelandbatey
force-pushed
the
demo-broken-cycle-handling
branch
from
bab60b7 to
d3c70b3
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What this PR does / why we need it
This PR is not meant to ever be merged. This is a demonstration of some broken behavior around handling datastructures with cyclic dependencies. The problem is if we provide a struct to
MarshalQueryWithOptions()and that struct has cyclic fields, even if not populated, and even if we provide a Field that limits the output to a set depth,MarshalQueryWithOptions()will still recurse infinitely and cause a panic due to exhausting the stack.This is a big problem because goql is a GraphQL library, and GraphQL is for querying graph datastructures, and datastructures in graphs will very commonly have cyclic relationships because graphs have cycles :)
The correct behavior would be to:
goqlvia aFieldsparam.Fieldsare provided, we can recursively walk already walked fields, but only to the depth specified in thoseFields.Future work will have to be done to get this behavior to work.
Jira ID
No jira for this (yet)
Notes for your reviewers