Summary: | A report containing a subquery that has a 'limit' will have that limit stripped out | ||
---|---|---|---|
Product: | Koha | Reporter: | Robin Sheat <robin> |
Component: | Reports | Assignee: | Robin Sheat <robin> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | chris, jonathan.druart, nengard, paul.poulain, semarie |
Version: | 3.8 | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | Sponsored | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 8594 - prevent the report system from breaking some subqueries
[SIGNED-OFF] Bug 8594 - prevent the report system from breaking some subqueries |
Description
Robin Sheat
2012-08-08 10:24:17 UTC
Created attachment 11453 [details] [review] 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. This also applies cleanly against 3.8 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. 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 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 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 :-/ 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. (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 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) 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. (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)
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. Patch pushed to master Pushed to 3.8.x, will be in 3.8.5 |