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 🙃

I think we could do it in a single iteration of one_of_condition:

Suggested change

partial_match = !full_match && one_of_condition.any? { |k| value.key?(k) }

any_match = false

full_match = true

one_of_condition.each do |k|

if value.key?(k)

any_match = true

else

full_match = false

end

end

partial_match = any_match && !full_match

Thanks for this suggestion, I'll go ahead and implement that!

Is this suggestion motivated by potential performance concerns? My GraphQL performance knowledge is limited. For most cases I imagine that one loop versus two loops for a relatively small amount of options would have a negligible impact. Of course though, it is still additional CPU cycles. But this is where my expertise and use case awareness falls off - are there use cases where this could potentially cause a performance impact?

Yes, it was performance-minded. In practice, I can't think of a situation where this would be called in a tight loop and become a real bottleneck. But I have certainly been surprised before about how rubber meets the road when GraphQL-Ruby is used in real-world applications.

In micro-benchmarks, it makes a difference, for example:

# loops.rb require "benchmark/ips" one_of_condition = [:a, :b, :c, :d] value = { a: 1, b: 2, c: 3 } Benchmark.ips do |x| x.report(".all + .any") do full_match = one_of_condition.all? { |k| value.key?(k) } partial_match = !full_match && one_of_condition.any? { |k| value.key?(k) } end x.report(".each") do any_match = false full_match = true one_of_condition.each do |k| if value.key?(k) any_match = true else full_match = false end end partial_match = any_match && !full_match end x.compare! end

Without YJIT, .all + .any is 23% slower:

$ ruby loops.rb ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +PRISM [x86_64-darwin22] Warming up -------------------------------------- .all + .any 247.839k i/100ms .each 273.355k i/100ms Calculating ------------------------------------- .all + .any 2.269M (± 5.2%) i/s (440.78 ns/i) - 11.401M in 5.039404s .each 2.797M (± 4.3%) i/s (357.52 ns/i) - 14.214M in 5.091697s Comparison: .each: 2797017.5 i/s .all + .any: 2268712.9 i/s - 1.23x slower

Interestingly, with YJIT enabled, there's a much bigger difference (greater than 2x faster with .each):

$ ruby --yjit loops.rb ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +YJIT +PRISM [x86_64-darwin22] Warming up -------------------------------------- .all + .any 324.644k i/100ms .each 759.224k i/100ms Calculating ------------------------------------- .all + .any 3.406M (± 5.4%) i/s (293.57 ns/i) - 17.206M in 5.066546s .each 8.336M (± 5.4%) i/s (119.95 ns/i) - 41.757M in 5.025170s Comparison: .each: 8336485.6 i/s .all + .any: 3406394.2 i/s - 2.45x slower

Maybe YJIT has special handling for Array#each 🤷

I really appreciate you taking the time to explain this, and provide this fulsome example. There is clearly a difference here. While it is minimal in this context, it's easy to address and removes any potential for this to be an added source of slowness.

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.

I'll have to ask the guy who wrote it 😅 I bet I was thinking "at least one of", not "exactly one of", but exactly one-of is the right idea.

rmosolgo

Hey, thanks for the detailed writeup and code change. I just have one suggestion on the implementation.

rmosolgo · 2025-01-28T15:51:15Z

lib/graphql/schema/validator/required_validator.rb

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


I'll have to ask the guy who wrote it 😅 I bet I was thinking "at least one of", not "exactly one of", but exactly one-of is the right idea.

rmosolgo · 2025-01-28T18:25:56Z

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 think we could do it in a single iteration of one_of_condition:

Suggested change

partial_match = !full_match && one_of_condition.any? { |k| value.key?(k) }

any_match = false

full_match = true

one_of_condition.each do |k|

if value.key?(k)

any_match = true

else

full_match = false

end

end

partial_match = any_match && !full_match

rmosolgo · 2025-01-29T13:40:45Z

Thanks again for this fix!

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

rmosolgo reviewed Jan 28, 2025

View reviewed changes

Determine full/partial match with one loop

8a7622a

rmosolgo added this to the 2.4.9 milestone Jan 29, 2025

rmosolgo merged commit a466893 into rmosolgo:master Jan 29, 2025
13 of 15 checks passed

harrylewis deleted the bug-fixes-in-required-validator branch January 29, 2025 17:20

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

rmosolgo Jan 28, 2025

harrylewis Jan 28, 2025

rmosolgo Jan 29, 2025

harrylewis Jan 29, 2025

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

harrylewis Jan 21, 2025

rmosolgo Jan 28, 2025

rmosolgo left a comment

rmosolgo Jan 28, 2025

rmosolgo Jan 28, 2025

rmosolgo commented Jan 29, 2025

-                partial_match = !full_match && one_of_condition.any? { |k| value.key?(k) }
+                any_match = false
+                full_match = true
+                one_of_condition.each do |k|
+                  if value.key?(k)
+                    any_match = true
+                  else
+                    full_match = false
+                  end
+                end
+                partial_match = any_match && !full_match

		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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmosolgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmosolgo commented Jan 29, 2025

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