trpc-openapi icon indicating copy to clipboard operation
trpc-openapi copied to clipboard

RFC: Reduce OpenAPI `meta` complexity

Open jlalmes opened this issue 3 years ago • 5 comments
trafficstars

Initially proposed by @mshd (https://github.com/jlalmes/trpc-openapi/issues/55) Additionally raised by @TheoBr (https://youtu.be/YAzzvhaRs6M?t=7086)

RFC: Reduce OpenAPI meta complexity

It has been mentioned a few times that the meta required to enable OpenAPI support on a tRPC procedure could be reduced. This RFC proposes setting some sensible defaults (can be overwritten), such that the minimum required meta will be as follows:

{ openapi: { enabled: true } }

Current requirements

  • meta.openapi.enabled: true
  • meta.openapi.method: GET | POST | PUT | PATCH | DELETE
  • meta.openapi.path: string

Sensible defaults

  • meta.openapi.enabled: defaults to false
  • meta.openapi.method: defaults to GET (query) | POST (mutation)
  • meta.openapi.path: defaults to computed value from tRPC procedure where:
    • nested routers = /
    • path = path.replace(/[A-Z]/g, (m) => "-" + m.toLowerCase())

Example (v10)

const postsRouter = t.router({
  getById: t.procedure
    .meta({ openapi: { enabled: true } }),  // { openapi: { enabled: true, method: 'GET', path: '/posts/get-by-id' } }
    .input(z.object({ id: z.string() })),
    .output(postSchema)
    .query(({ input }) => {
       ...
    })
  ...
})

const appRouter = t.router({
  posts: postsRouter,
  ...
})

Concerns

  • The defaults will likely not follow proper RESTful API design patterns in most cases, examples:
    • /posts/get-by-id should be /posts/{id}.
    • update endpoints should be using PUT or PATCH.
  • It is not clear when you will be introducing a breaking changes to your public OpenAPI, example:
    • If you refactor a procedure name, its path will change.

Not fully sold on this change, any thoughts are welcomed (cc @KATT @sachinraja)

jlalmes avatar Jul 14 '22 11:07 jlalmes

Sensible defaults look good. Is enabled necessary at all? If the meta.openapi object exists, that should be good enough to tell, so a minimum version could look like this:

{ openapi: true }

and if you pass any other info:

{ openapi: { path: '/posts/{id}' } }

That's a very simple change though, I think you need a bigger API enhancement for this. What about something like the user passing info about the structure of their procedures and being able to do more inference from that?

export const openApiDocument = generateOpenApiDocument(appRouter, {
  title: 'tRPC OpenAPI',
  version: '1.0.0',
  baseUrl: 'http://localhost:3000', 
  // maybe runs for all procedures, not just enabled ones
  procedureOverride(proc) {
    if (proc.name === 'byId') {
      return {
        path: `${proc.routerPath}/{id}`,
        ...proc.openapiMeta
      }
    }

    return proc.openapiMeta
  }
});

This way users can define structures for themselves and reduce boilerplate.

sachinraja avatar Jul 14 '22 12:07 sachinraja

Is enabled necessary at all?

Thanks @sachinraja - you're right. But I just want to make sure that it's really hard for developers to accidentally leak endpoints into their public API without realising.

Seeing as, typically, I would not expect public facing OpenAPI endpoints to change very frequently. I'd like to understand how much overhead you think there is to just write the openapi.meta once (takes <30 secs for each procedure) and then you can ignore it forever knowing that any refactors will not change how the endpoint is consumed?

jlalmes avatar Jul 14 '22 12:07 jlalmes

That's true, overhead will be reduced with the new defaults anyway. I think the current meta API is fine and the proposed ones with the defaults will fix some issues people had with boilerplate. Would be nice to get some input from an actual user though.

sachinraja avatar Jul 14 '22 12:07 sachinraja

What @sachinraja said. If you do anything automatically, it's good to have default logic that can be overridden, similar to how the error formatters in tRPC work.

KATT avatar Jul 14 '22 13:07 KATT

To me it seems like the only redundant property is enabled, which can be assumed to be true in case we provide the openapi object.

We cannot provide a really valid/proper default value for path, since OpenAPI conventions don't match (t)RPC conventions and most of the time users would (or should) provide a custom path. By providing a custom openapi.path, we can assume that user intents to make this procedure a (compilant) openapi.

In our usecase, we use the trpc client for our internal app, but expose the same api via openAPI for our users. We currently have close to 100 procedures, where most of them are openapi-enabled and there isn't a single instance where we wouldn't need to override the default path.

dodas avatar Jul 17 '22 00:07 dodas

  • Meta openapi.enabled will now default to true if unspecified.
  • Meta minimum requirement is now just { method: string, path: string }.

Will be released in next major v1.0.0. You can try it now using trpc-openapi@alpha

jlalmes avatar Aug 25 '22 12:08 jlalmes

I think you're overlooking how big some of those tRPC deployments are out there. For better adoption it would be ideal to have the path of least resistant to the request of "here is my router, give me an OpenAPI UI to look at"

It means:

  • Enable all queries and mutations by default -- this could be an option to generateOpenApiDocument so people can turn it off quickly
  • Do not ask for path and method and fall back to defaults.

This way people can give it a shot and see what their API will look like and start fine tuning it by providing those meta values. This is a more dev friendly API. I'm sure no serious deployment will just go with the defaults but letting them get to that generated API documentations page will make this project a lot more successful.

P.S. I'm the author of Swagger Editor and have a bunch of experience working in this domain.

mohsen1 avatar Aug 26 '22 16:08 mohsen1

From my understanding, the main use-case for trpc-openapi is exposing only a subset of procedures to third-party consumers AND dedicated endpoints for receiving webhooks.

For this reason we decided to design the library with an opt-in architecture so that you can incrementally adopt OpenAPI into your project rather than defaulting to enabled and then forcing each procedure to explicitly opt-out.

Because we are using this opt-in architecture, it does not seem unreasonable to me that we require you to provide the following: openapi: { method: string; path: string; }. This actually has many benefits (see above discussion).

@mohsen1, from what I can understand you don't wish to make use of trpc-openapi to serve an OpenAPI compliant version alongside your tRPC API, but rather as a documentation tool for your tRPC API (see what procedures exist & their inputs/outputs etc.). Is this true? (👈 if so I have some follow-up questions)

jlalmes avatar Aug 26 '22 18:08 jlalmes

The opt-in approach sounds good to me but I also wish there was a way of opting in everything at once.

I do want to use the API, not just the docs. The API is the way of opening up a tRPC backend to native apps usually

mohsen1 avatar Aug 26 '22 20:08 mohsen1

@mohsen1 are you aware that supplying an output schema is also a requirement for trpc-openapi enabled procedures?

We could add a property to opt-in everything at once (ie. unsafe_defaultEnabled), however this would likely just cause your server to throw runtime errors, because (I'd imagine) your procedures don't currently have output schemas defined.

At which point, if you're going through all of you procedures to add output schemas, you might as well just go add the meta.openapi fields too (meaning this new property is redundant).

Does this make sense?

jlalmes avatar Aug 29 '22 11:08 jlalmes

I missed that fact about output. It does makes sense


Thank you for this great library. Looking into deplying it for my project.

mohsen1 avatar Aug 29 '22 12:08 mohsen1