Fix hasNextPage when max_page_size is set

[rmosolgo]

About hasNextPage, the relay spec says:

HasNextPage(allEdges, before, after, first, last)

If first is set:
    Let edges be the result of calling [ApplyCursorsToEdges](https://relay.dev/graphql/connections.htm#ApplyCursorsToEdges())(allEdges, before, after).
    If edges contains more than first elements return true, otherwise false.
If before is set:
    If the server can efficiently determine that elements exist following before, return true.
Return false.

But GraphQL-Ruby's max_page_size was causing it to take the "if first is set..." branch even when the client didn't provide first.

After this change, it will fall down to the "Return false" branch of that spec unless first is explicitly provided by the client.

Fixes #4765

[porter77]

This change broke a lot of our clients since we were dependent on the old (incorrect) behavior. Clients were not explicitly passing first, but rather depending on the default page size values. After upgrading we starting getting hasNextPage: false for all connection requests even though there were more records. If anyone else stumbles into this issue, this was our temporary fix (replace MyAppSchema with your schema...)

module Types
  class BaseArgument < GraphQL::Schema::Argument
    def initialize(name, type, desc = nil, **kwargs)
      if name == :first && kwargs[:owner].connection?
        kwargs[:default_value] = kwargs[:owner].default_page_size || MyAppSchema.default_page_size
      end

      super
    end
  end
end

This will explicitly set the default value of the first argument to match your default_page_size value

[rmosolgo]

Thanks for sharing that solution, @porter77 🍻

[eapache-opslevel]

@rmosolgo we also just had a small incident because of this surprise. Frankly, the relay spec doesn't seem to support a default max-page-size at all... if no first is provided, the spec "requires" (if you read it literally and with no room for flexibility) that you return all edges. But max-page-size is clearly a nice feature and otherwise integrates well.

In this part of the spec, I would consider a default max-page-size value to count as "first being set" in the absence of all other values...

[eapache-opslevel]

I (think I) understand the index bug from #4765, but I don't understand the hasNextPage part of the discussion, why is hasNextPage: true wrong when the cursors return an empty list of edges but there are more edges available?

[rmosolgo]

Yeah, sorry about the trouble ... maybe this was the wrong approach. I'm open to making it return true if max_page_size is set. Even though this is a violation of the spec, as you noted, the spec doesn't cover max_page_size...

[rmosolgo]

Here's a PR to revert the change to hasNextPage, please let me know if you think we should go through with it: #4866

[rmosolgo]

Sorry for the disruption caused by this change -- I'm going to revert it in 2.2.12, so that hasNextPage is true (again) when max_page_size is applied.

[rmosolgo]

I just shipped the revert (#4866) in 2.2.12. Sorry again for the trouble from this!