Bug 30536 - Embeds should be defined in a single place
Summary: Embeds should be defined in a single place
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: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
: 30542 (view as bug list)
Depends on: 29810
Blocks: 30385 32923
  Show dependency treegraph
 
Reported: 2022-04-13 20:27 UTC by Tomás Cohen Arazi
Modified: 2023-02-09 09:35 UTC (History)
8 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


Attachments
Bug 30536: Refer to parameters enum (1.24 KB, patch)
2022-04-14 11:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Remove validation overhead (3.33 KB, patch)
2022-04-14 12:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Refer to parameters enum (1.24 KB, patch)
2022-04-14 14:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Remove validation overhead (3.49 KB, patch)
2022-04-14 14:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Use objects->find in Cashups controller (1.44 KB, patch)
2022-04-14 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Use objects->find in SMTP Servers controller (1.41 KB, patch)
2022-04-14 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Remove validation overhead (3.26 KB, patch)
2022-04-14 14:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Update spec files (8.56 KB, patch)
2022-04-14 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Unit tests (2.29 KB, patch)
2022-04-14 16:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30536: Refer to parameters enum (1.29 KB, patch)
2022-04-14 16:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30536: Remove validation overhead (3.31 KB, patch)
2022-04-14 16:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30536: Update spec files (8.62 KB, patch)
2022-04-14 16:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30536: Unit tests (2.29 KB, patch)
2022-05-03 07:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Refer to parameters enum (1.29 KB, patch)
2022-05-03 07:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Remove validation overhead (3.31 KB, patch)
2022-05-03 07:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Update spec files (8.62 KB, patch)
2022-05-03 07:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: (QA follow-up) POD + Spec Consistency (1.77 KB, patch)
2022-05-03 07:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Fix Query.t Unit Test (3.61 KB, patch)
2022-05-03 07:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30536: Unit tests (2.39 KB, patch)
2022-05-03 11:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30536: Refer to parameters enum (1.39 KB, patch)
2022-05-03 11:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30536: Remove validation overhead (3.41 KB, patch)
2022-05-03 11:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30536: Update spec files (8.71 KB, patch)
2022-05-03 11:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30536: (QA follow-up) POD + Spec Consistency (1.86 KB, patch)
2022-05-03 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30536: Fix Query.t Unit Test (3.71 KB, patch)
2022-05-03 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-04-13 20:27:06 UTC
Right now, our specs need two places:

(1) path top level x-koha-embed entry
(2) path header parameters definition for x-koha-embed as an enum

The first one was the right one in the first place, but JSON::Validator didn't manage that correctly (reliably I'd say). So we used it as in (1).

Bug 29810 added (2), which is great and correct, but our 'stash_embed' helper [1] called from Koha/REST/V1/Auth.pm is relying on (1) to build the allow-list for embeds.

So we need to have both, and we should fix it.

[1] Koha/REST/Plugin/Query.pm:237
Comment 1 Martin Renvoize 2022-04-14 11:07:38 UTC
*** Bug 30542 has been marked as a duplicate of this bug. ***
Comment 2 Martin Renvoize 2022-04-14 11:08:53 UTC
Description copied from bug 30542 - I jumped before I found this bug.. haha

>Now that we're documenting, and properly validating, the x-koha-embed parameter we can actually use this parameter definition list instead of the custom x-koha-embed array from the top level of the endpoint definition.

>This would reduce overhead of adding embeds.. we would only need to add them to the enum list in the parameter definition instead of to both the parameter definition and a top level x-koha-embed key.
Comment 3 Martin Renvoize 2022-04-14 11:28:04 UTC
Created attachment 133303 [details] [review]
Bug 30536: Refer to parameters enum

This patch updates the stash_embed method to use the parameters hash
enum instead of the top level x-koha-embed custom definition. This way
we can remove the requirement for defining this list twice.
Comment 4 Martin Renvoize 2022-04-14 11:29:09 UTC
Attached a first draft for golfing.. I have a hunch we can now drop at least some of the local validation we're doing in this method as we now validate properly at the openapi plugin level?
Comment 5 Martin Renvoize 2022-04-14 12:51:49 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize 2022-04-14 13:29:55 UTC
I'm still pottering here.. if we can move the call later, after validation has taken place, we can save ourselves both the validation and param extraction overhead.  Not sure how far down the rabbit hole it's worth going for now.

Of course.. we'd also want to clean up our spec files here, removing existing x-koha-embed top level keys.
Comment 7 Martin Renvoize 2022-04-14 14:16:54 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2022-04-14 14:16:59 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2022-04-14 14:17:04 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2022-04-14 14:17:08 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2022-04-14 14:53:28 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2022-04-14 15:08:45 UTC
Created attachment 133325 [details] [review]
Bug 30536: Update spec files

This patch removes superflous x-koha-embed defintions at the top level
of the endpoint specifications. It also replaces a few x-koha-embeds at
the top level with parameter lists where this had been missed in
preceeding patches.
Comment 13 Tomás Cohen Arazi 2022-04-14 16:02:23 UTC
Marking as NSO to gather other's opinions
Comment 14 Tomás Cohen Arazi 2022-04-14 16:02:59 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2022-04-14 16:03:04 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2022-04-14 16:03:09 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2022-04-14 16:03:16 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2022-04-29 06:35:54 UTC
QAing
Comment 19 Marcel de Rooy 2022-04-29 07:42:29 UTC
QA Comment:

[1] I would recommend to add a little bit more text on this POD:
 =head3 stash_embed

-    $c->stash_embed( $c->match->endpoint->pattern->defaults->{'openapi.op_spec'} );
+    $c->stash_embed( );

 =cut

[2] api/v1/swagger/paths/acquisitions_baskets.yaml
Here you do not specify collectionFormat: csv while you do at acquisitions_funds.yaml.
And again you dont in suggestions.yaml.
Note that as I understand that it is the default array format, we should not need them. But this looks inconsistent.

[3] Please fix t/Koha/REST/Plugin/Query.t
It fails now.
Comment 20 Martin Renvoize 2022-05-03 07:10:11 UTC Comment hidden (obsolete)
Comment 21 Martin Renvoize 2022-05-03 07:10:15 UTC Comment hidden (obsolete)
Comment 22 Martin Renvoize 2022-05-03 07:10:20 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2022-05-03 07:10:24 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2022-05-03 07:10:28 UTC
Created attachment 134483 [details] [review]
Bug 30536: (QA follow-up) POD + Spec Consistency

Well spotted, this patch fixes the specs to be consistent and adds a
little detail to the POD as requested.
Comment 25 Martin Renvoize 2022-05-03 07:10:33 UTC
Created attachment 134484 [details] [review]
Bug 30536: Fix Query.t Unit Test

The stash_embed method no longer validates the query so this patch drops
the validation tests and instead just focuses on varifying the correct
output structure of the stash after the method call.

We have not lost validation however, it now happens only once at the
OpenAPI level and tests are present in t/db_dependent/api/v1/query.t
Comment 26 Martin Renvoize 2022-05-03 07:13:52 UTC
(In reply to Marcel de Rooy from comment #19)

I updated the POD a little for you, hopefully it's a little clearer.

> [2] api/v1/swagger/paths/acquisitions_baskets.yaml
> Here you do not specify collectionFormat: csv while you do at
> acquisitions_funds.yaml.
> And again you dont in suggestions.yaml.
> Note that as I understand that it is the default array format, we should not
> need them. But this looks inconsistent.

Good catch, all sorted in the follow-up.

> [3] Please fix t/Koha/REST/Plugin/Query.t
> It fails now.

Another good catch, I somehow missed this test entirely when I wrote this.  Follow-up cleans up the test a little.. as we no longer validate in this method I dropped the validation side of the test and focused instead on just the structure testing.  Validation is however now tested in the test I added above at the API level.
Comment 27 Marcel de Rooy 2022-05-03 11:11:39 UTC
Created attachment 134506 [details] [review]
Bug 30536: Unit tests

This patch adds tests to make sure there's no behavior change regarding
error conditions. When requests include wrong x-koha-embed values, a 400
error should be returned, both in our original implementation or relying
on the Mojolicious::Plugin::OpenAPI features.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/query.t
=> SUCCESS: Tests pass
3. Apply the rest of the patches
4. Repeat 2
=> SUCCESS: Tests still pass
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2022-05-03 11:11:45 UTC
Created attachment 134507 [details] [review]
Bug 30536: Refer to parameters enum

This patch updates the stash_embed method to use the parameters hash
enum instead of the top level x-koha-embed custom definition. This way
we can remove the requirement for defining this list twice.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2022-05-03 11:11:52 UTC
Created attachment 134508 [details] [review]
Bug 30536: Remove validation overhead

With the introduction of proper validation of collection headers in
OpenAPI we no longer need to do our own validation here.

This patch removes the early validation of x-koha-embed headers in
preference to letting OpenAPI do it once and only once instead.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2022-05-03 11:11:57 UTC
Created attachment 134509 [details] [review]
Bug 30536: Update spec files

This patch removes superflous x-koha-embed defintions at the top level
of the endpoint specifications. It also replaces a few x-koha-embeds at
the top level with parameter lists where this had been missed in
preceeding patches.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2022-05-03 11:12:02 UTC
Created attachment 134510 [details] [review]
Bug 30536: (QA follow-up) POD + Spec Consistency

Well spotted, this patch fixes the specs to be consistent and adds a
little detail to the POD as requested.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-05-03 11:12:09 UTC
Created attachment 134511 [details] [review]
Bug 30536: Fix Query.t Unit Test

The stash_embed method no longer validates the query so this patch drops
the validation tests and instead just focuses on varifying the correct
output structure of the stash after the method call.

We have not lost validation however, it now happens only once at the
OpenAPI level and tests are present in t/db_dependent/api/v1/query.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Fridolin Somers 2022-05-03 21:27:56 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 34 Jonathan Druart 2023-01-31 11:40:44 UTC
> (2) path header parameters definition for x-koha-embed as an enum

I totally missed that, and regarding the number of occurrences, I am not the only one.

Is this a list that need to be updated?

% git grep x-koha-embed api/v1/swagger/paths/|grep -v 'name: x-koha-embed'
api/v1/swagger/paths/biblios_item_groups.yaml:    x-koha-embed:
api/v1/swagger/paths/biblios_item_groups.yaml:    x-koha-embed:
api/v1/swagger/paths/biblios_item_groups.yaml:    x-koha-embed:
api/v1/swagger/paths/biblios_item_groups.yaml:    x-koha-embed:
api/v1/swagger/paths/checkouts.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_agreements.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_agreements.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_packages.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_packages.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_packages.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_packages_resources.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_titles.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_titles.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_eholdings_titles_resources.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_licenses.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_licenses.yaml:    x-koha-embed:
api/v1/swagger/paths/erm_users.yaml:    x-koha-embed:
api/v1/swagger/paths/items.yaml:    x-koha-embed:
Comment 35 Martin Renvoize 2023-01-31 12:01:41 UTC
I spotted this the other day too and was surprised to see it had come out of sync so quickly.

Yes, we should update the guidelines and perhaps even add a QA script to prevent new occurrences.
Comment 36 Jonathan Druart 2023-02-09 09:35:04 UTC
I opened bug 30536.