rappasoft / laravel-livewire-tables

A dynamic table component for Laravel Livewire
https://rappasoft.com/docs/laravel-livewire-tables/v2/introduction
MIT License
1.79k stars 339 forks source link

[Bug]: SQL error when trying to use user relation in laravel-auditing package #1935

Closed veodko closed 1 month ago

veodko commented 2 months ago

What happened?

Displaying records from laravel-auditing package works fine until you attempt to use the user relation in the table, which causes SQL errors.

How to reproduce the bug

Install package: composer require owen-it/laravel-auditing Display in table with user relation:

<?php

namespace App\Livewire;

use Rappasoft\LaravelLivewireTables\DataTableComponent;
use Rappasoft\LaravelLivewireTables\Views\Column;
use OwenIT\Auditing\Models\Audit;

class AuditTable extends DataTableComponent
{
    protected $model = Audit::class;

    public function configure(): void
    {
        $this->setPrimaryKey((new Audit)->getKeyName());
        $this->setPaginationMethod('standard');
        $this->setPaginationDisabled();
        $this->setDefaultSort('created_at', 'desc');

        // $this->addAdditionalSelects(['event', 'auditable_type']);
    }

    public function columns(): array
    {
        return [
            Column::make('Created at', 'created_at')
                ->sortable(),
            Column::make('User', 'user.name')
                ->eagerLoadRelations(),
                //->label(fn($row) => $row->user->name) // doesnt work either
            Column::make('IP', 'ip_address')
                ->sortable()
                ->searchable(),
            Column::make('Event', 'event')
                ->format(fn($val) => __("audit.events.$val"))
                ->sortable()
        ];
    }
}

Package Version

13.6.8

PHP Version

8.3.x

Laravel Version

11.10.0

Alpine Version

No response

Theme

Bootstrap 5.x

Notes

I think this is caused by the way the user relationship is defined in the audit model, using morphTo:

public function user()
{
    $morphPrefix = Config::get('audit.user.morph_prefix', 'user');

    return $this->morphTo(__FUNCTION__, $morphPrefix . '_type', $morphPrefix . '_id');
}

This relationship works fine when used directly:

 dd(Audit::with('user')->get()->first()->user);

But here it seems that the SQL query is constructed wrongly, it tries to join audits table back to audits table instead of users table and the column name is empty. Accessing the relation when using a label function doesn't work either. I disabled pagination because it gives a better view on the query, otherwise it fails on a count(*) query.

Error Message

SQLSTATE[42601]: Syntax error: 7 ERROR: zero-length delimited identifier at or near """" LINE 1: ..."audits" as "user" on "audits"."user_id" = "user"."" order b... ^

select "audits"."created_at" as "created_at", "user"."name" as "user.name", "audits"."ip_address" as "ip_address" from "audits" left join "audits" as "user" on "audits"."user_id" = "user"."" order by "created_at" desc

lrljoe commented 2 months ago

Not familiar with the laravel-auditing package, but you should be able to just extend the package Model if it's causing an issue and reference it that way.

What do you get if you use the builder() approach?

i.e. Making sure to add to the top of your component:

use Illuminate\Database\Eloquent\Builder;

Then in the Component add:

    public function builder(): Builder {
            return Audit::query()->with(['user']);
    }
veodko commented 2 months ago

I tried and it results in the same error. I tracked the problem down to the joinRelation method in laravel-livewire-tables/src/Traits/WithData.php, but it seems to be the fact that laravel-auditing uses polymorphic relationship and laravel-livewire-tables is reconstructing the query manually, which won't work for morphs because in their case the related model is resolved dynamically (Relation::morphMap()). I could make it work by hardcoding stuff in the joinRelation method:

protected function joinRelation(Column $column): Builder
{
    if ($column->eagerLoadRelationsIsEnabled() || $this->eagerLoadAllRelationsIsEnabled()) {
        $this->setBuilder($this->getBuilder()->with($column->getRelationString()));
    }

    $table = false;
    $tableAlias = false;
    $foreign = false;
    $other = false;
    $lastAlias = false;
    $lastQuery = clone $this->getBuilder();

    // getRelations() returns [ 0 => "user" ]
    foreach ($column->getRelations() as $i => $relationPart) {
        $model = $lastQuery->getRelation($relationPart);
        // $model is instance of MorphTo
        $tableAlias = $this->getTableAlias($tableAlias, $relationPart);

        switch (true) {
            case $model instanceof MorphOne:
            case $model instanceof HasOne:
                $table = "{$model->getRelated()->getTable()} AS $tableAlias";
                $foreign = "$tableAlias.{$model->getForeignKeyName()}";
                $other = $i === 0
                    ? $model->getQualifiedParentKeyName()
                    : $lastAlias.'.'.$model->getLocalKeyName();

                break;

            case $model instanceof BelongsTo:
                // this case is fulfilled
                $table = "{$model->getRelated()->getTable()} AS $tableAlias";
                $table = "users AS $tableAlias"; // <--- if overwritten, it works. getRelated() points back to audits table instead of related users table
                $foreign = $i === 0
                    ? $model->getQualifiedForeignKeyName()
                    : $lastAlias.'.'.$model->getForeignKeyName();

                $other = "$tableAlias.{$model->getOwnerKeyName()}";
                $other = "$tableAlias.id"; // <--- getOwnerKeyName() returns empty string

                break;
        }

        if ($table) {
            $this->setBuilder($this->performJoin($table, $foreign, $other));
        }

        $lastAlias = $tableAlias;
        $lastQuery = $model->getQuery();
    }

    return $this->getBuilder();
}

It's enough to just do:

dd(Audit::query()->getRelation('user'));

And both child and related fields point to an Audit model, not User. It seems that in order to make morph relations work in livewire-tables, the morphing mechanism from Laravel would need to be reimplemented manually. The simplest although not ideal solution for now is to extend the model and define the relationship using simple BelongsTo like you said.

veodko commented 2 months ago

Exactly the same issues is present when using Spatie's alternative solution: activitylog. The causer relation there is also morphTo and causes same error when used in table.

Is there any chance that we could get support for polymorphic relations in Livewire Tables, or is it out of the scope of this package?

lrljoe commented 2 months ago

Exactly the same issues is present when using Spatie's alternative solution: activitylog. The causer relation there is also morphTo and causes same error when used in table.

Is there any chance that we could get support for polymorphic relations in Livewire Tables, or is it out of the scope of this package?

You'd need to add the relevant fields into setAdditionalSelects, and use a label column

I have various polymorphic relations used in this way.

veodko commented 2 months ago

Hmm, interesting. I just tried the label method on Spatie's activitylog package and indeed it seems to work properly:

$this->setAdditionalSelects(['causer_id']);
//...
Column::make(__('global.user'), 'causer.name')
    ->label(fn($row) => $row->causer->name)
    ->eagerLoadRelations(['causer'])

Looks like only the laravel-auditing package has some issues that it doesn't work even when done using label and additional selects

veodko commented 2 months ago

However I noticed that this is not the right way to go when talking about large tables. The relation is not properly eager loaded and a separate query is executed for each record (despite using eagerLoadRelations(). This slows down the page load & causes excessive load on the database, so as for now I'd rather eagerly fetch all the users & key the collection by the PK, then use format and retrieve user from collection. It's faster even on a small table.

lrljoe commented 2 months ago

For Spatie ActivityLog, as it's a morphTo, you'll always end up with two queries, if you use a with(['causer']) in the builder method

One to retrieve the Activity model

One to retrieve the list of causers associated (technically its one per causer_type)

The reason ActivityLog is like this, is the causer may not be a User. You could extend the Activity Model and adjust if it is always going to be a User Model in your case.

Otherwise, this is how a morphTo works with Laravel

You could also get around that with some joins, but that'd be less efficient from a database query perspective.

I imagine the same would apply for the audit package you're using

lrljoe commented 2 months ago

Do you have more than one "User" / "causer" type model that you're auditing?

veodko commented 1 month ago

No, atm there's only one user model. I somehow got it ONCE to work properly which executed 2 queries (second one was an IN query for user ids). But now I'm trying to replicate this and I'm getting 5 queries (there are 4 rows), and I'm pretty sure I'm doing the same thing (lol)

class ActivityTable extends DataTableComponent
{
    protected $model = Activity::class;

    public function configure(): void
    {
        $this->setPrimaryKey((new Activity)->getKeyName());
        $this->setPaginationMethod('standard');
        $this->setDefaultSort('created_at', 'desc');
        $this->setAdditionalSelects(['causer_type', 'causer_id']);
    }

    public function columns(): array
    {
        $morphMap = Relation::morphMap();
        return [
            Column::make('Date', 'created_at')
                ->sortable(),
            Column::make('User', 'causer.email')
                ->label(fn($row) => $row->causer->email)
                //->eagerLoadRelations(['causer']) // makes no difference
                ->sortable(),
            Column::make('Event', 'event')
                ->format(fn($value) => __("activity.events.$value"))
                ->sortable(),
            Column::make('Subject', 'subject_type')
                ->format(fn($value) => $morphMap[$value] ?? $value)
                ->sortable()
        ];
    }
}
select "causer_type", "causer_id", "activity_log"."created_at" as "created_at", "activity_log"."event" as "event", "activity_log"."subject_type" as "subject_type" from "activity_log" order by "created_at" desc limit 10 offset 0
select * from "users" where "users"."id" = 1 limit 1
select * from "users" where "users"."id" = 1 limit 1
select * from "users" where "users"."id" = 1 limit 1
select * from "users" where "users"."id" = 1 limit 1

What am I doing wrong?