graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

[v3] Complexity cost bug fixes

Open gmac opened this issue 1 year ago • 1 comments

I noticed a v3 milestone, so thought this would be a good time to submit a fix for several bugs in the complexity algorithm that have the potential to increase costs (slightly and rarely) and thus would technically be breaking.

Here's what's been weird...

  1. When multiple composite field possibilities merged, only the final selection was fully costed out. This made selection order matter, and defeated idempotence. The faulty assumption here was actually called out in a comment for years.
{
   entity {
     ...on Producer { cheese { kind } } # `cheese` costs 10 on Producer
     ...on Farm { cheese { kind } } # `cheese` costs 5 on Farm
     
      # last costed possibility wins => 5 + 1 + 1
      # should be MAX of possibilities => 10 + 1 + 1
   }
}
  1. The same issue as above also happened with merged leaf fields, but for a different reason. Leaf possibilities always directly assigned their field cost rather than hill-climbing for a maximum. Once again, last selection wins.

  2. Leaf fields would trigger hooks N-times for each leaf possibility, when really we should expect only one hook per field.

  3. Compounding upon item 3, there was also a crazy side effect here from https://github.com/rmosolgo/graphql-ruby/issues/4403. Because FieldsWillMerge was faulty about type consistency (fix slated for 3.0), it meant that the fields being merged could intermix leaf and composite scopes – which each had their own workflow for triggering hooks. So, in addition to leaf field hooks triggering multiple times, an additional hook could trigger once for composite possibilities.

gmac avatar Feb 15 '24 04:02 gmac

Hey, thanks so much for this detailed report and fix. As you can see, it's been a while since I dug into the complexity implementation! I'll review these closely soon.

rmosolgo avatar Feb 15 '24 16:02 rmosolgo