Lighthouse: Pagination maxCount is ignored when querying for `count: 0`

[spawnia]

Describe the bug

Copied from @ZaidBarghouthi, thanks for the report!

Guys! @spawnia @jbbr

So I have maxCount = 5

allUsers: [User!]! @paginate(maxCount: 5)

but when I query with count: 0 is ignores it.

{
  allUsers(count: 0) {
    data {
     id
     name
    }    
  }
}

Resutls:

{
  "data": {
    "allUsers": {
      "data": [
        {
          "id": "1"
        },
        {
          "id": "2"
        },
        {
          "id": "3"
        },
        {
          "id": "4"
        },
        {
          "id": "5"
        },
        {
          "id": "6"
        },
        {
          "id": "7"
        },
        {
          "id": "8"
        },
        {
          "id": "9"
        },
        {
          "id": "10"
        },
        {
          "id": "11"
        },
        {
          "id": "12"
        },
        {
          "id": "13"
        },
        {
          "id": "14"
        }
      ]
    }
  }
}

I guess somewhere there is if($maxCount)... causing this, when maxCount is zero it behaves like there in no maxCount!

Expected behavior

It should limit the maxCount in all cases.

[zjbarg]

@spawnia Thank you for filing the bug report

[jthomaschewski]

Good catch, thanks for reporting @ZaidBarghouthi

I quickly scanned my PR and didn't found a simple check like if($maxCount).
It basically only throws if count doesn't match expectations. And as 0 is smaller or equal to your maxCount setting, it doesn't throw.
So either count: 0 is interpreted as count: unlimited by lighthouse in general, or I missed something.

Throwing is done here: https://github.com/nuwave/lighthouse/blob/948475394062b88ab9d37d6f23571d121f34c8b9/src/Execution/Utils/Pagination.php#L31
It would be easy to add a check for count===0 and throw but that would be a dirty fix,

I only had a short glance at the code, haven't tried to reproduce this yet. But I will try to reproduce and fix this.

[zjbarg]

@jbbr

So either count: 0 is interpreted as count: unlimited by lighthouse in general, or I missed something.

I'm not sure how it is interpreted but I just realized that it somehow uses the $perPage in the model, which is set to 15 by default in laravel.

In: Illuminate\Database\Eloquent\Model

 /**
  * The number of models to return for pagination.
  *
  * @var int
  */
protected $perPage = 15;

so the maximum that it will return is 15 unless $perPage is overridden in the model.

[spawnia]

There are some subtle complexities involved in dealing with a count of 0.

We should definitely make sure the paginate_max_count is respected. Going from there, we have to options:

1. Disallow querying for a count of one, throwing an Error to notify the client their Query makes no sense.
2. Try to handle it gracefully.

Option 2. is subtly complex, as it involves dealing with the default behaviour of Builder::paginate(), which as @ZaidBarghouthi noted, defaults to the models $perPage.
Also, Laravels LengthAwarePaginator doesn't handle a $perPage of 0 gracefully, so we would have to work on that, too.

Because of the difficulties in dealing with 2. and the fact that querying for a pagination count of 0 makes no sense, i err on the side of 1.

@chrissm79 what do you think?

[zjbarg]

@spawnia
How about if we change the throwIfPaginateMaxCountExceeded method to something like :

public static function throwIfRequestedCountIsIllegal(?int $maxCount, int $requestedCount): void
{
    if ( $maxCount === null ) {
        return;
    }

    if ( $requestedCount > $maxCount ) {
        throw new Error(
            "Maximum number of {$maxCount} requested items exceeded. Fetch smaller chunks."
        );
    }

    if ( $requestedCount === 0 ) {
        throw new Error(
            "Make no sense?"
        );
    }
}