Bug 33080 - Add helpers that return result_set for further processing
Summary: Add helpers that return result_set for further processing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 32734 32735
  Show dependency treegraph
 
Reported: 2023-02-27 19:28 UTC by Agustín Moyano
Modified: 2023-12-28 20:43 UTC (History)
6 users (show)

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


Attachments
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (12.13 KB, patch)
2023-02-27 19:38 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (12.20 KB, patch)
2023-03-02 17:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: (follow-up) Minor cleanups (4.54 KB, patch)
2023-03-02 17:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: (follow-up) Move pagination logic out of search_rs (1.94 KB, patch)
2023-03-03 17:36 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (12.20 KB, patch)
2023-03-03 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33080: (follow-up) Minor cleanups (3.98 KB, patch)
2023-03-03 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33080: Add small warpper helper to call add_pagination_headers (2.97 KB, patch)
2023-03-03 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (12.20 KB, patch)
2023-03-06 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: (follow-up) Minor cleanups (4.03 KB, patch)
2023-03-06 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: Add small warpper helper to call add_pagination_headers (3.03 KB, patch)
2023-03-06 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (13.65 KB, patch)
2023-03-07 11:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33080: Allow pagination to be built from stashed values (20.67 KB, patch)
2023-03-07 11:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33080: Adapt other use-cases (3.90 KB, patch)
2023-03-07 11:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (13.64 KB, patch)
2023-03-09 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: Allow pagination to be built from stashed values (22.83 KB, patch)
2023-03-09 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: Adapt other use-cases (3.96 KB, patch)
2023-03-09 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api (13.70 KB, patch)
2023-03-10 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33080: Allow pagination to be built from stashed values (22.88 KB, patch)
2023-03-10 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33080: Adapt other use-cases (4.01 KB, patch)
2023-03-10 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2023-02-27 19:28:28 UTC
Add "search_rs" and "find_rs" helpers to REST API, which retrieves a result set with all search queries and attributes applied.
Comment 1 Agustín Moyano 2023-02-27 19:38:40 UTC
Created attachment 147469 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t
Comment 2 Martin Renvoize 2023-03-02 17:25:28 UTC
Created attachment 147662 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2023-03-02 17:25:31 UTC
Created attachment 147663 [details] [review]
Bug 33080: (follow-up) Minor cleanups

Whilst reviewing I spotted a few calls to the stash that were no longer
required and errors in the POD for the new/updated methods

This patch adds fixes for those minor issues.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2023-03-02 17:30:22 UTC
Comment on attachment 147662 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

Review of attachment 147662 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/Plugin/Objects.pm
@@ +243,3 @@
>  
>              # Perform search
>              my $objects = $result_set->search( $filtered_params, $attributes );

I think we need to rework the pagination handling a little for this change.  We can't really keep it inside this method now that it returns a resultset that we're expecting developers to then further filter and refine.

I think we should update the add_pagination_headers helper to accept a resultset.. it can then build the total and base_total fields itself by executing the count and looking at the pager in the resultset passed.

We could then call this automatically in the non-rs search header above and document that it should be called by the developer in their controller after they've applied their own additional filtering etc in the controller if they're using the _rs helper.

For this.. I'm failing QA.. But I do really like where this is headed.
Comment 5 Martin Renvoize 2023-03-03 15:55:10 UTC
I've reviewed the follow-up on the branch Tomas sent me to.. more than happy to signoff on the set now :)

Great work
Comment 6 Agustín Moyano 2023-03-03 17:32:51 UTC
(In reply to Martin Renvoize from comment #4)
> Comment on attachment 147662 [details] [review] [review]
> Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api
> 
> Review of attachment 147662 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/Plugin/Objects.pm
> @@ +243,3 @@
> >  
> >              # Perform search
> >              my $objects = $result_set->search( $filtered_params, $attributes );
> 
> I think we need to rework the pagination handling a little for this change. 
> We can't really keep it inside this method now that it returns a resultset
> that we're expecting developers to then further filter and refine.
> 
> I think we should update the add_pagination_headers helper to accept a
> resultset.. it can then build the total and base_total fields itself by
> executing the count and looking at the pager in the resultset passed.
> 
> We could then call this automatically in the non-rs search header above and
> document that it should be called by the developer in their controller after
> they've applied their own additional filtering etc in the controller if
> they're using the _rs helper.
> 
> For this.. I'm failing QA.. But I do really like where this is headed.

Hi Martin, thanks for pointing this out...

I do not believe we should change the add_pagination_headers helper, it really wouldn't change much of how we should call it. And it's actually called by some controllers, so we should change those also.

Now we are calling it with base_total, total, and params... but if we change it to resultsets we should call it with filtered_resultset (to get total), unfiltered_resultset (to get base_total), and params.

What I'll do is remove the add_pagination_headers call from objects->search_rs as you mentioned, and put that in objects->search instead.

Cheers
Comment 7 Agustín Moyano 2023-03-03 17:36:27 UTC
Created attachment 147720 [details] [review]
Bug 33080: (follow-up) Move pagination logic out of search_rs
Comment 8 Kyle M Hall 2023-03-03 18:13:19 UTC
Global symbol "$args" requires explicit package name (did you forget to declare "my $args"?) at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 128.
Comment 9 Agustín Moyano 2023-03-03 18:18:53 UTC
Sorry, made it in a hurry... And then Tomas told me he was actually working on this with a different approach. So I'll just leave it to him to delete my patch.
Comment 10 Tomás Cohen Arazi 2023-03-03 18:20:21 UTC
(In reply to Agustín Moyano from comment #9)
> Sorry, made it in a hurry... And then Tomas told me he was actually working
> on this with a different approach. So I'll just leave it to him to delete my
> patch.

Yeah, I've worked on this on the referenced repo/branch. See URL. 

But right now I'm struggling with the ERM endpoints, which do funky use of the pagination headers and I'm not finding unit tests to make sure I don't introduce regressions.
Comment 11 Agustín Moyano 2023-03-03 18:31:36 UTC
Hm... maybe you should leave add_pagination_headers as is, and create a new helper called something like 'calc_pagination_numbers' that creates total numbers and calls add_pagination_headers somehow
Comment 12 Tomás Cohen Arazi 2023-03-03 18:37:14 UTC
(In reply to Agustín Moyano from comment #11)
> Hm... maybe you should leave add_pagination_headers as is, and create a new
> helper called something like 'calc_pagination_numbers' that creates total
> numbers and calls add_pagination_headers somehow

I'm pondering an intermediate solution in which parameters can be passed as now, but there's a fallback to using the stashed values. To put it clear, the use case that is requiring this is: we need the base total available in objects.search to be passed to add_pagination_headers later.
Comment 13 Tomás Cohen Arazi 2023-03-03 19:57:24 UTC
Created attachment 147734 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Tomás Cohen Arazi 2023-03-03 19:57:28 UTC
Created attachment 147735 [details] [review]
Bug 33080: (follow-up) Minor cleanups

Whilst reviewing I spotted a few calls to the stash that were no longer
required and errors in the POD for the new/updated methods

This patch adds fixes for those minor issues.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Tomás Cohen Arazi 2023-03-03 19:57:31 UTC
Created attachment 147736 [details] [review]
Bug 33080: Add small warpper helper to call add_pagination_headers

This patch introduces a small helper that uses the stashed values in
`search_rs` to properly build the pagination headers based on the
resultset.

To test:
1. Run all the API tests and make sure nothing breaks
2. Run:
   $ ktd --shell
   $ t/db_dependent/Koha/REST/Plugin/Pagination.t
=> SUCCESS: Tests pass!
Comment 16 Martin Renvoize 2023-03-06 14:39:40 UTC
Created attachment 147788 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2023-03-06 14:39:43 UTC
Created attachment 147789 [details] [review]
Bug 33080: (follow-up) Minor cleanups

Whilst reviewing I spotted a few calls to the stash that were no longer
required and errors in the POD for the new/updated methods

This patch adds fixes for those minor issues.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Martin Renvoize 2023-03-06 14:39:46 UTC
Created attachment 147790 [details] [review]
Bug 33080: Add small warpper helper to call add_pagination_headers

This patch introduces a small helper that uses the stashed values in
`search_rs` to properly build the pagination headers based on the
resultset.

To test:
1. Run all the API tests and make sure nothing breaks
2. Run:
   $ ktd --shell
   $ t/db_dependent/Koha/REST/Plugin/Pagination.t
=> SUCCESS: Tests pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-03-06 14:53:25 UTC
Hmm.. well I liked where the branch was going.. but I wasn't aware of the additional calls we're already making to add_pagination_headers as yet so wasn't aware it was a bit of a faff.

However, now I've just reviewed this final patch it feels like a few bits have been missed from the commit.

The POD calls a helper `objects.total_from_resultset` but then defines it as `objects.generate_pagination_headers` and inside that new helper we call the non-existing `total_from_resultset`.. that feels like it should just be $result_set->count anyway.. but I'm also not entirely clear on the naming of `add_pagination_headers` and `generate_pagination_headers`.. that sort of feel like synonyms.. I could see it getting confusing down the line.

Would you like me to step in and lend a hand here Tomas?
Comment 20 Tomás Cohen Arazi 2023-03-07 11:42:06 UTC
Created attachment 147823 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Tomás Cohen Arazi 2023-03-07 11:42:09 UTC
Created attachment 147824 [details] [review]
Bug 33080: Allow pagination to be built from stashed values

The way the old `objects.search` was build implied several totals were
calculated there, and passed to the `$c->add_pagination_headers` helper.

With the introduction of `objects.search_rs` and the ability of doing
things to the resultset afterwards, it felt like out of place to have
the pagination headers be implicitly set inside `objects.search_rs`.

This patch makes the search_rs stash some required values (from the original request) and
makes `add_pagination_headers` accept those values. This way other
callers can still build their own pagination values, while allowing this
simplified implementation.

Full-stack tests still pass, and the helper tests are moved to the
`db_dependent` section and are now more meaningful as well

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
   $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/api/v1/
=> SUCCESS: Tests pass! Nothing broken!
3. Sign off :-D
Comment 22 Tomás Cohen Arazi 2023-03-07 11:42:12 UTC
Created attachment 147825 [details] [review]
Bug 33080: Adapt other use-cases

This patch adapts the other controllers that rely on
add_pagination_headers outside of `objects.search`.

To test:
1. Apply this patchset
2. Run:
   $ ktd --shell
   $ prove t/db_dependent/api/v1/* \
           t/db_dependent/Koha/REST/Plugin/Objects.t \
           t/db_dependent/Koha/REST/Plugin/Pagination.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 23 Martin Renvoize 2023-03-09 17:26:40 UTC
Created attachment 148020 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2023-03-09 17:26:43 UTC
Created attachment 148021 [details] [review]
Bug 33080: Allow pagination to be built from stashed values

The way the old `objects.search` was build implied several totals were
calculated there, and passed to the `$c->add_pagination_headers` helper.

With the introduction of `objects.search_rs` and the ability of doing
things to the resultset afterwards, it felt like out of place to have
the pagination headers be implicitly set inside `objects.search_rs`.

This patch makes the search_rs stash some required values (from the original request) and
makes `add_pagination_headers` accept those values. This way other
callers can still build their own pagination values, while allowing this
simplified implementation.

Full-stack tests still pass, and the helper tests are moved to the
`db_dependent` section and are now more meaningful as well

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
   $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/api/v1/
=> SUCCESS: Tests pass! Nothing broken!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2023-03-09 17:26:46 UTC
Created attachment 148022 [details] [review]
Bug 33080: Adapt other use-cases

This patch adapts the other controllers that rely on
add_pagination_headers outside of `objects.search`.

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2023-03-09 17:27:16 UTC
All working fantastically and feels much cleaner.

Signing off.
Comment 27 Kyle M Hall 2023-03-10 14:24:13 UTC
Created attachment 148071 [details] [review]
Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api

This patch introduces the mentioned helpers, and reimplements
objects.search and objects.find in terms of them.

To test:
1. Apply patch
2. restart_all
3. Check that any API that uses objects.search helper is still running
   (GET /api/v1/items, GET /api/v1/holds, etc)
4. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2023-03-10 14:24:21 UTC
Created attachment 148072 [details] [review]
Bug 33080: Allow pagination to be built from stashed values

The way the old `objects.search` was build implied several totals were
calculated there, and passed to the `$c->add_pagination_headers` helper.

With the introduction of `objects.search_rs` and the ability of doing
things to the resultset afterwards, it felt like out of place to have
the pagination headers be implicitly set inside `objects.search_rs`.

This patch makes the search_rs stash some required values (from the original request) and
makes `add_pagination_headers` accept those values. This way other
callers can still build their own pagination values, while allowing this
simplified implementation.

Full-stack tests still pass, and the helper tests are moved to the
`db_dependent` section and are now more meaningful as well

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
   $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/api/v1/
=> SUCCESS: Tests pass! Nothing broken!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2023-03-10 14:24:23 UTC
Created attachment 148073 [details] [review]
Bug 33080: Adapt other use-cases

This patch adapts the other controllers that rely on
add_pagination_headers outside of `objects.search`.

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Tomás Cohen Arazi 2023-03-10 15:03:51 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 31 Matt Blenkinsop 2023-03-13 11:07:30 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!
Comment 32 Martin Renvoize 2023-03-21 07:43:13 UTC
Required by bug 32734 which was backported already.. backporting for 22.11.x will be in 22.11.04
Comment 33 Caroline Cyr La Rose 2023-05-12 13:20:19 UTC
This seems like backend stuff only, nothing to document. Closing, but feel free to reopen with more details if I'm wrong.