Bug 17502

Summary: Add type check to output_pref and use exceptions
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, kyle, mtompset, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15822
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 18473    
Attachments: Bug 17502: Resolve internal server error on 0000-00-00
Bug 17502: Resolve internal server error on 0000-00-00 (with Plack)
Bug 17502: [Follow-up] Better use the str parameter
Bug 17502: Add type check to output_pref
Bug 17502: Throw some exceptions in output_pref
[SIGNED-OFF] Bug 17502: Add type check to output_pref
[SIGNED-OFF] Bug 17502: Throw some exceptions in output_pref
Bug 17502: Add type check to output_pref
Bug 17502: Throw some exceptions in output_pref
Bug 17502: Add type check to output_pref
Bug 17502: Throw some exceptions in output_pref
Bug 17502: Add info when throwing the exception

Description Marcel de Rooy 2016-10-26 13:58:52 UTC

    
Comment 1 Marcel de Rooy 2016-10-27 12:31:13 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-10-27 12:42:18 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-10-27 12:45:24 UTC
I am wondering about the severity of this patch.
Somehow we had a number of items with 0000-00-00 in onloan. (Dating from older Koha versions.)
You could argue that this would be exceptional. But still Koha should not crash on such a detail.
Comment 4 Jonathan Druart 2016-11-17 09:15:37 UTC
Did you try the str param of output_pref?
Comment 5 Marcel de Rooy 2016-11-17 09:23:22 UTC
(In reply to Jonathan Druart from comment #4)
> Did you try the str param of output_pref?

Now. Seems to work too. But the changes here add some more tests.
Comment 6 Jonathan Druart 2016-11-17 10:18:43 UTC
We should favour the use of
  output_pref({ str => $date_string });
instead of
  output_pref({ dt => dt_from_string( $date_string ) });

That said I agree that the new check in output_pref makes sense.
Comment 7 Marcel de Rooy 2016-11-17 10:32:42 UTC
(In reply to Jonathan Druart from comment #6)
> We should favour the use of
>   output_pref({ str => $date_string });
> instead of
>   output_pref({ dt => dt_from_string( $date_string ) });
> 
> That said I agree that the new check in output_pref makes sense.

I will add a follow-up
Comment 8 Marcel de Rooy 2016-11-17 13:01:22 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2017-01-03 15:57:02 UTC
Main change has been used in 15822.
So forget the rest.
Comment 10 Tomás Cohen Arazi 2017-01-04 16:00:38 UTC
(In reply to Marcel de Rooy from comment #9)
> Main change has been used in 15822.
> So forget the rest.

What is the status then?
Comment 11 Marcel de Rooy 2017-01-04 16:09:23 UTC
Closed
Comment 12 Mark Tompsett 2017-01-16 18:18:34 UTC
(In reply to Marcel de Rooy from comment #9)
> Main change has been used in 15822.
> So forget the rest.

Well, unless you care about type checking, if you actually right your test, you will still trigger an error, because dt is a string and not a DateTime.
A quick perusal of the code shows lots of dt_from_string calls, and lots of $foo's used. Given the number of $foo's, I suspect they probably are dates, because otherwise we would have hit this problem sooner. So leaving it closed, because type checking the programmer's code should be part of the QA process. :)
Comment 13 Marcel de Rooy 2017-01-17 13:35:00 UTC
(In reply to M. Tompsett from comment #12)
> (In reply to Marcel de Rooy from comment #9)
> > Main change has been used in 15822.
> > So forget the rest.
> 
> Well, unless you care about type checking, if you actually right your test,
> you will still trigger an error, because dt is a string and not a DateTime.
> A quick perusal of the code shows lots of dt_from_string calls, and lots of
> $foo's used. Given the number of $foo's, I suspect they probably are dates,
> because otherwise we would have hit this problem sooner. So leaving it
> closed, because type checking the programmer's code should be part of the QA
> process. :)

Probably you make a point. Coming back later..
Comment 14 Marcel de Rooy 2017-03-22 14:46:52 UTC
Created attachment 61458 [details] [review]
Bug 17502: Add type check to output_pref

This patch makes the following changes:
[1] In Koha/DateUtils.pm, sub output_pref:
    Add a test if $dt is really a DateTime before calling method ymd.
    Preventing a crash like:
    Can't locate object method "ymd" via package "dateonly".
    See also BZ 17502/15822.
[2] Adds a few unit tests in t/DateUtils.t.

Test plan:
[1] Run the adjusted unit test t/DateUtils.t
Comment 15 Marcel de Rooy 2017-03-22 14:48:32 UTC
Well, Mark, go ahead and sign off this one ;)
Comment 16 Jonathan Druart 2017-03-22 15:04:34 UTC
Hum, not sure about that patch.
Of course it is what we should do, but it will hide possible errors in our codebase.
If the app crashes because the call is wrong, at least we know there is something wrong. With this patch we are going to hide the problem.
Comment 17 Marcel de Rooy 2017-03-22 15:56:12 UTC
Throw an exception
Comment 18 Tomás Cohen Arazi 2017-03-22 17:39:05 UTC
(In reply to Marcel de Rooy from comment #17)
> Throw an exception

Yeah.
Comment 19 Marcel de Rooy 2017-03-23 08:40:56 UTC
Created attachment 61510 [details] [review]
Bug 17502: Throw some exceptions in output_pref

This patch adds Koha/Exceptions/DateUtils.pm.
It contains now 4 exceptions for output_pref:
[1] default general exception,
[2] conflicting parameters (dt and str),
[3] invalid date string,
[4] no date time object.

Test plan:
Run the adjusted t/DateUtils.t
Comment 20 Josef Moravec 2017-03-23 09:17:50 UTC
Created attachment 61518 [details] [review]
[SIGNED-OFF] Bug 17502: Add type check to output_pref

This patch makes the following changes:
[1] In Koha/DateUtils.pm, sub output_pref:
    Add a test if $dt is really a DateTime before calling method ymd.
    Preventing a crash like:
    Can't locate object method "ymd" via package "dateonly".
    See also BZ 17502/15822.
[2] Adds a few unit tests in t/DateUtils.t.

Test plan:
[1] Run the adjusted unit test t/DateUtils.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Josef Moravec 2017-03-23 09:17:57 UTC
Created attachment 61519 [details] [review]
[SIGNED-OFF] Bug 17502: Throw some exceptions in output_pref

This patch adds Koha/Exceptions/DateUtils.pm.
It contains now 4 exceptions for output_pref:
[1] default general exception,
[2] conflicting parameters (dt and str),
[3] invalid date string,
[4] no date time object.

Test plan:
Run the adjusted t/DateUtils.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Marcel de Rooy 2017-03-23 09:18:56 UTC
(In reply to Josef Moravec from comment #21)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Thanks Josef
Comment 23 Jonathan Druart 2017-03-23 19:01:19 UTC
Hi Marcel,
The classes of the exceptions should not be named with the module they are raised from.
Also I do not think we need 1 class per exception. If you want to raise an exception because the API has not been used correctly I guess we need (or have already?) a generic exception for it.
Comment 24 Jonathan Druart 2017-03-23 19:07:43 UTC
Moreover I do not know if it is very safe to explode if output_pref is called without a defined variable.
I can easily imagine some places where output_pref($var) is called with $var undefined.
Comment 25 Marcel de Rooy 2017-03-24 07:32:40 UTC
(In reply to Jonathan Druart from comment #23)
> The classes of the exceptions should not be named with the module they are
> raised from.

Hmm. Just following what we already did (and approved).
Like:

package Koha::Patron::Modification;
use Koha::Exceptions::Patron::Modification;
Koha::Exceptions::Patron::Modification::DuplicateVerificationToken->throw
===
package Koha::Exceptions::Patron::Modification;

    'Koha::Exceptions::Patron::Modification::DuplicateVerificationToken' => {
        isa => 'Koha::Exceptions::Patron::Modification',
        description => "The verification token given already exists"
    },
===

Please explain.
Should we formulate a coding guideline here?
Comment 26 Marcel de Rooy 2017-03-24 07:33:21 UTC
(In reply to Jonathan Druart from comment #24)
> Moreover I do not know if it is very safe to explode if output_pref is
> called without a defined variable.
> I can easily imagine some places where output_pref($var) is called with $var
> undefined.

Agreed. Changing status.
Comment 27 Tomás Cohen Arazi 2017-03-24 15:03:30 UTC
(In reply to Marcel de Rooy from comment #25)
> (In reply to Jonathan Druart from comment #23)
> > The classes of the exceptions should not be named with the module they are
> > raised from.
> 
> Hmm. Just following what we already did (and approved).
> Like:
> 
> package Koha::Patron::Modification;
> use Koha::Exceptions::Patron::Modification;
> Koha::Exceptions::Patron::Modification::DuplicateVerificationToken->throw
> ===
> package Koha::Exceptions::Patron::Modification;
> 
>     'Koha::Exceptions::Patron::Modification::DuplicateVerificationToken' => {
>         isa => 'Koha::Exceptions::Patron::Modification',
>         description => "The verification token given already exists"
>     },
> ===
> 
> Please explain.
> Should we formulate a coding guideline here?

We might need to.

I think the thing with the exceptions you add is:
- There's no need to have them tied to output_pref, they could jus tbe top-level for the DateUtils namespace. Remember when you catch them, you have the context in which it happened and can build the needed actions.
- The ones you add should be general exceptions: WrongParameters, MissingParameters, InvalidParamType. Keep in mind that you can use them like this:

  Koha::Exceptions::InvalidParamType->throw('DateTime object expected');
  Koha::Exceptions::WrongParameters->throw('Conflicting parameters dt and str passed');

instead of baking a too specific exception.

Regarding the Patron modification ones, I found that patron modification tokens were too specific, and they deserved its own exception. But it might be subject to future changes if someone raises it.
Comment 28 Marcel de Rooy 2017-04-14 11:52:09 UTC
Created attachment 62168 [details] [review]
Bug 17502: Add type check to output_pref

This patch makes the following changes:
[1] In Koha/DateUtils.pm, sub output_pref:
    Add a test if $dt is really a DateTime before calling method ymd.
    Preventing a crash like:
    Can't locate object method "ymd" via package "dateonly".
    See also BZ 17502/15822.
[2] Adds a few unit tests in t/DateUtils.t.

Test plan:
[1] Run the adjusted unit test t/DateUtils.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Marcel de Rooy 2017-04-14 11:52:14 UTC
Created attachment 62169 [details] [review]
Bug 17502: Throw some exceptions in output_pref

Test plan:
Run the adjusted t/DateUtils.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

NOTE: This patch is amended in QA. The exceptions are moved from a separate
module to the general section of Exceptions.pm.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Jonathan Druart 2017-04-19 18:34:58 UTC
Created attachment 62408 [details] [review]
Bug 17502: Add type check to output_pref

This patch makes the following changes:
[1] In Koha/DateUtils.pm, sub output_pref:
    Add a test if $dt is really a DateTime before calling method ymd.
    Preventing a crash like:
    Can't locate object method "ymd" via package "dateonly".
    See also BZ 17502/15822.
[2] Adds a few unit tests in t/DateUtils.t.

Test plan:
[1] Run the adjusted unit test t/DateUtils.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2017-04-19 18:35:05 UTC
Created attachment 62409 [details] [review]
Bug 17502: Throw some exceptions in output_pref

Test plan:
Run the adjusted t/DateUtils.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

NOTE: This patch is amended in QA. The exceptions are moved from a separate
module to the general section of Exceptions.pm.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2017-04-19 18:35:10 UTC
Created attachment 62410 [details] [review]
Bug 17502: Add info when throwing the exception

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2017-04-19 18:36:38 UTC
I am expecting explosions with these patches but that will help to discover hidden bugs.
Comment 34 Kyle M Hall 2017-04-21 11:36:42 UTC
Pushed to master for 17.05, thanks Marcel, Jonathan!
Comment 35 Katrin Fischer 2017-04-21 22:22:26 UTC
Hm, expecting explosions... should this be in stable?
Comment 36 Tomás Cohen Arazi 2017-04-21 22:41:38 UTC
(In reply to Katrin Fischer from comment #35)
> Hm, expecting explosions... should this be in stable?

I don't think so! Unless it fixes something, but it is too early to say it doesn't break things.
Comment 37 Katrin Fischer 2017-04-22 05:11:26 UTC
Thx Tomas!

This won't get ported back to 16.11.x as it is an enhancement.