Update libraries/joomla/database/query.php #1539

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

Change to __toString to make the union functionality work and a change to union since unions can be ordered.

@MarkRS-UK MarkRS-UK Update libraries/joomla/database/query.php
Change to _string to make the union functionality work and a change to union since unions can be ordered.
677cde3
Contributor

elinw commented Oct 5, 2012

Can you update the tests so they include order?

Contributor

LouisLandry commented Oct 9, 2012

@MarkRS-UK this is a great improvement. As @elinw requested I'd love to see a couple of additions to the unit tests so that we can verify that things work as expected and queries are being built correctly both now, and in the future when further changes are made to the class.

Also, I'd appreciate it if you can change the title to be a little more descriptive of the improvement/fix you are making so that it shows up in our change log appropriately. I'm going to set this for the 12.3 milestone (a couple of months).

I'm closing this pull request for now. That doesn't mean I am rejecting it, I'm just keeping it out of our active queue until you can make the adjustments mentioned above. When you have the tests added please re-open it and we can get things merged then. Thanks a bunch for the work!

LouisLandry closed this Oct 9, 2012

Hi Louis and @elinw. Sorry not to have got back to you about this so far, I've been silly-busy.

Unit test, yes. There's some controversy, it seems, over unit tests. The previous test tested an internal setting which was no indicator of the actual process required. A "proper" test would, I think, require a (very small) test dataset and a defined result. Do you(s) have a thought about this?

Contributor

elinw commented Oct 10, 2012

@MarkRS-UK The other tests all just were looking at the generated query strings. You may be able to piggy back on some of the datasets that are there already and that's probably preferable if possible.

Contributor

LouisLandry commented Oct 11, 2012

@MarkRS-UK I certainly understand your point on the unit tests. I don't know that you need to use a database to test the functionality, you could simply test by exercising the affected code and evaluated the resultant SQL against what is expected. In fact I would think this preferable since we've removed any external dependencies in the tests.

We have the capability to test against an actual database as well if that is something you feel strongly about, but I am not sure that it will increase the robustness of the validation.

Thanks for the feedback.

On further reflection I think you're right, it's better just to check strings. Avoiding external dependency is more important than I first considered.

Ok, I've spent the day with this (mostly) and I just want to check my findings.

My corrections cause all the existing tests union to fail, which is quite a result!
I think it's because the tests are wrong, in their assertions about the way SQL works, rather than my code being wrong.

They all assume that a query with a union in it should have "UNION" in front of each select, which is incorrect. In fact, I think the whole approach of JQuery is incorrect in its approach to union clauses, but we can avoid changing everything and take a smaller step first.

Currently the union method takes something like $q->union('SELECT a from b') and produces the SQL "UNION (SELECT a FROM b)", which is nonsense. Similarly $q->union(array('SELECT a from b', 'SELECT c FROM d')) produces "UNION (SELECT a FROM b) UNION (SELECT c FROM d)", which is also not right.

So, am I missing something obvious here, or is it really as incorrect as I think?

My new code renders the first instance sufficiently correctly as "(SELECT a FROM b)" and the second as "(SELECT a FROM b) UNION (SELECT c FROM d)". Still not completely right I'd say (parentheses around the first clause aren't necessary), but works.

Contributor

elinw commented Oct 12, 2012

You need to have two complete queries to then UNION the results of. This API basically you take a full query and append the UNION (another query ) to it. I think you are looking for UNION(a.b) give a UNION b? is that right? that's not the API described here. If the api is not working as described that's certainly a bug.

 * Add a query to UNION with the current query.

It would be cool to have UNION(a,b) but it's quite complex in fact.

https://github.com/joomla/joomla-cms/blob/master/administrator/components/com_finder/helpers/indexer/query.php#L1293

Is a place where we are using union in the cms.

Ah, ok. My discussions have been with someone who thought/thinks it only works as UNION(a, b). What you describe is how I had assumed it would/should work.

To be fair to my other correspondent, the parameter comments in the header of the union method say that it should accept an array of select statements. It is very easy to make it produce a correct result without having specified a prior select. I'll check if this is also easy to make it work with a select clause present.

How do I avoid the current test problem, ie the test for union passes successfully but in fact JQuery will not produce a full query with a union statement?

Contributor

elinw commented Oct 12, 2012

It does accept an array, they are each appended to the original query with the ) UNION ( glue. I have to look more closely at it which I will try to do this weekend if possible.

It is actually not that easy to do what you are proposing in the context of complex queries built using JDatabaseQuery. Of course it is trivial if you just want to use two simple string queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment