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.
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