Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect openapi definitions from hono-zod-openapi apps #369

Open
wants to merge 66 commits into
base: main
Choose a base branch
from

Conversation

brettimus
Copy link
Contributor

@brettimus brettimus commented Nov 19, 2024

UPDATE TYPES PACKAGE AFTER MERGE
UPDATE HONO-OTEL AFTER MERGE

Description

Automatically report openapi spec from the client library to the studio api.

Whenever a route probe response is received, Studio will try to match any incoming routes with a corresponding openapi definition for that route.

If one is found, a json object describing the parameters and response for the route is then saved in the database on a special column for the route. This mini-spec can be used to generate request parameters, which is useful, since the source code analysis can't do much to help with OpenApiHono apps (more on that below).

The spec for a route, if present, also will render a Docs tab for that route.

Feature Flagged

Adds a section in the settings page where you can point to a route that hosts a json openapi definition.
This is a more flexible approach than auto-detecting the spec from the Hono-Zod-OpenAPI integration.

Fun things along the way

  • OpenApiHono is not picked up by source code analysis (makes sense, since it is not of type Hono)
  • Had to add a special header that allows the client library to skip instrumentation for an incoming request. Otherwise we'd enter an infinite loop when explicitly looking up a spec from the target api ((_note that the "custom spec url" setting is behind a feature flag for now))
  • Because of said infinite loop, had to finally add a limit to the number of traces we return from the api. (My infinite loop produced close to 90K new spans, which ground the UI to a halt
  • Basic auth is wonky in studio

Considerations

Manually pulling the spec versus automatically receiving it — My first implementation relied on the user explicitly telling us where their spec was located, then we would try to pull it in to find matching specs for routes. However, things are a lot smoother if we only support apps built with HonoOpenAPI for now. In that case, we can ignore the URL specified on the settings page. Also, if we want to support "give us a URL and we'll try syncing", then we need to be mindful of things like the ALL method. It's a lot more work to support a link to a spec instead of relying on the Hono OpenAPI integration to report it to us.

Autopopulated parameters — This is tricky given that when you click on a Route, we might end up redirecting you to the last-made-request for that route. In which case the expected behavior is not clear. Should we merge expected query parameters from the OpenAPI spec with the actual query parameters made in the last request?

Screenshots

image image image image

TODO

  • Test headers

  • Show documentation for route somewhere

  • Handle .all method

  • Only allow openapi integration for versions of client library past 0.4.1

  • Expand schema definitions when possible into openApiSpec

  • Explain OpenAPI integration on the settings page

  • Auto-enable openapi integration when we can

Copy link

pkg-pr-new bot commented Nov 19, 2024

Open in Stackblitz

npm i https://pkg.pr.new/fiberplane/fpx/@fiberplane/studio@369
npm i https://pkg.pr.new/fiberplane/fpx/@fiberplane/hono-otel@369

commit: 00cb524

@brettimus brettimus force-pushed the fp-3897-detect-openapi-definitions-directly-from-hono-middleware branch from 5b8a9a8 to 77b6716 Compare November 21, 2024 07:30
const response = await fetch(resolvedSpecUrl, {
headers: {
// NOTE - This is to avoid infinite loops when the OpenAPI spec is fetched from Studio
// We need to make sure that the user has instrumented their app with @fiberplane/hono-otel >= 0.4.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is really crucial - we need to validate that the user has the correct version of hono-otel

@@ -35,6 +35,7 @@ app.get("/v1/traces", async (ctx) => {
const spans = await db.query.otelSpans.findMany({
where: sql`inner->>'scope_name' = 'fpx-tracer'`,
orderBy: desc(sql`inner->>'end_time'`),
limit: 1000,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a little left field but i finally hit the issue where i had so many spans it effectively broke the UI. i put a limit of 1000 here which seems okay but... who knows

updatedAt: text("updated_at").notNull().default(sql`(CURRENT_TIMESTAMP)`),
});

// TODO - Figure out how to use drizzle with "@hono/zod-openapi"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth noting: i wanted to use drizzle-zod to share schemas between the database and the api, in order to reuse a database schema as, e.g., the payload for a route

however, the openapi integration requires that you use a special version of zod that they export, which provides some additional fields that are necessary for the schema to be converted to an openapi schema

this forces you to redefine (as far as i can tell) the schemata, which is kind of good practice to separate api schemas from database schemas, but was a little bit of a pain for me considering i didn't care about that separation at this step

@brettimus brettimus force-pushed the fp-3897-detect-openapi-definitions-directly-from-hono-middleware branch from 0bf9c98 to d78c7dd Compare November 29, 2024 13:32
@brettimus brettimus changed the title WIP: Detect openapi definitions Detect openapi definitions from hono-zod-openapi apps Nov 29, 2024
@brettimus brettimus marked this pull request as ready for review November 29, 2024 17:00
Copy link
Member

@flenter flenter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -4,6 +4,9 @@ shared/dist
.envrc
.cursorrules

# Cursor
.cursorrules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line already exists in this file

* This function handles both single routes and arrays of routes.
*
* @param db - LibSQL database instance containing the OpenAPI specifications. Used to fetch the latest spec.
* @param routes - Single route or array of routes to be enriched. Each route should contain path, method,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe i'm reading this text the wrong way, but the way I understand this doc is as follows:

route: Route | Array<Route>

However it's always an array (though maybe with a singular route)

And that makes me think, I've seen in other prs, that the JSDocs are out of date even in the PR's that introduce them. Maybe it's better to leave the types out for parameters/return values? I do like the part of the JSDoc where you describe what a function is intended for but types/parameters/return values to me seems like we need to maintain an additional thing (like for instance if you refactor/rename a parameter you'd need to manually also update the docs).

const UserIdPathParamSchema = z.object({
id: z
// Path params are always strings
.string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also do:
z.number({ coerce: true}) and the id will be converted to a number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't sure how that'd play with validation — worth toying around with it

studio/src/pages/RequestorPage/CommandBar/CommandBar.tsx Outdated Show resolved Hide resolved
return (
<ScrollArea className="h-full pb-8">
<div className="p-2">
{/* Description Section - Updated styling */}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still relevant? I think it can go, not?

"db:studio": "drizzle-kit studio",
"db:touch": "wrangler d1 execute zod-openapi --local --command='SELECT 1'",
"db:migrate:prod": "GOOSIFY_ENV=production drizzle-kit migrate",
"db:studio:prod": "GOOSIFY_ENV=production drizzle-kit studio"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like it if we'd had some type checking/linting enabled also for our example projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants