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

Optimization - Skip processing non-existing fields in FieldsWillMerge

Open francisbeaudoin opened this issue 1 year ago • 1 comments

Context

It was observed that if a query includes thousands of non-existing fields, it takes a lot of processing time within the FieldsWillMerge#find_conflicts_within.

What

The iteration loop represents n(n-1^2)/2, where n is the number of duplicated fields being queried, and the underlying call to #find_conflict is using considerable time. For example, a query with 2000 times the same non-existing field i.e.query { a a ... 2000 times } takes ~6000 ms to process and skipping these fields reduces the response time to ~500ms.

Optimization

As far as I can tell, skipping non-existing fields from the FieldsWillMerge rule doesn't alter the business logic.

The only caveat of this optimization, is that if a service using this library was relying on the Schema#validate_timeout to safeguard from a potentially malicious query, it may no longer reach the configured timeout but rather instead return multiple undefinedField errors, which can be mitigated by the Schema#validate_max_errors.

francisbeaudoin avatar Oct 15 '24 18:10 francisbeaudoin

As an alternative, we may be able to further reduce the processing time by not adding the fields entirely here by conditionality adding on unless definition.nil? but I'm not clear on all of the implications hence if it makes sense.

https://github.com/rmosolgo/graphql-ruby/blob/8c0e25848e3615c639e2702daf4ab5e7543585b9/lib/graphql/static_validation/rules/fields_will_merge.rb#L345

francisbeaudoin avatar Oct 15 '24 20:10 francisbeaudoin

Thanks for this improvement!

rmosolgo avatar Oct 22 '24 14:10 rmosolgo