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 argument defaults #3476

Closed
wants to merge 1 commit into from

Conversation

eapache
Copy link
Contributor

@eapache eapache commented May 12, 2021

I'm not sure how I feel about this solution:

  • it's as lazy as possible to avoid late-bound types, which it does in
    the test cases we've got right now, but I don't think there's any
    guarantee?
  • it's so lazy it might just trigger during the first query execution
    rather than during schema boot, which seems bad and also leads to a
    random exception being raised because we have no good error-handling in
    this path
  • it's not cachable because it's context-dependent so it adds some small
    overhead of revalidation on every access of default_value

@rmosolgo @benjie this technically addresses #3248 and supersedes #3448 but... oof.

I'm not sure how I feel about this solution:
- it's as lazy as possible to avoid late-bound types, which it does in
the test cases we've got right now, but I don't think there's any
guarantee?
- it's so lazy it might just trigger during the first query execution
rather than during schema boot, which seems bad and also leads to a
random exception being raised because we have no good error-handling in
this path
- it's not cachable because it's context-dependent so it adds some small
overhead of revalidation on every access of default_value
@benjie
Copy link

benjie commented May 13, 2021

Does the default value need to be context dependent? Can you use a dummy context instead?

@eapache
Copy link
Contributor Author

eapache commented May 13, 2021

We can use a dummy-value, which would make this cacheable, though that feels weird in the paths where we have a real context available still (which we definitely can't cache).

The late-bound type handling and error/exception flow are bigger issues with this approach I think.

@@ -250,7 +258,7 @@ def coerce_into_values(parent_object, values, context, argument_values)
value = values[arg_key]
elsif default_value?
has_value = true
value = default_value
value = default_value(context)
Copy link
Contributor Author

@eapache eapache May 13, 2021

Choose a reason for hiding this comment

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

@rmosolgo I'm looking at this a little more and I realize I'm confused. We call coerce_input on this value a few lines down, which at best is a no-op but (since this value should already be specified as ruby-land-objects and not graphql-syntax-input) would most likely fail entirely even on main branch without this PR? What am I missing?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm admittedly fuzzy on the details but the intent here is to prepare some GraphQL input for handling by the application. (Eventually, argument_values here will populate a GraphQL::Execution::Interpreter::Arguments instance.) So, unless I've misunderstood, I'd expect type.coerce_input(...) below to prepare a GraphQL value for handling by Ruby. (Eg, turning enum strings into their value: ...s.)

But maybe that's the wrong approach for default_values, if they're already Ruby-style!

@eapache
Copy link
Contributor Author

eapache commented May 13, 2021

So barring my confusion (#3476 (comment)) about the way coerce_input is used, there's a slightly more/less hacky proposal of this approach:

  • only do the validation within coerce_into_values where I'm pretty sure we're actually guaranteed to not have any late-bound types anymore (correct @rmosolgo??)
  • always use NullContext for the validation (even though coerce_into_values has its own context already) so that it can be cached and saved
  • wrap it in context.schema.error_handler.with_error_handling to provide at least some semblance of a user hook for error handling

This would still suffer from the issue that it definitely only triggers at first query execution to use the argument, but... better than nothing I guess? It also suffers from potential issues that other flows which use the default values (e.g. introspection, converting to/from SDL, etc) might just use the invalid default straight up still, since when they run we can't be confident that enough late-bound types are resolved to successfully validate it in the first place.

How would this feel?

@rmosolgo
Copy link
Owner

rmosolgo commented May 13, 2021

Personally, I'm pretty keen on this. You're right that, annoyingly, you wouldn't find out that the default was invalid until a query comes in. But it's much simpler than trying to validate it at boot time, and making it context-dependent could come in handy.

As for the first-query-might-raise-an-error problem, maybe you could work around that by calling argument.default_value # just to check if it raises during the Schema.add_type flow. (But then, you run into the late-bound type problem. Better to just ignore it for now, and add it later if we actually encounter a problem?)

@eapache
Copy link
Contributor Author

eapache commented May 13, 2021

FWIW 30722fb is a draft of the other variation I mentioned (do it only at runtime, don't use context so it's cacheable, use with_error_handling, etc).

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.

3 participants