Description
Jonathan Druart
2022-02-23 14:29:19 UTC
Created attachment 131111 [details] [review] Bug 30165: Regression tests Created attachment 131706 [details] [review] Bug 30165: Regression tests 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> (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 (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. Created attachment 131759 [details] [review] Bug 30165: Regression tests 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> 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> Created attachment 131803 [details] [review] Bug 30165: Regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> 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> All seems to work in testing and unit tests are passing. Signing off (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. - # 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 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> 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> 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> That's awesome, thanks! Pushed to master for 22.05, thanks to everybody involved [U+1F984] This breaks t/db_dependent/Koha/REST/Plugin/Objects.t See https://jenkins.koha-community.org/view/master/job/Koha_Master_D10/556/testReport/ 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> 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> I'd say option 2. This may be useful in some other usages of Objects.search (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 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 Pushed to 21.11.x for 21.11.05 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).*/; (also I think it might not need "m//" but just "s///" if it has that lookahead) Adding solution: Bug 30677 |