fix: prevent duplicate table name errors

[Seb33300]

Fixes #2871, #880 (maybe all issues with Self Join tag )

This PR fixes the duplicate table name SQL error by adding aliases to joined tables.
It also supports multi levels joins:
Eg:

- article.author.name
- article.category.created_by.name
- article.comments[].user.name

⚠️ This PR includes a BC break

As you can see with the test case I modified, this PR comes with a small BC break.
If a column has defined a custom order by using orderColumn(), the table is now using an alias that will not match with the table name.
A new parameter has been added to the orderColumn() callable to pass the column name with alias.

DataTables::eloquent($model)
    ->orderColumn('relation.name', function ($query, $order, $column) {
        // $column = alias.name of the relation.name column
        $query->orderBy($column, $order);
    });

[Seb33300]

I will check failing test next week

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Seb33300]

Hi @yajra, PR is now ready for review 🙏

[obyajra]

The changes look good. Will hold off for now and review again in v12 (Laravel 12) release since there is a BC.

Will try to complete the v12 within this or earlier next week.

Thanks!

[yajra]

@Seb33300 can you provide an example of the table aliases generated in this PR? One concern I have is the Oracle limitation that only accepts up to 30 chars for object/table name length.

Initial Laravel 12 support merged on the master branch.

Thanks!

[Seb33300]

@yajra It concatenates all parent relation names to create a unique alias name:

updated_by.name => _updated_by.name
created_by.company.name => _created_by_company.name

Here is an example of query generated (on MySQL):

select `users`.* 
from `users` 
left join `users` as `_updated_by` on `users`.`updated_user_id` = `_updated_by`.`id` 
where `_updated_by`.`name` LIKE '%%seb%%' 
order by `_updated_by`.`name` asc 
limit 10 
offset 0

[Seb33300]

One concern I have is the Oracle limitation that only accepts up to 30 chars for object/table name length.

Please note that starting Oracle Database 12c Release 2 (12.2) released in 2017, the maximum size is now 128 chars.
https://oracle-base.com/articles/12c/long-identifiers-12cr2

And all Oracle versions prior that one are already no longer supported by Oracle:
https://en.wikipedia.org/wiki/Oracle_Database

[yajra]

One concern I have is the Oracle limitation that only accepts up to 30 chars for object/table name length.

Please note that starting Oracle Database 12c Release 2 (12.2) released in 2017, the maximum size is now 128 chars. https://oracle-base.com/articles/12c/long-identifiers-12cr2

And all Oracle versions prior that one are already no longer supported by Oracle: https://en.wikipedia.org/wiki/Oracle_Database

Yes, unfortunately - I still have projects that uses old Oracle version >.<

[Seb33300]

Unless you have columns with multiple joins, It's unlikely to happen, but still theoretically possible.

A workaround I can suggest is to use md5 to generate a hash of the alias in case it's more than 30 chars.

[yajra]

Thanks and will review first on an actual project. I just recalled that we rarely use relation and opt to use join statements for more stable results.

[yajra]

Tested on a new Laravel 12 app and works as expected. Tested self-join SQL and seems to be fixed:

SQL Generated:

select * from "users" left join "users" as "_creator" on "users"."created_by" = "_creator"."id" order by "_creator"."name" desc limit 10 offset 0

Is the _ on alias (_creator) intended?

[Seb33300]

Is the _ on alias (_creator) intended?

Yes, this is to prevent a potential conflict with a table name if the relation name is the same as the table name.

[yajra]

Thanks for the feedback. Released on v12 🚀

[Arkhas]

⚠️ This PR includes a BC break

As you can see with the test case I modified, this PR comes with a small BC break. If a column has defined a custom order by using orderColumn(), the table is now using an alias that will not match with the table name. A new parameter has been added to the orderColumn() callable to pass the column name with alias.

DataTables::eloquent($model)
    ->orderColumn('relation.name', function ($query, $order, $column) {
        // $column = alias.name of the relation.name column
        $query->orderBy($column, $order);
    });

That's not a small breaking change, every time we do a custom order we have to modify our code for example (not real code) :

DataTables::eloquent($model)
     ->orderColumn('item.total', function ($query, $order) {
         $query->orderBy(DB::raw('cart_items.amount * cart_items.quantity'), $order);
     });

we have to change it to :

DataTables::eloquent($model)
     ->orderColumn('item.total', function ($query, $order, $column) {
     	 $alias = explode('.', $column)[0];
         $query->orderBy(DB::raw("$alias.amount * $alias.quantity"), $order);
     });

please, find another way of doing it ?

Plus, why keeping the leading _ ? juste remove it if first char ?

[Seb33300]

Is the _ on alias (_creator) intended?

Yes, this is to prevent a potential conflict with a table name if the relation name is the same as the table name.

Hi @Arkhas, yes the _ is intended to prevent conflict, I need to do some additional test to confirm if we can remove it or not.

Also in #3229 we added a resolver to the filterColumn().
Maybe we can do the same with orderColumn() to simplify things.

[Arkhas]

having the column and the table as 2 seperate parameters would help :

->orderColumn('user.id', function ($query, $order, $column, $tableAlias)

but maybe we can have it as a configuration option ? we are struggling updating the package because of this commit :/

[Seb33300]

I will have a look on it tomorrow to see if I can improve by keeping backward compatibility

[Seb33300]

@Arkhas I finally had time to take a look on this and it's unfortunately not possible to as backward compatibility.

Even if we remove the front _ it will not help because we use the relation name, not the table name.
For example user.name will result in JOIN users as _user.

This is to support cases where we can join the same table multiple times such as:

• created_by.name => JOIN users as _created_by
• updated_by.name => JOIN users as _updated_by

Before, this was not working and resulted in an error.

In order to help, I created a new PR to replace the $column parameter by a $resolver parameter.
The resolver is callable you can use to get the alias from a column, in your case:

DataTables::eloquent($model)
     ->orderColumn('item.total', function ($query, $order, $resolver) {
         $query->orderBy(DB::raw("{$resolver('item.amount')} * {$resolver('item.quantity')}"), $order);
     });

Can you please share your feedback on this?

[Arkhas]

that would not help a lot, we would just need the table alias, (the name of the column is totally irrelevant as we are using a custom ordering.

what would be nice:

->orderColumn('item.total', function ($query, $order, $tableAlias) {

but what we would need is to be able to not use this feature to not completely break our app like something like this :

			if(config('datatables.use_smart_alias'){
				$lastAlias = $tableAlias ?: $lastQuery->getModel()->getTable();
	            $tableAlias = $tableAlias.'_'.$eachRelation;
	            $pivotAlias = $tableAlias.'_pivot';
            }else{
				$pivotAlias = $model->getTable();
			}

or something like that

[Seb33300]

The $tableAlias param will work only if you work with a single table.
But in more complex cases, if you need to work on values coming from different tables, this will not work.

for example if you have a case where you need to get the tax rate from another table, using the resolver you can do:

$resolver('item.amount') * $resolver('item.tax_rule.rate')

The resolver can be used to resolve relations between models. And it automatically generate joins by preventing duplicate table aliases.

I can try to implement a BC option as you suggested, but if that option is not enabled, you may face ambiguous column name errors.

[Seb33300]

Another option you may use is to re create the join by yourself.
On your base query, you can just add

$query->join('cart_items', ...)

And then, you will be able to use the table name cart_items like before.

[Arkhas]

I can try to implement a BC option as you suggested, but if that option is not enabled, you may face ambiguous column name errors.

it would be easier for us to upgrade, maybe on a datatable basis instead of a global configuration so that we could upgrade slowly each of our Datatables :

$datatable = new EloquentDataTable($this->query());
$datatable->smartTableAlias(false);

something like that ? maybe more versatile

[Seb33300]

Hi @Arkhas, please check that PR: #3234