Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dataloaders are changing the response order #4252

Open
cesarjr opened this issue Nov 21, 2022 · 4 comments
Open

Dataloaders are changing the response order #4252

cesarjr opened this issue Nov 21, 2022 · 4 comments

Comments

@cesarjr
Copy link

cesarjr commented Nov 21, 2022

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
Copy link
Owner

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
Copy link
Author

cesarjr commented Nov 22, 2022

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
Copy link

@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
Copy link
Author

cesarjr commented Dec 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants