[Bug 8594] New: A report containing a subquery that has a 'limit' will have that limit stripped out
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Priority: P5 - low Change sponsored?: --- Bug ID: 8594 Assignee: gmcharlt@gmail.com Summary: A report containing a subquery that has a 'limit' will have that limit stripped out Severity: normal Classification: Unclassified OS: All Reporter: robin@catalyst.net.nz Hardware: All Status: NEW Version: rel_3_8 Component: Reports Product: Koha 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Change sponsored?|--- |Sponsored Assignee|gmcharlt@gmail.com |robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #1 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 11453 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11453&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #2 from Robin Sheat <robin@catalyst.net.nz> --- This also applies cleanly against 3.8 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #3 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #4 from Robin Sheat <robin@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #11453|0 |1 is obsolete| | --- Comment #5 from Nicole C. Engard <nengard@gmail.com> --- Created attachment 11654 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11654&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |nengard@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #6 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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 :-/ -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #7 from Robin Sheat <robin@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.poulain@biblibre.com, | |semarie-koha@latrappe.fr --- Comment #9 from Paul Poulain <paul.poulain@biblibre.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #10 from Robin Sheat <robin@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #11 from Frère Sébastien Marie <semarie-koha@latrappe.fr> --- (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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 --- Comment #12 from Robin Sheat <robin@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #13 from Paul Poulain <paul.poulain@biblibre.com> --- Patch pushed to master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz --- Comment #14 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.8.x, will be in 3.8.5 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |saga-sumerki@list.ru When did the bot| |2015-05-17 last check this| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot|2015-05-17 00:00:00 |2015-05-17 13:00 last check this| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot|2015-05-17 13:00:00 |2015-05-04 13:00 last check this| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot|2015-05-04 13:00:00 | last check this| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot| |2015-05-22 last check this| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot|2015-05-22 00:00:00 | last check this| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Hamletik <saga-sumerki@list.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Whiteboard| |Fic ANA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Whiteboard|Fic ANA | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8594 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|saga-sumerki@list.ru | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org