Conventions for calling the API from the client

[davelopez]

I'm migrating the code from openapi-typescript-fetch to openapi-fetch. And I would like to take this opportunity to decide on a convention we all are comfortable with.

A typical call using the previous openapi-typescript-fetch looks like this:

import { fetcher } from "@/api/schema";

const datasetsFetcher = fetcher.path("/api/datasets/{id}").method("get").create();

const { data: dataset } = await datasetsFetcher({ id: "testID" });

The new openapi-fetch is more explicit about path, query, and body parameters and has different syntax for calling the API.

See documentation here

import { client } from "@/api/schema";

const { data: dataset } = await client.GET("/api/datasets/{id}", {
  params: {
    path: { id: "testID" },
  },
});

I'm considering the following conventions:

Option 1: Use the new openapi-fetch as is everywhere

It may be harder to swap to a different framework later but it should be more straightforward.

import { client } from "@/api/schema";

const { data: dataset } = await client.GET("/api/datasets/{id}", {
  params: {
    path: { id: "testID" },
  },
});

Option 2: Create a fetcher function that wraps the new openapi-fetch

It creates a facade to have a single point of change in case we want to migrate to a different framework in the future. On the other side, it is kind of redundant to handle the parameters and types in the function when the client already does it.

import { client } from "@/api/schema";

const datasetsFetcher = (id: string) =>
  client.GET("/api/datasets/{id}", {
    params: {
      path: { id: "testID" },
    },
  });

const { data: dataset } = await datasetsFetcher("testID");

Option 3: Your suggestion

Please post your suggestions in the comments.

Testing / Mocking

The openapi-fetch documentation has some recommendations for testing. Should we follow these recommendations? Like installing and using Mock Service Worker for mocking responses?

[mvdbeek]

I don't think Option 2 is a good option, you'd have to redefine the type signature unnecessarily it seems and the signature can still drift away from the actual types.

Is there anything in openapi-fetch that we directly benefit from ? Is it feasible to use generics to build up the simpler openapi-typescript-fetch interface ? It's nice that you can be specific about what's a path, cookie, body etc param, but it's unnecessarily verbose for our API.

[davelopez]

I agree, I'm not fully comfortable with any option right now, that is why I value your comments and opinions.

We could avoid signature drift using something like this:

import { client, ToolShedApiPaths } from "@/schema"

type MaliciousPathParams =
    ToolShedApiPaths["/api/repositories/{encoded_repository_id}/revisions/{changeset_revision}/malicious"]["put"]["parameters"]["path"]

const setMaliciousFetcher = (params: MaliciousPathParams) =>
    client.PUT("/api/repositories/{encoded_repository_id}/revisions/{changeset_revision}/malicious", {
        params: {
            path: params,
        },
    })

But it is still very verbose and ugly to me.

I guess Option 3 could be to stick with openapi-typescript-fetch instead of trying to use generics to build something similar to it. Citing the documentation, the main differences are:

openapi-typescript-fetch predates openapi-fetch, and is nearly identical in purpose, but differs mostly in syntax (so it’s more of an opinionated choice):

• openapi-typescript-fetch throws exceptions for non-OK responses (requiring you to wrap things in try/catch) rather than follow the fetch API spec of not throwing.
• openapi-typescript-fetch’s syntax is more verbose, and relies on chaining (.path(…).method(…).create()).

So it seems there is no huge difference between them, other than choosing one verbosity over the other 😅

[mvdbeek]

That's my impression too. The other advantage is that openapi-fetch development seems more active, but I don't know if that's worth moving around a lot of boilerplate.

[davelopez]

I'm currently migrating the ToolShed first. I can push the changes when I finish and see how we feel about it. Otherwise, I can drop that commit and try to update openapi-typescript-fetch to the latest and explicitly fix the new issues.

Apart from being more active and up to date (obviously) with openapi-typescript, it also claims to be 2x faster but I don't think that is a bottleneck right now 😆

[davelopez]

After some more investigation, and to ensure we explore all options, I tried to update openapi-typescript-fetch to the latest version to try and keep both fetchers for a transition period and avoid a huge PR replacing all calls.

It turns out that if we want to update to the latest openapi-typescript we need to replace the fetchers that have parameters (query or body) as even the newest version of openapi-typescript-fetch cannot correctly infer the parameters handling for the latest openapi-typescript.

Here are a couple of examples to illustrate what I mean:

API call without parameters

Those are easy to adapt, just cast the empty object to never. However, this is a bit ugly and even being forced to provide an empty object for parameterless calls before was already odd.

export const jobLockStatus = fetcher.path("/api/job_lock").method("get").create();

// Before upgrading
const response = await jobLockStatus({});

// After upgrading
const response = await jobLockStatus({} as never);

The equivalent in openapi-fetch would be:

const response = await client.GET("/api/job_lock");

API call with parameters

// Note that PUT /api/job_lock takes a request body with a single key, "active", which is a boolean.
export const jobLockUpdate = fetcher.path("/api/job_lock").method("put").create();

// Before upgrading
const response = await jobLockUpdate({ active: true });

// After upgrading
// The previous syntax no longer works (surprisingly only in some cases) with the new client schema types because it cannot infer the argument is the body and it fails with: 
// Argument of type '{ active: boolean; }' is not assignable to parameter of type 'never'.

// I couldn't find any way to provide the body payload. Even if you try:
const job = await jobLockUpdate({} as never, {
    body: {
        active: true,
    },
});

// It will fail with:
// Type '{ active: true; }' is not assignable to type 'BodyInit | null | undefined'.
// Object literal may only specify known properties, and 'active' does not exist in type 'ReadableStream<any> | Blob | ArrayBufferView | ArrayBuffer | FormData | URLSearchParams'.

The equivalent in openapi-fetch would be:

const response = await client.PUT("/api/job_lock", {
    body: {
        active: true,
    },
});

So I think this is a strong argument for the full migration to openapi-fetch. I will focus first on the cases that fail like these, and keep for now those that seem to work with the old fetcher to keep the changes to a minimum in #18532

[dannon]

I think we should just lean into option 1, using openapi-fetch everywhere. I personally prefer the syntax and think it's more readable. It's going to be better supported, more active, it's from the openapi-typescript folks directly, and I think we avoid problems down the road by swapping over from the third party library.

[davelopez]

Ok, I will start doing this for the ToolShed as I mentioned. I'm happy doing some back and forth on it until we are all happy with the result 👍

[davelopez]

This is how it looks so far in the Toolshed #18532

I must say that it feels a little better to write even with the extra explicitness. Another point in favor is that the response "data" can be undefined if there is an error, forcing you to take care of the error handling, which I think is good.

[davelopez]

The first migration phase (convert all fetcher calls to openapi-fetch) is done in #18532

Now comes the second phase, converting the mockFetcher used in client unit tests.

The recommended library for mocking API responses by openapi-fetch is Mock Service Worker which seems pretty powerful and future-proof.

I have experimented a bit with it in combination with openapi-msw and it looks very promising if we want to go this route.

Here is an example of how mocking the API will look with this new approach:

import { client, type HistoryDetailed, type HistorySummary, type MessageException } from "@/api";
import { clientMock } from "@/api/__mocks__";

const TEST_HISTORY_SUMMARY: HistorySummary = {
    model_class: "History",
    id: "test",
    name: "Test History",
    archived: false,
    deleted: false,
    purged: false,
    published: false,
    update_time: "2021-09-01T00:00:00",
    count: 0,
    annotation: "Test History Annotation",
    tags: [],
    url: "/api/histories/test",
};

const TEST_HISTORY_DETAILED: HistoryDetailed = {
    ...TEST_HISTORY_SUMMARY,
    create_time: "2021-09-01T00:00:00",
    contents_url: "/api/histories/test/contents",
    importable: false,
    slug: "testSlug",
    size: 0,
    user_id: "userID",
    username_and_slug: "username/slug",
    state: "ok",
    empty: true,
    hid_counter: 0,
    genome_build: null,
    state_ids: {},
    state_details: {},
};

const EXPECTED_500_ERROR: MessageException = { err_code: 500, err_msg: "Internal Server Error" };

// Mock the API client
// You can do whatever you want with the parameters and return values
// All API schema types must be strictly respected and will be up to date with the OpenAPI schema
clientMock.get("/api/histories/{history_id}", ({ params, query, response }) => {
    if (query.get("view") === "detailed") {
        return response(200).json(TEST_HISTORY_DETAILED);
    }
    if (params.history_id === "must-fail") {
        return response("5XX").json(EXPECTED_500_ERROR, { status: 500 });
    }
    return response(200).json(TEST_HISTORY_SUMMARY);
});

describe("clientMock", () => {
    it("mocks the API client", async () => {
        {
            const { data, error } = await client.GET("/api/histories/{history_id}", {
                params: {
                    path: { history_id: "test" },
                    query: { view: "summary" },
                },
            });

            expect(error).toBeUndefined();

            expect(data).toBeDefined();
            expect(data).toEqual(TEST_HISTORY_SUMMARY);
        }

        {
            const { data, error } = await client.GET("/api/histories/{history_id}", {
                params: {
                    path: { history_id: "test" },
                    query: { view: "detailed" },
                },
            });

            expect(error).toBeUndefined();

            expect(data).toBeDefined();
            expect(data).toEqual(TEST_HISTORY_DETAILED);
        }

        {
            const { data, error } = await client.GET("/api/histories/{history_id}", {
                params: {
                    path: { history_id: "must-fail" },
                },
            });

            expect(error).toBeDefined();
            expect(error).toEqual(EXPECTED_500_ERROR);

            expect(data).toBeUndefined();
        }
    });
});

Notice how all the types are strictly enforced and are in sync with the current OpenAPI schema.

I still need to figure out an issue with Webpack trying to import the MSW library.

[davelopez]

Thanks to @ElectronicBlueberry for the hints about the module import side effects preventing the API mocks from working properly. I'm in the process of adapting the client unit tests to the new API mocking system. One of the many advantages is that we can also remove the Axios MockAdapter as we can easily mock any server route with the same technique.

Now the syntax for calling the API client and mocking the responses will be slightly different but should be readable enough.

For requesting something to the API from any client code:

import { GalaxyApi } from "@/api";

const { data, error } = await GalaxyApi().GET("/api/histories/{history_id}", {
    params: {
        path: { history_id: "test" },
        query: { view: "summary" },
    },
});

// Handle data and error

For mocking the server response in a test:

import { useServerMock } from "@/api/client/__mocks__";

const { server, http } = useServerMock();

server.use(
    http.get("/api/histories/{history_id}", ({ params, query, response }) => {
        if (query.get("view") === "detailed") {
            return response(200).json(TEST_HISTORY_DETAILED);
        }
        if (params.history_id === "must-fail") {
            return response("5XX").json(EXPECTED_500_ERROR, { status: 500 });
        }
        return response(200).json(TEST_HISTORY_SUMMARY);
    })
);

[ARORA288]

Nice