Bug 40417 - Search_rs is re-instating deleted query parameters
Summary: Search_rs is re-instating deleted query parameters
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: Matt Blenkinsop
QA Contact: Tomás Cohen Arazi (tcohen)
URL: https://gitlab.com/koha-community/qa-...
Keywords:
Depends on: 40424
Blocks:
  Show dependency treegraph
 
Reported: 2025-07-16 16:10 UTC by Matt Blenkinsop
Modified: 2025-07-23 22:42 UTC (History)
4 users (show)

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


Attachments
Bug 40417: Regression test (2.42 KB, patch)
2025-07-16 16:56 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40417: Regression test (2.35 KB, patch)
2025-07-16 17:46 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40417: Remove 'validation' method (848 bytes, patch)
2025-07-16 17:46 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40417: Regression test (2.39 KB, patch)
2025-07-17 15:44 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40417: Remove 'validation' method (1.11 KB, patch)
2025-07-17 15:44 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 40417: Regression test (2.44 KB, patch)
2025-07-18 09:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 40417: Remove 'validation' method (1.16 KB, patch)
2025-07-18 09:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 40417: Regression test (2.44 KB, patch)
2025-07-18 17:32 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40417: Remove 'validation' method (1.16 KB, patch)
2025-07-18 17:32 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40417: Regression test (2.49 KB, patch)
2025-07-18 17:42 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40417: Remove 'validation' method (1.21 KB, patch)
2025-07-18 17:42 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Blenkinsop 2025-07-16 16:10:07 UTC
Any endpoint that is calling $c->objects->search after calling $c->req->params->remove('param_name') is having that parameter reinstated to the search object
Comment 1 Matt Blenkinsop 2025-07-16 16:56:25 UTC
Created attachment 184155 [details] [review]
Bug 40417: Regression test
Comment 2 Matt Blenkinsop 2025-07-16 16:56:55 UTC
To reproduce, apply the regression test patch and run the test - it will fail
Comment 3 Matt Blenkinsop 2025-07-16 17:46:14 UTC
Created attachment 184160 [details] [review]
Bug 40417: Regression test
Comment 4 Matt Blenkinsop 2025-07-16 17:46:16 UTC
Created attachment 184161 [details] [review]
Bug 40417: Remove 'validation' method
Comment 5 Tomás Cohen Arazi (tcohen) 2025-07-17 03:09:05 UTC
This looks correct, but we need to deal with other things for this not to break existing features. Look at the dependencies I added.

BTW. I added

```
use DDP;

p($c->validation->output);
p($c->req->params->to_hash);
```

at the beginning of `objects.search_rs` and run:

```
prove t/db_dependent/api/v1/query.t
```

and verified one thing I mentioned yesterday: when a query parameter is passed multiple times, the calculated structure is the same (i.e. `$args`). When it is only one, they differ. This might not be a real issue, but it is worth making sure we have tests and it doesn't break anything.
Comment 6 Tomás Cohen Arazi (tcohen) 2025-07-17 15:33:59 UTC
This looks good. I added a PR on the QA tools repo so the QA script doesn't look for `$c->validation` in the test file, as we really want it there for the regression test.

Please enhance the commit message.
Comment 7 Matt Blenkinsop 2025-07-17 15:44:31 UTC
Created attachment 184212 [details] [review]
Bug 40417: Regression test

prove t/db_dependent/Koha/REST/Plugin/Objects.t
Comment 8 Matt Blenkinsop 2025-07-17 15:44:34 UTC
Created attachment 184213 [details] [review]
Bug 40417: Remove 'validation' method

We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made

Test plan:
1) Apply the regression test patch and run the test
2) Test should fail
3) Apply this patch
4) Run test again
5) Test should pass
Comment 9 David Nind 2025-07-18 09:39:37 UTC
Created attachment 184317 [details] [review]
Bug 40417: Regression test

prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-07-18 09:39:40 UTC
Created attachment 184318 [details] [review]
Bug 40417: Remove 'validation' method

We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made

Test plan:
1) Apply the regression test patch and run the test
2) Test should fail
3) Apply this patch
4) Run test again
5) Test should pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Tomás Cohen Arazi (tcohen) 2025-07-18 17:32:03 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi (tcohen) 2025-07-18 17:32:06 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi (tcohen) 2025-07-18 17:42:28 UTC
Created attachment 184409 [details] [review]
Bug 40417: Regression test

prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi (tcohen) 2025-07-18 17:42:31 UTC
Created attachment 184410 [details] [review]
Bug 40417: Remove 'validation' method

We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made

Test plan:
1) Apply the regression test patch and run the test
2) Test should fail
3) Apply this patch
4) Run test again
5) Test should pass

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi (tcohen) 2025-07-18 17:44:30 UTC
I PQA this.

* Code is correct
* Side effects from using ->validation covered by tests
* QA script happy (with the linked PR applied locally for the QA test tools)

Good catch Matthew and thanks for the patches!
Comment 16 Lucas Gass (lukeg) 2025-07-23 22:42:58 UTC
Nice work everyone!

Pushed to main for 25.11