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

Validate default values #3448

Closed
wants to merge 1 commit into from

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Apr 26, 2021

For #3248

Still some things TODO

@eapache
Copy link
Contributor Author

eapache commented Apr 26, 2021

@rmosolgo cc @benjie @RobertWSaunders this kinda/sorta does the right thing up front, but it's surfaced a bunch of other complications:

  1. Enums change.
# Given
class DairyAnimal < GraphQL::Schema::Enum
  value("COW", "Animal with black and white spots", value: 1)
end

# This used to be valid and isn't anymore
argument :animal, DairyAnimal, required: false, default_value: 1
# Instead you need this, which used to be invalid
argument :animal, DairyAnimal, required: false, default_value: "COW"

Surprisingly (to me) it seems to get cast either way and the resolver still gets 1 which is important. But when you call to_graphql and it runs through the validator you end up with failures like:

GraphQL::Schema::InvalidTypeError: Field Query.cheese's return type is invalid: field "similarCheese" argument "nullableSource" default value ["COW"] is not valid for type [DairyAnimal!] (Can't resolve enum DairyAnimal for "COW")

And it would be a breaking change for everybody to update their enums.

  1. Default values of input objects don't quite work. Given default_value: { snake_case: "ASC" } this almost works properly, until it gets to

    if !input.key?(argument_name) && argument.type.non_null? && warden.get_argument(self, argument_name)

    argument_name here is :snake_case but the warden only responds to camel-case strings (e.g. "camelCase") so it fails to find the argument and decides it's invalid. This is probably fixable by string-key-ing all your default values, but... gross. I assume the warden could be made string-symbol agnostic though?

  2. It doesn't handle wrapped types. I think this is fixable with a bit of effort on my part though.

# This doesn't work right now because it tries to validate the wrapped `[1]` with the unwrapped `Integer`.
argument :animal, [GraphQL::Type::Integer], required: false, default_value: [1]
  1. For some reason it can no longer find GraphQL::CoercionError in tests unless I add a bunch of explicit requires. No idea what's going on here. Smells like a setup/config issue and not an actual code/logic issue.

@eapache
Copy link
Contributor Author

eapache commented Apr 26, 2021

FYI I've found and removed a test with an actually-invalid-default-value (see commit 5). That commit may be worth shipping separately.

@eapache
Copy link
Contributor Author

eapache commented Apr 26, 2021

There are a lot of bad hacks here (look a commit at a time to peel them apart) but I've got the core use case passing, and there's only really two remaining kinds of test failure:

  • Validating and printing schemas is broken because it tries to coerce enums on output, but default enum values are now already e.g. COW instead of 1; I tried getting rid of output coercion for defaults here but that caused even more tests to fail and I gave up. I think simply converting enum defaults as I did in this PR is probably the wrong solution anyway.
  • Building the schema from definition fails when there's a default value of an input object, since it generates some late-bound types nested inside the default value, which blows up in InputObject#validate_non_null_input because they don't respond to non_null? or anything normal.

@eapache
Copy link
Contributor Author

eapache commented Apr 27, 2021

Getting closer... two hacks replaced by a more elegant solution which fixed the enum conversion issues, and #3451 has been extracted into its own PR. Still need to look into the build-schema-from-definition issues.

eapache added a commit to Shopify/graphql-ruby that referenced this pull request Apr 30, 2021
Fixes a weird require/load-order issue that showed up while I was trying
to validate default values. See the explanation at
rmosolgo#3448 (comment)
@eapache
Copy link
Contributor Author

eapache commented May 11, 2021

On consideration, #3459 is annoying and weakens any solution to this problem, but isn't technically blocking it.

However, there is one remaining blocking problem which is much trickier: handling default values of type InputObject where the InputObject type has arguments whose types are late-bound. We end up in InputObject#validate_non_null_input for the concrete parent input type before the nested late-bound types are resolved, which means either:

  • InputObject#validate_non_null_input needs to learn to resolve late-bound types and deal with cycles when they occur
  • Argument#type= doesn't need to just defer validation when the type is late-bound (as it already does), it needs to defer validation while the type or any of that type's arguments recursively are late-bound, and then remember to trigger validation once that entire recursive graph has been resolved.

Originally I thought this was just an issue with schema-first development and build_from_definition.rb which I filed in #3472, but I realized that I can build a code-first schema that also triggers this issue. (#3472 is a valid bug regardless though).

@benjie this is basically the "recursion without infinite cycles is hard" problem we were worried about when you wrote the RFC. I think the RFC is still basically correct, and if I were writing a new implementation green-field I could probably make it work, but right now backporting this validation into the existing ruby implementation would seem to require substantial refactoring.

OR, maybe, this means that type= isn't the best hook point and I should try building this as a validation rule or something that runs later in the schema lifecycle when all the latebound types have already been resolved. @rmosolgo are there any other hooks that you think might make sense to try?

I don't have the time or interest to entirely rewrite how late-bound types work; I've got a few more things to explore, but barring a better solution I'm probably going to drop this.

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