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

Add failing test for directives with input cycles #3472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eapache
Copy link
Contributor

@eapache eapache commented May 11, 2021

It was noted in a comment in build_from_definition.rb that this might be
a problem. It is a problem (this test currently fails with
SystemStackError).

@rmosolgo this is incomplete right now given it's just a test and no actual fix, but I ran into this as part of digging around in #3448.

It was noted in a comment in build_from_definition.rb that this might be
a problem. It is a problem (this test currently fails with
SystemStackError).
@eapache
Copy link
Contributor Author

eapache commented May 11, 2021

The comment in build_from_definition.rb is

          # Make a different type resolver because we need to coerce directive arguments
          # _while_ building the schema.
          # It will dig for a type if it encounters a custom type. This could be a problem if there are cycles.

We've gotten away with it because cycles in directive arguments are presumably pretty rare. But the most obvious way to start validating default values means we need to coerce all arguments while building the schema (this is the "build-schema-from-definition issues" referred to in #3448), and cycles in regular field/mutation arguments are not nearly as rare.

@eapache eapache mentioned this pull request May 11, 2021
@rmosolgo
Copy link
Owner

Would it help for me to propose a hack to fix this test?

@rmosolgo
Copy link
Owner

I gave it a shot anyways, here's a proposed hack: 9e4eea9

Feel free to grab it here if it helps (or if you'd rather, I can merge that branch instead of this one -- I just couldn't push that commit here).

@eapache
Copy link
Contributor Author

eapache commented May 11, 2021

🤔 I think that just moves the problem; I could write an equally short test which passes on main but fails with that hack in place (make a schema with a directive argument pointing to a three-input-object-cycle, then try and use that directive with a fully-specified argument).

@rmosolgo
Copy link
Owner

Ah, sure, I added a failing test in that vein here: 7cafefb 😖

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