Bug 32923 - x-koha-embed must a header of collectionFormat csv
Summary: x-koha-embed must a header of collectionFormat csv
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact:
URL: https://github.com/jhthorsen/mojolici...
Keywords:
: 33306 (view as bug list)
Depends on: 30536 32898
Blocks: 32806
  Show dependency treegraph
 
Reported: 2023-02-09 09:34 UTC by Jonathan Druart
Modified: 2023-12-28 20:43 UTC (History)
6 users (show)

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


Attachments
Bug 32923: Fix new occurrences of x-koha-embed at top level (16.34 KB, patch)
2023-02-09 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32923: Some fixes in ERM specs (3.01 KB, patch)
2023-02-09 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32923: Some fixes in ERM specs (3.67 KB, patch)
2023-02-09 11:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32923: needed? (786 bytes, patch)
2023-02-09 16:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32923: Add 'vendor' for local packages (777 bytes, patch)
2023-02-14 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32923: Fix x-koha-embed from EHoldings/*/* (7.71 KB, patch)
2023-02-14 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32898: Remove unecessary async/await (6.07 KB, patch)
2023-02-16 18:37 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32898: Remove unecessary async/await (6.07 KB, patch)
2023-02-16 18:39 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32923: Fix new occurrences of x-koha-embed at top level (16.38 KB, patch)
2023-02-17 12:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32923: Some fixes in ERM specs (3.71 KB, patch)
2023-02-17 12:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32923: Add 'vendor' for local packages (834 bytes, patch)
2023-02-17 12:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32923: Fix x-koha-embed from EHoldings/*/* (7.76 KB, patch)
2023-02-17 12:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32923: Fix new occurrences of x-koha-embed at top level (16.43 KB, patch)
2023-02-24 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32923: Some fixes in ERM specs (3.77 KB, patch)
2023-02-24 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32923: Add 'vendor' for local packages (890 bytes, patch)
2023-02-24 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32923: Fix x-koha-embed from EHoldings/*/* (7.82 KB, patch)
2023-02-24 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32923: (follow-up) Fix x-koha-embed from EHoldings/*/* (1.94 KB, patch)
2023-03-17 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32923: Fix licenses spec (1.14 KB, patch)
2023-03-20 13:26 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32923: Allow to embed user_roles for agreements (1.07 KB, patch)
2023-03-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-02-09 09:34:43 UTC
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'
Comment 1 Jonathan Druart 2023-02-09 09:38:46 UTC
Created attachment 146421 [details] [review]
Bug 32923: Fix new occurrences of x-koha-embed at top level
Comment 2 Jonathan Druart 2023-02-09 09:39:08 UTC
How do we prevent that from happening again?
Comment 3 Jonathan Druart 2023-02-09 09:56:11 UTC
Created attachment 146422 [details] [review]
Bug 32923: Some fixes in ERM specs
Comment 4 Jonathan Druart 2023-02-09 10:20:52 UTC
(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.
Comment 5 Jonathan Druart 2023-02-09 10:22:21 UTC
I would not backport these patches, they can introduce hard to catch side-effects.
Comment 6 Jonathan Druart 2023-02-09 11:33:12 UTC
Created attachment 146425 [details] [review]
Bug 32923: Some fixes in ERM specs
Comment 7 Jonathan Druart 2023-02-09 16:36:07 UTC
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?
Comment 8 Jonathan Druart 2023-02-09 16:46:35 UTC
Created attachment 146443 [details] [review]
Bug 32923: needed?

I think we need that, see later.
Comment 9 Jonathan Druart 2023-02-14 14:14:49 UTC
Created attachment 146632 [details] [review]
Bug 32923: Add 'vendor' for local packages
Comment 10 Jonathan Druart 2023-02-14 14:14:52 UTC
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"
Comment 11 Pedro Amorim 2023-02-16 18:34:52 UTC
(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.
Comment 12 Pedro Amorim 2023-02-16 18:37:51 UTC Comment hidden (obsolete)
Comment 13 Pedro Amorim 2023-02-16 18:39:28 UTC Comment hidden (obsolete)
Comment 14 Pedro Amorim 2023-02-17 12:36:47 UTC
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>
Comment 15 Pedro Amorim 2023-02-17 12:36:50 UTC
Created attachment 146837 [details] [review]
Bug 32923: Some fixes in ERM specs

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 16 Pedro Amorim 2023-02-17 12:36:52 UTC
Created attachment 146838 [details] [review]
Bug 32923: Add 'vendor' for local packages

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 17 Pedro Amorim 2023-02-17 12:36:55 UTC
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>
Comment 18 Nick Clemens 2023-02-24 11:34:58 UTC
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>
Comment 19 Nick Clemens 2023-02-24 11:35:01 UTC
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>
Comment 20 Nick Clemens 2023-02-24 11:35:03 UTC
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>
Comment 21 Nick Clemens 2023-02-24 11:35:05 UTC
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>
Comment 22 Tomás Cohen Arazi 2023-02-24 13:10:20 UTC
(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.
Comment 23 Tomás Cohen Arazi 2023-02-24 17:04:54 UTC
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.
Comment 24 Tomás Cohen Arazi 2023-02-27 14:01:37 UTC
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);
}
```
Comment 25 Tomás Cohen Arazi 2023-02-27 14:22:42 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 26 Matt Blenkinsop 2023-02-28 17:58:17 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 27 Matt Blenkinsop 2023-02-28 18:55:32 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 28 Lucas Gass 2023-03-08 14:08:15 UTC
Not backporting to 22.05.x
Comment 29 Jonathan Druart 2023-03-17 14:08:12 UTC
Created attachment 148349 [details] [review]
Bug 32923: (follow-up) Fix x-koha-embed from EHoldings/*/*
Comment 30 Tomás Cohen Arazi 2023-03-17 14:26:15 UTC
Follow-up pushed. Thanks Jonathan.

Keeping the additional_work_needed so stable is aware.
Comment 31 Matt Blenkinsop 2023-03-17 14:46:48 UTC
Follow-up noted, added to queue for backporting to stable
Comment 32 Matt Blenkinsop 2023-03-17 17:21:45 UTC
Backported to 22.11.x, keeping additonal_work_required flag for 22.05 maintainer
Comment 33 Pedro Amorim 2023-03-20 13:26:22 UTC
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
Comment 34 Pedro Amorim 2023-03-20 13:55:45 UTC
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.
Comment 35 Martin Renvoize 2023-03-20 16:58:36 UTC
Final follow-up also pushed to 22.11.x for 22.11.04
Comment 36 Jonathan Druart 2023-03-22 09:26:14 UTC
Created attachment 148507 [details] [review]
Bug 32923: Allow to embed user_roles for agreements
Comment 37 Jonathan Druart 2023-03-22 09:27:14 UTC
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.
Comment 38 Jonathan Druart 2023-03-22 14:41:57 UTC
*** Bug 33306 has been marked as a duplicate of this bug. ***
Comment 39 Jonathan Druart 2023-03-23 14:04:59 UTC
(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.
Comment 40 Martin Renvoize 2023-03-24 10:28:02 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 41 Martin Renvoize 2023-03-24 10:33:18 UTC
Final follow-up pushed to 22.11.x for 22.11.04
Comment 42 Lucas Gass 2023-04-24 16:34:35 UTC
Not needed for 22.05.x, no backport