Bug 24229

Still, but without any info:
koha_1       | [17:33:12] t/db_dependent/api/v1/items.t ........................................... 
koha_1       | Failed 2/2 subtests

 75     $t->get_ok( "//$userid:$password@/api/v1/items" )
 76       ->status_is( 200, 'SWAGGER3.2.2' );

This test is the slower when I run it locally, and You got "Inactivity timeout" when it failed.

It there any kinds of timeout value we could extend?

That would explain why it fails randomly.

I do recreate the issue with running several time the tests:

for i in {1..6}; do timeout 60 prove -v t/db_dependent/api/v1/items.t& done

Make sure you have the timeout, or the whole thing will freeze!

I have tried to extend Mojo timeout
https://mojolicious.org/perldoc/Mojolicious/Guides/FAQ#What-does-Inactivity-timeout-mean

export MOJO_INACTIVITY_TIMEOUT=120
for i in {1..6}; do timeout 120 prove -v t/db_dependent/api/v1/items.t& done

I do no longer get the tests failing, but they all get stuck at this test.

Not directly related, but I noticed that we return ALL the items from the DB, and it is what the tests expect:

 75     $t->get_ok( "//$userid:$password@/api/v1/items" )
 76       ->status_is( 200, 'SWAGGER3.2.2' );
 77 
 78     my $items_count = Koha::Items->search->count;
 79     my $response_count = scalar @{ $t->tx->res->json };
 80 
 81     is( $items_count, $response_count, 'The API returns all the items' );

Wondering if this is correct. If not please open a separate bug report.

Tomas, I tried to remove the changes made by bug 24700 but it did not work.

However, this worked (it is not a fix, but I wanted to confirm it came from there):

diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm
index 81cc97633c..430518251c 100644
--- a/Koha/REST/Plugin/Objects.pm
+++ b/Koha/REST/Plugin/Objects.pm
@@ -116,6 +116,7 @@ sub register {
                 $filtered_params = $c->merge_q_params( $filtered_params, $query_params, $result_set );
             }
             # Perform search
+            $attributes->{page} = 1;
             my $objects = $result_set->search( $filtered_params, $attributes );
 
             if ($objects->is_paged) {
diff --git a/t/db_dependent/api/v1/items.t b/t/db_dependent/api/v1/items.t
index 9eb623b96c..8b141ed781 100644
--- a/t/db_dependent/api/v1/items.t
+++ b/t/db_dependent/api/v1/items.t
@@ -78,7 +78,7 @@ subtest 'list() tests' => sub {
     my $items_count = Koha::Items->search->count;
     my $response_count = scalar @{ $t->tx->res->json };
 
-    is( $items_count, $response_count, 'The API returns all the items' );
+    is( 10, $response_count, 'The API returns all the items' );
 
     $t->get_ok( "//$userid:$password@/api/v1/items?external_id=" . $item->barcode )
       ->status_is(200)

Created attachment 105163 [details] [review]
Bug 24229: Avoid fetching ALL items in test

I'm not sure what we need to test here, besides we get results. I'm
pretty sure we shouldn't allow a plain GET return all objects because it
could cause a DOS very easily. But it certainly belongs to a separate
bug/discussion.

In this case, I put a constraint on the items domain (by using
_per_page=10 in the query params) so it won't happen that under certain
conditions the user agent used by Test::Mojo time outs.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

(In reply to Jonathan Druart from comment #4)
> I have tried to extend Mojo timeout
> https://mojolicious.org/perldoc/Mojolicious/Guides/FAQ#What-does-Inactivity-
> timeout-mean
> 
> export MOJO_INACTIVITY_TIMEOUT=120
> for i in {1..6}; do timeout 120 prove -v t/db_dependent/api/v1/items.t& done
> 
> I do no longer get the tests failing, but they all get stuck at this test.

I reproduced the problem easily with your loop. My patch fixes that.

(In reply to Jonathan Druart from comment #5)
> Not directly related, but I noticed that we return ALL the items from the
> DB, and it is what the tests expect:
> 
>  75     $t->get_ok( "//$userid:$password@/api/v1/items" )
>  76       ->status_is( 200, 'SWAGGER3.2.2' );
>  77 
>  78     my $items_count = Koha::Items->search->count;
>  79     my $response_count = scalar @{ $t->tx->res->json };
>  80 
>  81     is( $items_count, $response_count, 'The API returns all the items' );
> 
> Wondering if this is correct. If not please open a separate bug report.

Tomas, can you answer this question please?

As I said yesterday during the dev meeting, it would mean that one could call 6 times the GET /items route to kick the server down?

(In reply to Jonathan Druart from comment #9)
> (In reply to Jonathan Druart from comment #5)
> > Not directly related, but I noticed that we return ALL the items from the
> > DB, and it is what the tests expect:
> > 
> >  75     $t->get_ok( "//$userid:$password@/api/v1/items" )
> >  76       ->status_is( 200, 'SWAGGER3.2.2' );
> >  77 
> >  78     my $items_count = Koha::Items->search->count;
> >  79     my $response_count = scalar @{ $t->tx->res->json };
> >  80 
> >  81     is( $items_count, $response_count, 'The API returns all the items' );
> > 
> > Wondering if this is correct. If not please open a separate bug report.
> 
> Tomas, can you answer this question please?
> 
> As I said yesterday during the dev meeting, it would mean that one could
> call 6 times the GET /items route to kick the server down?

I think I did on my commit message, Jonathan. I'll rephrase.

Test::Mojo creates a user agent that is used internally. This user agent has a low timeout. Loading the V1.pm app in Test::Mojo is not the same as loading it with Plack and I'm not sure about the internals.

This is not different than running a badly written report that takes too much. It might keep a starman worker busy for too long, etc.

I think we need to force pagination and at some point I was sure we did.

(In reply to Tomás Cohen Arazi from comment #10)
> I think we need to force pagination and at some point I was sure we did.

Can you open a bug report for that please?

Created attachment 105182 [details] [review]
Bug 24229: Avoid fetching ALL items in test

I'm not sure what we need to test here, besides we get results. I'm
pretty sure we shouldn't allow a plain GET return all objects because it
could cause a DOS very easily. But it certainly belongs to a separate
bug/discussion.

In this case, I put a constraint on the items domain (by using
_per_page=10 in the query params) so it won't happen that under certain
conditions the user agent used by Test::Mojo time outs.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 105193 [details] [review]
Bug 24229: Avoid fetching ALL items in test

I'm not sure what we need to test here, besides we get results. I'm
pretty sure we shouldn't allow a plain GET return all objects because it
could cause a DOS very easily. But it certainly belongs to a separate
bug/discussion.

In this case, I put a constraint on the items domain (by using
_per_page=10 in the query params) so it won't happen that under certain
conditions the user agent used by Test::Mojo time outs.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

(In reply to Jonathan Druart from comment #11)
> (In reply to Tomás Cohen Arazi from comment #10)
> > I think we need to force pagination and at some point I was sure we did.
> 
> Can you open a bug report for that please?

Tomas, Joubu said, I should tell you to file that bug ;)

(In reply to Katrin Fischer from comment #14)
> (In reply to Jonathan Druart from comment #11)
> > (In reply to Tomás Cohen Arazi from comment #10)
> > > I think we need to force pagination and at some point I was sure we did.
> > 
> > Can you open a bug report for that please?
> 
> Tomas, Joubu said, I should tell you to file that bug ;)

Done, bug 25570.

Tomas, do we need this if we have bug 25570?

Nice work everyone!

Pushed to master for 20.05

backported to 19.11.x for 19.11.07

Won't backport to 19.05.x, listing items didn't existing back then.

https://gitlab.com/koha-community/Koha/-/blob/19.05.x/api/v1/swagger/paths/items.json
https://gitlab.com/koha-community/Koha/-/blob/19.11.x/api/v1/swagger/paths/items.json

Hence the tests failing with 404 errors ^^"