Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation->format, with normal Mojo usage to avoid future headaches.
Created attachment 152212 [details] [review] Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect.
Created attachment 152213 [details] [review] Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D
Created attachment 152214 [details] [review] Bug 33556: Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes.
I did the trivial ones. Which could be pushed as-is while we figure the tricky stuff. The idea behind the previous sentence is we can make devs rely on better examples for ongoing stuff.
All looks good to me (review, tests (api+cypress) and a couple of manual tests). I am good with pushing as it, but we need a plan for the remaining occurrences.
Created attachment 152693 [details] [review] Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152694 [details] [review] Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152695 [details] [review] Bug 33556: Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I think this all looks solid.. so adding my signoff line. As for catching cases introduced in the future.. perhaps a QA script addition to spot `validation->param` introductions?
https://gitlab.com/koha-community/qa-test-tools/-/issues/68 I'll try to find a moment to have a look at submitting something for that.
(In reply to Martin Renvoize from comment #10) > https://gitlab.com/koha-community/qa-test-tools/-/issues/68 > > I'll try to find a moment to have a look at submitting something for that. Done
(In reply to Martin Renvoize from comment #11) > (In reply to Martin Renvoize from comment #10) > > https://gitlab.com/koha-community/qa-test-tools/-/issues/68 > > > > I'll try to find a moment to have a look at submitting something for that. > > Done I let you a comment there.
We are failing patches in qa tools with a reference to a patch that does not even apply???
(In reply to Marcel de Rooy from comment #13) > We are failing patches in qa tools with a reference to a patch that does not > even apply??? We don't want new occurrences.
Created attachment 153725 [details] [review] Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153726 [details] [review] Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153727 [details] [review] Bug 33556: Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Minor rebase against bug 33974
Seeing it too: An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Resources::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Resources::Local, ] merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 112.
Please clarify
(In reply to Marcel de Rooy from comment #20) > Please clarify I can't, and won't. It is an underlying problem with the modules design, which the author(s) should address. My feeling is it is related to this: use Mojo::Base 'Mojolicious::Controller'; use Koha::ERM::EHoldings::Titles; . We can leave out the ERM-related stuff from this bug if you ask me, as it is incomplete anyways. The trivial changes I'm making are trivial an unrelated to the error.
Created attachment 153784 [details] [review] Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 153785 [details] [review] Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Tomás Cohen Arazi from comment #21) > (In reply to Marcel de Rooy from comment #20) > > Please clarify > > I can't, and won't. It is an underlying problem with the modules design, > which the author(s) should address. My feeling is it is related to this: > > use Mojo::Base 'Mojolicious::Controller'; > > use Koha::ERM::EHoldings::Titles; > > . > > We can leave out the ERM-related stuff from this bug if you ask me, as it is > incomplete anyways. The trivial changes I'm making are trivial an unrelated > to the error. I think we simply asking to clarify the situation. You are first saying you don't want to go with ERM because of the errors, but the patch is still there. I moved it to bug 34339 so that this first step can be pushed.
(In reply to Jonathan Druart from comment #24) > > I think we simply asking to clarify the situation. You are first saying you > don't want to go with ERM because of the errors, but the patch is still > there. > > I moved it to bug 34339 so that this first step can be pushed. It's fine, I'm just answering I don't have an answer. And the change was so small... :-D
(In reply to Jonathan Druart from comment #24) > I think we simply asking to clarify the situation. I remember now. We got communication issues. There are two different problems with the ERM-related code, that are (1) addressed but separatedly and (2) left out: (1) I put the (minor) ERM changes on a separate bug so it doesn't pollute the other changes that are a lot and harder to rebase, because it fails badly and didn't allow to run 'qa' on the big one. (2) The ERM missing changes I'm referring to, are not those on that patch, but the fact this modules make use of `my $args = $c->validation->output` to be passed to the specific classes implementing the business logic for each case, and I didn't 100% understand how things worked (they feel a bit hacky) so wasn't sure about the right way to do it.
(In reply to Tomás Cohen Arazi from comment #26) > I remember now. We got communication issues. There are two different > problems with the ERM-related code, that are (1) addressed but separatedly > and (2) left out: > > (1) I put the (minor) ERM changes on a separate bug so it doesn't pollute > the other changes that are a lot and harder to rebase, because it fails > badly and didn't allow to run 'qa' on the big one. No, it was on this bug report, hence the confusion. > (2) The ERM missing changes I'm referring to, are not those on that patch, > but the fact this modules make use of `my $args = $c->validation->output` to > be passed to the specific classes implementing the business logic for each > case, and I didn't 100% understand how things worked (they feel a bit hacky) > so wasn't sure about the right way to do it. It certainly feels hacky, but I will someone to explain how to fix them properly. We are not manipulating usual Koha::Objects here, we are pulling data for an external service and serve it to the Koha ERM Vue app. So we cannot use the regular REST::Plugin::, we are trying to behave the same as the other objects so that the app does not need to do different for those endpoints.
(In reply to Jonathan Druart from comment #27) > > (2) The ERM missing changes I'm referring to, are not those on that patch, > > but the fact this modules make use of `my $args = $c->validation->output` to > > be passed to the specific classes implementing the business logic for each > > case, and I didn't 100% understand how things worked (they feel a bit hacky) > > so wasn't sure about the right way to do it. > > It certainly feels hacky, but I will someone to explain how to fix them > properly. > We are not manipulating usual Koha::Objects here, we are pulling data for an > external service and serve it to the Koha ERM Vue app. So we cannot use the > regular REST::Plugin::, we are trying to behave the same as the other > objects so that the app does not need to do different for those endpoints. I mean this: my $args = $c->validation->output; my $ebsco = Koha::ERM::Providers::EBSCO->new; ... my $total = $result->{totalResults}; $total = 10000 if $total > 10000; $c->add_pagination_headers( { base_total => $base_total, page => $page, per_page => $per_page, query_params => $args, total => $total, } ); and similar. I wasn't sure how relevant the $args are here. I also feels like it is not entirely correct, as $c->validation->output actually contains path parameters, not just query params. So I felt like I didn't have enough information to fix it.
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.03
Does not apply to 22.11.x. Not pushing.