Bug Report #2780

Database Table Prefixes aren't correctly handled when joining

Added by Paul Banks over 4 years ago. Updated about 4 years ago.

Status:ClosedStart date:04/05/2010
Priority:NormalDue date:
Assignee:John Heathco% Done:

0%

Category:Modules:Database
Target version:v3.0.8
Resolution:fixed Points:

Description

Table prefixes don't work when joining to an aliased table since the join alias isn't quoted as a table.

Example (simplified to illustrate the point but untested, the quoting is the important bit):

DB::select()
    ->from('table')
    ->join(array('table2', 'table_alias'))
    ->on('table1.id', '=', 'table2.table1_id')
    ->where('table2.field', '=', 1);

Now if the selected database is set up with a table prefix (lets us 'eg_' as an example) you get the following SQL:

SELECT * FROM `eg_table1` LEFT JOIN `eg_table2` AS `table_alias` ON (...) 
WHERE `eg_table_alias`.`field` = 1

Which throws an error about 'eg_table_alias' not existsing.

The problem seems to lie in the fact that `Database::quote_table()` catches the case where a table identifier is being used (so correctly adds 'eg_' to the table name in the join) but doesn't add the prefix to the alias (which it probably shouldn't most of the time. And the `Database::quote_identifier()` method catches the table references in the where clause (and elsewhere) and adds the prefix even though in this case the table reference is to an alias and not to an actual table.

Possible solutions

The only viable solution I can see here is to alias both the table AND the alias when passing an array as the first argument to `join()`. Not sure if there are wider ramifications of this although I can't immediately think of any.

In fact I think it would be correct to ALWAYS add the prefix to the alias of a table as well as the actual table given the fact that any reference to a field of the aliased table using where(), group_by(), order_by() etc will automatically have the prefix added to it by `quote_identifier()`.

It may require some consideration as to what ill effects such a change might have but it seems the only viable solution.


Related issues

Related to Kohana v3.x - Patch #2634: Kohana_ORM::with() does not work well when Database uses ... Closed 02/20/2010

Associated revisions

Revision da380c09
Added by Woody Gilk about 4 years ago

Reapplied a8c8cedfeffc80ba366e02a67b94640eafb2782c, which was lost in da2b5f9a658a21377044bdd98d25160a1a400841, fixes #2780

History

#1 Updated by Paul Banks over 4 years ago

Apologies for the typo in the example (table != table1) and the formatting blunders (I thought backticks highlighted as code spans).

This issue is actually general for all forms of table aliasing, not just joins, when using a prefix. I've never had to use a prefix but have had a recent issue with Jelly where a user is using table prefixes and found this to be a problem.

#2 Updated by Isaiah DeRose-Wilson over 4 years ago

ORM has the same problem (see related issue above), I'm not sure if this is something the database builder should try to deal with, or if the ORM layer should just work around it.

#3 Updated by John Heathco over 4 years ago

  • Assignee set to Woody Gilk

Woody, what do you think of the possible solution of adding the table prefix to the alias in the case of an alias being used?

If this is implemented, I'll have to rollback my (temporary?) fix for ORM that deals with this issue...

#4 Updated by John Heathco over 4 years ago

  • Target version changed from v3.0.5 to v3.0.4

#5 Updated by John Heathco over 4 years ago

  • Target version changed from v3.0.4 to v3.0.5

#6 Updated by Paul Banks over 4 years ago

I've looked through the ORM issue and fix. While that is the simplest fix and we will go with it in Jelly if that is the only option, It seems to me that the Database table prefix concept simply doesn't work with joins at all as it is - irrespective of the ORM layer. There is always a bit of a hack required somewhere.

I'd like to hear what people think of the suggestion of always adding the table prefix to the alias as well as the actual table name for a join (or anywhere else a table might be aliased) . It seems to me this is safe and is the only way The Database class can be consistent and transparent in it's implementation of prefixes without requiring work-arounds or special treatment for some functionality.

#7 Updated by Woody Gilk over 4 years ago

This is definitely a rather hard bug to work around. The only possible way I can see of fixing it is to store the aliases as they come in and then check them when calling `quote_identifier`. This is a non-trivial solution, but may be the only one that works.

#8 Updated by Paul Banks over 4 years ago

The only possible way I can see of fixing it is to...

Perhaps I am being really dull but can you point out for my benefit why my suggestion of simply adding the table prefix to the alias as well as the table identifier won't work?

Sure the actual SQL will have a different alias to the one the user supplied but then everywhere they have used that alias in their query (except in database expressions where we can't by definition do anything) will have had the table prefix added to the alias anyway.

I can't think of a situation where this wouldn't work exactly as expected and it is a one line change but perhaps I have missed something. Would be great if you can explain why this is a bad or incomplete solution.

#9 Updated by John Heathco over 4 years ago

Paul Banks wrote:

The only possible way I can see of fixing it is to...

Perhaps I am being really dull but can you point out for my benefit why my suggestion of simply adding the table prefix to the alias as well as the table identifier won't work?

Sure the actual SQL will have a different alias to the one the user supplied but then everywhere they have used that alias in their query (except in database expressions where we can't by definition do anything) will have had the table prefix added to the alias anyway.

I can't think of a situation where this wouldn't work exactly as expected and it is a one line change but perhaps I have missed something. Would be great if you can explain why this is a bad or incomplete solution.

This is much simpler to implement as well!

#10 Updated by Paul Banks over 4 years ago

This is much simpler to implement as well!

Indeed, does anyone have any objections? If not I'll try to find time to put together the one line patch for this and show that it works. If someone else gets a chance before I've updated here though feel free to do this yourself - it will likely be a single line and I may not get round to it for a little while...

#11 Updated by Woody Gilk over 4 years ago

It isn't my favorite solution, but it is the most simple and effective. If you want to patch it in a fork, Paul, it would be greatly appreciated.

#12 Updated by John Heathco about 4 years ago

  • Status changed from New to Closed
  • Assignee changed from Woody Gilk to John Heathco
  • Resolution set to fixed

#14 Updated by Human Internals about 4 years ago

Any chance to merge this to 3.0.7? It might take a long time until 3.1.0 is out, as its a critical bug that breaks things it might be better to fix this at the next release. or is the next version going to jump straight to 3.1.0?

#15 Updated by Woody Gilk about 4 years ago

It was part of v3.0.5, the change is already there.

#16 Updated by Human Internals about 4 years ago

Are you sure? I'm on 3.0.6.2, my quote_table method at modules/database/classes/kohana/database.php looks like:

    public function quote_table($value)
    {
        // Assign the table by reference from the value
        if (is_array($value))
        {
            $table =& $value[0];
        }
        else
        {
            $table =& $value;
        }

Currently I still need the hack I mentioned at #3019 to get it to work. Shouldn't the fix be inside the is_array() condition of quote_table? because it isn't. Can you please check it was really part of 3.0.5?

#17 Updated by Woody Gilk about 4 years ago

Yes, I am sure. The logic has been slightly altered, but the fix is there, http://github.com/kohana/database/blob/master/classes/kohana/database.php#L484

#18 Updated by Human Internals about 4 years ago

Are you sure its the right file/line/link? I think that's the code that was always there, and not the fixed one.

The code there doesn't fix the issue, the $table variable that the table_prefix is getting perpended to is a ref to $value0, which is the table name - the table alias isn't being changed.

#19 Updated by Jonas De Smet about 4 years ago

  • Status changed from Closed to Feedback
  • Target version changed from v3.0.5 to v3.0.8

Like Human Internals already sayed, I have the same issue.

The fix was there (http://github.com/kohana/database/commit/a8c8cedfeffc80ba366e02a67b94640eafb2782c) but was "reverted" here: http://github.com/kohana/database/commit/da2b5f9a658a21377044bdd98d25160a1a400841

I think this is slipped trough by not merging the new items...

#20 Updated by Woody Gilk about 4 years ago

  • Status changed from Feedback to Closed

Also available in: Atom PDF