Bug 40417

Summary: Search_rs is re-instating deleted query parameters
Product: Koha Reporter: Matt Blenkinsop <matt.blenkinsop>
Component: REST APIAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: Pushed to main --- QA Contact: Tomás Cohen Arazi (tcohen) <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: dcook, jonathan.druart, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/81
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:
Bug Depends on: 40424    
Bug Blocks:    
Attachments: Bug 40417: Regression test
Bug 40417: Regression test
Bug 40417: Remove 'validation' method
Bug 40417: Regression test
Bug 40417: Remove 'validation' method
Bug 40417: Regression test
Bug 40417: Remove 'validation' method
Bug 40417: Regression test
Bug 40417: Remove 'validation' method
Bug 40417: Regression test
Bug 40417: Remove 'validation' method

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