Ignore unspecified arguments instead of raising exception

[pareeohnos]

trafficstars

Not sure if it's a setting that I've not found documented, but it would be useful to have the server simply ignore variables that haven't explicitly been defined rather than raising an error as it does currently.

As an example, in my client code I have an 'Invoice' model which contains 'lineItems'. For some login in the client side, each lineItem has a key property which is never persisted on the server. Currently I'm submitting the following to the server:

mutation createInvoice($name: String!, $description: String, $lineItems: [InvoiceLineItemAttributes!]!) {
  createInvoice(name: $name, description: $description, lineItems: $lineItems) {
    estimate {
      id
      name
      description
      number
      currencyId
      exchangeRate
      status
      projectId
      gdv
    }
  }
}

I am then trying to run the mutation (using vue-apollo) with

this.$apollo.mutate({
  mutation: createEstimateMutation,
  variables: {
    ...this.estimate
  }
})

Because the lineItems within this.estimate contain the key property, this is submit and the server is rejecting it as being an invalid argument.

The obvious fix is to remove it on the client side before submission, but obviously this becomes quite ugly and something I'd ideally like to avoid.

Is there a way of simply ignoring these additional fields?

[rmosolgo]

No, there's not a way to simply ignore them, but I can see the value here. I think there's some similar feature where we're more lax about incoming values, for example, accepting "100" as input for an integer variable.

Let's keep this open, maybe we can add it to the library. Thanks for the suggestion!

[pareeohnos]

No problem, thanks for the fantastic library :)

[mmahalwy]

@rmosolgo if you point me to the right places in the code, I might be up to adding this as an option if you'd like :)

[rmosolgo]

Sure, I'd be happy to give it a try. I think the best approach to this would be adding a test or two, and then hacking until it passes 😅 But here's the code that I bet will be involved:

Here's where errors from variable validation are added:

https://github.com/rmosolgo/graphql-ruby/blob/ef9986625009fe84d5c00589b1a7d84846508fc0/lib/graphql/query/variables.rb#L64-L66

And I think this is where input objects reject key-value pairs that don't match their definition:

https://github.com/rmosolgo/graphql-ruby/blob/ef9986625009fe84d5c00589b1a7d84846508fc0/lib/graphql/schema/input_object.rb#L194-L199

The catch is, ☝️ that code is used for both literal inputs (hardcoded in GraphQL strings) and variable inputs (passed in HTTP Post bodies, for example). Literal inputs should never accept undefined inputs -- that'd be a violation of the spec. Speaking of, here's there spec for variable values: http://spec.graphql.org/draft/#sec-Coercing-Variable-Values

Oh, and now that I'm looking at it, I see that the behavior requested here is also a violation of the spec:

The value for an input object should be an input object literal or an unordered map supplied by a variable, otherwise a query error must be thrown. In either case, the input object literal or unordered map must not contain any entries with names not defined by a field of this input object type, otherwise an error must be thrown.

(http://spec.graphql.org/draft/#sec-Input-Objects.Input-Coercion, emphasis mine)

That said, I agree that the feature here makes sense 🤔 . How could it be implemented so that GraphQL-Ruby's default behavior isn't contrary to the spec? Maybe applications that want this behavior could extend Types::BaseInputObject.coerce_input(value, context) in some way that would support this behavior. Honestly, that's probably the best idea -- invalid literal inputs would still be rejected by https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/static_validation/rules/arguments_are_defined.rb#L8-L20

For example, you might implement it like this:

class Types::BaseInputObject < GraphQL::Schema::InputObject 
  class << self 
    # Override these methods to ignore any undefined key-value pairs that may have come in from variables 
    def validate_non_null_input(value, context)
      trimmed_input = remove_undefined_pairs(value)
      super(trimmed_input, context)
    end

    def coerce_input(value, context)
      trimmed_input = remove_undefined_pairs(value)
      super(trimmed_input, context)
    end  
    
    private 
    
    def remove_undefined_pairs(input_hash)
      if input_hash.is_a?(Hash)
        input_hash.select { |k, v| !!get_argument(k) }
      else 
        # It's possible that the client sent garbage for this value; 
        # let the validation catch that later on.
        super 
      end 
    end 
  end
end 

How about giving that a try? Let me know what you find!

(Sorry about the rambling ... as you can see, I changed my mind mid-comment. Rather than clean up my tracks, I thought I'd document my thought process, in case something else stands out to you!)

[rmosolgo]

I agree that handling incoming variables could be better, and I'd welcome further discussion in a new issue, but I don't have plans to work on this right now, so I'll close it out.