Bug 24229

Summary: /items API tests fail on Ubuntu 18.04
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Test SuiteAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: normal    
Priority: P5 - low CC: aleisha, dcook, jonathan.druart, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25514
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.07
Bug Depends on: 16825    
Bug Blocks: 25551, 25570    
Attachments: Bug 24229: Avoid fetching ALL items in test
Bug 24229: Avoid fetching ALL items in test
Bug 24229: Avoid fetching ALL items in test

Description Tomás Cohen Arazi 2019-12-12 13:10:42 UTC
Tests fail in a weird way:

kohadev-koha@7369d4443675:/kohadevbox/koha$ prove t/db_dependent/api/v1/items.t
t/db_dependent/api/v1/items.t ..     # Inactivity timeout

    #   Failed test 'GET //RTrSH14TLmPlTAsO1RAR9x8MhJtMO7XJS5IKOwBKfwcFnpzHSGSEPM7k:thePassword123@/api/v1/items'
    #   at t/db_dependent/api/v1/items.t line 75.

    #   Failed test 'SWAGGER3.2.2'
    #   at t/db_dependent/api/v1/items.t line 75.
    #          got: undef
    #     expected: '200'
    # Looks like you planned 12 tests but ran 5.
    # Looks like you failed 2 tests of 5 run.
t/db_dependent/api/v1/items.t .. 1/2
#   Failed test 'list() tests'
#   at t/db_dependent/api/v1/items.t line 95.
Can't use an undefined value as an ARRAY reference at t/db_dependent/api/v1/items.t line 79.
# Looks like your test exited with 255 just after 1.
t/db_dependent/api/v1/items.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/2 subtests

It happens almost randomly (I reproduced the first time I started koha-testing-docker with the master-bionic image, but then it wouldn't reproduce).
Comment 1 Jonathan Druart 2020-02-20 08:46:44 UTC
Still, but without any info:
koha_1       | [17:33:12] t/db_dependent/api/v1/items.t ........................................... 
koha_1       | Failed 2/2 subtests
Comment 2 Jonathan Druart 2020-05-15 09:52:35 UTC
 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.
Comment 3 Jonathan Druart 2020-05-15 11:08:14 UTC
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!
Comment 4 Jonathan Druart 2020-05-15 11:29:04 UTC
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.
Comment 5 Jonathan Druart 2020-05-15 13:48:33 UTC
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.
Comment 6 Jonathan Druart 2020-05-18 09:40:42 UTC
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)
Comment 7 Tomás Cohen Arazi 2020-05-20 22:10:58 UTC
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>
Comment 8 Tomás Cohen Arazi 2020-05-20 22:12:04 UTC
(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.
Comment 9 Jonathan Druart 2020-05-21 08:10:57 UTC
(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?
Comment 10 Tomás Cohen Arazi 2020-05-21 11:43:24 UTC
(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.
Comment 11 Jonathan Druart 2020-05-21 12:39:24 UTC
(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?
Comment 12 Jonathan Druart 2020-05-21 12:41:30 UTC
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>
Comment 13 Katrin Fischer 2020-05-21 13:29:48 UTC
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>
Comment 14 Katrin Fischer 2020-05-21 13:30:15 UTC
(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 ;)
Comment 15 Tomás Cohen Arazi 2020-05-21 22:16:22 UTC
(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.
Comment 16 Jonathan Druart 2020-05-22 08:11:07 UTC
Tomas, do we need this if we have bug 25570?
Comment 17 Martin Renvoize 2020-05-22 08:33:42 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Aleisha Amohia 2020-06-12 02:17:08 UTC
backported to 19.11.x for 19.11.07
Comment 19 Victor Grousset/tuxayo 2020-06-12 21:41:57 UTC
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 ^^"