cyrildewit / eloquent-viewable

Associate views with Eloquent models in Laravel
MIT License
829 stars 105 forks source link

Getting Column not found when using ->orderByViews() #263

Closed bojmaliev closed 1 year ago

bojmaliev commented 2 years ago

Description:

This is happening when I'm trying to use ->orderByViews()

My application is throwing the following exception when trying to go on the second page of my cursor paginate. I have tried using normal pagination but it still gets the same error. I'm confused why it works fine on the first page.

Column not found: 1054 Unknown column 'views_count' in 'where clause' (SQL: select ads., (select count() from views where ads.id = views.viewable_id and views.viewable_type = App\Models\Ad) as views_count from ads where (status = active and (sold_at is null or sold_at >= 2022-01-13 23:28:00) and refreshed_at >= 2021-07-18 23:28:00) and type in (selling, buying, renting, need_for_rent, provide, search) and id not in (3605, 3600, 1414, 1399) and id not in (232, 3957, 1367, 1735) and (views_count < 623) and ads.deleted_at is null order by views_count desc limit 13) {"exception":"[object] (Illuminate\Database\QueryException(code: 42S22): SQLSTATE[42S22]: Column not found: 1054 Unknown column 'views_count' in 'where clause' (SQL: select ads., (select count() from views where ads.id = views.viewable_id and views.viewable_type = App\Models\Ad) as views_count from ads where (status = active and (sold_at is null or sold_at >= 2022-01-13 23:28:00) and refreshed_at >= 2021-07-18 23:28:00) and type in (selling, buying, renting, need_for_rent, provide, search) and id not in (3605, 3600, 1414, 1399) and id not in (232, 3957, 1367, 1735) and (views_count < 623) and ads.deleted_at is null order by views_count desc limit 13)

The query is working totally fine when I'm not using orderByViews()

My research came to Following answer but I'm not sure if it is.

Anyway thank you in advance.

UPDATE

After comparing the query that makes the exception and the one that is not is and (views_count < 2) this part. Why orderByViews is adding this condition?

cyrildewit commented 2 years ago

Hey @Bojmaliev,

Can you write a testcase within this repository and create a pull request? Then I can see the isolated problem.

bojmaliev commented 2 years ago

Hey @cyrildewit,

Thanks for replying. I tried to make a test but the test go fine on Sqlite. The problem happens on mysql ( or mariadb). Because I can't reproduce it there, I made demo repo with Laravel 9.

https://github.com/Bojmaliev/laravel-viewable-test

Please clone the project, migrate it to mysql and try to visit "/". You will have cursor paginate response. Then try to go to the suggested next_page_url and See the error.

Now with Laravel 9 there is SQLSTATE[HY093]: Invalid parameter number

cyrildewit commented 2 years ago

Hey @Bojmaliev,

Thank for preparing a setup. I see that you're using the cursorPaginate method. I think I haven't really tested this yet.

I think it's time to upgrade the run-tests GitHub workflow to test the test suite with multiple database drivers.

For now, I'll just focus on the current issue(s).

cyrildewit commented 2 years ago
Screenshot 2022-03-13 at 11 24 25

I was able to reproduce it.

bojmaliev commented 2 years ago

Yup, thats it

cyrildewit commented 2 years ago

What I see is that the cursorPaginate is adding a where clause for the views_count alias. This is not possible, since the SQL engine cannot find the value for an alias. See https://dev.mysql.com/doc/refman/8.0/en/problems-with-alias.html.

Edit:

That's how the cursorPaginate appearantly works. See https://laravel.com/docs/9.x/pagination#cursor-pagination.

While paginate and simplePaginate create queries using the SQL "offset" clause, cursor pagination works by constructing "where" clauses that compare the values of the ordered columns contained in the query, providing the most efficient database performance available amongst all of Laravel's pagination methods

cyrildewit commented 2 years ago

I don't think we will find a solution to this problem soon. I suggest you to use the other pagination solutions. The cursorPaginate is quite limited in this regard.

bojmaliev commented 2 years ago

On the end I solved it caching the views count in the database and using cursor Paginate.

Its good to mention this issue in the documentation.

Thanks anyway for creating this package. Its awesome

cyrildewit commented 2 years ago

@Bojmaliev Nice to hear that.

Creating a temporarily column in the database does indeed fixes this issue for the cursor logic. I think it will also optimise the query speed for other queries as well.

What solution did you use to synchronise the current views with the tables and how often do run this. I have an idea about this, but It's always good to learn about other peoples ideas and implementations.

I still have some ideas to improve the performance of this package by implementing some optional archiving strategy:

bojmaliev commented 2 years ago

Hey @cyrildewit ,

Right now I have a command that runs every six hours and it looks like:

Ad::public() ->chunkById(100, function ($ads) { 
  $ads->each(function($ad) {
    $ad->unique_views_count = views($ad)->unique()->count();
    $ad->save(); 
  });
}, $column = 'id');

I don't personally like this solution but it was the quickest one and it works fine as long as I have few thousand rows.

I can improve it if I eager load the views but the update statement will be called again for each row.

Maybe i can create multiple jobs every six hours to update the views column. Each job will be responsible for specific range of data and run in different time intervals. What do you think?

I like your idea for archiving views because currently the views table has more data then any other table.