SpartnerNL / Laravel-Nova-Excel

🚀 Supercharged Excel exports for Laravel Nova Resources
https://docs.laravel-excel.com/nova/1.0/
MIT License
378 stars 73 forks source link

[BUG] Action query method called twice, causing duplicate joins #181

Open Synchro opened 6 days ago

Synchro commented 6 days ago

Prerequisites

Versions

Description

I have a query method in a Nova action which extends Maatwebsite\LaravelNovaExcel\Actions\DownloadExcel:

public function query()
    {
        return $this->query
            ->join('locations', 'locations.id', '=', 'scans.location_id')
            ->leftJoin('customers', 'locations.customer_id', '=', 'customers.id')
            ->join('users', 'users.id', '=', 'scans.user_id')
            ->leftJoin('product_scan', 'scans.id', '=', 'product_scan.scan_id')
            ->leftJoin('products', 'products.id', '=', 'product_scan.product_id')
            ->leftJoin('skus', 'products.sku_id', '=', 'skus.id')
            ->leftJoin('borrowings', 'borrowings.scan_id', '=', 'scans.id')
            ->select(
                [
                    'scans.uuid',
                    'scans.status',
                    'scans.created_at',
                    'scans.external_id',
                    'users.email as user_email',
                    'customers.name as customer_name',
                    'locations.name as location_name',
                    'locations.uuid as location_uuid',
                    'skus.brand as sku_brand',
                    'skus.name as sku_name',
                    'skus.type as sku_type',
                    'products.serial as product_serial',
                    'borrowings.status as borrowing_status',
                    'borrowings.sku_type as borrowing_sku_type',
                ]
            );
    }

When I run this query standalone (e.g. in Tinkerwell), it works correctly. When I run it from within Nova, it generates an error:

Syntax error or access violation: 1066 Not unique table/alias: 'locations' (
Connection: mysql, SQL: select `scans`.`uuid`, `scans`.`status`, `scans`.`created_at`, `scans`.`external_id`, `users`.`email` as
 `user_email`, `customers`.`name` as `customer_name`, `locations`.`name` as `location_name`, `locations`.`uuid` as `location_uui
d`, `skus`.`brand` as `sku_brand`, `skus`.`name` as `sku_name`, `skus`.`type` as `sku_type`, `products`.`serial` as `product_ser
ial`, `borrowings`.`status` as `borrowing_status`, `borrowings`.`sku_type` as `borrowing_sku_type` from `scans` inner join `loca
tions` on `locations`.`id` = `scans`.`location_id` left join `customers` on `locations`.`customer_id` = `customers`.`id` inner j
oin `users` on `users`.`id` = `scans`.`user_id` left join `product_scan` on `scans`.`id` = `product_scan`.`scan_id` left join `p
roducts` on `products`.`id` = `product_scan`.`product_id` left join `skus` on `products`.`sku_id` = `skus`.`id` left join `borro
wings` on `borrowings`.`scan_id` = `scans`.`id` inner join `locations` on `locations`.`id` = `scans`.`location_id` left join `cu
stomers` on `locations`.`customer_id` = `customers`.`id` inner join `users` on `users`.`id` = `scans`.`user_id` left join `produ
ct_scan` on `scans`.`id` = `product_scan`.`scan_id` left join `products` on `products`.`id` = `product_scan`.`product_id` left j
oin `skus` on `products`.`sku_id` = `skus`.`id` left join `borrowings` on `borrowings`.`scan_id` = `scans`.`id` where `scans`.`i
d` in...

If I extract the query and just show the joins, it looks like this:

from `scans`
         inner join `locations` on `locations`.`id` = `scans`.`location_id`
         left join `customers` on `locations`.`customer_id` = `customers`.`id`
         inner join `users` on `users`.`id` = `scans`.`user_id`
         left join `product_scan` on `scans`.`id` = `product_scan`.`scan_id`
         left join `products` on `products`.`id` = `product_scan`.`product_id`
         left join `skus` on `products`.`sku_id` = `skus`.`id`
         left join `borrowings` on `borrowings`.`scan_id` = `scans`.`id`
         inner join `locations` on `locations`.`id` = `scans`.`location_id`
         left join `customers` on `locations`.`customer_id` = `customers`.`id`
         inner join `users` on `users`.`id` = `scans`.`user_id`
         left join `product_scan` on `scans`.`id` = `product_scan`.`scan_id`
         left join `products` on `products`.`id` = `product_scan`.`product_id`
         left join `skus` on `products`.`sku_id` = `skus`.`id`
         left join `borrowings` on `borrowings`.`scan_id` = `scans`.`id`

While you can see there are no duplicate joins in my query builder code, there are duplicate joins in the generated query, and that part isn't done by my code. If I replace any table name with an alias, for example:

->join('locations as locations2', 'locations2.id', '=', 'scans.location_id')

I get the same duplicate table error on the new name, even though I'm clearly only specifying it once. To confirm that these query clauses are generated from this code and not elsewhere, if I remove one join from this builder, both that join and its duplicate disappear from the resulting query. Essentially I can't do any query that contains a join.

There is a very similar bug reported in Nova itself, but it was closed without being resolved; I don't know where the problem actually lies, whether in Nova or LNE.

I expect this code to generate a query that doesn't contain duplicate tables. I don't see how I could cause this to happen intentionally, so I can only assume it's a bug.

Synchro commented 5 days ago

It occurred to me how this effect can be created – if the query method is called twice, the select will be overridden (and thus have no effect), but the joins will accumulate, causing the duplication. So I stuck a Log call in the method, and sure enough, it is called twice. Now to track down why...

Synchro commented 5 days ago

Still not tracked down why this happens, but I can get the download to proceed by adding this at the top of my query method:

        static $done = false;
        if ($done) {
            return $this->query;
        }
        $done = true;

i.e. if it's been called before, don't alter the query, return it as-is. This prevents the error and the download occurs, but the CSV file contains only a header row, no data, no matter how many rows were selected.

Synchro commented 5 days ago

I've discovered where the duplicate call comes from. It's called once from Maatwebsite\Excel\Sheet, and then again a few lines later:

    public function fromQuery(FromQuery $sheetExport, Worksheet $worksheet)
    {
        if ($sheetExport->query() instanceof \Laravel\Scout\Builder) {
            $this->fromScout($sheetExport, $worksheet);

            return;
        }

        $sheetExport->query()->chunk($this->getChunkSize($sheetExport), function ($chunk) use ($sheetExport) {
            $this->appendRows($chunk, $sheetExport);
        });
    }

In my case $sheetExport is not an instance of \Laravel\Scout\Builder (I'm not using Scout at all), so it is called a second time before being passed to the chunk operation.

I see that this has been reported in the Laravel-Excel repo and again, but the problem wasn't fixed before being auto-closed.

The problem here is that the query() method, which is documented as the right place to alter the query before it is executed, is also used to fetch the query without altering it, as can be seen in the method above, and these uses are incompatible; if you modify the query in a non-idempotent way, it's going to break. This is a design issue.

I reinvestigated my earlier idea (with the static to track the double call) again, as I couldn't why it wouldn't work, and found that I had selected rows that resulted in no contents after the joins 🤦🏻. Selecting some others makes it workable.

patrickbrouwers commented 5 days ago

Wouldn't putting the '$sheetExport->query()' call into a temporary variable fix it? If so, can you PR it?

Synchro commented 5 days ago

Yes, that will probably do it. PR incoming...