Bug 38226 - Add RPC endpoint for batch filling empty callnumbers
Summary: Add RPC endpoint for batch filling empty callnumbers
Status: In Discussion
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: Testopia
URL:
Keywords:
Depends on:
Blocks: 34784 38224
  Show dependency treegraph
 
Reported: 2024-10-22 12:40 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-01-29 23:56 UTC (History)
7 users (show)

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


Attachments
Bug 38226: API spec (6.04 KB, patch)
2024-10-22 12:41 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38226: Add RPC route for updating item callnumbers for a biblio (10.16 KB, patch)
2024-10-22 12:42 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38226: API spec (6.10 KB, patch)
2024-11-07 14:43 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38226: Add RPC route for updating item callnumbers for a biblio (10.21 KB, patch)
2024-11-07 14:43 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38226: API spec (6.10 KB, patch)
2025-01-29 12:56 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38226: Add RPC route for updating item callnumbers for a biblio (10.76 KB, patch)
2025-01-29 12:56 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 Tomás Cohen Arazi (tcohen) 2024-10-22 12:40:03 UTC
Originally proposed on bug 34784, I'm moving the patches here to ease readability and have a smaller testing scope.
Comment 1 Tomás Cohen Arazi (tcohen) 2024-10-22 12:41:57 UTC
Created attachment 173132 [details] [review]
Bug 38226: API spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2024-10-22 12:42:00 UTC
Created attachment 173133 [details] [review]
Bug 38226: Add RPC route for updating item callnumbers for a biblio

This patch takes the idea implemented on the original patch, moves it to
the /rpc namespace to make it clear this is not a RESTful endpoint.

Code gets improved a bit, using some patterns we are using nowadays.
The return value gets a bit more information and.

A big change is it now uses `Koha::Items->batch_update`. It originally
used `Koha::Items->update` with the `no_trigger` option. It felt like
`batch_update` is a bit more smart in terms of triggering indexing at
the end of the update, and only for the host biblio.

It looks like that was the spirit of the original implementation, but
missed to actually trigger indexing at the end of the call.

To test:
1. Apply this patches
2.Run:
   $ ktd --shell
  k$ yarn api:bundle
  k$ restart_all
3. Run the tests:
  k$ prove t/db_dependent/api/v1/rpc/biblios.t
=> SUCCESS: Tests pass!
4. Try the new routes with Postman or your favourite tool (if they don't
   match)
=> SUCCESS: The desired behavior is what you get.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Nick Clemens (kidclamp) 2024-11-07 14:43:05 UTC
Created attachment 174242 [details] [review]
Bug 38226: API spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens (kidclamp) 2024-11-07 14:43:07 UTC
Created attachment 174243 [details] [review]
Bug 38226: Add RPC route for updating item callnumbers for a biblio

This patch takes the idea implemented on the original patch, moves it to
the /rpc namespace to make it clear this is not a RESTful endpoint.

Code gets improved a bit, using some patterns we are using nowadays.
The return value gets a bit more information and.

A big change is it now uses `Koha::Items->batch_update`. It originally
used `Koha::Items->update` with the `no_trigger` option. It felt like
`batch_update` is a bit more smart in terms of triggering indexing at
the end of the update, and only for the host biblio.

It looks like that was the spirit of the original implementation, but
missed to actually trigger indexing at the end of the call.

To test:
1. Apply this patches
2.Run:
   $ ktd --shell
  k$ yarn api:bundle
  k$ restart_all
3. Run the tests:
  k$ prove t/db_dependent/api/v1/rpc/biblios.t
=> SUCCESS: Tests pass!
4. Try the new routes with Postman or your favourite tool (if they don't
   match)
=> SUCCESS: The desired behavior is what you get.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Jonathan Druart 2025-01-16 13:13:58 UTC
This /rpc namespace needs to be documented.
We should make sure it does not become a mess quickly because it would be easier to write such route than think of a good REST API pattern.
Comment 6 Jonathan Druart 2025-01-16 13:25:09 UTC
curl -u koha:koha http://localhost:8081/rpc/biblios/1/items/1/populate_empty_callnumbers

It gets a 404, what am I missing?
Comment 7 Jonathan Druart 2025-01-28 15:39:28 UTC
Adjusting status to get attention.
Comment 8 David Cook 2025-01-29 00:38:22 UTC
(In reply to Jonathan Druart from comment #6)
> curl -u koha:koha
> http://localhost:8081/rpc/biblios/1/items/1/populate_empty_callnumbers
> 
> It gets a 404, what am I missing?

Wouldn't you still need /api/v1/ in front of /rpc?
Comment 9 David Cook 2025-01-29 00:40:50 UTC
(In reply to Jonathan Druart from comment #5)
> This /rpc namespace needs to be documented.
> We should make sure it does not become a mess quickly because it would be
> easier to write such route than think of a good REST API pattern.

Yeah, sounds like this needs some thought. I'm not sure about a /rpc namespace...

I think we're a bit overly dogmatic sometimes with our REST API, but yeah good to think about API usage.
Comment 10 Jonathan Druart 2025-01-29 08:17:59 UTC
(In reply to David Cook from comment #8)
> (In reply to Jonathan Druart from comment #6)
> > curl -u koha:koha
> > http://localhost:8081/rpc/biblios/1/items/1/populate_empty_callnumbers
> > 
> > It gets a 404, what am I missing?
> 
> Wouldn't you still need /api/v1/ in front of /rpc?

Yes, sorry, I guess I was trying stuffs, the main problem was the method, it's a POST.

% curl -X POST -u koha:koha http://localhost:8081/api/v1/rpc/biblios/1/items/1/populate_empty_callnumbers

This is working.


However, this is wrong IMO:
%curl -X POST -u koha:koha http://localhost:8081/api/v1/rpc/biblios/1/items/99/populate_empty_callnumbers
{"error":"Callnumber fields not found","error_code":"missing_configuration"}

There is no item_id=99 for biblio_id=1. Shouldn't we return 404 instead?


Adjusting the status regarding the last comments.
Comment 11 Tomás Cohen Arazi (tcohen) 2025-01-29 12:09:26 UTC
Hi all. I picked the RPC keyword only because I wanted a namespace that makes it clear that endpoints defined in it are not RESTful.

I don't see an issue with having a non-RESTful portion of the API for special uses (I'm actually 100% onboard with that), as not everything falls into the CRUD pattern. As an example of this, I'm sure we shouldn't be using the CRUD endpoints for displaying dropdowns. It makes endpoints permissions requirements a real mess e.g. why do you need to access the full library objects in order to display a list of library code/name? Library information is *almost* trivial and not important, but think of other resources.

In another context I proposed we made this batch-like endpoints just be `POST /jobs` (or `POST /jobs/batch_cancel_holds`) and this could be better case here. We just need a pattern to follow.

That said, the 'RPC nature' on this two introduced endpoints is the fact there's a verb in the path: 'populate_empty_callnumbers'.

This particular endpoint is self-documented so I assume Jonathan refers to the guidelines this time. I agree we should document it, if we decide to go this path. As I said, I'd prefer some sort of `POST /jobs` but it would require the consumer to retrieve the job results, etc. Unless we use a pseudo job that (i.e. nothing is queued, etc).

(In reply to Jonathan Druart from comment #10)
>
> %curl -X POST -u koha:koha
> http://localhost:8081/api/v1/rpc/biblios/1/items/99/
> populate_empty_callnumbers
> {"error":"Callnumber fields not found","error_code":"missing_configuration"}
> 
> There is no item_id=99 for biblio_id=1. Shouldn't we return 404 instead?

That's a bug. Thanks for taking the time to test it :-D
Comment 12 Tomás Cohen Arazi (tcohen) 2025-01-29 12:56:53 UTC
Created attachment 177313 [details] [review]
Bug 38226: API spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Tomás Cohen Arazi (tcohen) 2025-01-29 12:56:55 UTC
Created attachment 177314 [details] [review]
Bug 38226: Add RPC route for updating item callnumbers for a biblio

This patch takes the idea implemented on the original patch, moves it to
the /rpc namespace to make it clear this is not a RESTful endpoint.

Code gets improved a bit, using some patterns we are using nowadays.
The return value gets a bit more information and.

A big change is it now uses `Koha::Items->batch_update`. It originally
used `Koha::Items->update` with the `no_trigger` option. It felt like
`batch_update` is a bit more smart in terms of triggering indexing at
the end of the update, and only for the host biblio.

It looks like that was the spirit of the original implementation, but
missed to actually trigger indexing at the end of the call.

To test:
1. Apply this patches
2.Run:
   $ ktd --shell
  k$ yarn api:bundle
  k$ restart_all
3. Run the tests:
  k$ prove t/db_dependent/api/v1/rpc/biblios.t
=> SUCCESS: Tests pass!
4. Try the new routes with Postman or your favourite tool (if they don't
   match)
=> SUCCESS: The desired behavior is what you get.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Tomás Cohen Arazi (tcohen) 2025-01-29 12:57:44 UTC
(In reply to Jonathan Druart from comment #10)
> 
> However, this is wrong IMO:
> %curl -X POST -u koha:koha
> http://localhost:8081/api/v1/rpc/biblios/1/items/99/
> populate_empty_callnumbers
> {"error":"Callnumber fields not found","error_code":"missing_configuration"}
> 
> There is no item_id=99 for biblio_id=1. Shouldn't we return 404 instead?

Solved!
Comment 15 David Cook 2025-01-29 23:51:40 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #11)
> Hi all. I picked the RPC keyword only because I wanted a namespace that
> makes it clear that endpoints defined in it are not RESTful.
> 
> I don't see an issue with having a non-RESTful portion of the API for
> special uses (I'm actually 100% onboard with that), as not everything falls
> into the CRUD pattern. As an example of this, I'm sure we shouldn't be using
> the CRUD endpoints for displaying dropdowns. It makes endpoints permissions
> requirements a real mess e.g. why do you need to access the full library
> objects in order to display a list of library code/name? Library information
> is *almost* trivial and not important, but think of other resources.
> 
> In another context I proposed we made this batch-like endpoints just be
> `POST /jobs` (or `POST /jobs/batch_cancel_holds`) and this could be better
> case here. We just need a pattern to follow.
> 
> That said, the 'RPC nature' on this two introduced endpoints is the fact
> there's a verb in the path: 'populate_empty_callnumbers'.
> 
> This particular endpoint is self-documented so I assume Jonathan refers to
> the guidelines this time. I agree we should document it, if we decide to go
> this path. As I said, I'd prefer some sort of `POST /jobs` but it would
> require the consumer to retrieve the job results, etc. Unless we use a
> pseudo job that (i.e. nothing is queued, etc).

I've found the thread on Mattermost quite interesting! Based on this comment of yours, I think that you and I have come to the same conclusion, which is that it boils down to 3 choices:

1. Try to force things into REST semantics (e.g. POST /jobs or POST /jobs/batch_populate_empty_callnumbers or PUT /batch_record_modification with parameters in POSTDATA)
2. Extend/hybridize REST endpoints with actions in the path (e.g. POST /api/v1/biblios/{biblio_id}/items/populate_empty_callnumbers)
3. Create a RPC endpoint (POST /api/v1/rpc/biblios/{biblio_id}/items/populate_empty_callnumbers or POST /api/v1/rpc with parameters in POSTDATA)
#NOTE: I think that gRPC uses paths but JSON-RPC does not use paths. Depends on the standard or lack of standard used...

Technically, we actually already do the 2nd option quite a bit. Here's a few examples:
POST /api/v1/return_claims
POST /api/v1/erm/usage_data_providers/{erm_usage_data_provider_id}/process_COUNTER_file
POST /api/v1/holds/{hold_id}/suspension
PUT /api/v1/import_batches/{import_batch_id}/records/{import_record_id}/matches/chosen

I think we're worried about /api/v1/rpc becoming the wild west, but it seems like we're already in a bit of a wild west scenario with the "REST" API.

REST is great for CRUD, but Koha - server side - does a lot more than just CRUD. 

We're not expecting the REST API consumer to do processing. We're expecting Koha to do the processing, which means we need to support performing actions on the server by API somehow.

I don't think there's really a "right or wrong" here. At the moment, I think options 1 and 2 are probably the most practical, as they're already established practices in Koha.
Comment 16 David Cook 2025-01-29 23:56:04 UTC
That being said, I'm not opposed to /api/v1/rpc. 

If we do go that route, I think we'd want to (at some point) migrate our "extended/hybridized REST endpoints with actions in the path" to the /api/v1/rpc and keep /api/v1/{not rpc} strictly for CRUD-style REST. At the very least forbid new non-CRUD REST endpoints and require RPC endpoints for actions going forward.

It does make for a simple visual grouping of functionality.