rmosolgo / graphql-ruby

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

Return ordered result hashes #5060

Open rmosolgo opened 3 months ago

rmosolgo commented 3 months ago

Fixes #5011

The spec says that JSON key-value pairs should come back in the same order that they were requested in the query. I need an implementation that complies with the spec and adds as little overhead as possible (memory and runtime). Possibly, if it adds noticeable overhead, make it opt-in, since this is critical performance code

I need to somehow maintain the sequence of this result even when Dataloader or GraphQL-Batch write to it out of order (see #4252).

I think the selections_by_name hash(es) have the correct order of keys here: https://github.com/rmosolgo/graphql-ruby/blob/d9bd9cf9ac6f01a9aa36c30a025bfa922eda4f46/lib/graphql/execution/interpreter/runtime.rb#L120-L192

I can't find any way to re-order a Ruby Hash. So if I'm going to use a Hash, need to write the keys in the right order the first time. (Technically, you can move a key to the back by doing h[k] = h.delete(k), but I don't think that's a promising approach.)

Another approach might be to use an Array to hold values and then build a Hash from there. For example, an array of [k1, v1, k2, v2, ...] could be used. One problem here is that it will require a second pass through the whole data structure. GraphQL-Ruby used to do this, but it was slow. I'd like to not make it do that again.

Maybe I could mitigate the downside of the second iteration by implementing Result#to_json to return a properly-ordered string. This would work out-of-the-box with Rails... but I know a lot of JSON serializers are really optimized. Could GraphQL-Ruby deliver good performance in that case? (And if I take that approach, I should implement .to_ordered_h and test it against .to_ordered_h.to_json with an optimized JSON serializer.)

graphql-js handles this by writing promises into the result hash. This is also how GraphQL-Ruby happens to handle GraphQL::Execution::Lazy instances (eg, GraphQL-Batch). Perhaps a similar approach can be made to work with Dataloader. But the challenge is cleaning up those placeholders. A Promise has its own catch block to do that, but Dataloader doesn't have that... (yet?)