laravel-enso / tables

Bulma themed, VueJS powered Datatable with server-side loading and JSON template setup
https://www.laravel-enso.com/examples/table
632 stars 77 forks source link

Entries count <> Rows if you have deleted rows #226

Closed mauthi closed 3 years ago

mauthi commented 4 years ago

This is a bug.

Prerequisites

Description

The entries count in table footer is wrong if you have soft deleted rows in your database. image

Steps to Reproduce

  1. Create a table for a model which uses soft deletes
  2. Delete one entry
  3. Check sum row in footer

Expected behavior

In my screenshot the footer row should be "From 1 to 12 of 12 entries"

Actual behavior

In my screenshot the footer row is "From 1 to 12 of 14 entries"

aocneanu commented 4 years ago

@mauthi please test

mauthi commented 3 years ago

Works for me. Thx for fixing.

aocneanu commented 3 years ago

Unfortunately this has serious performance issues on big tables with complex queries / subqueries.

We will revert @raftx24

aocneanu commented 3 years ago

@mauthi please try the fix/softDeletes branch and report back.

@vmcvlad , @gandesc you can review too.

mauthi commented 3 years ago

Works perfect for my use case. Thx.

mauthi commented 3 years ago

Found an error: If I use a table with join I get the following error:

"message": "SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'deleted_at' in where clause is ambiguous (SQL: select count(*) as aggregate fromsuppliersleft jointax_typesonsuppliers.tax_type_id=tax_types.idwheredeleted_atis null)",

My used select:

return Supplier::select(
            \DB::raw('suppliers.id as id, suppliers.name as supplier_name, tax_types.name as tax_type_name')
        )
            ->leftJoin('tax_types', 'suppliers.tax_type_id', '=', 'tax_types.id');
aocneanu commented 3 years ago

Fixed.

mauthi commented 3 years ago

Tested - error is fixed.

aocneanu commented 3 years ago

But thinking more about it, I guess this approach is not enough. It will solve your case but won't work when you will want your query withTrashed, right?

mauthi commented 3 years ago

Right - if I use return CustomerGroup::withTrashed()->withCount('customers'); I get the same amount of rows (# of rows with deleted is null)

aocneanu commented 3 years ago

Could you paste here your table query captured with telescope?

mauthi commented 3 years ago

I got the deleted rows in table but footer is wrong: image

Query:

select
  `customer_groups`.*,
  (
    select
      count(*)
    from
      `customers`
    where
      `customer_groups`.`id` = `customers`.`customer_group_id`
      and `customers`.`deleted_at` is null
  ) as `customers_count`
from
  `customer_groups`
order by
  `customer_groups`.`id` asc
limit
  20 offset 0

So the total row count is not updated and if I see that correctly it's even not fetched again?

mauthi commented 3 years ago

And one additional thing: The delete button is also visible for deleted items and leads to a 404 (but thats not a normal use case to show deleted rows)

aocneanu commented 3 years ago

ok, this needs more work but I guess we're on the right path

mauthi commented 3 years ago

yes thx - for me it’s fixed for now :)

mauthi commented 3 years ago

I found another issue:

    public function query(): Builder
    {
        return Customer::where('post_status','<>','trash')->selectRaw('*');
    }

In the table I have 6 rows, one with post_status = trash Now I get From 1 to 5 of 6 entries as footer.

raftx24 commented 3 years ago

Hi @mauthi could you please check my commit too? I checked it and it worked with withTrash,... thanks

mauthi commented 3 years ago

Hi @raftx24,

I just tried it. For me it's not working for both - soft deletes and normal where statements (like described in https://github.com/laravel-enso/tables/issues/226#issuecomment-734920345)

With not working I mean the following:

raftx24 commented 3 years ago

@mauthi. I created 6 companies, and I set the first company name to trash(similar to your situation). this is my CompanyTable

class CompanyTable implements Table
{
    protected const TemplatePath = __DIR__.'/../Templates/companies.json';

    public function query(): Builder
    {
        return Company::where('name', '<>', 'trash')->selectRaw('*');
    }

    public function templatePath(): string
    {
        return static::TemplatePath;
    }
}

and this is the result. image

mauthi commented 3 years ago

I tried if again (with your branch /softDeletesWithApplyingScopes):

query(): return CustomerGroup::where('name', 'like', '%an%');

Result: image

raftx24 commented 3 years ago

@mauthi I guess there is another problem with your table. because I used this table(10 companies with (8 + 2 deleted)) image

and I tried to simulate your situation with the following Table class

class CompanyTable implements Table
{
    protected const TemplatePath = __DIR__.'/../Templates/companies.json';

    public function query(): Builder
    {
        return Company::where('name', 'like', '%a%');
    }

    public function templatePath(): string
    {
        return static::TemplatePath;
    }
}

and I got this result. image


Maybe your cache is enabled and you cannot see the current count.

mauthi commented 3 years ago

Hi again,

Thx for the hint with the cache - I deleted it with php artisan enso:tables:clear and now it looks fine. But the question is how this cache works and why this is enabled? Do you have a short explaination for me?

raftx24 commented 3 years ago

You're welcome. Yes, from docs

count, is a boolean, default is true. When counting the number of results for the table, a count query has to be performed and for costly queries there can be quite a performance improvement if the count result is cached. Note that if caching the count, you should use the TableCache trait on the main/base model, so that the count cache is invalidated if when a model is created or deleted.

mauthi commented 3 years ago

Thx - I will check this.

So we can close this topic?

mauthi commented 3 years ago

@aocneanu the fix of @raftx24 works for me (as written above) - is there a reason why his PR is still open or didn't you have time to check? If you need any feedback or help to test this feel free to ask.

aocneanu commented 3 years ago

merged