ZF-3309: joinUsing() produces unusable SQL with array table definitions

Description


$select->from('table1')->joinUsing( array('t2'=>'table2'), 'column1')
// produces:   JOIN "table2"  AS "t2" ON "table1".column1 = "table2".column1
// should be:  JOIN "table2"  AS "t2" ON "table1".column1 = "t2".column1

if table is defines as array to use an table alias it should use the alias NOT the table name!

Comments

Please evaluate and fix/categorize as necessary.

Actually, the SQL fragment should be:


JOIN "table2" AS "t2" USING ("column1")

This problem is still apparent in the latest release.

This is a pretty major bug to be honest. The least that could be done is update the docs to explain that it is not possible to to use joinUsing with aliased tables.

The relevant place in the docs is: http://framework.zend.com/manual/en/…

Attached is a patch that seems to address this for me. I've not done a huge amount of testing tho' so YMMV.

I have sadly noticed another major flaw in join*Using() calls tho' that make them mostly useless for me. I've reported this as ZF-5165

Another description of same problem? With an alternative patch to fix it. Worth taking a look.

Could fix of ZF-6721 you help?

No, this problem still exists in 1.9.5 (and probably also 1.9.6), so the fix to ZF-6721 does not affect this bug.

I've reviewed the patch provided by Colin Guthrie, added some unit tests to expose the issue, and attached a patch file back to this issue. Once patch is applied the Zend_Db test suite still passes, and the suggested modifications to _joinUsing don't appear to change the behavior of the applications i've explicitly used joinUsing in.

Any objections to me committing this patch to trunk?

I would also fix the SQL fragment as stated by Bill.

Technically yes, but does adding that syntax provide any benefit over using JOIN ON? I've been looking into implementing Bill's suggestion, and from what I can see we'll need to add a new joinType for JOIN USING, then add logic in each adapter to render the join as JOIN USING rather than JOIN ON if the column names are identical. Right now joinUsing isn't treated special, as it's just a shortcut for JOIN ON used when the join column names match. Unless there is a compelling argument I don't see the need to implement JOIN ... USING separately.

In my opinion, nothing substantial is gained by using JOIN USING rather than JOIN ON.

Thoughts? I'm no SQL expert, so I may be way of base here...if so, let me know :)

Adam - your right. Mysql specifies those identical ...


join_condition:
    ON conditional_expr
  | USING (column_list)

The minor difference is that if you are using JOIN USING, it requires that the column names are identical in both tables. But the main point is to have a nice, clean and readable SQL.

However, I still think that ZF's joinUsing() does not support DWIM acronym "Do What I Mean". For example Mysql's JOIN USING supports column list to match multiple columns at once.


$select->from('a')->joinUsing('b', array('c1', 'c2', c3'));

/* expected */ SELECT ... FROM `a` JOIN `b` USING (`c1`, `c2`, `c3`);
/* result */   SELECT ... FROM `a` INNER JOIN `b` ON `b`.Array = `a`.Array

edit: see ZF-3792

Kim:

Good point. JOIN USING simplifies the SQL code, and is more DWIM than "emulating" JOIN USING using JOIN ON, as the current implementation does. I've attached a new patch which essentially modifies _join() to accept an argument with the type (ON or USING), which is used when rendering the FROM portion of the query. It, however, does not enforce that the condition value passed to _join() matches that join type (ON or USING).


$select = $this->_db->select();
$select->from('table1')->joinUsing('table2', array('c1','c2','c3'));
// Result:  SELECT "table1".*, "table2".* FROM "table1" INNER JOIN "table2" USING (c1,c2,c3)

$select = $this->_db->select();
$select->from('table1')->joinUsing(array('t2'=>'table2'), array('column1'));
// Result:  SELECT "table1".*, "table2".* FROM "table1" INNER JOIN "table2" AS "t2" USING (column1)

$select = $this->_db->select();
$select->from('table1')->joinInner('table2', 'table1.c1 = table2.c2');
// Result:  SELECT "table1".*, "table2".* FROM "table1" INNER JOIN "table2" ON table1.c1 = table2.c2

One thing to note is that identifiers aren't automatically quoted, as you can see in the above example. There are a bunch of existing unit tests for JOIN USING which quote identifiers before they are run through the query. Should joinUsing() auto-quote identifiers such as column names, or leave that up to the user (as the existing unit tests seem to indicated)?

Thoughts?

bq. Should joinUsing() auto-quote identifiers such as column names, or leave that up to the user (as the existing unit tests seem to indicated)?

I wouldn't rewrite all those test because of this. I think it's ok to leave this up to the user, which I base to the fact that this kind of behavior is already well documented in reference guide ...

"Note: No quoting is applied to the expression you specify for the join condition; if you have column names that need to be quoted, you must use quoteIdentifier() as you form the string for the join condition."

The patch looks good for me. I have tested it successfully in my project which uses lot of joinUsing(). Nice work Adam!

@Kim: Thanks. Since the reference guide places the onus on the end user to do the quoting, i'll leave the patch and existing unit-tests as-is from that perspective. I did, however, update those unit tests (in Zend_Db_Select_StaticTest and Zend_Db_Table_Select_StaticTest) to reflect the change in behavior of join*Using().

After reviewing the patch again, I realized that most of _joinUsing() is now dead code. This is all that is really necessary:


public function _joinUsing($type, $name, $cond, $cols = '*', $schema = null)
{
    if (empty($this->_parts[self::FROM])) {
        require_once 'Zend/Db/Select/Exception.php';
        throw new Zend_Db_Select_Exception("You can only perform a joinUsing after specifying a FROM table");
    }

    $cond = "(" . implode(",", (array)$cond) . ")";        
    return $this->_join($type, $name, $cond, $cols, $schema, self::SQL_USING);
}

I've attached an updated patch to this issue.

Fixed in trunk r24149

Backed r24149 out from trunk pending rewrite (r24411).

After speaking with [~ralph] on the issue, two possible issues were found:

SQL Server and Sybase do not support USING clause

JOIN USING and JOIN ON differ in the way they return columns. ie: JOIN ON would have {{t1.col}} and {{t2.col}} in the result set, but JOIN USING would have only {{col}}, so references to {{t1.col}} or {{t2.col}} in the WHERE clause would fail.

More information here: http://en.wikipedia.org/wiki/Join_%28SQL%29/…

I will re-evaluate the fix and related issues to see if there are feasible workarounds which could be implemented.

I will fall back on the plan to just reproduce and fix the issue at hand: joinUsing does not respect table aliases. Will do that ASAP.

Attached patch ({{ZF-3309.patch}}) which implements a fix for the original problem.

{ZF-3309.patch

ZF-3309.patch

this is not a fix this is workaround. You should fix method _uniqueCorrelation. Replace :


if (is_array($name)) {
            $c = end($name);
        } else {

With :


     if (is_array($name)) {
            $key = key ($name);
            if (is_string($key)) {
                $c = $key;
            } else {
                $c = end($name);
            }
        } else {

Thank you, Maks! That is a much cleaner fix. I've attached {{ZF-3309_rev4.patch}}, which includes your suggested fix and an updated version of my unit tests.

Why it is still not in trunk?

I am waiting for feedback on the patch from the component maintainer. I'm hopeful that the fix will make it into the next mini release, whenever that happens.

Fixed in trunk (1.12.0): r24755 Fixed in release-1.11 (1.11.12): r24756

Not applicable to ZF2 Zend\Db

Thanks Adam :)