graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

Add failing test for directives with input cycles

Open eapache opened this issue 3 years ago • 5 comments

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 https://github.com/rmosolgo/graphql-ruby/pull/3448.

eapache avatar May 11 '21 19:05 eapache

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 avatar May 11 '21 19:05 eapache

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

rmosolgo avatar May 11 '21 20:05 rmosolgo

I gave it a shot anyways, here's a proposed hack: https://github.com/rmosolgo/graphql-ruby/commit/9e4eea99c3344c4c3a3cbcbefc224b4ebe07d902

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).

rmosolgo avatar May 11 '21 20:05 rmosolgo

🤔 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).

eapache avatar May 11 '21 20:05 eapache

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

rmosolgo avatar May 11 '21 21:05 rmosolgo