Bug 30165 - Several q parameters break the filters
Summary: Several q parameters break the filters
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 30677
  Show dependency treegraph
 
Reported: 2022-02-23 14:29 UTC by Jonathan Druart
Modified: 2022-05-04 15:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05


Attachments
Bug 30165: Regression tests (3.59 KB, patch)
2022-02-25 11:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30165: Regression tests (3.59 KB, patch)
2022-03-15 12:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30165: Make q parameter 'multi' (4.14 KB, patch)
2022-03-15 12:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30165: Regression tests (3.59 KB, patch)
2022-03-15 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30165: Make q parameter 'multi' (3.62 KB, patch)
2022-03-15 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30165: (follow-up) Fix GET /acquisitions/orders (4.65 KB, patch)
2022-03-15 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30165: Regression tests (3.65 KB, patch)
2022-03-16 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30165: Make q parameter 'multi' (3.69 KB, patch)
2022-03-16 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30165: (follow-up) Fix GET /acquisitions/orders (4.71 KB, patch)
2022-03-16 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30165: Regression tests (3.73 KB, patch)
2022-03-17 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30165: Make q parameter 'multi' (3.76 KB, patch)
2022-03-17 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30165: (follow-up) Fix GET /acquisitions/orders (4.79 KB, patch)
2022-03-17 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
[OPTION 1] Bug 30165: (follow-up) Address OpenAPI vs. no-OpenAPI in the tests (4.74 KB, patch)
2022-03-24 12:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[OPTION 2] Bug 30165: (follow-up) Allow objects.search usage outside OpenAPI (2.32 KB, patch)
2022-03-24 12:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-02-23 14:29:19 UTC
If several q parameters are passed, only one will be taken into account.
See bug 30164 for more details.

Basically
/api/v1/cities?q={"name":{"like":"%bou%"}}&q=[]&_page=1&_per_page=20&_match=contains&_order_by=+me.name

will retrieve all the cities, only the second q is used to generate the filters (q=[])
Comment 1 Tomás Cohen Arazi 2022-02-25 11:52:01 UTC
Created attachment 131111 [details] [review]
Bug 30165: Regression tests
Comment 2 Tomás Cohen Arazi 2022-03-15 12:48:07 UTC
Created attachment 131706 [details] [review]
Bug 30165: Regression tests
Comment 3 Tomás Cohen Arazi 2022-03-15 12:48:14 UTC
Created attachment 131707 [details] [review]
Bug 30165: Make q parameter 'multi'

This patch changes the q_param definition so the defined query parameter
is repeatable. This way JSON::Validator will always generate an arrayref
for it and won't skip occurences.

The objects.search helper is updated to always consider the 'q'
parameter as an array, as expected.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/query.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Jonathan Druart 2022-03-15 12:57:14 UTC
Does this need to depend on bug 30194?
Comment 5 Tomás Cohen Arazi 2022-03-15 13:25:29 UTC
(In reply to Jonathan Druart from comment #4)
> Does this need to depend on bug 30194?

My branch had it because there are several collectionFormat handling fixes in newer JSON::Validator. But you can try without it. You just need to remove the bundle portion.

My suggestion is to keep the dependency
Comment 6 Tomás Cohen Arazi 2022-03-15 18:22:35 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > Does this need to depend on bug 30194?
> 
> My branch had it because there are several collectionFormat handling fixes
> in newer JSON::Validator. But you can try without it. You just need to
> remove the bundle portion.
> 
> My suggestion is to keep the dependency

It works with the older library versions. Removing the dependency so it can be backported.
Comment 7 Tomás Cohen Arazi 2022-03-15 18:23:01 UTC
Created attachment 131759 [details] [review]
Bug 30165: Regression tests
Comment 8 Tomás Cohen Arazi 2022-03-15 18:23:06 UTC
Created attachment 131760 [details] [review]
Bug 30165: Make q parameter 'multi'

This patch changes the q_param definition so the defined query parameter
is repeatable. This way JSON::Validator will always generate an arrayref
for it and won't skip occurences.

The objects.search helper is updated to always consider the 'q'
parameter as an array, as expected.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/query.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-03-15 18:23:12 UTC
Created attachment 131761 [details] [review]
Bug 30165: (follow-up) Fix GET /acquisitions/orders

This patch fixes the particular use case of the orders route, which has
a slightly modified version of the objects.search helped, embeded in the
controller itself. This controller gets adjusted to the fact q will now
be an array.

Because of the latter, we end up requiring more code duplication
regarding the query fix, so I moved it to an internal sub that gets
reused.

To test:
1. Apply the previous patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/*
=> FAIL: It t/db_dependent/api/v1/acquisitions_orders.t fails!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Martin Renvoize 2022-03-16 14:41:51 UTC
Created attachment 131803 [details] [review]
Bug 30165: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2022-03-16 14:41:55 UTC
Created attachment 131804 [details] [review]
Bug 30165: Make q parameter 'multi'

This patch changes the q_param definition so the defined query parameter
is repeatable. This way JSON::Validator will always generate an arrayref
for it and won't skip occurences.

The objects.search helper is updated to always consider the 'q'
parameter as an array, as expected.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/query.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2022-03-16 14:41:59 UTC
Created attachment 131805 [details] [review]
Bug 30165: (follow-up) Fix GET /acquisitions/orders

This patch fixes the particular use case of the orders route, which has
a slightly modified version of the objects.search helped, embeded in the
controller itself. This controller gets adjusted to the fact q will now
be an array.

Because of the latter, we end up requiring more code duplication
regarding the query fix, so I moved it to an internal sub that gets
reused.

To test:
1. Apply the previous patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/*
=> FAIL: It t/db_dependent/api/v1/acquisitions_orders.t fails!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-03-16 14:42:33 UTC
All seems to work in testing and unit tests are passing.  Signing off
Comment 14 Tomás Cohen Arazi 2022-03-16 17:02:48 UTC
(In reply to Martin Renvoize from comment #13)
> All seems to work in testing and unit tests are passing.  Signing off

Thanks. The most ugly bit is the changes in the /acquisitions/orders controller. I will try to make something more generic so we don't need to patch that separatedly each time.
Comment 15 Jonathan Druart 2022-03-17 08:30:48 UTC
-            # FIXME The following lines are an ugly fix to deal with isbn and ean searches
-            # This must NOT be reused or extended
-            # Instead we need a better and global solution in a Koha::*Biblio method


+The Koha::Biblio representation includes the biblioitem.* attributes. This is handy
+for API consumers but as they are different tables, converting the queries that mention
+biblioitem columns can be tricky. This method renames known column names as used on Koha's
+UI.

Well, that's not equivalent :D
Comment 16 Jonathan Druart 2022-03-17 08:35:00 UTC
Created attachment 131821 [details] [review]
Bug 30165: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2022-03-17 08:35:04 UTC
Created attachment 131822 [details] [review]
Bug 30165: Make q parameter 'multi'

This patch changes the q_param definition so the defined query parameter
is repeatable. This way JSON::Validator will always generate an arrayref
for it and won't skip occurences.

The objects.search helper is updated to always consider the 'q'
parameter as an array, as expected.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/query.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2022-03-17 08:35:09 UTC
Created attachment 131823 [details] [review]
Bug 30165: (follow-up) Fix GET /acquisitions/orders

This patch fixes the particular use case of the orders route, which has
a slightly modified version of the objects.search helped, embeded in the
controller itself. This controller gets adjusted to the fact q will now
be an array.

Because of the latter, we end up requiring more code duplication
regarding the query fix, so I moved it to an internal sub that gets
reused.

To test:
1. Apply the previous patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/*
=> FAIL: It t/db_dependent/api/v1/acquisitions_orders.t fails!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2022-03-17 08:39:23 UTC
That's awesome, thanks!
Comment 20 Fridolin Somers 2022-03-22 20:19:24 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 21 Fridolin Somers 2022-03-22 23:22:58 UTC
This breaks t/db_dependent/Koha/REST/Plugin/Objects.t

See https://jenkins.koha-community.org/view/master/job/Koha_Master_D10/556/testReport/
Comment 22 Tomás Cohen Arazi 2022-03-24 12:46:26 UTC
Created attachment 132138 [details] [review]
[OPTION 1] Bug 30165: (follow-up) Address OpenAPI vs. no-OpenAPI in the tests

Our objects.search helper is written for being used in our API
controllers, which get their query parameters processed by the OpenAPI
plugin, using JSON::Validator.

Particularly, the 'q' parameter is defined as 'multi' on our spec, which
means objects.search always gets it as an arrayref.

As the Objects.t tests are not using the OpenAPI plugin, a hashref is
generated as only one q= is being passed. This patch adds an extra
(empty) q param to the tests to overcome this.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2022-03-24 12:56:00 UTC
Created attachment 132139 [details] [review]
[OPTION 2] Bug 30165: (follow-up) Allow objects.search usage outside OpenAPI

Our objects.search helper is written for being used in our API
controllers, which get their query parameters processed by the OpenAPI
plugin, using JSON::Validator.

Particularly, the 'q' parameter is defined as 'multi' on our spec, which
means objects.search always gets it as an arrayref.

As the Objects.t tests are not using the OpenAPI plugin, a hashref is
generated as only one q= is being passed. This patch adds an extra
validation on objects.search, for the non-arrayref use case and does the
right thing.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Fridolin Somers 2022-03-24 19:26:27 UTC
I'd say option 2.
This may be useful in some other usages of Objects.search
Comment 25 Tomás Cohen Arazi 2022-03-25 02:46:52 UTC
(In reply to Fridolin Somers from comment #24)
> I'd say option 2.
> This may be useful in some other usages of Objects.search

I agree.
Comment 26 Fridolin Somers 2022-03-25 06:08:22 UTC
Comment on attachment 132138 [details] [review]
[OPTION 1] Bug 30165: (follow-up) Address OpenAPI vs. no-OpenAPI in the tests

Lets go for option 2.

> Bug 30165: (follow-up) Allow objects.search usage outside OpenAPI
Pushed to master
Comment 27 Kyle M Hall 2022-03-25 13:37:54 UTC
Pushed to 21.11.x for 21.11.05
Comment 28 Andrew Nugged 2022-05-04 08:58:56 UTC
Seems this is the source of exception:

[ERROR] GET /api/v1/acquisitions/orders: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'biblioitem.title' in 'where clause' at /usr/share/perl5/Data/Page.pm line 49

Because here is the experimental dump of what sub table_name_fixer replaces:

in:

  'q' => '{"-and":[{"basket.vendor_id":2},[[{"basket.name":{"like":"%f%"}}],[{"basket.basket_group.name":{"like":"%f%"}}],[{"me.order_id":{"like":"%f%"}}],[{"biblio.title":{"like":"%f%"}},{"biblio.author":{"like":"%f%"}},{"biblio.isbn":{"like":"%f%"}},{"biblio.publisher":{"like":"%f%"}},{"me.internal_note":{"like":"%f%"}},{"me.vendor_note":{"like":"%f%"}}],[{"me.replacement_price":{"like":"%f%"}}],[{"me.quantity":{"like":"%f%"}}],[{"me.ecost":{"like":"%f%"}}],[{"fund.name":{"like":"%f%"}}]]]}'
};

out:

  'q' => '{"-and":[{"basket.vendor_id":2},[[{"basket.name":{"like":"%f%"}}],[{"basket.basket_group.name":{"like":"%f%"}}],[{"me.order_id":{"like":"%f%"}}],[{"biblio.biblioitem.title":{"like":"%f%"}},{"biblio.biblioitem.author":{"like":"%f%"}},{"biblio.biblioitem.isbn":{"like":"%f%"}},{"biblio.biblioitem.publisher":{"like":"%f%"}},{"me.internal_note":{"like":"%f%"}},{"me.vendor_note":{"like":"%f%"}}],[{"me.replacement_price":{"like":"%f%"}}],[{"me.quantity":{"like":"%f%"}}],[{"me.ecost":{"like":"%f%"}}],[{"fund.name":{"like":"%f%"}}]]]}'
};


That because this:

    $q =~ s|biblio\.|biblio\.biblioitem\.|g
      if $q =~ m/.*(isbn|ean|publisher).*/;

should be with lookahead to exclude false matches:

    $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g
      if $q =~ m/.*(isbn|ean|publisher).*/;
Comment 29 Andrew Nugged 2022-05-04 09:00:30 UTC
(also I think it might not need "m//" but just "s///" if it has that lookahead)
Comment 30 Andrew Nugged 2022-05-04 14:30:04 UTC
Adding solution: Bug 30677