Bug 7802 - A MySQLism is used to get the current date.
Summary: A MySQLism is used to get the current date.
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Galen Charlton
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 7365
  Show dependency treegraph
 
Reported: 2012-03-22 14:18 UTC by Marc Balmer
Modified: 2019-04-28 18:04 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Remove CUR_DATE() and CURRENT_DATE() MySQLism (12.69 KB, patch)
2012-03-22 15:18 UTC, Marc Balmer
Details | Diff | Splinter Review
Remove date MySQLism (12.67 KB, patch)
2012-03-24 08:04 UTC, Marc Balmer
Details | Diff | Splinter Review
Update patch tto remove a MySQLism (11.32 KB, patch)
2012-04-05 06:46 UTC, Marc Balmer
Details | Diff | Splinter Review
Bug 7802: [SIGNED-OFF] Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date) (11.38 KB, patch)
2012-04-06 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date) (889 bytes, patch)
2012-04-06 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (9.88 KB, patch)
2013-07-15 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (4.96 KB, patch)
2013-07-15 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (9.86 KB, patch)
2013-08-02 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (4.96 KB, patch)
2013-08-02 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (5.01 KB, patch)
2013-08-30 12:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (4.68 KB, patch)
2013-11-06 15:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (5.02 KB, patch)
2013-11-06 15:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (4.25 KB, patch)
2013-11-07 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (5.02 KB, patch)
2013-11-07 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (4.30 KB, patch)
2013-11-08 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). (5.01 KB, patch)
2013-11-08 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Balmer 2012-03-22 14:18:01 UTC
To get todays date, the MySQL only function CURDATE() or CURRENT_DATE() is being used in many places.

Neither CURDATE() nor CURRENT_DATE() must be used, but instead the standard form CAST(now() AS date).
Comment 1 Marc Balmer 2012-03-22 15:18:40 UTC Comment hidden (obsolete)
Comment 2 Marc Balmer 2012-03-24 08:04:08 UTC Comment hidden (obsolete)
Comment 3 Marc Balmer 2012-04-05 06:46:06 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-04-06 13:01:35 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-04-06 13:02:41 UTC Comment hidden (obsolete)
Comment 6 Ian Walls 2012-04-16 18:48:26 UTC
In the first patch, changes in C4/Overdues.pm from NOW() to CAST(now() as DATE) are no longer equivalent since the introduction of hourly loans.  date_due is now a datetime, so the check on NOW() should not be cast.

Marking first patch Failed QA.  Will address second patch after this has been fixed.
Comment 7 Jonathan Druart 2013-07-15 10:34:55 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-07-15 10:34:59 UTC Comment hidden (obsolete)
Comment 9 David Cook 2013-07-18 07:19:09 UTC
(In reply to Jonathan Druart from comment #7)
> Created attachment 19643 [details] [review] [review]
> Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by
> CAST(now() AS date).
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Jonathan: Is this still waiting for Sign Off?
Comment 10 Jonathan Druart 2013-07-18 07:44:17 UTC
(In reply to David Cook from comment #9)
> Jonathan: Is this still waiting for Sign Off?

David,
Yes, the followup needs a signoff.
Comment 11 Kyle M Hall 2013-08-02 15:02:24 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-08-02 15:02:56 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2013-08-26 14:29:46 UTC
(In reply to Kyle M Hall from comment #12)
> Created attachment 20058 [details] [review] [review]
> Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace
> by CAST(now() AS date).

Kyle, you forgot to add the Signed-off-by line to the second patch.
Comment 14 Kyle M Hall 2013-08-30 12:21:36 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2013-09-07 20:34:06 UTC
Do we have a reference about CURRENT_DATE being a mysqlism?
I found it here: http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt

Also bit worried about changing all those statements without tests.
Comment 16 Jonathan Druart 2013-09-09 08:35:29 UTC
(In reply to Katrin Fischer from comment #15)
> Do we have a reference about CURRENT_DATE being a mysqlism?
> I found it here: http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt

At least, it does not work with PostgreSQL:

postgres=# select CURRENT_DATE();
ERREUR:  erreur de syntaxe sur ou près de « ( »
LIGNE 1 : select CURRENT_DATE();
                             ^
postgres=# select current_date;
    date    
------------
 2013-09-09


> Also bit worried about changing all those statements without tests.

I don't have the time to provide unit tests for all modified routines.

But

MariaDB [koha_devstetienne]> select CURRENT_DATE();
+----------------+
| CURRENT_DATE() |
+----------------+
| 2013-09-09     |
+----------------+
1 row in set (0.00 sec)

MariaDB [koha_devstetienne]> SELECT CAST(now() AS date);
+---------------------+
| CAST(now() AS date) |
+---------------------+
| 2013-09-09          |
+---------------------+
1 row in set (0.00 sec)
Comment 17 Katrin Fischer 2013-09-09 08:45:21 UTC
Weird, it's in the Postgres documentation too?
http://www.postgresql.org/docs/8.1/static/functions-datetime.html
Comment 18 Jonathan Druart 2013-09-09 08:51:51 UTC
(In reply to Katrin Fischer from comment #17)
> Weird, it's in the Postgres documentation too?
> http://www.postgresql.org/docs/8.1/static/functions-datetime.html

Yes
select current_date;   -- works
select current_date(); -- does not work
Comment 19 Jonathan Druart 2013-11-04 10:16:40 UTC
back to need QA.
Comment 20 Jonathan Druart 2013-11-06 15:51:45 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2013-11-06 15:51:53 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2013-11-07 08:23:26 UTC
I see the following change:

-          AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY)
+          AND reserves.reservedate <= CAST(now() AS date)

Fixing a so detested mysqlism should at least preserve functionality. These changes seem to be made in a hurry, since we are changing functionality here.
Here you change a date_add with just a now. The parameter for adding a number of days is just ignored. This change was a red traffic light for me :) I did not look any further..

Please correct and add a (more detailed) test plan for this patch.
Comment 23 Jonathan Druart 2013-11-07 09:08:14 UTC
(In reply to M. de Rooy from comment #22)
> I see the following change:
> 
> -          AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY)
> +          AND reserves.reservedate <= CAST(now() AS date)
> 
> Fixing a so detested mysqlism should at least preserve functionality. These
> changes seem to be made in a hurry, since we are changing functionality here.

No, not the changes, but the rebase yes. Only the last patch introduces this bad conflict merge. I consider after 3 rebase and 1 year, the rebase is not safe at all, there is here a good example...

> Here you change a date_add with just a now. The parameter for adding a
> number of days is just ignored. This change was a red traffic light for me
> :) I did not look any further..
> 
> Please correct and add a (more detailed) test plan for this patch.

There is no valid test plan. Check that the CUR_DATE(), CURRENT_DATE() mysql functions return the same as CAST(NOW() AS DATE) in your mysql cli.
Your can verify, looking at the diff code, the patch is correct. If you want to test, you can play with reserves and holds queue.
Comment 24 Jonathan Druart 2013-11-07 09:09:37 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2013-11-07 09:09:43 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-11-08 13:14:04 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2013-11-08 13:14:17 UTC Comment hidden (obsolete)
Comment 28 Galen Charlton 2014-03-10 19:43:17 UTC
Comment on attachment 22817 [details] [review]
Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date).

Review of attachment 22817 [details] [review]:
-----------------------------------------------------------------

::: C4/Koha.pm
@@ +1431,4 @@
>          # Fall through... we also return a random quote as a catch-all if all else fails
>      }
>      else {
> +        $query = 'SELECT * FROM quotes WHERE timestamp LIKE CONCAT(CAST(now() AS date),\'%\') ORDER BY timestamp DESC LIMIT 0,1';

Ironically, PostgreSQL will happily accept CURRENT_DATE (without the parentheses).  It won't accept "LIMIT 0,1".  It also won't like trying to compare a timestamp to a string datatype.

Closer to the mark (as in, I've actually tested this on both MariaDB and Pg is "select * from quotes where cast(now() as date) = cast(timestamp as date) limit 1 offset 0;"

A broader point: I don't think that the point should be to search-and-replace fragments of non-portable SQL.  Actually ensuring that the output is portable would be more on target.
Comment 29 Galen Charlton 2014-03-10 19:45:10 UTC
Comment on attachment 22816 [details] [review]
Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date).

Review of attachment 22816 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/longoverdue.pl
@@ +130,5 @@
>      SELECT items.itemnumber, borrowernumber, date_due
>        FROM issues, items
>       WHERE items.itemnumber = issues.itemnumber
> +      AND  DATE_SUB(CAST(now() AS date), INTERVAL ? DAY)  > date_due
> +      AND  DATE_SUB(CAST(now() AS date), INTERVAL ? DAY) <= date_due

DATE_SUB() is not portable to PostgreSQL.  As implied by my previous comment, I'm much more interested in seeing patches that provably improve portability of the SQL, not ones that just search-and-replace.
Comment 30 Galen Charlton 2014-03-10 19:48:33 UTC
(In reply to Jonathan Druart from comment #16)
> I don't have the time to provide unit tests for all modified routines.

And I don't have the inclination to push a patch series that touches a bunch of routines without adequate test coverage, particularly (as my two immediately previous comments indicate) as it includes at least one case where it has the effective of making it harder to tell that a particular SQL statement is not portable.

Setting to failed QA.  Feel free to resubmit a partial patch that covers routines that have test cases, and feel free to write UTs.
Comment 31 Jonathan Druart 2014-03-13 13:51:47 UTC
I don't plan to provide a better way to fix this. I reset the assignee to default.
Comment 32 Manos PETRIDIS 2015-10-16 12:18:22 UTC
Wouldn't a user_defined_function be better suited to the task? One could create a koha_getdate() or even a koha_getdatetime() and they would only need be be changed once, depending on RDBMS used, until koha starts using a db abstraction layer that is.
Comment 33 Katrin Fischer 2019-04-28 18:04:46 UTC
Still an issue:

C4/Acquisition.pm:                DATEDIFF(CURDATE( ),closedate) AS latesince
C4/Acquisition.pm:            claimed_date = CURDATE()
C4/NewsChannels.pm:     AND   DATE(timestamp) < DATE_ADD(CURDATE(), INTERVAL 1 DAY)
circ/overdue.pl:        $strsth .= " AND borrowers.debarred >=  CURDATE()" ;
misc/cronjobs/longoverdue.pl:our $bounds_sth = C4::Context->dbh->prepare("SELECT DATE_SUB(CURDATE(), INTERVAL ? DAY)");
misc/cronjobs/longoverdue.pl:      AND  DATE_SUB(CURDATE(), INTERVAL ? DAY)  > date_due
misc/cronjobs/longoverdue.pl:      AND  DATE_SUB(CURDATE(), INTERVAL ? DAY) <= date_due
misc/cronjobs/remove_temporary_edifiles.pl:and datediff( CURDATE(), transfer_date ) > 5