Bug 40424 - Handling query-in-body relies on wrong data structure
Summary: Handling query-in-body relies on wrong data structure
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 40423
Blocks: 40417
  Show dependency treegraph
 
Reported: 2025-07-17 03:00 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-07-23 22:43 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 40424: Add missing tests for search in body (3.86 KB, patch)
2025-07-17 15:01 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40424: Simplify existing code - preparation (2.70 KB, patch)
2025-07-17 15:01 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40424: Make query-in-body rely on `$c->req->json` (2.99 KB, patch)
2025-07-17 15:01 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40424: Add missing tests for search in body (3.92 KB, patch)
2025-07-17 16:04 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40424: Simplify existing code - preparation (2.76 KB, patch)
2025-07-17 16:04 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40424: Make query-in-body rely on `$c->req->json` (3.05 KB, patch)
2025-07-17 16:04 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40424: Add missing tests for search in body (3.98 KB, patch)
2025-07-23 14:29 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40424: Simplify existing code - preparation (2.82 KB, patch)
2025-07-23 14:29 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40424: Make query-in-body rely on `$c->req->json` (3.12 KB, patch)
2025-07-23 14:29 UTC, Martin Renvoize (ashimema)
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 (tcohen) 2025-07-17 03:00:39 UTC
Similar to `x-koha-request-id` handling, it relies on `extract_reserved_params` and the use of `$c->validation->output`, which is incorrect.

We need tests for the feature.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-07-17 15:01:27 UTC
Created attachment 184208 [details] [review]
Bug 40424: Add missing tests for search in body
Comment 2 Tomás Cohen Arazi (tcohen) 2025-07-17 15:01:30 UTC
Created attachment 184209 [details] [review]
Bug 40424: Simplify existing code - preparation

I wanted to isolate the changes for the query-in-body handling and it
made sense to refactor this dupicated code at the same time. So I add
this patch for tidying the area in preparation for the other part.

Duplicated code gets simplified.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/Koha/REST/Plugin/Query.t \
           t/db_dependent/api/v1/*.t \
           t/db_dependent/Koha/REST/Plugin/
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 3 Tomás Cohen Arazi (tcohen) 2025-07-17 15:01:33 UTC
Created attachment 184210 [details] [review]
Bug 40424: Make query-in-body rely on `$c->req->json`

This patch removes the reliance on `$c->validation->output` for the
query-in-body feature. This will allow bug 40417 to move forward.

It also fixes some wrong design decisions we made 5-6 years ago. This
was discussed with the Mojolicious::Plugin::OpenAPI maintainer and the
agreement was `$c->validation->output` was not stable nor intended to be
accessed like we do.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/Koha/REST/Plugin/Query.t \
           t/db_dependent/api/v1/*.t \
           t/db_dependent/Koha/REST/Plugin/*
=> SUCCESS: Tests pass!
2. Apply this patchset
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D
Comment 4 Matt Blenkinsop 2025-07-17 16:04:14 UTC
Created attachment 184218 [details] [review]
Bug 40424: Add missing tests for search in body

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 5 Matt Blenkinsop 2025-07-17 16:04:17 UTC
Created attachment 184219 [details] [review]
Bug 40424: Simplify existing code - preparation

I wanted to isolate the changes for the query-in-body handling and it
made sense to refactor this dupicated code at the same time. So I add
this patch for tidying the area in preparation for the other part.

Duplicated code gets simplified.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/Koha/REST/Plugin/Query.t \
           t/db_dependent/api/v1/*.t \
           t/db_dependent/Koha/REST/Plugin/
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 6 Matt Blenkinsop 2025-07-17 16:04:19 UTC
Created attachment 184220 [details] [review]
Bug 40424: Make query-in-body rely on `$c->req->json`

This patch removes the reliance on `$c->validation->output` for the
query-in-body feature. This will allow bug 40417 to move forward.

It also fixes some wrong design decisions we made 5-6 years ago. This
was discussed with the Mojolicious::Plugin::OpenAPI maintainer and the
agreement was `$c->validation->output` was not stable nor intended to be
accessed like we do.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/Koha/REST/Plugin/Query.t \
           t/db_dependent/api/v1/*.t \
           t/db_dependent/Koha/REST/Plugin/*
=> SUCCESS: Tests pass!
2. Apply this patchset
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 7 Martin Renvoize (ashimema) 2025-07-23 14:29:23 UTC
Created attachment 184554 [details] [review]
Bug 40424: Add missing tests for search in body

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 8 Martin Renvoize (ashimema) 2025-07-23 14:29:25 UTC
Created attachment 184555 [details] [review]
Bug 40424: Simplify existing code - preparation

I wanted to isolate the changes for the query-in-body handling and it
made sense to refactor this dupicated code at the same time. So I add
this patch for tidying the area in preparation for the other part.

Duplicated code gets simplified.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/Koha/REST/Plugin/Query.t \
           t/db_dependent/api/v1/*.t \
           t/db_dependent/Koha/REST/Plugin/
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 9 Martin Renvoize (ashimema) 2025-07-23 14:29:28 UTC
Created attachment 184556 [details] [review]
Bug 40424: Make query-in-body rely on `$c->req->json`

This patch removes the reliance on `$c->validation->output` for the
query-in-body feature. This will allow bug 40417 to move forward.

It also fixes some wrong design decisions we made 5-6 years ago. This
was discussed with the Mojolicious::Plugin::OpenAPI maintainer and the
agreement was `$c->validation->output` was not stable nor intended to be
accessed like we do.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/Koha/REST/Plugin/Query.t \
           t/db_dependent/api/v1/*.t \
           t/db_dependent/Koha/REST/Plugin/*
=> SUCCESS: Tests pass!
2. Apply this patchset
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 10 Martin Renvoize (ashimema) 2025-07-23 14:30:03 UTC
Nice consistent approach here.
Comment 11 Lucas Gass (lukeg) 2025-07-23 22:43:27 UTC
Nice work everyone!

Pushed to main for 25.11