Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
veodko opened this issue Sep 9, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@veodko
Copy link

veodko commented Sep 9, 2024

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

@veodko veodko added the bug Something isn't working label Sep 9, 2024
@lrljoe
Copy link
Sponsor Collaborator

lrljoe commented Sep 9, 2024

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
Copy link
Author

veodko commented Sep 10, 2024

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
Copy link
Author

veodko commented Sep 10, 2024

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
Copy link
Sponsor Collaborator

lrljoe commented Sep 11, 2024

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
Copy link
Author

veodko commented Sep 12, 2024

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
Copy link
Author

veodko commented Sep 13, 2024

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
Copy link
Sponsor Collaborator

lrljoe commented Sep 14, 2024

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
Copy link
Sponsor Collaborator

lrljoe commented Sep 14, 2024

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

@veodko
Copy link
Author

veodko commented Sep 16, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants