rmosolgo / graphql-ruby

Ruby implementation of GraphQL
http://graphql-ruby.org
MIT License
5.38k stars 1.39k forks source link

Optimization - Skip processing non-existing fields in FieldsWillMerge #5125

Closed francisbeaudoin closed 1 month ago

francisbeaudoin commented 1 month ago

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 commented 1 month ago

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

rmosolgo commented 1 month ago

Thanks for this improvement!