Bug 36369 - Make APIClient be more useful
Summary: Make APIClient be more useful
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-20 14:20 UTC by Tomás Cohen Arazi
Modified: 2024-04-26 14:42 UTC (History)
9 users (show)

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


Attachments
Bug 36369: Introduce an APIClientBase class (7.40 KB, patch)
2024-04-01 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36369: Introduce an APIClientBase class (8.11 KB, patch)
2024-04-01 16:00 UTC, Tomás Cohen Arazi
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 2024-03-20 14:20:05 UTC
I file this report for discussion.

In my opinion, there's too much duplicated code on the API client libraries.
And the APIClient "class" is just a shortcut to avoid instantiating the module-specific API clients:

```javascript
import ERMAPIClient from "./erm-api-client";
...

export const APIClient = {
    erm: new ERMAPIClient(),
```

Then used everywhere like this:

```javascript
import { APIClient } from "../../fetch/api-client.js"
...
            const client = APIClient.erm
            await client.agreements.count().then(
```

IMHO, we need a real APIClient class that would implement generic methods like:

```javascript
    create: params =>
        this.post({
            endpoint: params.endpoint ? params.endpoint : "/",
            body: params.resource,
            headers: params.headers
        }),
    ...
    getAll: params =>
        this.get({
            endpoint: params.endpoint ? params.endpoint : "/",
            params.query,
            headers: params.headers,
        }),
...
    count: params =>
        this.count({
            endpoint: params.endpoint ? params.endpoint : "/",
                "?" +
                new URLSearchParams({
                    _page: 1,
                    _per_page: 1,
                    ...(params.query && { q: JSON.stringify(params.query) }),
                }),
        }),
```

and the API Client classes (like ERMAPIClient) should inherit from it, somehow. For doing so, we will face another design roadblock:

While I uderstand that ERMAPIClient is a single place to have all the ERM module-needed API actions, it still feels weird from an OOP perspective how things are organized.

I feel like we should have individual classes, like:

```javascript
export RecordSourcesAPIClient extends APIClient
```

and then have a new type of class along this lines:

```javascript
import CitiesAPIClient from "./cities-api-client";
import RecordSourcesAPIClient from "./record-sources-api-client";
...

export const AdminAPIClient = {
    cities: new CitiesAPIClient(),
    record_sources: new RecordSourcesAPIClient(),
    ...
};
```

this way, we will avoid repeating ourselves (less bugs!) and the code is (I expect) more reusable.

I also think the `embed` handling should just be a list for the different methods, like:

embed: [ 'patron', 'patron.category' ]

and generically handled by APIClient. No need to build the headers manually everywhere.
Comment 1 Pedro Amorim 2024-03-25 12:01:48 UTC
I think this makes sense.
Tomas are you able to provide a PoC we can use as a starting point to look at / discuss?
Comment 2 Kyle M Hall 2024-03-26 10:49:42 UTC
Is there any reason not to build the javascript version of our OpenAPI spec and use that internally?
Comment 3 Tomás Cohen Arazi 2024-03-26 11:20:08 UTC
(In reply to Pedro Amorim from comment #1)
> I think this makes sense.
> Tomas are you able to provide a PoC we can use as a starting point to look
> at / discuss?

Yes, it is my plan for this morning.
Comment 4 Tomás Cohen Arazi 2024-03-26 12:30:36 UTC
(In reply to Kyle M Hall from comment #2)
> Is there any reason not to build the javascript version of our OpenAPI spec
> and use that internally?

You mean for validating outgoing requests?
Comment 5 Kyle M Hall 2024-03-26 13:05:43 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> (In reply to Kyle M Hall from comment #2)
> > Is there any reason not to build the javascript version of our OpenAPI spec
> > and use that internally?
> 
> You mean for validating outgoing requests?

I just did some tests and at least the clients generated by https://editor.swagger.io/ do not validate data. Their are other SDK generators out there but this one at least doesn't do it.
Comment 6 Paul Derscheid 2024-03-26 15:17:33 UTC
If you want to validate input before passing it to api clients, please use zod or something similar.

That makes it very declarative and easy to maintain.

Here's a list of options compared by performance: https://moltar.github.io/typescript-runtime-type-benchmarks/
Comment 7 Tomás Cohen Arazi 2024-04-01 15:36:15 UTC
Created attachment 164201 [details] [review]
Bug 36369: Introduce an APIClientBase class
Comment 8 Tomás Cohen Arazi 2024-04-01 15:39:16 UTC
Sorry for the delay y'all. I didn't manage to make this work. But I think it highlights the idea of what I tried to propose.

api-client-base.js could be implemented as an http-client.js tweak, but I wanted to make it self-contained to ease explaining it. And also, I prefer we use the already implemented method names instead of plain HTTP verbs.

Happy to Meet/Jitsi/Zoom to talk about this if needed.

Please take this as a constructive review of the already great toolset y've all built.
Comment 9 Tomás Cohen Arazi 2024-04-01 16:00:19 UTC Comment hidden (obsolete)
Comment 10 Paul Derscheid 2024-04-01 20:01:11 UTC
I like it.
Comment 11 Jonathan Druart 2024-04-02 09:28:59 UTC
It could work to reduce the number of lines in the -api-client.js.

Two points however:
1. The endpoint need to be configurable. 'this._baseURL + "/" + id' is too simple and does not work for everything (eg. /patrons/{patron_id}/holds, /trains/{train_id}/items/{item_id})

2. So far we have decided to provide what is implemented/used. With this we will offer in the view usage of routes that may not be implemented yet.
Comment 12 Paul Derscheid 2024-04-02 09:36:46 UTC
I have implemented something similar before. In this case we can't have simplicity and flexibility. The more flexible the api-client-base is, the more complex it will become.

I think we should agree on the interface (and its quirks) first and then structure the base class around it.

Maybe a meeting like Tomas suggested would be helpful.
Comment 13 Tomás Cohen Arazi 2024-04-02 19:57:14 UTC
(In reply to Jonathan Druart from comment #11)
> It could work to reduce the number of lines in the -api-client.js.
> 
> Two points however:
> 1. The endpoint need to be configurable. 'this._baseURL + "/" + id' is too
> simple and does not work for everything (eg. /patrons/{patron_id}/holds,
> /trains/{train_id}/items/{item_id})

In my opinion, such cases should be handled, maybe with some more parameters to api-client-base. I totally get it and saw it in the code.

> 2. So far we have decided to provide what is implemented/used. With this we
> will offer in the view usage of routes that may not be implemented yet.

Am I right to interpret what you say like: 'if the Vue page doesn't allow deleting the resource, we shouldn't implement a delete() method'?

It feels to me that this shouldn't be an issue... and would allow people to reuse this more?
Comment 14 Jonathan Druart 2024-04-03 08:11:12 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> (In reply to Jonathan Druart from comment #11)
> > 2. So far we have decided to provide what is implemented/used. With this we
> > will offer in the view usage of routes that may not be implemented yet.
> 
> Am I right to interpret what you say like: 'if the Vue page doesn't allow
> deleting the resource, we shouldn't implement a delete() method'?
> 
> It feels to me that this shouldn't be an issue... and would allow people to
> reuse this more?

Yes it is what I meant for "used". And I agree it is not really a problem.
However it can be a problem for "implemented": If the endpoint does not exist in the controller should we provide it in the api-client? It does not make sense to me.
Comment 15 Tomás Cohen Arazi 2024-04-26 14:42:28 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Tomás Cohen Arazi from comment #13)
> > (In reply to Jonathan Druart from comment #11)
> > > 2. So far we have decided to provide what is implemented/used. With this we
> > > will offer in the view usage of routes that may not be implemented yet.
> > 
> > Am I right to interpret what you say like: 'if the Vue page doesn't allow
> > deleting the resource, we shouldn't implement a delete() method'?
> > 
> > It feels to me that this shouldn't be an issue... and would allow people to
> > reuse this more?
> 
> Yes it is what I meant for "used". And I agree it is not really a problem.
> However it can be a problem for "implemented": If the endpoint does not
> exist in the controller should we provide it in the api-client? It does not
> make sense to me.

I think it is fair that a dev can try, but then get a 404. I would like to hear from others.