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.

Sign up for GitHub

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

Jump to bottom

Fix compilation and execution for circular dependencies in input types #82

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix compilation and execution for circular dependencies in input types #82

wants to merge 8 commits into from

Conversation

boopathi
Copy link
Member

@boopathi boopathi commented May 17, 2020 •
edited

Previously the following schema would result in compilation error -

type Query {
  test(input: Foo): String
}
input Foo {
  foo: Foo # circular dependency
}

When input types are recursive in nature, the compilation depended on generating it inline. This PR hoists it to a function and generates it only once for input object types. This way, instead of relying on inlined object access, we have a getter and a setter defined at the top of the generated code - getPath and setPath - this moves the object access (the path - input["var"]["foo"]) to runtime - via getPath and setPath - so as to pass different paths as we move down the tree.

Extras:

Also, we compute the circular dependencies at runtime and throw them as separate user errors/ For this we now have the Set of visited inputs captured at the top-level. At the time of this writing, I've not handled the false positives where - var1.foo is the same object as var2.foo - which does not form a circular dependency, but would still result in a circular dependency error. These forms of runtime values are unintended because variables are generally passed down via request and is most likely deserialized from JSON. A JSON.parse cannot result in such cases anyway.

For the common case, we disable checking for runtime circular dependencies. To enable this, you can use the new compiler option

// default: false
variablesCircularReferenceCheck: true

@boopathi boopathi requested a review from ruiaraujo May 18, 2020 06:36
ruiaraujo
ruiaraujo reviewed May 18, 2020
View reviewed changes
src/variables.ts Outdated Show resolved Hide resolved
ruiaraujo
ruiaraujo reviewed May 18, 2020
View reviewed changes
Copy link
Collaborator

@ruiaraujo ruiaraujo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to think a bit more about the approach, I would love to have some benchmarks that stress query execution with highly nested variables (like the filter test in this PR) between master and this PR.

If this is punishing for the before case, maybe we can consider detecting the circular case in the types and bail out to such an implementation while relying on inline access for the normal case.

@ruiaraujo ruiaraujo changed the base branch from master to main November 30, 2021 20:01
@jgoux
Copy link

jgoux commented Jan 19, 2022

Hello, is there anything particular blocking this PR? It would unlock a lot of people if it can be merged. 🤞

@tuan231195
Copy link

@boopathi @ruiaraujo, is there any update about this PR? It would solve lots of issues for us

This was referenced Sep 15, 2023
Open
Open
@mnlbox
Copy link

mnlbox commented Oct 18, 2023

Any plan to finalize and merge this PR ??

@Urigo
Copy link

Urigo commented Feb 1, 2024

@boopathi @ruiaraujo What can we do in order to help this land?
Should we open another PR with a different approach?

@BabakScript
Copy link

@boopathi Thanks for this awesome project but this issue and the PR have been open for 4 years. I'm trying to figure out what is the blocker here because we would like to help to get this issue resolved.

How we can help to proceed with the solution and fix this issue?
This issue is blocking us from using GraphQL-Mesh as it uses graphQL-jit behind the scenes. We would like to connect Hasura to GraphQL-Mesh too and as we tried in a POC, we have this circular dependency issue there as well. So we are glad to contribute and help on this issue to get it resolved.

@oporkka
Copy link
Member

oporkka commented Feb 8, 2024

@boopathi Thanks for this awesome project but this issue and the PR have been open for 4 years. I'm trying to figure out what is the blocker here because we would like to help to get this issue resolved.

How we can help to proceed with the solution and fix this issue? This issue is blocking us from using GraphQL-Mesh as it uses graphQL-jit behind the scenes. We would like to connect Hasura to GraphQL-Mesh too and as we tried in a POC, we have this circular dependency issue there as well. So we are glad to contribute and help on this issue to get it resolved.

@BabakScript I think what is pending, is the missing benchmarks that @ruiaraujo was mentioning earlier.

I would love to have some benchmarks that stress query execution with highly nested variables (like the filter test in this PR) between master and this PR.

If this is punishing for the before case, maybe we can consider detecting the circular case in the types and bail out to such an implementation while relying on inline access for the normal case.

@BabakScript would you be able to contribute the above benchmarks?

@boopathi
Copy link
Member Author

boopathi commented Feb 9, 2024

Some benchmarks are available here in these PRs - #228 and #229 and it's not performant and I was trying to optimize it. I'll get back to this when I find some time.

@BabakScript
Copy link

@BabakScript would you be able to contribute the above benchmarks?

@oporkka I guess as @boopathi mentioned the benchmarks part is somehow done. We didn't contribute to this project yet but we can try to read the code and check if we can help with the optimization part.

BTW, Thanks @boopathi for the update. I guess #228 is also a promising and good approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruiaraujo ruiaraujo ruiaraujo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request variables with circular dependency
8 participants
@boopathi @jgoux @tuan231195 @mnlbox @Urigo @BabakScript @oporkka @ruiaraujo