mnahkies / openapi-code-generator

A code generation tool for openapi 3 / 3.1 specifications written in typescript, primarily aimed at generating typescript clients and server stubs. Other target languages may be added in future.
https://openapi-code-generator.nahkies.co.nz/
MIT License
20 stars 2 forks source link

Generated client has no server url override #266

Open ADRFranklin opened 1 day ago

ADRFranklin commented 1 day ago

Hi,

Path objects allow overriding the main server url, so that specific paths may deviate from the original server url.

/user:
  get:
    servers:
      - url: https://plex.tv/api/v2
    tags:
      - Authentication
    summary: Get Token Details
    description: Get the User data from the provided X-Plex-Token
    operationId: getTokenDetails
    responses:
      "200":
        description: Logged in user details
        content:
          application/json:
            schema:
              $ref: ../../models/UserPlexAccount.yaml
      "400":
        $ref: "../../responses/400.yaml"
      "401":
        $ref: "../../responses/401.yaml"

what is generated is this

  async getTokenDetails(
    timeout?: number,
    opts: AxiosRequestConfig = {},
  ): Promise<AxiosResponse<t_getTokenDetailsJson200Response>> {
    const url = `/user`;
    const headers = this._headers({}, opts.headers);

    return this._request({
      url: url,
      method: 'GET',
      ...(timeout ? { timeout } : {}),
      ...opts,
      headers,
    });
  }

which I believe to be wrong, since the only way to work around it is overriding the baseUrl with opts, or override the function in another class that extends the generated client. But it would be nice to have the url override the format so it becomes instead

  const url = `https://plex.tv/api/v2/user`;
mnahkies commented 1 day ago

Yeah that's not yet supported (btw in case you haven't seen I've added a compatibility table to try and document what properties are used https://openapi-code-generator.nahkies.co.nz/overview/compatibility#operation-object), but can think about adding it.

The servers array in general is something that's always seemed a bit tricky to support tbh - I recently added support for optionally using the root servers object to generate string literal types as hints in https://github.com/mnahkies/openapi-code-generator/pull/246 but haven't actually started using it at runtime.

There's two main question marks in my head:

Choosing which one explicitly by making it a type hint rather than a runtime concern seemed like a good trade-off to me, as generally speaking I'd expect things like base URLs to be sourced from your applications configuration in the 12 factor app style anyway (eg: environment variables/config files/cli args).

Since it's not uncommon to list out dev / stage hosts in definitions there's also an aspect of wanting to avoid leaking information about these into the transpiled bundles (when using it for frontend applications).

Perhaps one option could be to support servers at the operation level, but iff it's an array of length 1, ie: there's no ambiguity.

I guess ideally variables would be parsed and probably just surfaced in the same way that route placeholders are.

It would be nice if the server object had a id / key property that could be used to tag which related to eachother, but I don't think there's an official attribute for this (eg: tag a URL as dev / prod so that changing the root base URL would also automatically select the correct path level URLs)

Be curious if you had any thoughts on the best way to support this, and also how important/unimportant you think support for variables is

ADRFranklin commented 22 hours ago

I was playing around with it and I think the variable route is the best way to go.

type Server = {
  '{protocol}://www.example.com': {
    variables: {
      protocol: 'https' | 'http',
    },
  },
  '{protocol}://www.example.com/api/{version}': {
    variables: {
      protocol: 'https' | 'http',
      version: 'v1' | 'v2',
    },
  },
};

const ServersWithDefaults: Server = {
  '{protocol}://www.example.com': {
    variables: {
      protocol: 'https',
    },
  },
  '{protocol}://www.example.com/api/{version}': {
    variables: {
      protocol: 'https',
      version: 'v1',
    },
  },
};

type ServerItem = keyof Server;

type ServerOptions = {
  servers?: Server,
  defaultServer?: ServerItem,
}

class TestRequestClient {
  constructor(private readonly options: ServerOptions) {
    const servers = this.options.servers ?? ServersWithDefaults;
    const defaultServer = this.options.defaultServer ?? Object.keys(servers)[0];
  }
}

const testRequestClient = new TestRequestClient({
  defaultServer: '{protocol}://www.example.com',
});

Ideally something like this would be ideal, though this is just a rough idea of making it easier to pass to the client, and specify which url is going to be default. The variables should be replaced in the string in the constructor and using the url strings themselves as the identifier. If there is just one server url, I think that should be automatically made the default so you don;t need to specify it.

I believe this should be possible and having them as a type, means passing them in is easier, Hopefully this is something you agree with or at least gives you something more to go on.