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

Extract logic for schema setup, validate default values while building the schema #3496

Merged
merged 3 commits into from
May 28, 2021

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented May 24, 2021

These methods were just tacked on to Schema, but I'm interested in adding a step or two in order to validate default argument values, as described in #3448.

All I've done here is copy-pasted, so the code isn't any better, but at least these methods have a bit of isolation now.

TODO:

  • Add a step for validating and default values on arguments that were added

(This is just like Schema::Traversal from the old dsl)

Sorry, something went wrong.

rmosolgo added 2 commits May 24, 2021 16:11

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@rmosolgo
Copy link
Owner Author

👋 @eapache , what do you think of this approach? TL;DR: While adding types to the schema, I made it gather a list of arguments with default values. Then, after all types are added (including after resolving lazy types), I made it go through the list and call #validate_default_value on them.

It seems to me like it should work fine, but am I missing anything here?

@eapache
Copy link
Contributor

eapache commented May 26, 2021

As described I think this makes sense (there's a lot of complex code here and I definitely haven't done more than skim it at this point).

Without actually analyzing it one way or another, there's two cases where I'd be worried this might fall down:

  • build_from_definition.rb still does some funky stuff with directives and late-bound types (and particularly the built-in scalars); could be an issue there?
  • in my other approaches I ran into cases (possibly related to the build_from_definition issues) where some type deep in the default value (e.g. whatever the type of argument baz would be in default value { foo: { bar: { baz: { bam: 1 } } } }) would still be late-bound even after the default value itself was "resolved"

It's not clear to me whether the "extra" step here runs after the entire schema has loaded and resolved, or whether it runs as each self-contained referential "chunk" of the schema is resolved? If the latter, then that definition might not be complete right now for some reason.

@rmosolgo
Copy link
Owner Author

Yep, it's the latter: basically, every method that adds some type info the the schema (query(...),mutation(...), orphan_types(...), a couple others) would kick of a process of:

  • Traversal, finding every type that can be reached from the given starting point
  • Resolving late-bound types, then re-starting traversal from those resolved late-bound types

I've added a third step:

  • Validate each argument that defined a default value (Conceptually, that part of the change is just here: 6a7efe6)

Yeah, that's basically a "chunk" of the schema, but because of the traversal, I'd expect the chunk to have everything it needs to validate itself (ie, field's argument's types are traversed, input object argument types are traversed, and so on). In any case, I'll add a test for a deeply nested example.

I think build-from-definition is OK because it uses those same entry points. If any late-bound types couldn't be resolved by the time it validates arguments, then it would've raised an error.

Anyhow, if no red flags jump out at you, I'll flesh out a few more tricky tests and see how it goes. Thanks for taking a look.

@rmosolgo rmosolgo added this to the 1.12.11 milestone May 28, 2021
@rmosolgo rmosolgo merged commit da44191 into master May 28, 2021
@rmosolgo rmosolgo deleted the better-schema-setup branch May 28, 2021 12:57
@rmosolgo rmosolgo changed the title Extract logic for schema setup Extract logic for schema setup, validate default values while building the schema Jun 10, 2021
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.

None yet

2 participants