gearbox-solutions / eloquent-filemaker

A Model extension and Eloquent driver for Laravel connecting to FileMaker through the Data API
https://gearboxgo.com
MIT License
56 stars 16 forks source link

whereIn() not being applied when using paginate() method on Eloquent Model #31

Closed lgebing closed 1 year ago

lgebing commented 2 years ago

Hi, i am having problems with the newly introduced whereIn() method.

I am running a query similar to the following:

Order:whereIn('userId', $userIds)->all();
{
  "query": [
    {
      "deleted": "0",
      "userId": "1"
    },
    {
      "deleted": "0",
      "userId": "2"
    },
    ...
  ],
  "limit": 1000000000000000000
}

However, when running the same query, but switching ->all() for ->paginate(10) I get:

Order:whereIn('userId', $userIds)->paginate(10);
{
  "query": [
    {
      "deleted": "0"
    }
  ],
  "limit": 10
}

As you can see, the whereIn(...) method gets completely ignored.

I believe this is because the computeWhereIns() method of the FMBaseBuilder class does not get called. It does get called in the get() and update() methods of the same class, but not in the forPage() method which is used by the FMEloquentBuilder's paginate() method:

public function paginate($perPage = null, $columns = ['*'], $pageName = 'page', $page = null)
{
    ...

    /** @var FMBaseBuilder $query */
    $query = $this->toBase()->forPage($page, $perPage);

    ...
}

I don't know if overwriting the forPage() method of Illuminate\Database\Query\Builder is the best solution, but I guess adding the following to FMBaseBuilder could be an option:

public function forPage($page, $perPage = 15)
{
    $this->computeWhereIns();

    parent::forPage($page, $perPage);
}

Would be great if this could be resolved so we can take advantage of both whereIn() and paginate() at the same time.

likeadeckofcards commented 2 years ago

@lgebing Thank you for this bug. We did find the issue and currently have a PR open that solves the issue and cleaned up some other methods that the whereIn clauses will affect but has not been reported yet. This PR is currently waiting for review and should be a part of the next release.

lgebing commented 1 year ago

Hi again.

I noticed another issue and thought I might as well add it to this thread.

When running the following query Order::all(); the returned records get reduced to 100 results (as is the default for the Data API, but not the expected result of calling all()), but when adding the query() function (or any where() condition, although that might alter the record count) all matching records get returned as expected.

Order::all()->count(); // 100

Order::query()->all()->count(); // > 100

I am not sure if this would be fixed by the currently pending PR (#32). Speaking of which, any estimates on when this PR might get merged?

Smef commented 1 year ago

query()->all() isn't actually valid, but was working as currently built. I've change the all() method to properly work the way it does with native Laravel and only work on the model. This has been released as 0.2.6.

The PR review and merge is something I'm supposed to be doing this week, but things keep coming up...

Smef commented 1 year ago

Fixed in 0.2.7.