Bug 14704

Summary: members/statistics.pl crashes because the SQL 'return'-keyword might be reserved at 'return FROM ('
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: PatronsAssignee: Bugs List <koha-bugs>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, kyle.m.hall
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14719
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14704 - members/statistics.pl crashes because the SQL 'return'-keyword might be reserved at 'return FROM ('

Description Olli-Antti Kivilahti 2015-08-21 10:42:10 UTC
At Ubuntu 14.04 using MariaDB, the following SQL doesn't work:

SELECT COUNT(*) as count_precedent_state,issues,onloan,return
FROM (
    SELECT it.* FROM issues i, items it
    WHERE i.borrowernumber = 1234 AND
          i.itemnumber = it.itemnumber AND
          DATE(i.issuedate) < CAST(now() AS date)
    UNION
    SELECT it.* FROM old_issues oi, items it
    WHERE oi.borrowernumber = 1234 AND
          oi.itemnumber = it.itemnumber AND
          DATE(oi.issuedate) < CAST(now() AS date) AND
          DATE(oi.returndate) = CAST(now() AS date)
    ) tmp
GROUP BY issues,onloan,return;

We get "Syntax error near 'return FROM ('"

This patch changes the SQL to prefix the selected columns with a table name prefix, so we get:

SELECT COUNT(*) as count_precedent_state,tmp.issues,tmp.onloan,tmp.return
FROM (
    SELECT it.* FROM issues i, items it
    WHERE i.borrowernumber = 1234 AND
          i.itemnumber = it.itemnumber AND
          DATE(i.issuedate) < CAST(now() AS date)
    UNION
    SELECT it.* FROM old_issues oi, items it
    WHERE oi.borrowernumber = 1234 AND
          oi.itemnumber = it.itemnumber AND
          DATE(oi.issuedate) < CAST(now() AS date) AND
          DATE(oi.returndate) = CAST(now() AS date)
    ) tmp
GROUP BY tmp.issues,tmp.onloan,tmp.return;

Which is more formal and less implicit.
Comment 1 Olli-Antti Kivilahti 2015-08-21 10:44:46 UTC
Created attachment 41775 [details] [review]
Bug 14704 - members/statistics.pl crashes because the SQL 'return'-keyword might be reserved at 'return FROM ('
Comment 2 Jonathan Druart 2015-08-21 11:10:11 UTC
I got the following if I execute directly the second query in my MariaDB CLI:

ERROR 1054 (42S22): Unknown column 'tmp.return' in 'field list'
Comment 3 Olli-Antti Kivilahti 2015-08-25 08:59:30 UTC
(In reply to Jonathan Druart from comment #2)
> I got the following if I execute directly the second query in my MariaDB CLI:
> 
> ERROR 1054 (42S22): Unknown column 'tmp.return' in 'field list'

That is because my hastily created example SQL was missing explicit definition for issues.return and old_issues.return.
This wasn't the case in the code (hopefully).

Try this:

SELECT COUNT(*) as count_precedent_state,tmp.issues,tmp.onloan,tmp.return
FROM (
    SELECT it.*, i.return FROM issues i, items it
    WHERE i.borrowernumber = 1234 AND
          i.itemnumber = it.itemnumber AND
          DATE(i.issuedate) < CAST(now() AS date)
    UNION
    SELECT it.*, oi.return FROM old_issues oi, items it
    WHERE oi.borrowernumber = 1234 AND
          oi.itemnumber = it.itemnumber AND
          DATE(oi.issuedate) < CAST(now() AS date) AND
          DATE(oi.returndate) = CAST(now() AS date)
    ) tmp
GROUP BY tmp.issues,tmp.onloan,tmp.return;
Comment 4 Olli-Antti Kivilahti 2015-08-25 09:01:15 UTC
The dynamically created SQL here is problematic and needs to be overhauled on a more broader level. Since the columns desired can be gained from a syspref, the SQL needs to take that into account as well. This is an optimal scenario to leverage DBIx and its more configurable search building.