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 (tcohen)
Modified: 2025-05-29 12:07 UTC (History)
10 users (show)

See Also:
GIT URL:
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 (tcohen)
Details | Diff | Splinter Review
Bug 36369: Introduce an APIClientBase class (8.11 KB, patch)
2024-04-01 16:00 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-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 (khall) 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 (tcohen) 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 (tcohen) 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 (khall) 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 (tcohen) 2024-04-01 15:36:15 UTC
Created attachment 164201 [details] [review]
Bug 36369: Introduce an APIClientBase class
Comment 8 Tomás Cohen Arazi (tcohen) 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 (tcohen) 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 (tcohen) 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 (tcohen) 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.
Comment 16 Tomás Cohen Arazi (tcohen) 2025-05-28 13:54:10 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.

Maybe we can just code an override for not implemented verbs/endpoints that throws an exception. So devs don't get confused?

I'm willing to be back to this work/discussion.
Comment 17 Michał 2025-05-29 12:07:07 UTC
Imho if you want to re-do an API client, it would make a lot of sense to do it in the most modern/universal and proper way possible. So something that could be used standalone also outside of the Koha UIs, maybe even on a separate repo and published on NPM (both to allow external JS programs to interact with Koha, but to also ensure it's written in an objective manner, with no hacks or short-circuits that'd unnecessarily bind it inherently to the Koha UI?).

Then it also begs to finally use TypeScript, as type-safe API client is a very good thing, and you have OpenAPI specs that define everything about the APIs. So not doing that would be a big waste.

The only problem is that a project like https://openapi-ts.dev/ for automatically translating OpenAPI specs to TypeScript types requires OpenAPI 3.x in current versions, but Koha still didn't migrate from 2.x (would be a good opportunity to do it). Of course the JS parts could still use it after building and not be too concerned about it. But in a proper setup, even then, a proper IDE setup could still give developers the type hints in their IntelliSense, which'd still be an easy win. So we should use something like that.

Another win of using automated OpenAPI->TypeScript type generation would be that new endpoints defined in the schema could already be available for usage without manually needing to define them in yet another place. And without needing to manually type out all the code to handle the new endpoints (which would be pretty nice if we want to accelerate the API development and phase out the old one and classic Perl scripting)

There's two projects:
1. https://openapi-ts.dev/
2. https://heyapi.dev/

It seems that #1 focuses on more "bare" but usable schema generation, which can be used with their openapi-fetch client to access APIs in a type-safe but close to the original specs manner. For example (mind you that TS detects the exact path used by strings and type-checks the parameters!!):

const { data, error } = await client.GET("/blogposts/{post_id}", {
  params: {
    path: { post_id: "my-post" },
    query: { version: 2 },
  },
});

It seems that #2 generates more of "prettier" SDKs already and focuses on direct integrations with plugins and middlewares etc. Their example is more like:

const { data, error } = await getPetById({
  path: {
    // random id 1-10
    petId: BigInt(Math.floor(Math.random() * (10 - 1 + 1) + 1)),
  },
});

Not sure which approach seems nicer. Also technically it could be that simply generated types lived on a separate repo generated for all versions (including point-releases or major version tags), with clients being more responsible with how they'd directly pull/integrate them?

I think everything really depends whether people would prefer using something like in #1 and maybe writing some more functional simpler wrappers around some of them (such as `blogposts.getPostById(id: number)`, or directly use something as in #2 which is something in between.

Also the #2 having integrations with something like Zod or TanStack Query could prove quite useful in the future developments with Vue components (validating and auto-fetching of stuff etc.). I mean just look at the example here: https://tanstack.com/query/latest/docs/framework/vue/quick-start

It would remain to be checked which one would work better with paginated endpoints though. It seems the #2's TanStack Query's integration has some support for infinite queries there in that regard.