No error thrown when query value first exceeds max query amount

[hsdonkin]

Description

When a query is made exceeding the max query amount, no error is thrown to indicate that the value of first exceeds the amount. Instead, the query succeeds, but returns only the max query amount.

We expected some type of error, since the max query amount is not visible for a user of the API.

https://www.wpgraphql.com/filters/graphql_connection_max_query_amount

Steps to reproduce

Reproducible in repl:

{
  posts(first:10000000){
    nodes {
      id
    }
  }
}

{
  "data": {
    "posts": {
      "nodes": [
        {
          "id": "cG9zdDox"
        }
      ]
    }
  },
  "extensions": {
    "debug": [
      {
        "type": "DEBUG_LOGS_INACTIVE",
        "message": "GraphQL Debug logging is not active. To see debug logs, GRAPHQL_DEBUG must be enabled."
      }
    ]
  }
}

Additional context

This could be part of a larger conversation around query costs, which is how Shopify handles this issue with their GraphQL API.

https://shopify.engineering/rate-limiting-graphql-apis-calculating-query-complexity

{
  "errors": [
    {
      "message": "Query cost is 2002, which exceeds the single query max cost limit (1000).\n\nSee https://shopify.dev/concepts/about-apis/rate-limits for more information on how the\ncost of a query is calculated.\n\nTo query larger amounts of data with fewer limits, bulk operations should be used instead.\nSee https://shopify.dev/api/usage/bulk-operations/queries for usage details.\n",
      "extensions": {
        "code": "MAX_COST_EXCEEDED",
        "cost": 2002,
        "maxCost": 1000,
        "documentation": "https://shopify.dev/api/usage/rate-limits"
      }
    }
  ]
}

WPGraphQL Version

1.19.0

WordPress Version

N/A

PHP Version

N/A

Additional environment details

No response

Please confirm that you have searched existing issues in the repo.

• Yes

Please confirm that you have disabled ALL plugins except for WPGraphQL.

• Yes
• My issue is with compatibility with a specific WordPress plugin, and I have listed all my installed plugins (and version info) above.

[justlevine]

Two thoughts:

• if we were to log anything, it should be as a debug message (disabled in the shared snippet). Unlike Shopify, we're limiting query depth not query cost, so we can predictably return a result instead of failing altogether.
• Errors are obscured from unauthenticated users and similarly the security recommendation is to disable debugging logs in production. Shopify doesn't have this issue, since all requests require some level of auth.

Which brings me to the expectation case:

We expected some type of error, since the max query amount is not visible for a user of the API.

I'm not sure what the context is of user here is.

If it's anybody hitting the WPGraphQL then I don't think we can solve this (for the above reasons) let alone the security implications (is it better to tell an API abuser that they need to lower their connection amount to optimally flood the server without getting rate limited? Should we error on a bad connection amount instead of returning the first 100 - even if those repeat calls are cached the second time around - just in case)?

If it's just the site developers, then maybe a debug message provides a good guardrail. It's additive, hidden behind the debug flag, and doesn't break back-compat.

Thoughts?

cc #2749

[hsdonkin]

These are good points! I'm really talking about the DX working with the API, by "user".

Errors are obscured from unauthenticated users

You still get errors for incorrect types, right? Like querying first: "foo" produces an error, because "foo" is an inappropriate value for first

is it better to tell an API abuser that they need to lower their connection amount to optimally flood the server without getting rate limited?

I'm not really sure what's implemented in terms of rate limiting. With the current behavior, they could just request first:10000000 though, so they'll always be querying for a maximum amount of data!

I would say that high level, it's better to have an error than no error because it's better DX. If I'm asking for 200, but I'm only allowed 100 and getting 100 back, then my request isn't succeeding, it's silent failing, and I should be getting some kind of feedback so I can properly handle the failure. Getting a different result from what I asked for is the opposite of predictability.

Practically speaking, we had this issue come up because we were missing data in parts of our app, because the published resource count had grown higher than 100! One of our devs had written some code requesting 200 resources, got 80 some back, and assumed we had a high query amount.

[justlevine]

Errors are obscured from unauthenticated users

You still get errors for incorrect types, right? Like querying first: "foo" produces an error, because "foo" is an inappropriate value for first

By "obscured" I mean that the error pattern for sensitive infor is to throw a generic GraphQL error when unauthenticated (e.g. Couldn't log in) and then add a debug message that explains how to fix(e.g. Incorrect password). If the the goal is to give guidance to any API consumer, then this pattern won't solve the issue.

The larger question though is whether a value over node limit is an "inappropriate value". It's a server limitation not a schema limitation, and as far as GraphQL is concerned the server returned everything it was allowed to for the query and it all (pagination, filters, etc) still works as expected. Similar to how querying users{ nodes {...} } } as an unauthenticated user, returns only post authors, and not an error about all the users that the server refused to return.

With the current behavior, they could just request first:10000000 though, so they'll always be querying for a maximum amount of data!

With proper caching, calling the same first: INT_CONST repeatedly will return a cached result into it has a reason to invalidate. But the issue isn't about querying the maximum amount of allowed data, it's about creating a complex query that's too large and locks the db and/or times-out the server.

IRL, script kiddies are smart enough to use pagination. I'm betting the real intent of this limit is to prevent site owners from doing something stupid like they would in PHP with $posts_per_page = -1; 🤷

we had this issue come up because we were missing data in parts of our app, because the published resource count had grown higher than 100! One of our devs had written some code requesting 200 resources, got 80 some back, and assumed we had a high query amount.

Agreed. 100 is an arbitrary number and nobody should be expected to guess that's the limit, even if they're smart enough to intuit that a limit exists.

Which brings me back to the question:

• do we treat this as an Error and fail,
• or do we return programmatically-consistent but logically-unexpected results and output a debug message that informs the dev of the current server limit and recommends pagination or changing the filter?

The latter is the approach I'm leaning towards (also because it would maintain back-compat), but if there's a strong use case for truly erroring out and/or providing guidance to any API consumer, then maybe the behavior change is justified 🤔

[justlevine]

Hey @hsdonkin

#3013 approach uses the extensions.debug message (screenshot and summarized rationale there). I really think that approach offers a better and more reliable DX than throwing a GraphQLError, but if it doesn't we can keep this ticket open and revisit when work begins in earnest on a v2.0 release (since that would be a breaking change).

Please let me know what you think.

[jasonbahl]

I would say that high level, it's better to have an error than no error because it's better DX. If I'm asking for 200, but I'm only allowed 100 and getting 100 back, then my request isn't succeeding, it's silent failing, and I should be getting some kind of feedback so I can properly handle the failure. Getting a different result from what I asked for is the opposite of predictability.

@hsdonkin there could be any number of reasons fewer items than what was asked for will be returned. The feedback you get is in the pageInfo field.

You can query for first: 10000 and if the server returns 1 result, 100 results, or all 10,000 results, the pageInfo.hasNextPage returns either true or false if there are more results.

So, if you ask for 1,000 items and the server decides the optimal result set to return is 100, then it will return those 100 and also return hasNextPage: true allowing you to ask for the next page until you either receive the full 10,000 you were seeking, or until the server returns hasNextPage: false at which point you know that you've reached the end of the record set.

I believe an error in this case would actually be WORSE DX than better because there are many reasons a server's logic might determine a lower number of nodes to process than the client asked for.

Using the hasNextPage / hasPreviousPage values from the query will be a much better DX as that will allow the server to make server decisions (like temporarily increasing or lowering the limit of nodes to return, etc) without the client failing due to errors.

[jasonbahl]

or do we return programmatically-consistent but logically-unexpected results and output a debug message that informs the dev of the current server limit and recommends pagination or changing the filter?

If you query for hasNextPage I don't think this is "logically unexpected" anymore.

The server can make whatever decisions it needs to to fulfill whatever number of nodes it feels "right" to return.

Of course the default 100 upper limit is arbitrary. Some low-cost servers might do better reducing that to 50. Some servers might perform fine upping it to 1,000. Some individual queries might be "cheap" enough to have higher limits than other queries. You can filter the limit to be whatever you want. I definitely wouldn't recommend allowing unlimited, but if you want to on your server, you certainly can.

In any case, the logic the server applies to determine this can be different for every WP install and even every query, so the standard "logically expected" results is for the query to return however many nodes it deemed appropriate to return, and communicate via "hasNextPage" whether there are or are not more pages of data.

[jasonbahl]

I really think that approach offers a better and more reliable DX than throwing a GraphQLError, but if it doesn't we can keep this ticket open and revisit when work begins in earnest on a v2.0 release

You can also use filters to return a GraphQL Error if you believe that's the best experience for your specific use case.

I believe it's a worse DX compared to pageInfo.hasNextPage, so we won't support it by default, but via hooks/filters we enable you to do this if you feel like it's the right decision for your applications.

[hsdonkin]

Thanks for these thoughts, it's not my ideal, but I appreciate you guys looking at it!