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
*** Bug 30542 has been marked as a duplicate of this bug. ***
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.
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.
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?
Created attachment 133309 [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.
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.
Created attachment 133313 [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.
Created attachment 133314 [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.
Created attachment 133315 [details] [review] Bug 30536: Use objects->find in Cashups controller This patch removes the direct reference to the koha.embed stashed variable in the Cashups controller and switches us to the modern objects->find approach.
Created attachment 133316 [details] [review] Bug 30536: Use objects->find in SMTP Servers controller This patch removes the direct reference to the koha.embed stashed variable in the SMTP controller and switches us to the modern objects->find approach.
Created attachment 133324 [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.
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.
Marking as NSO to gather other's opinions
Created attachment 133327 [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>
Created attachment 133328 [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>
Created attachment 133329 [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>
Created attachment 133330 [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>
QAing
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.
Created attachment 134479 [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>
Created attachment 134480 [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>
Created attachment 134481 [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>
Created attachment 134482 [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>
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.
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
(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.
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>
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>
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>
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>
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>
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>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
> (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:
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.
I opened bug 30536.