rmosolgo/graphql-ruby

`one_of` directive doesn't work in `InputObject`s with `ActionController::Parameters`

maciesielka opened this issue · 4 comments

maciesielka commented

Describe the bug

Using the one_of directive doesn't seem compatible for input objects when passing ActionController::Parameters as variables. Attempting to do so creates the following error:

NoMethodError: undefined method `size' for #<ActionController::Parameters {"string"=>"abc"} permitted: false>

I've added a failing test to confirm this behavior at master...maciesielka:graphql-ruby:maciesielka/one-of-action-parameters and hopefully that helps to understand the issue even if my explanation here doesn't do a great job.

Versions

graphql version: 2.3.2
rails: 7.0

GraphQL schema

Include relevant types and fields (in Ruby is best, in GraphQL IDL is ok). Any custom extensions, etc?

class OneOfInputObject < GraphQL::Schema::InputObject
  directive GraphQL::Schema::Directive::OneOf

  argument :int, GraphQL::Types::Int, required: false
  argument :string, GraphQL::Types::String, required: false
end

class OneOfOutput < GraphQL::Schema::Object
  field :string, GraphQL::Types::String
  field :int, GraphQL::Types::Int
end

class QueryType < GraphQL::Schema::Object
  field :one_of_field, OneOfOutput, null: false do
    argument :one_of_arg, OneOfInputObject, required: true
  end
  def one_of_field(one_of_arg:)
    one_of_arg
  end
end

class ApplicationSchema < GraphQL::Schema
  query QueryType
end

GraphQL query

Example GraphQL query and response (if query execution is involved)

query test($oneOfInput: OneOfInputObject!) {
  oneOfField(oneOfArg: $oneOfInput) {
    string
    int
  }
}

variables:

ActionController::Parameters.new(
  "variables" => {
    "oneOfInput" => { "string" => "abc" }
  }
)

Stacktrace

Full error 
NoMethodError: undefined method `size' for #<ActionController::Parameters {"string"=>"abc"} permitted: false>
    lib/graphql/schema/input_object.rb:191:in `validate_non_null_input'
    lib/graphql/schema/member/validates_input.rb:15:in `validate_input'
    lib/graphql/schema/non_null.rb:40:in `validate_input'
    lib/graphql/query/variables.rb:38:in `block in initialize'
    lib/graphql/query/variables.rb:19:in `each'
    lib/graphql/query/variables.rb:19:in `each_with_object'
    lib/graphql/query/variables.rb:19:in `initialize'
    lib/graphql/query.rb:257:in `new'
    lib/graphql/query.rb:257:in `block in variables'
    lib/graphql/query.rb:460:in `with_prepared_ast'
    lib/graphql/query.rb:256:in `variables'
    lib/graphql/query/validation_pipeline.rb:76:in `ensure_has_validated'
    lib/graphql/query/validation_pipeline.rb:33:in `valid?'
    lib/graphql/query.rb:324:in `valid?'
    lib/graphql/analysis/ast.rb:27:in `block (2 levels) in analyze_multiplex'
    lib/graphql/analysis/ast.rb:26:in `map'
    lib/graphql/analysis/ast.rb:26:in `block in analyze_multiplex'
    lib/graphql/tracing/trace.rb:32:in `analyze_multiplex'
    lib/graphql/tracing/legacy_trace.rb:21:in `block in analyze_multiplex'
    lib/graphql/tracing.rb:45:in `trace'
    lib/graphql/tracing/legacy_trace.rb:21:in `analyze_multiplex'
    lib/graphql/analysis/ast.rb:25:in `analyze_multiplex'
    lib/graphql/execution/interpreter.rb:46:in `block in run_all'
    lib/graphql/tracing/trace.rb:40:in `execute_multiplex'
    lib/graphql/tracing/legacy_trace.rb:29:in `block in execute_multiplex'
    lib/graphql/tracing.rb:45:in `trace'
    lib/graphql/tracing/legacy_trace.rb:29:in `execute_multiplex'
    lib/graphql/execution/interpreter.rb:37:in `run_all'
    lib/graphql/schema.rb:1323:in `multiplex'
    lib/graphql/schema.rb:1299:in `execute'

Steps to reproduce

See failing test added here

Expected behavior

The test linked below seems to be ensuring ActionController::Parameters should be okay to use as variables, so I'd expect them to work for one_of input objects too.

graphql-ruby/spec/integration/rails/graphql/query/variables_spec.rb

Lines 310 to 332 in 1c3af64

describe "with a ActionController::Parameters" do
let(:query_string) { <<-GRAPHQL
query getCheeses($source: DairyAnimal!, $fatContent: Float!){
searchDairy(product: [{source: $source, fatContent: $fatContent}]) {
... on Cheese { flavor }
}
}
GRAPHQL
}
let(:params) do
ActionController::Parameters.new(
"variables" => {
"source" => "COW",
"fatContent" => 0.4,
}
)
end
it "works" do
res = schema.execute(query_string, variables: params["variables"])
assert_equal 1, res["data"]["searchDairy"].length
end
end

Actual behavior

What specifically went wrong?

A runtime error occurs when attempting to pass ActionController::Parameters to a one_of input object.

Place full backtrace here (if a Ruby exception is involved):

Full error 
NoMethodError: undefined method `size' for #<ActionController::Parameters {"string"=>"abc"} permitted: false>
    lib/graphql/schema/input_object.rb:191:in `validate_non_null_input'
    lib/graphql/schema/member/validates_input.rb:15:in `validate_input'
    lib/graphql/schema/non_null.rb:40:in `validate_input'
    lib/graphql/query/variables.rb:38:in `block in initialize'
    lib/graphql/query/variables.rb:19:in `each'
    lib/graphql/query/variables.rb:19:in `each_with_object'
    lib/graphql/query/variables.rb:19:in `initialize'
    lib/graphql/query.rb:257:in `new'
    lib/graphql/query.rb:257:in `block in variables'
    lib/graphql/query.rb:460:in `with_prepared_ast'
    lib/graphql/query.rb:256:in `variables'
    lib/graphql/query/validation_pipeline.rb:76:in `ensure_has_validated'
    lib/graphql/query/validation_pipeline.rb:33:in `valid?'
    lib/graphql/query.rb:324:in `valid?'
    lib/graphql/analysis/ast.rb:27:in `block (2 levels) in analyze_multiplex'
    lib/graphql/analysis/ast.rb:26:in `map'
    lib/graphql/analysis/ast.rb:26:in `block in analyze_multiplex'
    lib/graphql/tracing/trace.rb:32:in `analyze_multiplex'
    lib/graphql/tracing/legacy_trace.rb:21:in `block in analyze_multiplex'
    lib/graphql/tracing.rb:45:in `trace'
    lib/graphql/tracing/legacy_trace.rb:21:in `analyze_multiplex'
    lib/graphql/analysis/ast.rb:25:in `analyze_multiplex'
    lib/graphql/execution/interpreter.rb:46:in `block in run_all'
    lib/graphql/tracing/trace.rb:40:in `execute_multiplex'
    lib/graphql/tracing/legacy_trace.rb:29:in `block in execute_multiplex'
    lib/graphql/tracing.rb:45:in `trace'
    lib/graphql/tracing/legacy_trace.rb:29:in `execute_multiplex'
    lib/graphql/execution/interpreter.rb:37:in `run_all'
    lib/graphql/schema.rb:1323:in `multiplex'
    lib/graphql/schema.rb:1299:in `execute'

Additional context

n/a

rmosolgo commented

Hey, sorry for the trouble and thanks for the detailed report. The gem's current suggestion is to use .to_unsafe_h on incoming parameters:

graphql-ruby/lib/generators/graphql/templates/graphql_controller.erb

Lines 38 to 39 in adb80e8

when ActionController::Parameters
variables_param.to_unsafe_hash # GraphQL-Ruby will validate name and type of incoming variables.

Although it's not usually the right thing to do, I think it makes sense in GraphQL's case because the query itself will validate the variable values.

What do you think of that approach?

maciesielka commented

The gem's current suggestion is to use .to_unsafe_h on incoming parameters:

👋🏻 I should've mentioned in my report that I'd seen that recommended in another not-quite-the-same-but-similar issue and tried it out but it ended up creating some additional side-quests in my real-world use case. Some of those complications on my end are worth solving on their own, but I figured I'd reach out about the support story since it seems unexpected that ActionController::Parameters are largely supported as variables, unless you happen to make use of one_of.

rmosolgo commented

Thanks for sharing that background info. What do you think of using .to_unsafe_h?

Yes, ActionController::Parameters mostly works for variables since the keys are referenced by name. But one_of requires mass access (.size), which ActionController::Parameters doesn't allow. Maybe it'd be theoretically possible to call .permit(...) and whitelist incoming keys, but that would require either significant ahead-of-time analysis (to know what keys the incoming query string expects to use) or additional runtime processing (eg, each argument and input object manually .permiting its keys). Since calling .to_unsafe_h is much easier, and doesn't cause any security issues in this case, I'm happy recommending it. Do you think it's a good fit in your case?

  • 👍1
maciesielka commented

Yup the reasoning makes sense to me and I think we'll be able to make that conversion. Appreciate the help!