rmosolgo / graphql-ruby

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

Dataloaders are changing the response order #4252

Open cesarjr opened 1 year ago

cesarjr commented 1 year ago

Describe the bug

When a dataloader is used, the order of fields in the response is affected.

All the fields using dataloaders are delivered after the others fields.

According to GraphQL Specs the order of the fields in the answer must follow the order of fields in the query.

Versions

ruby-graphql: 2.0.15 rails: 7.0.4 ruby: 3.0.3

GraphQL schema

I've prepared a complete app reproducing the problem using rspec.

https://github.com/cesarjr/graphql-dataloader

Include relevant types and fields (in Ruby is best, in GraphQL IDL is ok). Any custom extensions, etc?

class Types::Person < Types::BaseObject
  field :id, ID,
        null: false

  field :name, String,
        null: false,
        description: 'Name.'

  field :city_id, ID,
        null: false,
        description: 'City ID.'

  field :city, Types::City,
        null: false,
        description: 'City.'

  field :seller_id, ID,
        null: false,
        description: 'Seller ID.'

  field :seller, Types::Seller,
        null: false,
        description: 'Seller.'

  # comment the lines below if you want to see
  # the specs passing.
  def city
    dataloader
      .with(Sources::ActiveRecordObject, City)
      .load(object.city_id)
  end
end

class GraphqlDataloaderSchema < GraphQL::Schema
  mutation(Types::MutationType)
  query(Types::QueryType)

  # For batch-loading (see https://graphql-ruby.org/dataloader/overview.html)
  use GraphQL::Dataloader
  # …
end

GraphQL query

Example GraphQL query and response (if query execution is involved)

query { 
    getPeople {
        id
        name
        city {
            id
            name
            state
        }
        seller {
            id
            name
          }
    }
}

Steps to reproduce

Steps to reproduce the behavior

Expected behavior

{
    "data": {
        "getPeople": [
            {
                "id": "1",
                "name": "Carl",
                "city": {
                    "id": "1",
                    "name": "Rio Branco",
                    "state": "Acre"
                },
                "seller": {
                    "id": "1",
                    "name": "John"
                }
            }
        ]
    }
}

It is expected that city to be returned first than seller as it was queried.

Actual behavior

{
    "data": {
        "getPeople": [
            {
                "id": "1",
                "name": "Carl",
                "seller": {
                    "id": "1",
                    "name": "John"
                },
                "city": {
                    "id": "1",
                    "name": "Rio Branco",
                    "state": "Acre"
                }
            }
        ]
    }
}

Look that seller is being returned before than city.

Additional context

I created an app very simples rails app reproducing the problem.

https://github.com/cesarjr/graphql-dataloader

$ git clone https://github.com/cesarjr/graphql-dataloader
$ cd graphql-dataloader
$ bundle install
$ bin/rails db:create
$ bin/rails db:migrate
$ bin/rspec
rmosolgo commented 1 year ago

Hi! Thanks for the detailed report. Although it's in the spec, GraphQL-Ruby has never actually enforced key-value pair order in the result set 🙈 (Besides using dataloader, using GraphQL-Batch would also cause the results to be out-of-order.)

I'm definitely open to matching the spec here, but out of curiosity, how did this become a problem for you?

cesarjr commented 1 year ago

Hi! First of all, it's a pleasure for me to be answered by you.

You did a great job with ruby-graphql. Actually, you still have done a great job. Thank you so much.

Well, let me explain how it is affecting me.

I'm applying ruby-graph in a huge software. We're doing this about two years. During this time we're maintaining rest and graphql. Some parts of the sofware are already using graphql but others not.

In our rest controllers we're using graphql internally instead of regular Rails. It helped us to write all the resolvers and queries safely.

In theses rest controllers, we have tests comparing the result (of graphql) with the old serializer (active model serializer).

In our app, we're solving n+1 problem using active record include instead of any other technology. It's not a problem for us right now, but I confess that it destroys my soul to know that I can do a better job 😂.

So this weekend I resolved to study how to resolve this correctly and so stumbled in this problem. All the rest test has broken because the keys are in a different order between rest and graphql.

I really don't know if it is a real problem because it's not common for me to use others graphql APIs different than ours. So I just though it was a bug.

Please let me know if you need something else.

Thanks!

olivierbuffon commented 9 months ago

@cesarjr I'm a bit late in this thread but if it still something that's causing you issues you should take a look at this gist It's another way to deal with ActiveRecord associations with dataloader. We're using it in production and it's working like a charm.

cesarjr commented 9 months ago

Thanks, @olivierbuffon ! I'm going to test and keep you in touch.