reinink/advanced-eloquent

SQLite issue with order by sub

Closed this issue ยท 7 comments

NOTE: To be frank, I'm not sure whether it's an issue with the SQLite driver of Laravel or a syntax/escaping problem within this package, but here it comes...

I am experiencing PDOException: SQLSTATE[HY000]: General error: 25 column index out of range errors when running an orderBySub() on SQLite. This only happens when the model contains a withCount hook that filters to a date (I know ๐Ÿ˜…). Using MySQL, it works without a problem.

I've put together a dummy repo to showcase above. Run the tests to see it fail.

The issue is that the default $withCount binding (created_at) is getting added, even though it's not used or needed.

public function clicksToday()
{
    return $this->clicks()->whereDate('created_at', today());
}

MySQL doesn't seem to care if you pass an unused binding. SQLite and Postgres do.

The problem here is that you're adding a default sub select to the Offer model, which means whenever a query builder instance is created for it, it adds these count subqueries. However, when the orderBy subquery runs, it only needs a single column (name). Meaning, the counts are excluded. HOWEVER, Eloquent isn't removing the bindings that were added for those count subqueries.

How to fix this, I honestly don't know.

For what it's worth, I am personally not a fan of using the $withCount property, since it doesn't give you anyway to disable it.

For example:

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Offer extends Model
{
    protected static function boot()
    {
        parent::boot();

        static::addGlobalScope('counts', function ($query) {
            $query->withCount('clicks', 'clicksToday');
        });
    }

    public function clicks()
    {
        return $this->hasMany(Click::class);
    }

    public function clicksToday()
    {
        return $this->clicks()->whereDate('created_at', today());
    }
}

And now you can do this:

Click::orderBySub(
    Offer::withoutGlobalScope('counts')
        ->select('name')
        ->whereColumn('offer_id', 'offers.id')
)->get();

Another way to avoid this problem is to just use the query builder and not Eloquent:

Click::orderBySub(
    DB::table('offers')
        ->select('name')
        ->whereColumn('offer_id', 'offers.id')
)->get()

Finally, it's worth mentioning that in Laravel 6 this package won't even be needed. You can do this:

Click::orderBy(function ($query) {
        $query->select('name')
                ->from('offers')
                ->whereColumn('offer_id', 'offers.id');
})->get()

Good luck! ๐Ÿ‘

PS. Thanks for putting together the dummy repo. That was perfect. It helped me understand this problem much more easily. ๐Ÿ’ช

Thanks @reinink, I do appreciate you spending your time on this, it was really helpful to me. Above makes complete sense and I think if I were to use the counts as a scope instead that would allow me to work around the problem. ๐Ÿ‘ ๐Ÿ’ช

Happy I could help! It's worth noting that this will be an issue even outside of this package if you ever want to select some specific columns. For example, this is busted too:

$offers = Offer::select('name')->get();

Makes me think that Laravel needs a default way to undo the default withCount subqueries. I might try to hack on a PR to add this.

Just FYI really - replacing the $withCount = ['offers'] with scopeWithOffersCount() did the trick and SQLite is happy to cooperate. Thanks once again!

Awesome! ๐Ÿ’ช