Bug 33556 - $c->validation should be avoided (part 1)
Summary: $c->validation should be avoided (part 1)
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 33852 33974
Blocks: 34339
  Show dependency treegraph
 
Reported: 2023-04-18 12:32 UTC by Tomás Cohen Arazi
Modified: 2023-08-28 10:06 UTC (History)
8 users (show)

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


Attachments
Bug 33556: Path parameters are handled explicitly in the controllers (2.10 KB, patch)
2023-06-08 16:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33556: Avoid relying on $c->validation (82.62 KB, patch)
2023-06-08 16:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33556: Easy ERM-related changes (18.89 KB, patch)
2023-06-08 16:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33556: Path parameters are handled explicitly in the controllers (2.16 KB, patch)
2023-06-26 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33556: Avoid relying on $c->validation (82.61 KB, patch)
2023-06-26 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33556: Easy ERM-related changes (18.93 KB, patch)
2023-06-26 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33556: Path parameters are handled explicitly in the controllers (1.87 KB, patch)
2023-07-20 15:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33556: Avoid relying on $c->validation (85.85 KB, patch)
2023-07-20 15:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33556: Easy ERM-related changes (18.96 KB, patch)
2023-07-20 15:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33556: Path parameters are handled explicitly in the controllers (1.95 KB, patch)
2023-07-21 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33556: Avoid relying on $c->validation (85.92 KB, patch)
2023-07-21 13:09 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 Tomás Cohen Arazi 2023-04-18 12:32:15 UTC
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.
Comment 1 Tomás Cohen Arazi 2023-06-08 16:30:44 UTC
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.
Comment 2 Tomás Cohen Arazi 2023-06-08 16:30:47 UTC
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
Comment 3 Tomás Cohen Arazi 2023-06-08 16:30:50 UTC
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.
Comment 4 Tomás Cohen Arazi 2023-06-08 16:38:10 UTC
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.
Comment 5 Jonathan Druart 2023-06-09 07:21:12 UTC
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.
Comment 6 Martin Renvoize 2023-06-26 14:39:21 UTC
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>
Comment 7 Martin Renvoize 2023-06-26 14:39:24 UTC
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>
Comment 8 Martin Renvoize 2023-06-26 14:39:27 UTC
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>
Comment 9 Martin Renvoize 2023-06-26 14:40:31 UTC
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?
Comment 10 Martin Renvoize 2023-06-26 14:43:34 UTC
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.
Comment 11 Martin Renvoize 2023-07-03 13:51:50 UTC
(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
Comment 12 Jonathan Druart 2023-07-07 08:20:34 UTC
(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.
Comment 13 Marcel de Rooy 2023-07-14 06:19:27 UTC
We are failing patches in qa tools with a reference to a patch that does not even apply???
Comment 14 Jonathan Druart 2023-07-15 09:47:07 UTC
(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.
Comment 15 Tomás Cohen Arazi 2023-07-20 15:28:08 UTC
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>
Comment 16 Tomás Cohen Arazi 2023-07-20 15:28:12 UTC
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>
Comment 17 Tomás Cohen Arazi 2023-07-20 15:28:15 UTC
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>
Comment 18 Tomás Cohen Arazi 2023-07-20 15:30:06 UTC
Minor rebase against bug 33974
Comment 19 Marcel de Rooy 2023-07-21 06:58:54 UTC
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.
Comment 20 Marcel de Rooy 2023-07-21 06:59:15 UTC
Please clarify
Comment 21 Tomás Cohen Arazi 2023-07-21 12:48:11 UTC
(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.
Comment 22 Jonathan Druart 2023-07-21 13:09:03 UTC
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>
Comment 23 Jonathan Druart 2023-07-21 13:09:57 UTC
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>
Comment 24 Jonathan Druart 2023-07-21 13:14:23 UTC
(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.
Comment 25 Tomás Cohen Arazi 2023-07-21 13:15:29 UTC
(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
Comment 26 Tomás Cohen Arazi 2023-07-21 13:21:15 UTC
(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.
Comment 27 Jonathan Druart 2023-07-21 13:42:47 UTC
(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.
Comment 28 Tomás Cohen Arazi 2023-07-21 13:54:11 UTC
(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.
Comment 29 Tomás Cohen Arazi 2023-07-21 20:15:24 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 30 Fridolin Somers 2023-08-04 02:02:14 UTC
Pushed to 23.05.x for 23.05.03
Comment 31 Pedro Amorim 2023-08-28 10:06:49 UTC
Does not apply to 22.11.x. Not pushing.