From d3c89079a622d72975280dfde2c2b2fe8cd24f46 Mon Sep 17 00:00:00 2001 From: Robin Sheat Date: Wed, 8 Aug 2012 18:02:13 +0200 Subject: [PATCH] [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 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 --- C4/Reports/Guided.pm | 43 ++++++++++++++++-- t/db_dependent/Reports/Guided.t | 92 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 5 deletions(-) create mode 100644 t/db_dependent/Reports/Guided.t diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index fbc25d8..f682fe8 100644 --- 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 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; +} -- 1.7.2.3