Attachment 154920 Details for Bug 27249 – Bug 27249: Prevent infinite loop when searching for an open day

[patch] Bug 27249: Prevent infinite loop when searching for an open day

Bug-27249-Prevent-infinite-loop-when-searching-for.patch (text/plain), 8.50 KB, created by David Nind on 2023-08-29 20:29:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2023-08-29 20:29:05 UTC

Size: 8.50 KB

patch

obsolete

>From db4639e4c073ac104abce390848f92724d92e270 Mon Sep 17 00:00:00 2001
>From: Julian Maurice <julian.maurice@biblibre.com>
>Date: Tue, 18 Jul 2023 10:26:03 +0200
>Subject: [PATCH] Bug 27249: Prevent infinite loop when searching for an open
> day
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>Calendars can be configured in a way that all days are closed.
>The simplest way to do that is to configure a repeatable holiday for
>every day of the week.
>With such calendars, searching for an open day will literally take
>forever.
>
>This patch sets a hard limit on how many iterations are allowed before
>giving up. This limit is set to the arbitrary value of 5000, which
>should be large enough to be able to consider there is no open days if
>we haven't found any with that many iterations, and small enough to
>allow the loop to end quickly
>
>Test plan:
>1. Set system preference 'useDaysMode' to 'Use the calendar to push the
>   due date to the next open day' ('Datedue'). Make sure the existing
>   circulation rules do not conflict with that setting.
>2. Browse to Tools Â» Calendar
>3. Set every day of the week to "Holiday repeated every same day of the
>   week"
>4. Issue an item to a patron
>5. Check the box and select 'Renew selected items'
>6. The renewal should fail pretty quickly
>
>Signed-off-by: Sam Lau <samalau@gmail.com>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> Koha/Calendar.pm                             | 28 ++++++++-
> Koha/Exceptions/Calendar.pm                  | 17 ++++++
> koha-tmpl/intranet-tmpl/prog/js/checkouts.js |  2 +
> svc/renew                                    |  2 +
> t/db_dependent/Holidays.t                    | 61 +++++++++++++++++++-
> 5 files changed, 107 insertions(+), 3 deletions(-)
> create mode 100644 Koha/Exceptions/Calendar.pm
>
>diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm
>index 5ffebb47ab..645b538700 100644
>--- a/Koha/Calendar.pm
>+++ b/Koha/Calendar.pm
>@@ -8,6 +8,15 @@ use DateTime::Duration;
> use C4::Context;
> use Koha::Caches;
> use Koha::Exceptions;
>+use Koha::Exceptions::Calendar;
>+
>+# This limit avoids an infinite loop when searching for an open day in an
>+# always closed library
>+# The value is arbitrary, but it should be large enough to be able to
>+# consider there is no open days if we haven't found any with that many
>+# iterations, and small enough to allow the loop to end quickly
>+# See next_open_days and prev_open_days
>+use constant OPEN_DAYS_SEARCH_MAX_ITERATIONS => 5000;
> 
> sub new {
>     my ( $classname, %options ) = @_;
>@@ -261,10 +270,18 @@ sub next_open_days {
>     my $base_date = $dt->clone();
> 
>     $base_date->add(days => $to_add);
>-    while ($self->is_holiday($base_date)) {
>+    my $i = 0;
>+    while ( $self->is_holiday($base_date) && $i < OPEN_DAYS_SEARCH_MAX_ITERATIONS ) {
>         my $add_next = $self->get_push_amt($base_date);
>         $base_date->add(days => $add_next);
>+        ++$i;
>+    }
>+
>+    if ( $self->is_holiday($base_date) ) {
>+        Koha::Exceptions::Calendar::NoOpenDays->throw(
>+            sprintf( 'Unable to find an open day for library %s', $self->{branchcode} ) );
>     }
>+
>     return $base_date;
> }
> 
>@@ -282,11 +299,18 @@ sub prev_open_days {
> 
>     $base_date->add(days => $to_sub);
> 
>-    while ($self->is_holiday($base_date)) {
>+    my $i = 0;
>+    while ( $self->is_holiday($base_date) && $i < OPEN_DAYS_SEARCH_MAX_ITERATIONS ) {
>         my $sub_next = $self->get_push_amt($base_date);
>         # Ensure we're subtracting when we need to be
>         $sub_next = $sub_next > 0 ? 0 - $sub_next : $sub_next;
>         $base_date->add(days => $sub_next);
>+        ++$i;
>+    }
>+
>+    if ( $self->is_holiday($base_date) ) {
>+        Koha::Exceptions::Calendar::NoOpenDays->throw(
>+            sprintf( 'Unable to find an open day for library %s', $self->{branchcode} ) );
>     }
> 
>     return $base_date;
>diff --git a/Koha/Exceptions/Calendar.pm b/Koha/Exceptions/Calendar.pm
>new file mode 100644
>index 0000000000..3891c3bced
>--- /dev/null
>+++ b/Koha/Exceptions/Calendar.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Calendar;
>+
>+use Modern::Perl;
>+
>+use Koha::Exception;
>+
>+use Exception::Class (
>+    'Koha::Exceptions::Calendar' => {
>+        isa => 'Koha::Exception',
>+    },
>+    'Koha::Exceptions::Calendar::NoOpenDays' => {
>+        isa => 'Koha::Exceptions::Calendar',
>+        description => 'Library has no open days',
>+    },
>+);
>+
>+1;
>diff --git a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js
>index 3ed706fa7e..1b174ab3a5 100644
>--- a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js
>+++ b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js
>@@ -200,6 +200,8 @@ $(document).ready(function() {
>                             content += __("Not allowed: patron restricted");
>                         } else if ( data.error == "overdue" ) {
>                             content += __("Not allowed: overdue");
>+                        } else if ( data.error == 'no_open_days' ) {
>+                            content += __('Unable to find an open day');
>                         } else if ( data.error ) {
>                             content += data.error;
>                         } else {
>diff --git a/svc/renew b/svc/renew
>index ce026cfc46..eca18e647f 100755
>--- a/svc/renew
>+++ b/svc/renew
>@@ -87,6 +87,8 @@ if ( $data->{renew_okay} || ( $seen && $data->{error} eq 'too_unseen') ) {
>     } catch {
>         if ( ref($_) eq 'Koha::Exceptions::Checkout::FailedRenewal' ) {
>             $data->{error} = 'renewal_failed';
>+        } elsif ( ref($_) eq 'Koha::Exceptions::Calendar::NoOpenDays' ) {
>+            $data->{error} = 'no_open_days';
>         } else {
>             $_->rethrow;
>         }
>diff --git a/t/db_dependent/Holidays.t b/t/db_dependent/Holidays.t
>index 607906a169..42c2ce604e 100755
>--- a/t/db_dependent/Holidays.t
>+++ b/t/db_dependent/Holidays.t
>@@ -17,7 +17,8 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 12;
>+use Test::More tests => 14;
>+use Test::Exception;
> 
> use DateTime;
> use DateTime::TimeZone;
>@@ -277,5 +278,63 @@ subtest 'copy_to_branch' => sub {
> 
> };
> 
>+subtest 'with a library that is never open' => sub {
>+    plan tests => 2;
>+
>+    $schema->storage->txn_begin;
>+
>+    my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode};
>+    my $calendar   = C4::Calendar->new( branchcode => $branchcode );
>+    foreach my $weekday ( 0 .. 6 ) {
>+        $calendar->insert_week_day_holiday( weekday => $weekday, title => '', description => '' );
>+    }
>+
>+    my $now = DateTime->now;
>+
>+    subtest 'next_open_days should throw an exception' => sub {
>+        my $kcalendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' );
>+        throws_ok { $kcalendar->next_open_days( $now, 1 ) } 'Koha::Exceptions::Calendar::NoOpenDays';
>+    };
>+
>+    subtest 'prev_open_days should throw an exception' => sub {
>+        my $kcalendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' );
>+        throws_ok { $kcalendar->prev_open_days( $now, 1 ) } 'Koha::Exceptions::Calendar::NoOpenDays';
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
>+subtest 'with a library that is *almost* never open' => sub {
>+    plan tests => 2;
>+
>+    $schema->storage->txn_begin;
>+
>+    my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode};
>+    my $calendar   = C4::Calendar->new( branchcode => $branchcode );
>+    foreach my $weekday ( 0 .. 6 ) {
>+        $calendar->insert_week_day_holiday( weekday => $weekday, title => '', description => '' );
>+    }
>+
>+    my $now                    = DateTime->now;
>+    my $open_day_in_the_future = $now->clone()->add( years => 1 );
>+    my $open_day_in_the_past   = $now->clone()->subtract( years => 1 );
>+    $calendar->insert_exception_holiday( date => $open_day_in_the_future->ymd, title => '', description => '' );
>+    $calendar->insert_exception_holiday( date => $open_day_in_the_past->ymd,   title => '', description => '' );
>+
>+    subtest 'next_open_days should find the open day' => sub {
>+        my $kcalendar     = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' );
>+        my $next_open_day = $kcalendar->next_open_days( $now, 1 );
>+        is( $next_open_day->ymd, $open_day_in_the_future->ymd );
>+    };
>+
>+    subtest 'prev_open_days should find the open day' => sub {
>+        my $kcalendar     = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' );
>+        my $prev_open_day = $kcalendar->prev_open_days( $now, 1 );
>+        is( $prev_open_day->ymd, $open_day_in_the_past->ymd );
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
> # Clear cache
> Koha::Caches->get_instance->flush_all;
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 27249: 153585 | 153682 | 154728 | 154920 | 154921 | 156786 | 156787 | 156788 | 157163 | 157164 | 157165 | 157166 | 157167