feat(api): richer OpenAPI metadata for typed endpoints - #41638
Open
ggazzo wants to merge 16 commits into
Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: f5a13bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThis change adds typed OpenAPI 3.1 route documentation, converts AJV schemas, updates Meteor API integration, removes legacy typed-route metadata, and adds unit and end-to-end validation. ChangesOpenAPI schema and router foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ggazzo
force-pushed
the
chore/api-openapi-metadata
branch
from
July 30, 2026 14:57
8fb5bc8 to
87107d3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41638 +/- ##
===========================================
+ Coverage 68.75% 68.77% +0.01%
===========================================
Files 4151 4156 +5
Lines 159513 159825 +312
Branches 27997 28070 +73
===========================================
+ Hits 109681 109927 +246
- Misses 44657 44722 +65
- Partials 5175 5176 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The generated OpenAPI document was missing most of what makes a spec usable:
path parameters were emitted with Express syntax and no parameter objects,
query parameters were collapsed into a single opaque `query` object, response
descriptions were hardcoded as empty strings, and there was no way for an
endpoint to declare a summary, a description, examples or a deprecation notice.
Route options now accept `summary`, `description`, `operationId`, `deprecated`,
`externalDocs`, `params`, `bodyContentType`, `responseDescriptions` and
`examples`. Everything else is derived from options the framework already has:
permissions, license modules and two-factor requirements become `x-*`
extensions plus a note in the description, `deprecation` sets `deprecated`,
rate limiting documents its response headers and `429`, and the error responses
implied by the route (400, 401, 403, 429, 500) are injected when not declared.
Path parameters are now derived from the path pattern itself, so every `{param}`
in the document has a matching parameter object, and query schemas are exploded
into one parameter per property, honoring `allOf`/`anyOf`/`oneOf` composition.
Both copies of the OpenAPI conversion were replaced by a single builder in
`@rocket.chat/http-router`. The copy living in `APIClass` wrote to a
`typedRoutes` field that nothing ever read - the served document has always
come from the router - so it was removed along with its legacy counterpart.
The `example` annotation is now registered as an AJV vocabulary, so schemas can
carry their own `description` and `example` and have them flow straight into the
document. The banners endpoints show the pattern and drop the stale `@openapi`
JSDoc blocks that nothing had been reading.
ggazzo
force-pushed
the
chore/api-openapi-metadata
branch
from
July 30, 2026 15:17
87107d3 to
d991e7a
Compare
`swaggerUi.setup` runs at import time, before settings are loaded, so
`settings.get('Site_Url')` was interpolated as `undefined` and the UI ended up
fetching `/api-docs/undefined/api/docs/json`. The served HTML came back instead
of the spec, and Swagger UI reported a missing `openapi` version field.
The document is always served from the same origin as the UI, so a relative url
needs no setting at all.
Auditing the hand-written definitions in RocketChat/Rocket.Chat-Open-API showed
which features the published documentation relies on and this framework could
not express:
- named example scenarios, by far the most used feature there (1010 example
objects, 156 payloads documenting two to six alternatives). `examples.body`
and `examples.response[code]` now accept either a single value or a map of
named Example Objects with their own summary and description;
- a response content type per status code, for the endpoints answering images,
plain text or files instead of JSON;
- response headers per status code, merged with the rate limit ones the router
already documents.
`operationId` now follows the convention of the ids published at
developer.rocket.chat (`get-api-v1-banners-id`) instead of a camel case variant,
so deep links and generated clients survive the change of source.
Documented paths also collapse duplicate slashes, which were leaking into three
path keys (`/api//info`, `/api//docs/json`, `/api/apps//{id}/export-logs`) when a
router prefixed a subpath that already started with one.
The four error payload schemas were inlined into every operation that declares them, so the document repeated them 372 times - and repeated the same spec violation with them: the `details` field of a bad request allowed `type: array` with no `items`, which no OpenAPI tool accepts. A schema that carries an `$id` is now hoisted into `components.schemas` and referenced, so `BadRequestError`, `UnauthorizedError`, `ForbiddenError` and `NotFoundError` are described once. The mechanism is generic: any shared schema that names itself is deduplicated the same way. Also adds the missing `items` to the two arrays that lacked it - the `details` of a bad request and the `logs` of the app log endpoints. Both are left as an empty schema on purpose: the keyword is required on arrays, but the payloads really do hold anything the endpoint attached, and constraining them here would start rejecting responses at runtime.
`/api/docs/json` answered `API.default.success(document)`, which adds a `success` key to the root of the document. It is not an OpenAPI field, and validators reject it (`Property 'success' is not expected here` at `#/`). Also adds `items` to the eight remaining arrays whose schema lacked it - the spec requires the keyword on every array, and validators refuse the schema without it.
The document declared 3.0.3 while carrying schemas written in JSON Schema 2020, the dialect the endpoints validate against at runtime. Validators rejected 51 of them: union types (`type: ["number", "null"]`, 39 of them), `const` (9), `unevaluatedProperties` and `propertyNames`. OpenAPI 3.1 *is* JSON Schema 2020, so declaring it makes the document honest and all 51 errors disappear without touching a single schema. `typia` now generates its component schemas for 3.1 as well, which drops `nullable` in favour of the union types 3.1 accepts. The UI we serve (swagger-ui 5) renders 3.1 documents.
Generating the component schemas for OpenAPI 3.1 exposed two places where typia's output no longer matched what the runtime expects: - tuples come out with `prefixItems` (JSON Schema 2020) next to `additionalItems`, a keyword 2020 replaced with `items`. AJV runs in 2020 and aborts on the unknown keyword - `Error: strict mode: unknown keyword: "additionalItems"` - taking the server down at boot. The rename happens once, on the generated output, together with the `minItems` a closed tuple implies and AJV asks for; - single valued types come out as `const` instead of `enum`, which silently broke the heuristic that locks down the plain file attachment branch, bringing back the ambiguous `oneOf` that fails response validation for messages carrying files. Both spellings are now recognized. AJV also implements `discriminator` but rejects its `mapping`, which 3.1 output includes. It is dropped from the copy AJV compiles, and kept in the one the document serializes, where it is what tools actually use.
Declaring the document as 3.1 left 725 structural errors behind: `nullable`, the way 3.0 marked a field as accepting null, is not a JSON Schema keyword, so 3.1 rejects it. The 895 schemas that use it keep it - AJV understands it - and the document now carries the 3.1 equivalent instead, a `null` member of `type`, converted on a copy while each operation is built. Also drops the trailing slash from the server url, which `Site_Url` often carries and which would make every path in the document resolve with a double one.
The document declares security schemes, so an operation with no `security` field is indistinguishable from one that forgot to declare it, and validators flag all 19 public endpoints (`shield.svg`, `pw.getPolicy`, `method.callAnon` and friends). Every operation now spells its requirement out: the auth headers when the route requires them, both the headers and nothing when it accepts anonymous access, and an empty list when it is public.
A validator that compiles the component schemas with AJV cannot register `IMessage`, `IRoom`, `IIntegrationHistory`, `PartialIMessage` or `IUploadWithUser`, because AJV implements `discriminator` but rejects its `mapping`. Everything that references those schemas then reports `can't resolve reference`, which is how one unsupported keyword turns into 38 errors and stops a generator that refuses to run on an invalid document. The mapping is now dropped where the schemas are generated, so the document and the runtime share it, and the strip the API bootstrap did for AJV alone is gone. What the mapping described is still there: each branch carries the `const` of its discriminating property. Also defines the `_id` and `t` that the loose `channels.info` branch requires, so the schema stops requiring properties it never declares.
Every 2xx body is typed as `{ success: true } & T`, so returning the OpenAPI document without the envelope did not compile - and the lint task, which runs the same typecheck, failed with it.
…ypes Every route exports its options to its callers through `Endpoints`, minus the response validators. With documentation in those options - summary, description, examples, tags - the augmentation grew a type graph large enough to collapse: `Type 'Endpoints' recursively references itself as a base type`, fifty times, and two thousand errors cascading through the client that consumes those types. Documentation describes an endpoint for readers and has no place in the type its callers see, so it is omitted alongside the response validators. Also destructures in the test what the lint rule asks to destructure.
Omitting summary, description, examples and tags from what `Endpoints` exposes did not stop the augmentation from collapsing: the trigger is the tags being declared in the route options at all, not what the type exposes afterwards. The fix belongs where the tags are written, so this goes back to what it was.
Generating for 3.1 turned every nullable field into `oneOf: [{ type: 'null' },
{ type: 'string' }]`, where 3.0 wrote `nullable`. The API validates responses with
`coerceTypes`, which coerces the value for each branch in turn until more than one
matches - and then `oneOf`, meaning exactly one, rejects a perfectly valid payload:
at path '/data/9/users/0/avatarETag': must match exactly one schema in oneOf
(passingSchemas: 0,1)
That is what took `video-conference.list` down in the apps test suite. Branches that
only name a type now collapse into a single `type` array, which says the same thing
and leaves nothing to disambiguate. Verified against the payload from the failing
run: it validates with the collapse and reproduces the failure without it.
The discriminator strip is also scoped to the discriminator now, instead of dropping
any property named `mapping` wherever it appeared.
Member
Author
|
/jira ARCH-1464 |
ggazzo
marked this pull request as ready for review
August 3, 2026 14:53
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/core-typings/src/Ajv.ts (1)
75-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation comments from these TypeScript files.
These new comments violate the repository TypeScript guideline.
packages/core-typings/src/Ajv.ts#L75-L92: remove the normalization rationale comment.packages/rest-typings/src/v1/Ajv.ts#L23-L24: remove the AJV vocabulary comment.packages/rest-typings/src/v1/Ajv.ts#L66-L67: remove the unrestricted-array comment.apps/meteor/server/api/validation/ajv.ts#L27-L28: remove the Typia version-format comment.As per coding guidelines, “Avoid code comments in the implementation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core-typings/src/Ajv.ts` around lines 75 - 92, Remove the implementation rationale comments at packages/core-typings/src/Ajv.ts lines 75-92, packages/rest-typings/src/v1/Ajv.ts lines 23-24 and 66-67, and apps/meteor/server/api/validation/ajv.ts lines 27-28, while leaving the surrounding AJV normalization and validation logic unchanged.Source: Coding guidelines
packages/http-router/src/openapi.spec.ts (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion for the optional path segment parameter.
The suite converts
/api/v1/settings/:_id?to a templated path, but it never asserts the resulting parameter object. Add a case that checks therequiredflag of the derived parameter for an optional segment. This locks the behavior discussed onpackages/http-router/src/openapi.tsLines 284-304.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http-router/src/openapi.spec.ts` around lines 27 - 36, The toOpenAPIPath tests only verify the converted path string, not the optional parameter metadata. Extend the relevant test coverage around toOpenAPIPath to inspect the derived OpenAPI parameter for the optional _id segment and assert that its required flag is false.apps/meteor/tests/end-to-end/api/openapi.ts (1)
84-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that every
$refresolves insidecomponents.schemas.The router hoists any schema carrying an
$idintocomponents.schemasand replaces it with a$ref. No test verifies that each emitted$reftarget exists. A dangling reference breaks Swagger UI and code generators while every current assertion still passes. Walk the document, collect each$refvalue, and assert the referenced key exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/end-to-end/api/openapi.ts` around lines 84 - 93, Extend the OpenAPI validation tests near “should name and schema every parameter” to recursively walk the generated document, collect every $ref value, and verify each target key exists in components.schemas. Preserve the existing parameter assertions and ensure references emitted by the router’s $id hoisting are checked for dangling targets.packages/http-router/src/openapi.ts (1)
229-248: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDetect conflicting
$idregistrations insharedSchemas.
referenceSchemawrites into the module-levelsharedSchemasmap keyed only by$id. The repository compiles schemas with several AJV instances (for exampleajvandajvQueryinpackages/rest-typings), so AJV cannot detect a duplicate$idacross instances. If two different schemas share an$id, the last registration wins and every operation that referenced the first one points at the wrong schema. Consider comparing an incoming definition with an existing entry and failing fast on a mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http-router/src/openapi.ts` around lines 229 - 248, Update referenceSchema to detect conflicting definitions before overwriting an existing sharedSchemas entry: when the same $id is already registered, compare the incoming definition with the stored one and fail fast if they differ; retain the existing entry and reference behavior when they match, while preserving registration for new IDs.packages/rest-typings/src/default/index.ts (1)
53-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the new root fields in the
/docs/jsonresponse type.
makeOpenAPIResponseinapps/meteor/server/api/default/openApi.tsnow emitsexternalDocsandtagsat the document root. This type does not declare them, so typed clients cannot read those fields. Add them to keep the endpoint type aligned with the served document.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rest-typings/src/default/index.ts` around lines 53 - 83, Update the /docs/json GET response type in the endpoint definitions to declare the root-level externalDocs and tags fields emitted by makeOpenAPIResponse, preserving their OpenAPI document shapes so typed clients can access them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/api/default/openApi.ts`:
- Around line 75-80: Update the OpenAPI document construction around the servers
configuration to omit the entire servers entry when Site_Url is unset, rather
than creating a Server Object with an undefined url. Preserve the existing
trailing-slash removal when Site_Url is present and ensure the resulting
document contains no servers array in the unset case.
In `@packages/http-router/src/openapi.ts`:
- Around line 284-304: The OpenAPI path parameter builder must mark every
templated path parameter as required, including Express optional segments. In
packages/http-router/src/openapi.ts lines 284-304, update buildPathParameters or
extractPathParameterNames so derived parameters use required: true. In
packages/http-router/src/openapi.spec.ts lines 27-36, add coverage for
/api/v1/settings/:_id? and assert the derived parameter includes required: true.
- Around line 465-472: Update the response-status check guarding the injected
response in the OpenAPI generation flow so any declared status below 400 counts
as a documented outcome, including redirects such as 302 and 307. Preserve the
existing fallback 200 response for routes with no declared status below 400.
- Around line 214-219: Update the nullable handling in toOpenAPI31 so primitive
string types retain the existing type-plus-null behavior, arrays return
rest.type after ensuring null is included, and missing or non-array type schemas
are not spread as string arrays. Specifically guard the final branch against
absent or object-valued rest.type and return the schema safely for type: {
nullable: true }.
In `@packages/http-router/src/Router.ts`:
- Around line 339-344: Update the typedRoutes merge in the Router method
handling innerRouter. When multiple entries normalize to the same OpenAPI path,
preserve the existing method map from this.typedRoutes and merge the nested
routes into it rather than replacing the entire map; ensure nested methods
override only matching methods while parent-only operations remain.
---
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/openapi.ts`:
- Around line 84-93: Extend the OpenAPI validation tests near “should name and
schema every parameter” to recursively walk the generated document, collect
every $ref value, and verify each target key exists in components.schemas.
Preserve the existing parameter assertions and ensure references emitted by the
router’s $id hoisting are checked for dangling targets.
In `@packages/core-typings/src/Ajv.ts`:
- Around line 75-92: Remove the implementation rationale comments at
packages/core-typings/src/Ajv.ts lines 75-92,
packages/rest-typings/src/v1/Ajv.ts lines 23-24 and 66-67, and
apps/meteor/server/api/validation/ajv.ts lines 27-28, while leaving the
surrounding AJV normalization and validation logic unchanged.
In `@packages/http-router/src/openapi.spec.ts`:
- Around line 27-36: The toOpenAPIPath tests only verify the converted path
string, not the optional parameter metadata. Extend the relevant test coverage
around toOpenAPIPath to inspect the derived OpenAPI parameter for the optional
_id segment and assert that its required flag is false.
In `@packages/http-router/src/openapi.ts`:
- Around line 229-248: Update referenceSchema to detect conflicting definitions
before overwriting an existing sharedSchemas entry: when the same $id is already
registered, compare the incoming definition with the stored one and fail fast if
they differ; retain the existing entry and reference behavior when they match,
while preserving registration for new IDs.
In `@packages/rest-typings/src/default/index.ts`:
- Around line 53-83: Update the /docs/json GET response type in the endpoint
definitions to declare the root-level externalDocs and tags fields emitted by
makeOpenAPIResponse, preserving their OpenAPI document shapes so typed clients
can access them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0076f859-3787-4a1a-9bcb-f79a5c857d20
📒 Files selected for processing (24)
.changeset/loud-otters-document.md.changeset/plain-lions-agree.mdapps/meteor/ee/server/apps/communication/endpoints/appGeneralLogsHandler.tsapps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/default/openApi.tsapps/meteor/server/api/definition.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/channels.tsapps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/stats.tsapps/meteor/server/api/v1/users.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/tests/end-to-end/api/openapi.tspackages/core-typings/src/Ajv.tspackages/http-router/src/Router.spec.tspackages/http-router/src/Router.tspackages/http-router/src/definition.tspackages/http-router/src/index.tspackages/http-router/src/openapi.spec.tspackages/http-router/src/openapi.tspackages/rest-typings/src/default/index.tspackages/rest-typings/src/v1/Ajv.tspackages/rest-typings/src/v1/banners.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/http-router/src/index.tsapps/meteor/ee/server/apps/communication/endpoints/appGeneralLogsHandler.tsapps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/channels.tspackages/http-router/src/Router.spec.tsapps/meteor/server/api/definition.tspackages/http-router/src/definition.tspackages/rest-typings/src/default/index.tsapps/meteor/server/api/validation/ajv.tspackages/http-router/src/openapi.spec.tsapps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.tspackages/rest-typings/src/v1/Ajv.tsapps/meteor/server/api/v1/stats.tsapps/meteor/tests/end-to-end/api/openapi.tspackages/rest-typings/src/v1/banners.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/users.tspackages/core-typings/src/Ajv.tsapps/meteor/server/api/default/openApi.tsapps/meteor/server/api/ApiClass.tspackages/http-router/src/Router.tspackages/http-router/src/openapi.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/http-router/src/Router.spec.tspackages/http-router/src/openapi.spec.ts
🧠 Learnings (10)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/http-router/src/index.tsapps/meteor/ee/server/apps/communication/endpoints/appGeneralLogsHandler.tsapps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/channels.tspackages/http-router/src/Router.spec.tsapps/meteor/server/api/definition.tspackages/http-router/src/definition.tspackages/rest-typings/src/default/index.tsapps/meteor/server/api/validation/ajv.tspackages/http-router/src/openapi.spec.tsapps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.tspackages/rest-typings/src/v1/Ajv.tsapps/meteor/server/api/v1/stats.tsapps/meteor/tests/end-to-end/api/openapi.tspackages/rest-typings/src/v1/banners.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/users.tspackages/core-typings/src/Ajv.tsapps/meteor/server/api/default/openApi.tsapps/meteor/server/api/ApiClass.tspackages/http-router/src/Router.tspackages/http-router/src/openapi.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/http-router/src/index.tsapps/meteor/ee/server/apps/communication/endpoints/appGeneralLogsHandler.tsapps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/channels.tspackages/http-router/src/Router.spec.tsapps/meteor/server/api/definition.tspackages/http-router/src/definition.tspackages/rest-typings/src/default/index.tsapps/meteor/server/api/validation/ajv.tspackages/http-router/src/openapi.spec.tsapps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.tspackages/rest-typings/src/v1/Ajv.tsapps/meteor/server/api/v1/stats.tsapps/meteor/tests/end-to-end/api/openapi.tspackages/rest-typings/src/v1/banners.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/users.tspackages/core-typings/src/Ajv.tsapps/meteor/server/api/default/openApi.tsapps/meteor/server/api/ApiClass.tspackages/http-router/src/Router.tspackages/http-router/src/openapi.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/http-router/src/index.tsapps/meteor/ee/server/apps/communication/endpoints/appGeneralLogsHandler.tsapps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/channels.tspackages/http-router/src/Router.spec.tsapps/meteor/server/api/definition.tspackages/http-router/src/definition.tspackages/rest-typings/src/default/index.tsapps/meteor/server/api/validation/ajv.tspackages/http-router/src/openapi.spec.tsapps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.tspackages/rest-typings/src/v1/Ajv.tsapps/meteor/server/api/v1/stats.tsapps/meteor/tests/end-to-end/api/openapi.tspackages/rest-typings/src/v1/banners.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/users.tspackages/core-typings/src/Ajv.tsapps/meteor/server/api/default/openApi.tsapps/meteor/server/api/ApiClass.tspackages/http-router/src/Router.tspackages/http-router/src/openapi.ts
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/plain-lions-agree.md.changeset/loud-otters-document.md
📚 Learning: 2026-07-29T23:45:21.859Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41632
File: apps/meteor/server/api/v1/groups.ts:948-959
Timestamp: 2026-07-29T23:45:21.859Z
Learning: For API v1 routes under apps/meteor/server/api/v1, keep item-level response schemas strict by using `$ref`-based schemas for list and messages (and ensure they intentionally mirror the corresponding route contracts, as done in channels.ts). Only use “loose”/non-`$ref` item schemas when the underlying data source is inherently partial (e.g., uploads where `content` can be `null`, or queries like `findUsersOfRoom` with a fixed projection). Do not relax item schemas merely because the route supports an optional client `fields` projection—optional field selection alone is not a reason to change schema strictness.
Applied to files:
apps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/channels.tsapps/meteor/server/api/v1/stats.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/users.ts
📚 Learning: 2026-07-31T02:44:35.111Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/sessions.ts:114-138
Timestamp: 2026-07-31T02:44:35.111Z
Learning: In Rocket.Chat typed REST response schemas, accept the composition of a Typia-generated entity schema with an `allOf` branch requiring `success: true`: `allOf: [{ $ref: <entity schema> }, { properties: { success: { type: 'boolean', enum: [true] } }, required: ['success'] }]`. Do not flag this pattern when used for REST endpoints, provided TEST_MODE response validation passes, as demonstrated by the `IOAuthApps` and `IEmailInbox` endpoints.
Applied to files:
apps/meteor/server/api/v1/misc.tsapps/meteor/server/api/v1/channels.tsapps/meteor/server/api/definition.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/server/api/v1/stats.tsapps/meteor/server/api/v1/banners.tsapps/meteor/server/api/v1/users.tsapps/meteor/server/api/default/openApi.tsapps/meteor/server/api/ApiClass.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
packages/http-router/src/Router.spec.tspackages/http-router/src/openapi.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
packages/http-router/src/Router.spec.tspackages/http-router/src/openapi.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
packages/http-router/src/Router.spec.tspackages/http-router/src/openapi.spec.ts
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.
Applied to files:
packages/rest-typings/src/default/index.tspackages/rest-typings/src/v1/Ajv.tspackages/rest-typings/src/v1/banners.ts
🔇 Additional comments (33)
packages/core-typings/src/Ajv.ts (1)
32-73: LGTM!Also applies to: 93-129
packages/rest-typings/src/v1/Ajv.ts (1)
25-26: LGTM!Also applies to: 59-65, 68-68, 85-85, 109-109, 130-130
packages/rest-typings/src/v1/banners.ts (2)
19-20: LGTM!Also applies to: 29-40, 54-55
42-42: 🎯 Functional CorrectnessNo duplicate declaration issue remains.
The referenced declarations each occur only once in their respective blocks.
> Likely an incorrect or invalid review comment.apps/meteor/ee/server/apps/communication/endpoints/appGeneralLogsHandler.ts (1)
36-36: LGTM!apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts (1)
34-34: LGTM!apps/meteor/server/api/v1/channels.ts (1)
136-144: LGTM!apps/meteor/server/api/v1/misc.ts (1)
498-498: LGTM!apps/meteor/server/api/v1/stats.ts (1)
27-27: LGTM!apps/meteor/server/api/v1/users.ts (1)
787-787: LGTM!Also applies to: 1522-1522, 1683-1683, 1832-1832
apps/meteor/server/api/validation/ajv.ts (1)
30-31: LGTM!packages/http-router/src/Router.spec.ts (1)
755-784: LGTM!.changeset/plain-lions-agree.md (1)
1-6: LGTM!packages/http-router/src/openapi.ts (6)
3-110: LGTM!
306-332: LGTM!
334-412: LGTM!
414-439: LGTM!
483-520: LGTM!
522-531: LGTM!packages/http-router/src/Router.ts (1)
10-11: LGTM!Also applies to: 79-82
packages/http-router/src/definition.ts (1)
5-6: LGTM!Also applies to: 49-59
packages/http-router/src/index.ts (1)
2-2: LGTM!packages/http-router/src/openapi.spec.ts (2)
1-25: LGTM!
274-325: LGTM!apps/meteor/server/api/v1/banners.ts (1)
5-5: LGTM!Also applies to: 35-46, 67-76, 96-105
apps/meteor/server/api/default/openApi.ts (4)
3-11: LGTM!Also applies to: 46-61
104-117: LGTM!
131-137: LGTM!
146-148: No change needed.docs/jsonsetsauthRequired: false, so Swagger UI can fetch the relative URL without authentication headers.apps/meteor/tests/end-to-end/api/openapi.ts (1)
1-45: LGTM!apps/meteor/server/api/ApiClass.ts (1)
2-2: No change needed fortypedRoutesusage.
API.apiis aRocketChatAPIRouter, andRouterexposestypedRoutes; no remainingAPIClass.typedRoutesconsumers exist..changeset/loud-otters-document.md (1)
1-6: No action required.
@rocket.chat/http-routeris private, so separate.changesetentries should not include it, even though the package gained OpenAPI-related exports/type changes.> Likely an incorrect or invalid review comment.apps/meteor/server/api/definition.ts (1)
289-299: No change needed.SharedOptionsandOpenAPIDocumentationdo not redeclare the OpenAPI documentation properties with conflicting types.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/api/v1/stats.ts">
<violation number="1" location="apps/meteor/server/api/v1/stats.ts:27">
P3: `items: {}` is an empty JSON Schema, so in the generated OpenAPI document the statistics.paginated list endpoint's array items render as an untyped placeholder rather than the actual element shape. Since the goal of this PR is richer OpenAPI metadata, this doesn't add useful item documentation and is effectively a no-op (any value passes it). Consider typing the items (e.g. `items: { type: 'object' }`), or better, referencing the `IStats` schema, so consumers of `/api/docs/json` get a meaningful item type.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| type: 'object', | ||
| properties: { | ||
| statistics: { type: 'array' }, | ||
| statistics: { type: 'array', items: {} }, |
Contributor
There was a problem hiding this comment.
P3: items: {} is an empty JSON Schema, so in the generated OpenAPI document the statistics.paginated list endpoint's array items render as an untyped placeholder rather than the actual element shape. Since the goal of this PR is richer OpenAPI metadata, this doesn't add useful item documentation and is effectively a no-op (any value passes it). Consider typing the items (e.g. items: { type: 'object' }), or better, referencing the IStats schema, so consumers of /api/docs/json get a meaningful item type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/api/v1/stats.ts, line 27:
<comment>`items: {}` is an empty JSON Schema, so in the generated OpenAPI document the statistics.paginated list endpoint's array items render as an untyped placeholder rather than the actual element shape. Since the goal of this PR is richer OpenAPI metadata, this doesn't add useful item documentation and is effectively a no-op (any value passes it). Consider typing the items (e.g. `items: { type: 'object' }`), or better, referencing the `IStats` schema, so consumers of `/api/docs/json` get a meaningful item type.</comment>
<file context>
@@ -24,7 +24,7 @@ const statisticsResponseSchema = ajv.compile<IStats>({
type: 'object',
properties: {
- statistics: { type: 'array' },
+ statistics: { type: 'array', items: {} },
count: { type: 'number' },
offset: { type: 'number' },
</file context>
- a path parameter is always required: an optional express segment describes a different path, not an optional parameter, and the spec has no room for one; - a route that declares only a redirect no longer receives an invented `200` with a schema it never answers - anything below 400 counts as documented; - `nullable` on a schema with no usable `type` is left alone instead of spreading a missing value, which would have failed the route registration; - `x-permissions` and the description read the permissions of the method over the wildcard ones, the way `checkPermissionsForInvocation` does at runtime; - a nested router's paths merge into the parent per path: normalizing the keys can bring two distinct paths onto the same one, and replacing the map would drop the methods the other router documented; - an unset `Site_Url` means no `servers` at all, rather than a Server Object with no url, which the spec does not allow; - the `/api-docs` UI reads the document relative to itself, so a workspace hosted under ROOT_URL_PATH_PREFIX keeps its prefix; - the `additionalItems` rename is confined to the keyword: inside `properties` and friends, a name is a field, not a keyword. Same mistake the discriminator strip made; - the API test states the relation between the two documents instead of demanding that untyped routes exist, since they are meant to disappear.
The review fixes were copied across from the branch stacked on top of this one, and brought its tag map with them: this branch has no operationTags module, so the typecheck could not resolve the import.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
The document served at
/api/docs/jsonwas missing most of what makes an OpenAPI spec usable, and part of it was not even valid:/api/v1/banners/:id) and noparametersentries at all, so every/:idendpoint was unusable in Swagger UI and in generated clients;queryholding the whole object schema, always marked as required;responses[].descriptionwas hardcoded to'', and the field is required by the spec;permissionsRequired,rateLimiterOptions,queryFields,typed) leaked into the operation object, and only when the route required auth;schemaswas duplicated at the root of the document, which is not an OpenAPI field.The document is now OpenAPI 3.1
It declared 3.0.3 while carrying schemas written in JSON Schema 2020 - the dialect the endpoints validate against at runtime - so validators rejected 51 of them: union types,
const,unevaluatedProperties,propertyNames. 3.1 is JSON Schema 2020, so declaring it makes the document honest and those errors disappear without touching a single schema.typiagenerates the component schemas for 3.1 as well.Generating for 3.1 surfaced three places where its output no longer matched what AJV reads, all fixed where the schemas are generated:
prefixItemsnext toadditionalItems, a keyword 2020 replaced withitems. AJV aborts on the unknown keyword, which took the server down at boot;oneOf: [{ type: 'null' }, { type: 'string' }]. The API validates withcoerceTypes, which coerces the value for each branch until more than one matches, and thenoneOf- exactly one - rejects a valid payload. Branches that only name a type collapse into a singletypearray;discriminator.mapping, which AJV rejects outright: a validator that chokes onIMessageleaves every schema referencing it unresolvable, and one unsupported keyword became 38 errors in an external generator. What the mapping described is still in theconstof each branch.Single valued types also stopped being recognized by the heuristic that closes the plain file attachment branch, which had silently brought back an ambiguous
oneOf; both spellings are recognized now.Shared error payloads and explicit security
The four error payload schemas were inlined into every operation that declares them - 372 copies of the same schema, and of the same spec violation, since the
detailsof a bad request allowedtype: arraywith noitems. A schema that carries an$idis now hoisted intocomponents.schemasand referenced, soBadRequestError,UnauthorizedError,ForbiddenErrorandNotFoundErrorare described once.Every operation also declares its security requirement: the auth headers when the route requires them, both the headers and nothing when it accepts anonymous access, and an empty list when it is public. Without it, a public endpoint is indistinguishable from one that forgot to declare anything, and validators flag all 19 of them.
Route options
Endpoints can now declare
summary,description,operationId,deprecated,externalDocs,params,bodyContentType,responseDescriptionsandexamples. Everything else is derived from options the framework already needs:permissionsRequiredx-permissions+ a line in the descriptionlicensex-license+ a line in the descriptiontwoFactorRequiredx-two-factor-required, thex-2fa-code/x-2fa-methodheader parameters, and a403deprecationdeprecated: true+ the target version and the alternativesrateLimiterOptionsX-RateLimit-*response headers and a429authRequiredsecurity+ a401Error responses implied by the route (
400,401,403,429,500) are injected when the route does not declare them, and undocumented (legacy) routes get a generic200so their operation is at least valid.Parameters
Path parameters are derived from the path pattern itself, so every
{param}in the document has a matching parameter object; the optionalparamsschema only adds descriptions, examples and types on top. Query schemas are exploded into one parameter per property with the realrequiredflag, followingallOf/anyOf/oneOfcomposition (allOfunions the requirements,anyOf/oneOfkeep only the ones shared by every branch).Self-documenting schemas
exampleis now registered as an AJV vocabulary, so a schema can carry its owndescriptionandexampleand have them flow straight into the document with no framework change.packages/rest-typings/src/v1/banners.tsshows the pattern.Cleanup
Both copies of the conversion were replaced by a single builder in
@rocket.chat/http-router. The copy living inAPIClasswrote to atypedRoutesfield that nothing ever read — the served document has always come from the router — so it and its legacy counterpart were removed. The three stale@openapiJSDoc blocks inbanners.ts(dead since the switch to the runtime generator) were migrated into the route options and deleted.Issue(s)
Steps to test or reproduce
/api-docs; the banners endpoints now show a summary, a description, a typedidpath parameter with an example, and aplatformquery field instead of a JSON blob.curl -s localhost:3000/api/docs/json | jq '.paths["/api/v1/banners/{id}"].get'curl -s 'localhost:3000/api/docs/json?withUndocumented=true' | jq -r '.paths | keys[]' | grep ':'returns nothing.yarn workspace @rocket.chat/http-router testapps/meteor/tests/end-to-end/api/openapi.tsasserts the invariants over every registered route — no Express syntax left, every path template has a parameter, every operation has at least one response and every response a description, uniqueoperationIds, and undocumented routes stay hidden unless asked for.Further comments
Measured on the document a running server generates, validated with
redocly lintand with an AJV based validator:nullable725 to 0, arrays withoutitems382 to 0, structural errors 51 to 0, unresolvable$ref37 to 0, operations withoutsecurity19 to 0,successno longer at the root of the document. What is left is 58operation-summarywarnings, for endpoints nothing describes yet.Bugs found while wiring this up:
operationIdhas to be generated by the document builder, not at registration time: routers register their operations before the parent router prefixes them, so generating it early producedgetV1BannersIdinstead ofgetApiV1BannersId. It is now filled in bywithOperationIdsfrom the final path.403was only injected forpermissionsRequiredin array form; the{ POST: { operation, permissions } }shape was silently skipped.Deliberately left out:
params— path values always arrive as strings and the endpoints already validate them, so the schema is documentation only (marked in the type);validateParamsintoquery/bodyfor documentation purposes: the router validates whatever it is given, so documenting those 274 legacy routes that way would enable a second round of validation on them;openapi.jsonartifact and adding spectral/oasdiff to CI. Worth doing next; the API test is the minimum gate in the meantime;items: {}, the empty schema, which is what the spec asks for and what validators accept, but documents nothing. Constraining them here would start rejecting real responses at runtime -detailsand the app logs really do hold anything the endpoint attached - so a meaningful shape has to be written per endpoint.Task: ARCH-2324
Summary by CodeRabbit
New Features
Bug Fixes
Tests