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

Fix for double scope_items method call (Issue #4253) #4254

Conversation

andrey-lizunov
Copy link

You can find the description of the case in this issue.

@rmosolgo
Copy link
Owner

I think this would remove one of the calls to .scope_items, but if I understand correctly, I think it would remove the call to the connection field instead of the call to the list of nodes and/or edges. I don't think we want this for two reasons:

  • For pagination, we want .scope_items to be called at the connection level, not during the list of nodes. That way, the final list of nodes and hasNextPage/hasPreviousPage would be based on the scoped list instead of the unscoped list.
  • If we made a query that used both nodes { ... } and edges { ... }, we'd still get .scope_items called twice. (Currently in the library, it'd be called three times 🤦 ).

It strikes me now that one option might be to add scope: false to the default options for nodes and edges fields:

base_field_options = {
name: :edges,
type: [edge_type_class, null: edge_nullable],
null: edges_nullable,
description: "A list of edges.",
connection: false,
}

base_field_options = {
name: :nodes,
type: [@node_type, null: nullable],
null: nullable,
description: "A list of nodes.",
connection: false,
}

I suppose you could even use that as a work-around in your app in the meantime:

class Types::BaseConnection < Types::BaseObject 
  include GraphQL::Types::Relay::ConnectionBehaviors 
  # don't re-apply scoping to these fields, see https://github.com/rmosolgo/graphql-ruby/issues/4253
  nodes_field(field_options: { scope: false })
  edges_field(field_options: { scope: false }) 
end 

Want to give it a try?

@rmosolgo
Copy link
Owner

I think this was addressed by #4263 👍

@rmosolgo rmosolgo closed this Dec 14, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants