-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
default values for invisible arguments #5198
Comments
Hey, yeah, I don't think it'd be good to change the current behavior from a compatibility standpoint. I don't know of a good way of accomplishing this now apart from kwargs with duplicated default values, like you mentioned above (and this doesn't really count as a good way...). A couple of other possibilities that come to mind:
|
Hum, yeah. What about making the args hash "strict" (is that a thing that ruby lets you do?) so that accessing a missing key at least raises instead of producing unexpected/undefined behaviour? Also, I know @derek-etherton-opslevel / @Farjaad were bit by a variant of this issue recently as well, maybe they'll have opinions on how to best avoid it. |
No, afaik Ruby just makes a plain hash out of Another edge-casey thing about the example above is that, since require "bundler/inline"
gemfile do
gem "graphql", "2.4.10"
end
class MySchema < GraphQL::Schema
class Query < GraphQL::Schema::Object
field :echo, String do
argument :foo, Boolean, required: false, default_value: true
end
def echo(**kwargs)
kwargs[:foo].inspect
end
end
query(Query)
end
pp MySchema.execute("{ echo(foo: null) }").to_h
# {"data" => {"echo" => "nil"}} |
If you have an argument with a
default_value:
, and the argument is not visible for some reason, then the key (and thus default value) is not even present in the arguments hash. This is logically correct, but has caught us in a bug a couple of time where somebody will write, e.g.And then the wrong branch will execute when somebody runs against the public schema (i.e. with
foo
not visible) because it will be nil and interpreted asfalse
instead of the desired "default" oftrue
.The current behaviour is correct/coherent, and there are a couple of obvious ways around this (e.g. listing out all of the args as kwargs to the resolve method, which requires duplicating all the default values and is annoying). But I wanted to ask in case anybody has a better pattern for avoiding this, or if the description of the problem prompts a clever idea for a solution.
It may not be spec-compliant / probably causes just as many problems as it solves, but injecting the default value even for invisible arguments is kind of the most "obvious" solution here.
The text was updated successfully, but these errors were encountered: