spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.53k stars 306 forks source link

Document behavior when both `first` and `last` are specified for pagination #1055

Closed cocet33000 closed 2 months ago

cocet33000 commented 2 months ago

Hello! I have a question about GraphQL cursor-based pagination.

Current situation

Request

query {
  items(
    first: 6
    after: "T18z"
    last: 2
    before: "T185"
  ){
    edges {
      node {
        id
      }
    }
  }
}

※ T18z (O_3), T185 (O_9)

ScrollSubrange Object at Controller

image Implicitly first and afeter values are selected

Issues

  1. Potential confusion for client
  2. May lead to unintended results
  3. Differs from common GraphQL practices

What I've checked

I've looked through official docs, source code, and forums, but found no explanation of this behavior. If I've missed any information, please let me know!

Suggestions

  1. Prohibit simultaneous use
  2. Clearly document the behavior
  3. Implement a warning system

Points for discussion

  1. Is this behavior intentional?
  2. Should we align with common GraphQL practices?
  3. What would be the impact of changing this?

I'd love to hear your thoughts and ideas!

bclozel commented 2 months ago

Differs from common GraphQL practices

I'm not aware of such practices. Can you explain and point us to existing resources about this? As far as I'm aware, the behavior enforced here is aligned with this note in the spec:

Including a value for both first and last is strongly discouraged, as it is likely to lead to confusing queries and results. The PageInfo section goes into more detail here.

Also in the PageInfo section:

When both first and last are included, both of the fields should be set according to the above algorithms, but their meaning as it relates to pagination becomes unclear. This is among the reasons that pagination with both first and last is discouraged.

I think we could indeed reject this combination entirely because supporting this can only lead to strange results and inefficient data fetching for database engines. I'll discuss that with the rest of the team.

rstoyanchev commented 2 months ago

@cocet33000 we've discussed this. Indeed if we find first/after, we look no further, which provides lenient handling, and allows pagination to work despite the confusing input, assuming the client makes automated decisions based on PageInfo.

We are okay to document this choice, but could you clarify what you are referring to by common GraphQL practices? The spec does not suggest the request should be rejected, but only that clients shouldn't do this. It would be good to hear also how you came across this and how it worked out from a user perspective.

cocet33000 commented 2 months ago

Thank you so much for your prompt response.

https://relay.dev/graphql/connections.htm#sec-Pagination-algorithm I did not know about the You are absolutely right. >The statement “Differs from common GraphQL practices” was incorrect. There was no basis for this statement. 🙇 🙇

Personally, I would.

reject this combination entirely

is also a good idea. I support it. Please consider this.

rstoyanchev commented 2 months ago

Thanks for the additional feedback. We have considered it, but prefer more lenient handling, which works in more cases, unless of course there is a good reason to reject, and so far I don't see any. If the client has specified first/after there is enough for us to act on. Pagination should still work, or somewhat work, and the user will see some data.

cocet33000 commented 2 months ago

Thanks!!

May I ask another question? I will create a new question if needed.

The relay specification states the following.

All types whose name ends in “Connection” are considered Connection Types in this specification. Object” is defined in the ‘Type System’ section of the GraphQL specification.

It is also stated that “Object” is defined in the “Type System” section of the GraphQL specification. In fact, the framework already creates a Connection with edge and pageInfo according to the specification.

I would like to add totalCount fields as in this example, can I extend an existing Connection to use it? I would like to know if you have such a method or idea.

Thank you for your thoughtful response to my immature question.

rstoyanchev commented 2 months ago

@cocet33000 please review #920 on the topic of having a totalCount. If there is a specific request for something we can do, please create a separate issue.