Tpac - Server error with bad due dates

[Ben Shum]

Evergreen version: ~2.2 beta2

With Tpac, if a circulation has a bad due date entry for a checked out item, it can crash the page and return a 500 internal server error message. The only indication you have is in the apache error log where it tells you that egweb had a problem with the specific date entry.

In our case, we had a circulation with a due date in the database like: '0201-04-26 23:59:59-04:56:02' so presumably a staff member mistakenly hand-keyed a due date as year '201' instead of '2012'.

We should probably make Tpac a little more lenient with possible date exceptions.

[Michael Peters (mrpeters)]

Tested and confirmed in master (2506f44).

[Dan Scott (denials)]

Hmm. The sickness seems to go deep. I added some logging code in OpenILS/WWW/EGCatLoader/Util.pm around line 570:

###
    $ro_object_subs->{parse_datetime} = sub {
        my $date = shift;

        $logger->warn("My date is $date");
        $date = DateTime::Format::ISO8601->new->parse_datetime(cleanse_ISO8601($date));
###

At the added logger line with a due date of "0212-10-08", the output is: "[WARN:26756:Util.pm:186:] My date is 212-10-08T23:59:27-0517". I confirmed that the date in the database itself is '0212-10-08 23:59:59-05:17:32'. So the leading 0's are stripped from the date before the TPAC code even tries to transform it, suggesting that CStoreEditor itself is doing the stripping.

See user/dbs/bad_dates_are_okay in working for a fix for this problem at the TPAC layer. I think this should be applied to master through rel_2_2 as soon as it gets some confirmation that it fixes the reported problem. As it is defensive code, it won't hurt to keep this in place in the future.

We could also consider trying to address this case in cleanse_iso8601() in OpenSRF, although I'm a bit less comfortable munging dates in a more generic situation.

I think we must investigate why CStoreEditor is returning the dates with the leading 0s stripped off and fix that problem as well, as that will also resolve some problems in the staff client (e.g. displaying checked out items for a patron returns an empty string in this situation currently) and probably other functional areas as well.

Once again, that's http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbs/bad_dates_are_okay for the TPAC-level fix.

[Bill Erickson (berick)]

The problem is Cstore, not CSToreEditor. Note the date in the srfsh response:

srfsh# request open-ils.cstore open-ils.cstore.direct.action.circulation.retrieve 1

Received Data: {
  "__c":"circ",
  "__p":[
    null,
    null,
    null,
    4,
    1,
    "f",
    "201-04-26T23:59:57-0456",
   ....

Database shows:

due_date | 0201-04-26 23:59:59-04:56:02

Is this a libdbi thing?

[Mike Rylander (mrylander)]

I offer an alternative that should generate ISO-correct dates from data coming out of the DB:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/strict-iso-dates

From the commit message:

Be more strict about dates we generate

ISO-8601 dates must have a 4-character year component, however strftime does
not left-pad years to 4 characters when the century is one character long even
though the man page for strftime(3) suggests otherwise:

       %F Equivalent to %Y-%m-%d (the ISO 8601 date format). (C99)

This makes stricter ISO-8601 parsers, such as Perl's DateTime module, unhappy.
So, we'll do it ourselves using the glibc extensions available to strftime for
specifying a padding character and desired length.

[Dan Scott (denials)]

Cool. I've added a second commit to my branch that addresses one of the entry points for the bad data; a year such as "0212" gets parsed in the staff client as "212-mm-dd". Some hard-coded logic in check_past() grabs sections of the date from specific offsets, so interprets the year as "212-", and that returns a string object containing "invalid date" from the Date constructor, which then does not trigger the "check_past" logic and does not disable the due date override box.

So... the additional commit checks to see if we have an invalid date in check_past() and throws an execption; check_date() in turn catches the exception and disables the due date override box, and we prevent bad data from getting inserted into the system.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbs/bad_dates_are_okay again (last two commits).

I guess if we wanted to be super defensive we could also add a check constraint on the action.circulation table to prevent due dates prior to 2000 or something (although I know we migrated some open transactions with due dates in the 90's...)

[Lebbeous Fogle-Weekley (lebbeous)]

Thanks Dan and Mike. Your branches both test successfully and play nicely together. I have signed-off and pushed back through rel_2_2.