Bug 8594 - A report containing a subquery that has a 'limit' will have that limit stripped out
Summary: A report containing a subquery that has a 'limit' will have that limit stripp...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Robin Sheat
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-08 10:24 UTC by Robin Sheat
Modified: 2015-05-03 20:08 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8594 - prevent the report system from breaking some subqueries (7.27 KB, patch)
2012-08-08 16:09 UTC, Robin Sheat
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8594 - prevent the report system from breaking some subqueries (7.53 KB, patch)
2012-08-16 23:42 UTC, Nicole C. Engard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2012-08-08 10:24:17 UTC
The limit is stripped out when Koha fetches the headers* so that another limit can be applied. However, if you have a subquery in the fields part, then you probably need to apply a limit for things to work. So fetching the headers fails.

I'm thinking that the limit should be removed after the last where clause, or if there's no where clause, do what is currently done. This won't fix every case, but should result in more things working.

* in other news, I'm not a fan of it having to run the query twice, which it seems to do, just to grab the headers.
Comment 1 Robin Sheat 2012-08-08 16:09:52 UTC Comment hidden (obsolete)
Comment 2 Robin Sheat 2012-08-08 16:13:31 UTC
This also applies cleanly against 3.8
Comment 3 Jared Camins-Esakov 2012-08-11 16:49:35 UTC
Could you give an example of a query other than the one you use in the unit tests? I don't have the data needed for that query, and I can't come up with a query using subqueries that A) doesn't work on current master and B) works with the patch.
Comment 4 Robin Sheat 2012-08-14 09:33:27 UTC
OK, here's a totally artificial query that should be able to reproduce this issue:

select biblionumber, (select itemnumber from items where items.biblionumber=biblio.biblionumber LIMIT 1) from biblio where biblionumber<1000;

Try it from the mysql prompt with data where you have a case of one biblio with multiple items and it should work unless you remove the limit, in which case it says:

ERROR 1242 (21000): Subquery returns more than 1 row
Comment 5 Nicole C. Engard 2012-08-16 23:42:04 UTC
Created attachment 11654 [details] [review]
[SIGNED-OFF] Bug 8594 - prevent the report system from breaking some subqueries

If you had a report query that had a subquery in the fields list, and
that subquery had a LIMIT specifier, then it would be removed which
could break your query. This patch prevents this case from breaking by
ensuring that only a LIMIT that follows the last WHERE in the query is
removed.

If you don't have a WHERE, then it will behave like it always
did, removing all the cases of LIMIT (which would still break a subquery
but this is a) more rare, and b) would require more intelligent parsing
to deal with.

Also adds test cases and function documentation.

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

Tested with this report:

select biblionumber, (select itemnumber from items where items.biblionumber=biblio.biblionumber LIMIT 1) from biblio where biblionumber<1000;

and it worked like a charm
Comment 6 Jonathan Druart 2012-08-17 10:40:53 UTC
QA comments:

All seems good.
But I have 2 questions :)
Why did you create the test file into the db_dependent directory ?
And could you explain me (quickly) what is the use of the \G assertion ? I read the doc but I did not understand :-/
Comment 7 Robin Sheat 2012-08-17 11:01:27 UTC
1) The Reports::Guided module looks like it's database dependant when it initialises.

2) \G starts a search from the location that a previous search finished. In this case, it first looks for 'WHERE', and then starts the search to do the replacing from that point, and not the start of the string.
Comment 8 Jonathan Druart 2012-08-17 11:42:54 UTC
(In reply to comment #7)
> 1) The Reports::Guided module looks like it's database dependant when it
> initialises.
> 
> 2) \G starts a search from the location that a previous search finished. In
> this case, it first looks for 'WHERE', and then starts the search to do the
> replacing from that point, and not the start of the string.

Ok, thank you Robin !
Marked as Passed QA
Comment 9 Paul Poulain 2012-08-29 16:28:45 UTC
As some reports are publicly accessible, this patch can be a hole to a security issue.

Adding Frere Sébastien Marie to this bug, please, give us your POV on this patch: is it safe ?

(doing nothing until I've feedback)
Comment 10 Robin Sheat 2012-08-30 09:08:24 UTC
It doesn't really change anything that wasn't there already, it just does it in a way that has a greater chance of success.
Comment 11 Sébastien Marie 2012-08-30 10:12:55 UTC
(En réponse au commentaire 10)
> It doesn't really change anything that wasn't there already, it just does it
> in a way that has a greater chance of success.

I agreed with Robin: the patch (without context) isn't a problem. It is just a function which take a string (a SQL query) and return:
 - a new query (a "subset" of the previous one)
 - the offset and limit (interpolate from previous query)

For me, the function is suffisant, but be aware that it could be by-passed (with a $sql *well-formed*, the LIMIT isn't detected, and not removed). The fact that it is a problem, or not, depend of context...

just for example:

sql_strip("SELECT * FROM test LIMIT /* a comment */ 10")
 return ("SELECT * FROM test LIMIT /* a comment */ 10",   0,   undef)

But for me it is ok (if there is not security implication to have a LIMIT not removed... but it needs a global review of the module, not just this patch)
Comment 12 Robin Sheat 2012-08-30 10:47:35 UTC
Yes, it's definitely possible for this function to not work in some cases, however those cases are fewer than the previous version :) 

Really, it should be changed such that it doesn't do this at all, and works out the info it needs from the results it receives, that way it won't need to do queries twice anyway, and I always think that processing and changing SQL like this is a bit of a code-smell.
Comment 13 Paul Poulain 2012-09-05 08:22:54 UTC
Patch pushed to master
Comment 14 Chris Cormack 2012-09-05 23:05:30 UTC
Pushed to 3.8.x, will be in 3.8.5