See bug 30536, since then new patches added more x-koha-embed not being enum. % git grep x-koha-embed api/v1/swagger/paths/|grep -v 'name: x-koha-embed'
Created attachment 146421 [details] [review] Bug 32923: Fix new occurrences of x-koha-embed at top level
How do we prevent that from happening again?
Created attachment 146422 [details] [review] Bug 32923: Some fixes in ERM specs
(In reply to Jonathan Druart from comment #2) > How do we prevent that from happening again? I've created a merge request there: https://gitlab.com/koha-community/qa-test-tools/-/issues/65 But it feels like we need a test in Koha to catch specs inconsistencies.
I would not backport these patches, they can introduce hard to catch side-effects.
Created attachment 146425 [details] [review] Bug 32923: Some fixes in ERM specs
Tomas, Martin, I am seeing something weird with x-koha-embed here. With a request with the following x-koha-header: package_agreements,package_agreements.agreement,resources+count,vendor I am correctly seeing it it Koha/REST/Plugin/Query.pm (in stash_embed) However, I am getting the header later, using $c->req->headers->header('x-koha-embed') and I am seeing "package_agreements" only (!) I guess I could use $c->stash('koha.embed') instead, but then it's no longer a string an I need to change the logic in several places. Is that expected?
Created attachment 146443 [details] [review] Bug 32923: needed? I think we need that, see later.
Created attachment 146632 [details] [review] Bug 32923: Add 'vendor' for local packages
Created attachment 146633 [details] [review] Bug 32923: Fix x-koha-embed from EHoldings/*/* We were validating twice the input and it broke the x-koha-embed header. I don't really know what's happening, but here is how to recreate the problem: 1. Edit Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm 2. Add the following lines in Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm, in the list method warn $c->req->headers->header('x-koha-embed'); 3. curl -v -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/erm/eholdings/ebsco/packages --header "x-koha-embed: resources+count,vendor" => There is no vendor in the response => The warn displays "resources+count" If you apply this patch you will notice that "vendor" is part of the response and the warn displays "resources+count, vendor"
(In reply to Jonathan Druart from comment #10) > Created attachment 146633 [details] [review] [review] > Bug 32923: Fix x-koha-embed from EHoldings/*/* > > We were validating twice the input and it broke the x-koha-embed header. > I don't really know what's happening, but here is how to recreate the > problem: > 1. Edit Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm > 2. Add the following lines in Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm, > in the list method > warn $c->req->headers->header('x-koha-embed'); > 3. curl -v -s -u koha:koha --request GET > http://kohadev-intra.mydnsname.org:8081/api/v1/erm/eholdings/ebsco/packages > --header "x-koha-embed: resources+count,vendor" > > => There is no vendor in the response > => The warn displays "resources+count" > > If you apply this patch you will notice that "vendor" is part of the > response and the warn displays "resources+count, vendor" Did a lot of testing with this but openapi->valid_input in EBSCO.pm is getting rid of all arguments in the x-koha-embed header except the first one. Lot of trial and error in erm_eholdings_packages.yaml also came up with no breakthrough. Found this fix related to default collectionFormat in version 5.06 (we're using 5.05) so upgraded to Mojolicious::Plugin::OpenAPI to 5.08 but result is the same: https://github.com/jhthorsen/mojolicious-plugin-openapi/commit/c972fdd02a05dc635257715a4b8c541b218e2817 This weird behaviour seems to be caused by mojo. The code implemented to work around this is working as intended.
Created attachment 146748 [details] [review] Bug 32898: Remove unecessary async/await myFetch awaits already Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 146749 [details] [review] Bug 32898: Remove unecessary async/await myFetch awaits already Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 146836 [details] [review] Bug 32923: Fix new occurrences of x-koha-embed at top level Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 146837 [details] [review] Bug 32923: Some fixes in ERM specs Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 146838 [details] [review] Bug 32923: Add 'vendor' for local packages Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 146839 [details] [review] Bug 32923: Fix x-koha-embed from EHoldings/*/* We were validating twice the input and it broke the x-koha-embed header. I don't really know what's happening, but here is how to recreate the problem: 1. Edit Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm 2. Add the following lines in Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm, in the list method warn $c->req->headers->header('x-koha-embed'); 3. curl -v -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/erm/eholdings/ebsco/packages --header "x-koha-embed: resources+count,vendor" => There is no vendor in the response => The warn displays "resources+count" If you apply this patch you will notice that "vendor" is part of the response and the warn displays "resources+count, vendor" Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 147292 [details] [review] Bug 32923: Fix new occurrences of x-koha-embed at top level Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147293 [details] [review] Bug 32923: Some fixes in ERM specs Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147294 [details] [review] Bug 32923: Add 'vendor' for local packages Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147295 [details] [review] Bug 32923: Fix x-koha-embed from EHoldings/*/* We were validating twice the input and it broke the x-koha-embed header. I don't really know what's happening, but here is how to recreate the problem: 1. Edit Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm 2. Add the following lines in Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm, in the list method warn $c->req->headers->header('x-koha-embed'); 3. curl -v -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/erm/eholdings/ebsco/packages --header "x-koha-embed: resources+count,vendor" => There is no vendor in the response => The warn displays "resources+count" If you apply this patch you will notice that "vendor" is part of the response and the warn displays "resources+count, vendor" Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Jonathan Druart from comment #5) > I would not backport these patches, they can introduce hard to catch > side-effects. Having reviewed it, I think this needs to be backported.
Ok, I agree with the patches, generally. I'd like a separate bug for the non-ERM fixes to the specs, so it can be backported more easily. Regarding the last patch, I've added a regression test patch for Mojolicious::Plugin::OpenAPI to highlight the issue we are making a workaround for. Let's see what the maintainer says and then decide how to move forward with this. Deadline for the rmaint answer is next monday, so we all know this is to be pushed anyway and nothing blocks the dependencies.
After an exchange with the Mojolicious::Plugin::OpenAPI maintainer and exchanging pull requests, the conclusion is: - Validation is not supposed to happen twice - The validation step is expected to normalize the ->params and ->headers in a specific way, and we shouldn't be relying on the $c->validation->output structure as it subject to change and at some point they thought of removing it. Once validation happens we need to use ->params and ->headers. - When valid_input is called, the header effectively gets converted into an array and expecting it to still be a CSv string will lead to problems as we've seen. - When collectionFormat is used, we should be using multi, the maintainer really dislikes the 'csv' implementation. - I would've expected the plugin to provide us a consistent and usable way to access parameters through ->validation->output and leave the params untouched, but this is not the case, by design. And is not going to change. Having said all that, I agree with this patchset QA note: I have doubts about the real need for this API spec that points to an intermediate controller that later calls the *real controller*. I would've just duplicated the endpoint spec, hardcoding 'ebsco' and 'local' and calling the appropriate method. The only reason I would accept to keep this, is because you (ERM devs) might be thinking of extending this with plugins that implement provider-specific implementations, along the lines of this mocked code: ```perl sub get { my $c = shift->openapi->valid_input or return; my @plugins = GetPlugins({ method => 'erm_packages' }); my $provider = $c->validation->param('provider'); if ( $provider eq 'ebsco' ) { return Koha::REST::V1::ERM::EHoldings::Packages::EBSCO::get($c, $validated_output); } elsif (@plugins) { foreach my $plugin (@plugins) { if ( $provide eq $plugin->erm_provider ) { return $plugin->erm_package_controller::get($c); } } } return Koha::REST::V1::ERM::EHoldings::Packages::Local::get($c, $validated_output); } ```
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work everyone! Pushed to stable for 22.11.x
Not backporting to 22.05.x
Created attachment 148349 [details] [review] Bug 32923: (follow-up) Fix x-koha-embed from EHoldings/*/*
Follow-up pushed. Thanks Jonathan. Keeping the additional_work_needed so stable is aware.
Follow-up noted, added to queue for backporting to stable
Backported to 22.11.x, keeping additonal_work_required flag for 22.05 maintainer
Created attachment 148413 [details] [review] Bug 32923: Fix licenses spec Bug 32713 is now validating embed and brought this flaw to light. Before this patch, listing licenses on ERM would result in a 400 bad request
Adding some thougths here for discussion, please discard if it doesn't make sense: Changes to biblios_item_groups.yaml, checkouts.yaml and items.yaml in attachment 147292 [details] [review] may make sense to backport to 22.05, but that attachment also contains ERM changes that will likely not apply nicely. Other attachments in this bug are exclusive to ERM so don't need to be considered.
Final follow-up also pushed to 22.11.x for 22.11.04
Created attachment 148507 [details] [review] Bug 32923: Allow to embed user_roles for agreements
Is this still in time for 22.11.04? Or should I open a new bug? The agreement list table is not displayed correctly without this last patch.
*** Bug 33306 has been marked as a duplicate of this bug. ***
(In reply to Jonathan Druart from comment #36) > Created attachment 148507 [details] [review] [review] > Bug 32923: Allow to embed user_roles for agreements Patch pushed to master for 23.05.
Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release
Final follow-up pushed to 22.11.x for 22.11.04
Not needed for 22.05.x, no backport