Fix two bugs (order dependency and false positive) in the `required` validation #5210

harrylewis · 2025-01-21T18:08:27Z

The goal of this PR is to fix two bugs identified in the required validation.

Bug #1: Order dependency for matching sets

When defining a required validation, if the first option defined is a set and that option is matched, the validation will pass, even if other options in the mutually exclusive group would match.

Consider the following query.

{
  findObject(node_id: '1', object_type: 'Invoice', object_id: '1') {
    id
  }
}

The behaviour of the required validation is different depending on how the validation is defined.

field :find_object, Node, null: true do
  argument :node_id, ID, required: false
  argument :object_type, String, required: false
  argument :object_id, Integer, required: false

  # This will return a validation error.
  validates required: { one_of: [:node_id, [:object_type, :object_id]] }

  # This will NOT return a validation error.
  validates required: { one_of: [[:object_type, :object_id], :node_id] }
end

This is addressed in f2e4c0d.

Bug #2: False positive for partially matched sets

When defining a required validation, if any of the options is matched alongside a set that is partially matched, the validation will pass. This violates the mutual exclusion rule.

Consider the following query.

{
  findObject(node_id: '1', object_type: 'Invoice') {
    id
  }
}

I would expect this to fail, given the following field definition.

field :find_object, Node, null: true do
  argument :node_id, ID, required: false
  argument :object_type, String, required: false
  argument :object_id, Integer, required: false

  # This will NOT return a validation error.
  validates required: { one_of: [:node_id, [:object_type, :object_id]] }
end

This is addressed in f2e4c0d.

Note: I came across this behaviour during development, and was surprised and confused by it. I can anticipate a perspective that deems this behaviour as acceptable because it still technically satisfies the property that "exactly one" option is matched. While I can appreciate this perspective, in my opinion it leaves room for ambiguity in the behaviour of validation.

Developer impact

The first bug requires a developer to be specific when defining the order of the mutually exclusive options. This leads to a brittle implementation, where a seemingly innocuous reordering of the options can causing a breaking change in the API.

The second bug does not uphold the contract of "exactly one" and "mutually exclusive" that the required validation is meant to provide, in my opinion. If I am checking for the presence of arguments in the resolver to determine which of the mutually exclusive option has been provided, depending on the order I check the presence of the keys, if one of those options is a set that has been partially matched, I could attempt to access the other keys in the set which have not matched and cause a runtime error.

Breaking changes

I consider both of these bug fixes to be breaking changes. A client passing a certain collection of inputs which may be valid under the existing behaviour may now experience a validation error, if they are not providing strictly mutually exclusive arguments.

When using the required validator, if the first option is a set and it is matched, the validation will always be valid, even if other options in the mutually exclusive group would match. This is due to the presence of a `break` statement, which will cause a premature exit if a set option is matched.

When using the required validator, if any of the options is a set that is partially matched alongside another match, the validation is valid, which violates the mutual exclusion of the group.

harrylewis

I see CI is failing for certain checks. I see the same failures on other PR's, so it doesn't appear to be related to my changes. I'll take a look and see what is going wrong.

harrylewis · 2025-01-21T18:10:29Z

lib/graphql/schema/validator/required_validator.rb

-                  matched_conditions += 1
-                  break
+                full_match = one_of_condition.all? { |k| value.key?(k) }
+                partial_match = !full_match && one_of_condition.any? { |k| value.key?(k) }


I discovered that #some? is only available in Rails 🙃

harrylewis · 2025-01-21T18:11:16Z

lib/graphql/schema/validator/required_validator.rb

-          if matched_conditions == 1
+          if fully_matched_conditions == 1 && partially_matched_conditions == 0


A partial match alongside a full match violates a stricter definition of "exactly one" and "mutually exclusive".

harrylewis · 2025-01-21T18:13:23Z

spec/graphql/schema/validator/required_validator_spec.rb

+        { query: "{ validated: multiValidated(a: 1, b: 2) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: a, (b and c)."] },
+        { query: "{ validated: multiValidated(a: 1, c: 3) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: a, (b and c)."] },


These tests exercise the partial matches. Now every combination of the three arguments in this example is evaluated. This has been duplicated below for other cases.

harrylewis · 2025-01-21T18:13:31Z

spec/graphql/schema/validator/required_validator_spec.rb

+    {
+      name: "Definition order independence",
+      config: { one_of: [[:a, :b], :c] },
+      cases: [
+        { query: "{ validated: multiValidated(c: 1) }", result: 1, error_messages: [] },
+        { query: "{ validated: multiValidated(a: 2, b: 3) }", result: 5, error_messages: [] },
+        { query: "{ validated: multiValidated }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: (a and b), c."] },
+        { query: "{ validated: multiValidated(a: 1, b: 2, c: 3) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: (a and b), c."] },
+        { query: "{ validated: multiValidated(a: 1, c: 3) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: (a and b), c."] },
+        { query: "{ validated: multiValidated(b: 2, c: 3) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: (a and b), c."] },
+        { query: "{ validated: multiValidated(a: 3) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: (a and b), c."] },
+        { query: "{ validated: multiValidated(b: 2) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: (a and b), c."] },
+      ]
+    },


This exercises the order dependency issue.

harrylewis · 2025-01-21T18:14:49Z

lib/graphql/schema/validator/required_validator.rb

                end
              when Array
-                if one_of_condition.all? { |k| value.key?(k) }
-                  matched_conditions += 1
-                  break


This break statement is what caused the order dependency issue. I looked in the Git history to see why it was present. It existed in the original commit, without an explicit explanation. Upon further evaluation and experimentation, I discovered it led to the order dependency mentioned. Removing it outright fixes this order depedency.

harrylewis added 2 commits January 21, 2025 08:07

Fix false positive in required validator

f5e2fe1

When using the required validator, if any of the options is a set that is partially matched alongside another match, the validation is valid, which violates the mutual exclusion of the group.

harrylewis commented Jan 21, 2025

View reviewed changes

harrylewis marked this pull request as ready for review January 21, 2025 18:24

harrylewis changed the title ~~Fix two bugs (order dependency and false positive) in the required valiation~~ Fix two bugs (order dependency and false positive) in the required validation Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix two bugs (order dependency and false positive) in the `required` validation #5210

Fix two bugs (order dependency and false positive) in the `required` validation #5210

harrylewis commented Jan 21, 2025 •

edited

Loading

harrylewis left a comment

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

		if matched_conditions == 1
		if fully_matched_conditions == 1 && partially_matched_conditions == 0

		{ query: "{ validated: multiValidated(a: 1, b: 2) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: a, (b and c)."] },
		{ query: "{ validated: multiValidated(a: 1, c: 3) }", result: nil, error_messages: ["multiValidated must include exactly one of the following arguments: a, (b and c)."] },

Fix two bugs (order dependency and false positive) in the required validation #5210

Are you sure you want to change the base?

Fix two bugs (order dependency and false positive) in the required validation #5210

Conversation

harrylewis commented Jan 21, 2025 • edited Loading

Bug #1: Order dependency for matching sets

Bug #2: False positive for partially matched sets

Developer impact

Breaking changes

harrylewis left a comment

Choose a reason for hiding this comment

harrylewis Jan 21, 2025

Choose a reason for hiding this comment

harrylewis Jan 21, 2025

Choose a reason for hiding this comment

harrylewis Jan 21, 2025

Choose a reason for hiding this comment

harrylewis Jan 21, 2025

Choose a reason for hiding this comment

harrylewis Jan 21, 2025

Choose a reason for hiding this comment

Fix two bugs (order dependency and false positive) in the `required` validation #5210

Fix two bugs (order dependency and false positive) in the `required` validation #5210

harrylewis commented Jan 21, 2025 •

edited

Loading