[ENH] Add OpenAPI spec to Rust server #3815
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
philipithomas
changed the title
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| #[derive(Serialize, ToSchema)] |
There was a problem hiding this comment.
nit: it might make sense to gate these derivations behind a feature flag
There was a problem hiding this comment.
Talked to @codetheweb and we decided it was ok to do this as a follow-up task
| get, | ||
| path = "/api/v2/healthcheck", |
There was a problem hiding this comment.
curious if it's possible for utoipa to infer this so we don't have to define the method and path twice?
There was a problem hiding this comment.
Unfortunately, no. I just looked through the docs to confirm.
| responses( | ||
| (status = 200, description = "Success", body = HeartbeatResponse), | ||
| (status = 500, description = "Server error", body = ErrorResponse) | ||
| ) |
There was a problem hiding this comment.
is there a way to define a common set of error types instead of re-defining for each route?
There was a problem hiding this comment.
Good feedback - turns out there is. Working on implementing this now.
There was a problem hiding this comment.
There was a problem hiding this comment.
I looked into the IntoResponses, and it seems that we can't mix per-path response (like HeartBeatResponse) with the CommonResponses. So, it won't work for our use case
| .route("/api/v2/tenants/:tenant_name", get(get_tenant)) | ||
| .route("/api/v2/tenants/:tenant_id/databases", get(list_databases).post(create_database)) | ||
| .route("/api/v2/tenants/:tenant_id/databases/:name", get(get_database).delete(delete_database)) | ||
| .route("/api/v2/tenants/{tenant_name}", get(get_tenant)) |
There was a problem hiding this comment.
Axum 0.8 changed the way path params are specified: https://github.com/tokio-rs/axum/releases/tag/axum-v0.8.0
breaking: Upgrade matchit to 0.8, changing the path parameter syntax from /:single and /*many to /{single} and /{*many}; the old syntax produces a panic to avoid silent change in behavior (tokio-rs/axum#2645)
There was a problem hiding this comment.
Ah but I misnamed it - should keep as tenant_id. Will fix that.
There was a problem hiding this comment.
No I think it was always tenant_name?
There was a problem hiding this comment.
Yeah looks like you're right. I just renamed everything to standardize on tenant and database, which matches our existing OpenAPI spec.
The different variable names were causing issues with code generation, too.
There was a problem hiding this comment.
I don't think we should standardize on tenant and database tbh. It leads to a lot of bugs in confusion between name vs id for things like database. Wdyt? I guess maybe its fine since id is UUID and will fail to parse? But users get confused so the explicitness is nice.
IIRC @drewkim explicitly did it this way for clarity.
There was a problem hiding this comment.
I was basing it on our client experience, which call the params just tenant and database:
client = chromadb.HttpClient(
ssl=True,
host='api.trychroma.com',
tenant='00000000-0000-0000-0000-000000000000',
database='foo',
headers={
'x-chroma-token': 'YOUR_API_KEY'
}
)
I think database and tenant look clean in the client libraries, and I think it helps to maintain the same variable name full-stack. If we change the backend to refer to them as database_name and tenant_id, then somewhere becomes a boundary that converts them from tenant -> tenant_id. That can be the client library.
So the main question to answer, which we can work backwards from, is:
Are we happy with the client library dev ex (tenant instead of tenant_id)?
There was a problem hiding this comment.
✏️ Discussed offline - sticking with tenant and database
| request_body = CreateTenantPayload, | ||
| responses( | ||
| (status = 200, description = "Tenant created successfully", body = CreateTenantResponse), | ||
| (status = 401, description = "Unauthorized", body = ErrorResponse), |
There was a problem hiding this comment.
are we forced to manually specify the error type this way? It would be nice if it could somehow be inferred off the return type.
There was a problem hiding this comment.
Working on defining common error types, which should simplify this: https://docs.rs/utoipa-gen/latest/utoipa_gen/derive.IntoResponses.html
There was a problem hiding this comment.
Ok - not sure what you have in mind but might talk to @Sicheng-Pan and @codetheweb the first as I know they thought a bit about the current error types.
There was a problem hiding this comment.
Just posted about this here: #3815 (comment)
Summary is that the IntoResponses standard errors aren't compatible with our pattern. So, we'll stick with the standard ErrorResponse that the frontend already uses for now.
| responses( | ||
| (status = 200, description = "Database retrieved successfully", body = GetDatabaseResponse), | ||
| (status = 401, description = "Unauthorized", body = ErrorResponse), | ||
| (status = 404, description = "Database not found", body = ErrorResponse), |
There was a problem hiding this comment.
I wish it were possible to infer this from the response type :(
There was a problem hiding this comment.
I dug in, and it seems that the way we're doing typed returns elsewhere are making it hard right now. I propose we add it as a follow-up task.
…ram names to match legacy api
## Description of changes - Improvements & Bug fixes - Updates `axum` to `0.8` - New functionality - Adds `/openapi.json` route that renders an OpenAPI spec - Adds `/docs/` route that renders Swagger - Adds code generation of OpenAPI spec with [`utoipa`](https://github.com/juhaku/utoipa) - Standardizes path params to be `tenant` (*not tenant_id*) and `database` (*not database_name*) Here's a video demo of how it works: https://github.com/user-attachments/assets/e66d9fc0-8b8b-408f-8b3a-2e975646edf3 Notes: * The routes and basic functionality mirror current Python functionality (see: https://api.trychroma.com:8000/docs) * OpenAPI spec is substantially improved, including: typed errors, typed responses, security headers
Description of changes
axumto0.8/openapi.jsonroute that renders an OpenAPI spec/docs/route that renders Swaggerutoipatenant(not tenant_id) anddatabase(not database_name)Here's a video demo of how it works:
OpenAPI.mp4
Notes: