Bug 7806

Summary: Don't use 0000-00-00 to signal a non-existing date
Product: Koha Reporter: Marc Balmer <marc>
Component: DatabaseAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, martin.renvoize, oleonard, phil, tmisilo, veron, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17320
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10
Bug Depends on: 26997, 27276, 27821    
Bug Blocks: 7365    
Attachments: Bug 7806: Fix remaining occurrences of 0000-00-00
Bug 7806: Fix remaining occurrences of 0000-00-00
Bug 7806: (follow-up) Update to use sanitize_zero_date
Bug 7806: Fix remaining occurrences of 0000-00-00
Bug 7806: (follow-up) Update to use sanitize_zero_date
Bug 7806: [20.05] Fix remaining occurrences of 0000-00-00
Bug 7806: [20.05] (follow-up) Update to use sanitize_zero_date

Description Marc Balmer 2012-03-22 15:57:21 UTC
The illegal date '0000-00-00' is being used in Koha to signal a non-existing date.  MySQL allows for such illegal dates.  A database like PostgreSQL, which is much more strict when it comes to correct data, does not allow illegal values as date values.

If a date is inexistant/invalid etc. an SQL NULL must be used instead.
Comment 1 Owen Leonard 2014-07-10 20:03:44 UTC
My search of Koha code returns only instances where checks are being done for a '0000-00-00' value but no instances where a '0000-00-00' value is being set, with one exception: sample_quotes.sql
Comment 2 Jonathan Druart 2015-03-31 08:57:49 UTC
How could we remove all the checks of 0000-00-00 in the code?
I personally have no good idea.

We could remove the not null constraint and update all date fields with NULL.
Or keep the constraint and update with 1970-01-01.
Comment 3 Katrin Fischer 2015-05-15 21:41:24 UTC
I like NULL better and it looks less dangerous and cleaner than 1970-01-01
Comment 4 Marc Balmer 2015-05-16 22:17:43 UTC
Using a valid date like 1.1.1970 to denote an invalid date is so obviously a bad idea.  I suggest to go the NULL route.
Comment 5 Jonathan Druart 2015-05-18 09:25:25 UTC
I really don't know where I have found this stupid idea :)
Comment 6 Katrin Fischer 2019-04-28 18:02:50 UTC
Actually, I find tons of 0000-00-00 now in the codebase...
Comment 7 Jonathan Druart 2019-04-28 23:34:52 UTC
It could be useful to know if production DBs still contain such values (?)
Comment 8 Martin Renvoize 2020-03-24 19:22:20 UTC
I've recently spotted a fair few errors during upgrades where we have these bad dates.. I'll try to draw up a list of where I've seen it.. versions being upgraded and tables affected etc.
Comment 9 Jonathan Druart 2020-12-18 14:56:30 UTC
Created attachment 114528 [details] [review]
Bug 7806: Fix remaining occurrences of 0000-00-00

We should remove all SQL queries that contain 0000-00-00 and finally
assume we do not longer have such value in our DB (for date type)

We already dealt with such values in previous update DB entries.
The 2 added by this one haven't been replaced already.

The code will now assume that either a valid date exist, or NULL/undef.

Test plan:
QA review is needed and test of the different places where code is
modified.

Not sure about the change from reports/issues_avg_stats.pl
Comment 10 Phil Ringnalda 2021-02-24 03:39:50 UTC
(In reply to Jonathan Druart from comment #7)
> It could be useful to know if production DBs still contain such values (?)

I have 175 in items.replacementpricedate, looks like a chunk from the original import to this system, another chunk from importing 7 more libraries, then a chunk starting a couple weeks ago when we updated to 20.05, which apparently inserts 0000-00-00 when you mispaste your replacementprice into replacementpricedate's input. Not sure where else to look for 0000-00-00, though.
Comment 11 Martin Renvoize 2021-02-24 15:11:24 UTC
Created attachment 117266 [details] [review]
Bug 7806: Fix remaining occurrences of 0000-00-00

We should remove all SQL queries that contain 0000-00-00 and finally
assume we do not longer have such value in our DB (for date type)

We already dealt with such values in previous update DB entries.
The 2 added by this one haven't been replaced already.

The code will now assume that either a valid date exist, or NULL/undef.

Test plan:
QA review is needed and test of the different places where code is
modified.

Not sure about the change from reports/issues_avg_stats.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-02-24 15:11:27 UTC
Created attachment 117267 [details] [review]
Bug 7806: (follow-up) Update to use sanitize_zero_date

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-02-24 15:12:54 UTC
These patches appear to work as expected for me so signing off.

However, I do wonder about the tooltips left behind in a number of templates..

<span title="0000-00-00">

Those could be misleading but I'm not entirely sure how they're intended to work..
Comment 14 Owen Leonard 2021-02-24 16:09:00 UTC
(In reply to Martin Renvoize from comment #13)
> <span title="0000-00-00">

In many places DataTable columns are sorted not by the value displayed but the value in the title attribute as seen above. Instances where "0000-00-00" is hard-coded into the template indicate a place where an empty date might be misinterpreted by the sorting algorithm.
Comment 15 Martin Renvoize 2021-02-25 09:15:51 UTC
(In reply to Owen Leonard from comment #14)
> (In reply to Martin Renvoize from comment #13)
> > <span title="0000-00-00">
> 
> In many places DataTable columns are sorted not by the value displayed but
> the value in the title attribute as seen above. Instances where "0000-00-00"
> is hard-coded into the template indicate a place where an empty date might
> be misinterpreted by the sorting algorithm.

Thanks Owen, that's really helpful to know :)
Comment 16 Marcel de Rooy 2021-02-26 07:16:27 UTC
(In reply to Jonathan Druart from comment #9)
> Test plan:
> QA review is needed and test of the different places where code is
> modified.

Hahaha, good plan ;)
Comment 17 Marcel de Rooy 2021-02-26 07:17:26 UTC
(In reply to Martin Renvoize from comment #13)
> These patches appear to work as expected for me so signing off.

Since there is no test plan, it would be beneficial for us to know what you exactly tested and why you conclude that they appear to work?
Comment 18 Marcel de Rooy 2021-02-26 07:29:02 UTC
(In reply to Jonathan Druart from comment #9)
> Not sure about the change from reports/issues_avg_stats.pl

Since returndate comes from oldissues and you sanitize the field in the atomic update, we should be fine imo.
Comment 19 Marcel de Rooy 2021-02-26 08:13:21 UTC
Although this looks quite good to me, it is really hard to be convinced that all appearances of 0000-00-00 in the data now are caught correctly by the codebase.

I found at least one issue (no blocker):
select count(*) from items where dateaccessioned= '0000-00-00';
+----------+
| count(*) |
+----------+
|      335 |
+----------+
1 row in set (0.12 sec)
This comes from a production database.
So I tested with an item. Set dateaccessioned to 0*. Open item editor. You see the 0* on the form. Tab thru the field so that the date gets FOCUS. This will update the 0* in the old situation, but not in the new one (obviously).
Note that the CLICK event uses the force parameter and will set the date.

Could you add this one to the atomicupdate too?
And please provide me some evidence that we found the last case ;) For instance by checking date columns in a larger production database being used for a longer time..

PQA for now.
Comment 20 Marcel de Rooy 2021-02-26 08:14:01 UTC
Created attachment 117361 [details] [review]
Bug 7806: Fix remaining occurrences of 0000-00-00

We should remove all SQL queries that contain 0000-00-00 and finally
assume we do not longer have such value in our DB (for date type)

We already dealt with such values in previous update DB entries.
The 2 added by this one haven't been replaced already.

The code will now assume that either a valid date exist, or NULL/undef.

Test plan:
QA review is needed and test of the different places where code is
modified.

Not sure about the change from reports/issues_avg_stats.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2021-02-26 08:14:05 UTC
Created attachment 117362 [details] [review]
Bug 7806: (follow-up) Update to use sanitize_zero_date

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Martin Renvoize 2021-02-26 10:28:09 UTC
(In reply to Marcel de Rooy from comment #19)
> Although this looks quite good to me, it is really hard to be convinced that
> all appearances of 0000-00-00 in the data now are caught correctly by the
> codebase.
> 
> I found at least one issue (no blocker):
> select count(*) from items where dateaccessioned= '0000-00-00';
> +----------+
> | count(*) |
> +----------+
> |      335 |
> +----------+
> 1 row in set (0.12 sec)
> This comes from a production database.
> So I tested with an item. Set dateaccessioned to 0*. Open item editor. You
> see the 0* on the form. Tab thru the field so that the date gets FOCUS. This
> will update the 0* in the old situation, but not in the new one (obviously).
> Note that the CLICK event uses the force parameter and will set the date.
> 
> Could you add this one to the atomicupdate too?
> And please provide me some evidence that we found the last case ;) For
> instance by checking date columns in a larger production database being used
> for a longer time..
> 
> PQA for now.

Thanks for QAing this Marcel, your right, I should have detailed my testing :(.. I basically looked at where the code changed and tried to trigger those actions via the UI to check things weren't broken.

As for this comment, I'm pretty sure we'll have left behind some bad data, but it should get picked up by upgrades now that we throw errors in the updatedatabase script.

We could I suppose write a one off atomicupdate to check and fix all possible datefields at this point in time..

I used the following to get a list of such fields.... we have 210!

'''
SELECT col.table_schema     AS database_name,
       col.table_name,
       col.ordinal_position AS column_id,
       col.column_name,
       col.data_type,
       col.datetime_precision
FROM   information_schema.columns col
       JOIN information_schema.tables tab
         ON tab.table_schema = col.table_schema
            AND tab.table_name = col.table_name
            AND tab.table_type = 'BASE TABLE'
WHERE  col.data_type IN ( 'date', 'time', 'datetime', 'year', 'timestamp' )
       AND col.table_schema NOT IN ( 'information_schema', 'sys',
                                     'performance_schema',
                                     'mysql' )
       AND col.table_schema = 'database_name'
ORDER  BY col.table_schema,
          col.table_name,
          col.ordinal_position;
'''
Comment 23 Martin Renvoize 2021-02-26 10:37:56 UTC
142 when I reduce the query to only those fields that are nullable:

SELECT Concat(col.table_name, '.', col.column_name),
       col.data_type
FROM   information_schema.columns col
       JOIN information_schema.tables tab
         ON tab.table_schema = col.table_schema
            AND tab.table_name = col.table_name
            AND tab.table_type = 'BASE TABLE'
WHERE  col.data_type IN ( 'date', 'time', 'datetime', 'year', 'timestamp' )
       AND col.is_nullable = 'YES'
       AND col.table_schema NOT IN ( 'information_schema', 'sys',
                                     'performance_schema',
                                     'mysql' )
       AND col.table_schema = 'koha_demo1'
ORDER  BY col.table_schema,
          col.table_name,
          col.ordinal_position;
Comment 24 Marcel de Rooy 2021-02-26 10:45:18 UTC
Can't call method "subtract" on an undefined value at /usr/share/koha/Koha/Patron.pm line 770
When dateexpiry == 0000-00-00
But I did put it in explicitly ;)
Comment 25 Marcel de Rooy 2021-02-26 10:58:00 UTC
(In reply to Martin Renvoize from comment #22)
> Thanks for QAing this Marcel, your right, I should have detailed my testing
> :(.. I basically looked at where the code changed and tried to trigger those
> actions via the UI to check things weren't broken.

Great. This will build up more confidence for pushing it.
Comment 26 Jonathan Druart 2021-03-01 10:15:58 UTC
I wrote bug 27821, otherwise the cast on old_issues.returndate didn't work.
Comment 27 Jonathan Druart 2021-03-01 10:28:42 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 28 Fridolin Somers 2021-03-04 09:28:28 UTC
Pushed to 20.11.x for 20.11.04
Comment 29 Andrew Fuerste-Henry 2021-03-05 20:35:57 UTC
This doesn't apply cleanly to 20.05, please rebase. I got the following test failure after clearing merge errors myself:

root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/Circulation/dateexpiry.t
t/db_dependent/Circulation/dateexpiry.t .. 1/2     # No tests run!
t/db_dependent/Circulation/dateexpiry.t .. 2/2 
#   Failed test 'No tests run for subtest "Tests for CalcDateDue related to dateexpiry"'
#   at t/db_dependent/Circulation/dateexpiry.t line 46.
Can't call method "effective_itemtype" on unblessed reference at t/db_dependent/Circulation/dateexpiry.t line 109.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Circulation/dateexpiry.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/dateexpiry.t (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
Files=1, Tests=2,  4 wallclock secs ( 0.01 usr  0.01 sys +  2.24 cusr  0.42 csys =  2.68 CPU)
Result: FAIL
Comment 30 Jonathan Druart 2021-03-08 11:06:44 UTC
Andrew, resolve the conflict like that:

108     $patron->{dateexpiry} = undef;
109     my $d = C4::Circulation::CalcDateDue( $today, $item->{itype}, $branch->{branchcode}, $patron );
110     is( ref $d eq "DateTime" && $d->mdy() =~ /^\d+/, 1, "CalcDateDue with expiry undef" );
Comment 31 Jonathan Druart 2021-03-08 11:07:24 UTC
Created attachment 117915 [details] [review]
Bug 7806: [20.05] Fix remaining occurrences of 0000-00-00

We should remove all SQL queries that contain 0000-00-00 and finally
assume we do not longer have such value in our DB (for date type)

We already dealt with such values in previous update DB entries.
The 2 added by this one haven't been replaced already.

The code will now assume that either a valid date exist, or NULL/undef.

Test plan:
QA review is needed and test of the different places where code is
modified.

Not sure about the change from reports/issues_avg_stats.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Jonathan Druart 2021-03-08 11:07:28 UTC
Created attachment 117916 [details] [review]
Bug 7806: [20.05] (follow-up) Update to use sanitize_zero_date

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Andrew Fuerste-Henry 2021-03-08 14:59:57 UTC
Thanks, Jonathan!
Pushed to 20.05.x for 20.05.10
Comment 34 Victor Grousset/tuxayo 2021-03-22 16:11:16 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.