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
Posted by Wil Sinclair (wil) on 2008-06-09T12:09:02.000+0000
Please evaluate and fix/categorize as necessary.
Posted by Bill Karwin (bkarwin) on 2008-07-29T09:12:51.000+0000
Actually, the SQL fragment should be:
Posted by Colin Guthrie (coling) on 2008-12-05T06:36:48.000+0000
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/…
Posted by Colin Guthrie (coling) on 2008-12-05T07:53:00.000+0000
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
Posted by Daniel Berstein (danielb) on 2009-01-09T07:12:52.000+0000
Another description of same problem? With an alternative patch to fix it. Worth taking a look.
Posted by old of Satoru Yoshida (yoshida@zend.co.jp) on 2009-05-31T03:42:57.000+0000
Could fix of ZF-6721 you help?
Posted by Jack Tanner (jacktanner) on 2009-11-29T19:58:01.000+0000
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.
Posted by Adam Lundrigan (adamlundrigan) on 2011-05-07T21:37:07.000+0000
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?
Posted by Kim Blomqvist (kblomqvist) on 2011-05-07T22:17:44.000+0000
I would also fix the SQL fragment as stated by Bill.
Posted by Adam Lundrigan (adamlundrigan) on 2011-05-08T01:11:09.000+0000
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 :)
Posted by Kim Blomqvist (kblomqvist) on 2011-05-08T06:48:01.000+0000
Adam - your right. Mysql specifies those identical ...
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.
edit: see ZF-3792
Posted by Adam Lundrigan (adamlundrigan) on 2011-06-07T01:38:27.000+0000
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).
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?
Posted by Kim Blomqvist (kblomqvist) on 2011-06-07T15:04:30.000+0000
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!
Posted by Adam Lundrigan (adamlundrigan) on 2011-06-07T21:04:11.000+0000
@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().
Posted by Adam Lundrigan (adamlundrigan) on 2011-06-07T21:24:51.000+0000
After reviewing the patch again, I realized that most of _joinUsing() is now dead code. This is all that is really necessary:
I've attached an updated patch to this issue.
Posted by Adam Lundrigan (adamlundrigan) on 2011-06-24T12:28:47.000+0000
Fixed in trunk r24149
Posted by Adam Lundrigan (adamlundrigan) on 2011-08-27T16:26:07.000+0000
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.
Posted by Adam Lundrigan (adamlundrigan) on 2011-09-23T15:17:57.000+0000
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.
Posted by Adam Lundrigan (adamlundrigan) on 2011-10-04T21:05:24.000+0000
Attached patch ({{ZF-3309.patch}}) which implements a fix for the original problem.
Posted by Maksym Sliesarenko (sm) on 2011-10-10T13:53:32.000+0000
Posted by Maksym Sliesarenko (sm) on 2011-10-10T13:54:05.000+0000
ZF-3309.patch
Posted by Maksym Sliesarenko (sm) on 2011-10-10T13:57:55.000+0000
this is not a fix this is workaround. You should fix method _uniqueCorrelation. Replace :
With :
Posted by Adam Lundrigan (adamlundrigan) on 2011-10-14T18:38:53.000+0000
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.
Posted by Maksym Sliesarenko (sm) on 2011-11-04T16:31:29.000+0000
Why it is still not in trunk?
Posted by Adam Lundrigan (adamlundrigan) on 2011-11-04T16:59:19.000+0000
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.
Posted by Adam Lundrigan (adamlundrigan) on 2012-05-05T02:48:05.000+0000
Fixed in trunk (1.12.0): r24755 Fixed in release-1.11 (1.11.12): r24756
Not applicable to ZF2 Zend\Db
Posted by Kim Blomqvist (kblomqvist) on 2012-05-05T06:17:06.000+0000
Thanks Adam :)