Conversation
|
|
||
| # Compute maximum possible cost of child selections; | ||
| # composites merge their maximums, while leaf scopes are always zero. | ||
| # FieldsWillMerge validation assures all scopes are uniformly one or the other. |
There was a problem hiding this comment.
With FieldsWillMerge getting fixed, this should now be consistent about always having scopes of the same kind. Either way, it's written to be fault-tolerant in the event that it's run without first validating.
|
|
||
| # Identify the maximum cost and scope among possibilities | ||
| maximum_cost = 0 | ||
| maximum_scope = child_scopes.reduce(child_scopes.last) do |max_scope, possible_scope| |
There was a problem hiding this comment.
This switches to always hill-climb for the maximum possibility among scopes, regardless if they are leaf or composite scopes.
| field_cost = composite_scopes.last.own_complexity(child_complexity) | ||
| field_complexity(composite_scopes.last, max_complexity: field_cost, child_complexity: child_complexity) | ||
| end | ||
| field_complexity( |
There was a problem hiding this comment.
Lastly, always trigger exactly one hook per field with the resolved maximums among scopes, costs, and children costs. Before leaf scopes would receive nil as their child_complexity – now they receive 0. This feels more inline with the own_complexity call that has always sent zero child cost to leaf scopes. If the hook cares to differentiate, it can always inspect the scope to determine if there are child selections.
gmac
force-pushed
the
gmac/v3_complexity_update
branch
from
bd94574 to
d325c38
Compare
|
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. |
|
I just updated this branch with master and started on a migration scheme to help people cross from legacy to future algorithm. I'll add some proper tests for the migration code before I merge this. |
| complexity_cost_calculation_mode(:future) # or `:legacy`, `:compare` | ||
|
|
||
| GRAPHQL | ||
| max_possible_complexity |
There was a problem hiding this comment.
Hi @rmosolgo , you mention that there is a need to "opt into the future behavior", but from what I can see here, :future is already the new mode by default if nothing is configured explicitly ? (by the way our logging system got spammed by the warning above, but we quickly noticed it)
There was a problem hiding this comment.
Derp, you're right -- I've opened #5343 to address it.
3 participants
I noticed a
v3milestone, 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...
{ 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 } }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.
Leaf fields would trigger hooks N-times for each leaf possibility, when really we should expect only one hook per field.
Compounding upon item 3, there was also a crazy side effect here from Type comparison missing from
FieldsWillMergevalidation #4403. BecauseFieldsWillMergewas faulty about type consistency (fix slated for3.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.