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

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
index fbc25d8..f682fe8 100644 (file)
--- a/C4/Reports/Guided.pm
+++ b/C4/Reports/Guided.pm
@@ -413,11 +413,44 @@ sub select_2_select_count ($) {
     $sql =~ s/\bSELECT\W+(?:\w+\W+){1,}?FROM\b|\bSELECT\W\*\WFROM\b/SELECT count(*) FROM /ig;
     return $sql;
 }
-sub strip_limit ($) {
-    my $sql = shift or return;
-    ($sql =~ /\bLIMIT\b/i) or return ($sql, 0, undef);
-    $sql =~ s/\bLIMIT\b\s*(\d+)(\s*\,\s*(\d+))?\s*/ /ig;
-    return ($sql, (defined $2 ? $1 : 0), (defined $3 ? $3 : $1));   # offset can default to 0, LIMIT cannot!
+
+# This removes the LIMIT from the query so that a custom one can be specified.
+# Usage:
+#   ($new_sql, $offset, $limit) = strip_limit($sql);
+#
+# Where:
+#   $sql is the query to modify
+#   $new_sql is the resulting query
+#   $offset is the offset value, if the LIMIT was the two-argument form,
+#       0 if it wasn't otherwise given.
+#   $limit is the limit value
+#
+# Notes:
+#   * This makes an effort to not break subqueries that have their own
+#     LIMIT specified. It does that by only removing a LIMIT if it comes after
+#     a WHERE clause (which isn't perfect, but at least should make more cases
+#     work - subqueries with a limit in the WHERE will still break.)
+#   * If your query doesn't have a WHERE clause then all LIMITs will be
+#     removed. This may break some subqueries, but is hopefully rare enough
+#     to not be a big issue.
+sub strip_limit {
+    my ($sql) = @_;
+
+    return unless $sql;
+    return ($sql, 0, undef) unless $sql =~ /\bLIMIT\b/i;
+
+    # Two options: if there's no WHERE clause in the SQL, we simply capture
+    # any LIMIT that's there. If there is a WHERE, we make sure that we only
+    # capture a LIMIT after the last one. This prevents stomping on subqueries.
+    if ($sql !~ /\bWHERE\b/i) {
+        (my $res = $sql) =~ s/\bLIMIT\b\s*(\d+)(\s*\,\s*(\d+))?\s*/ /ig;
+        return ($res, (defined $2 ? $1 : 0), (defined $3 ? $3 : $1));
+    } else {
+        my $res = $sql;
+        $res =~ m/.*\bWHERE\b/gsi;
+        $res =~ s/\G(.*)\bLIMIT\b\s*(\d+)(\s*\,\s*(\d+))?\s*/$1 /is;
+        return ($res, (defined $3 ? $2 : 0), (defined $4 ? $4 : $2));
+    }
 }
 
 sub execute_query ($;$$$) {

diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t
new file mode 100644 (file)
index 0000000..5963066
--- /dev/null
+++ b/t/db_dependent/Reports/Guided.t
@@ -0,0 +1,92 @@
+# Copyright 2012 Catalyst IT Ltd.
+#
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it under the
+# terms of the GNU General Public License as published by the Free Software
+# Foundation; either version 2 of the License, or (at your option) any later
+# version.
+#
+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+use strict;
+use warnings;
+
+use Test::More tests => 19;                      # last test to print
+
+use_ok('C4::Reports::Guided');
+
+# This is the query I found that triggered bug 8594.
+my $sql = "SELECT aqorders.ordernumber, biblio.title, biblio.biblionumber, items.homebranch,
+    aqorders.entrydate, aqorders.datereceived,
+    (SELECT DATE(datetime) FROM statistics
+        WHERE itemnumber=items.itemnumber AND
+            (type='return' OR type='issue') LIMIT 1)
+    AS shelvedate,
+    DATEDIFF(COALESCE(
+        (SELECT DATE(datetime) FROM statistics
+            WHERE itemnumber=items.itemnumber AND
+            (type='return' OR type='issue') LIMIT 1),
+    aqorders.datereceived), aqorders.entrydate) AS totaldays
+FROM aqorders
+LEFT JOIN biblio USING (biblionumber)
+LEFT JOIN items ON (items.biblionumber = biblio.biblionumber
+    AND dateaccessioned=aqorders.datereceived)
+WHERE (entrydate >= '2011-01-01' AND (datereceived < '2011-02-01' OR datereceived IS NULL))
+    AND items.homebranch LIKE 'INFO'
+ORDER BY title";
+
+my ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($sql);
+is($res_sql, $sql, "Not breaking subqueries");
+is($res_lim1, 0, "Returns correct default offset");
+is($res_lim2, undef, "Returns correct default LIMIT");
+
+# Now the same thing, but we want it to remove the LIMIT from the end
+
+my $test_sql = $res_sql . " LIMIT 242";
+($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql);
+# The replacement drops a ' ' where the limit was
+is(trim($res_sql), $sql, "Correctly removes only final LIMIT");
+is($res_lim1, 0, "Returns correct default offset");
+is($res_lim2, 242, "Returns correct extracted LIMIT");
+
+$test_sql = $res_sql . " LIMIT 13,242";
+($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql);
+# The replacement drops a ' ' where the limit was
+is(trim($res_sql), $sql, "Correctly removes only final LIMIT (with offset)");
+is($res_lim1, 13, "Returns correct extracted offset");
+is($res_lim2, 242, "Returns correct extracted LIMIT");
+
+# After here is the simpler case, where there isn't a WHERE clause to worry
+# about.
+
+# First case with nothing to change
+$sql = "SELECT * FROM items";
+($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($sql);
+is($res_sql, $sql, "Not breaking simple queries");
+is($res_lim1, 0, "Returns correct default offset");
+is($res_lim2, undef, "Returns correct default LIMIT");
+
+$test_sql = $sql . " LIMIT 242";
+($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql);
+is(trim($res_sql), $sql, "Correctly removes LIMIT in simple case");
+is($res_lim1, 0, "Returns correct default offset");
+is($res_lim2, 242, "Returns correct extracted LIMIT");
+
+$test_sql = $sql . " LIMIT 13,242";
+($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql);
+is(trim($res_sql), $sql, "Correctly removes LIMIT in simple case (with offset)");
+is($res_lim1, 13, "Returns correct extracted offset");
+is($res_lim2, 242, "Returns correct extracted LIMIT");
+
+sub trim {
+    my ($s) = @_;
+    $s =~ s/^\s*(.*?)\s*$/$1/s;
+    return $s;
+}