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:	Patrons	Assignee:	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:
Circulation function:
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.