chore: Add OpenAPI Support to push.test API#36882
chore: Add OpenAPI Support to push.test API#36882ggazzo merged 11 commits intoRocketChat:developfrom
Conversation
|
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: 63a58e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
WalkthroughMigrates the push.test endpoint to a chained Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API.v1 /v1/push.test
participant RL as RateLimiter
participant Auth as Auth/Permissions
participant AJV as AJV Validators
participant Conf as Settings (Push_enable)
participant Svc as executePushTest
Client->>API: POST /v1/push.test
API->>RL: Check rate limit (1 req / 1000ms)
RL-->>API: Allowed / Throttled
alt Throttled
API-->>Client: 429 Too Many Requests
else Allowed
API->>Auth: Validate auth + "test-push-notifications"
Auth-->>API: Authorized / Unauthorized
alt Unauthorized
API-->>Client: 401 Unauthorized (validated schema)
else Authorized
API->>AJV: Validate body (empty object schema)
AJV-->>API: Valid / Invalid
alt Invalid
API-->>Client: 400 Bad Request (validated schema)
else Valid
API->>Conf: Read Push_enable
Conf-->>API: true / false
alt Push disabled
API-->>Client: 400 Bad Request (error)
else Push enabled
API->>Svc: executePushTest()
Svc-->>API: tokensCount:number
API-->>Client: 200 { tokensCount, success: true } (validated schema)
end
end
end
end
rect rgba(200,235,255,0.25)
note right of API: New: chained route registration, AJV body/response validators, rate limiting, permissions, and OpenAPI typings via augmentation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36882 +/- ##
===========================================
- Coverage 66.60% 63.33% -3.27%
===========================================
Files 3346 2911 -435
Lines 114661 107327 -7334
Branches 21099 19329 -1770
===========================================
- Hits 76365 67977 -8388
- Misses 35601 37346 +1745
+ Partials 2695 2004 -691
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/app/api/server/v1/push.ts (2)
169-173: Use the route name in error context for consistency.Minor: align the
methodfield with the endpoint id.Apply this diff:
- throw new Meteor.Error('error-push-disabled', 'Push is disabled', { - method: 'push_test', - }); + throw new Meteor.Error('error-push-disabled', 'Push is disabled', { + method: 'push.test', + });
180-185: Potential naming confusion with PushEndpoints.You now have a local
PushEndpointsalias and aPushEndpointstype in@rocket.chat/rest-typings(sans push.test). It compiles due to scope, but can be confusing to readers. Consider renaming the local alias (e.g.,PushTestEndpoints) and extendingEndpointsfrom that.-export type PushEndpoints = ExtractRoutesFromAPI<typeof pushEndpoints>; +export type PushTestEndpoints = ExtractRoutesFromAPI<typeof pushEndpoints>; declare module '@rocket.chat/rest-typings' { // eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface - interface Endpoints extends PushEndpoints {} + interface Endpoints extends PushTestEndpoints {} }.changeset/spicy-drinks-carry.md (1)
6-6: Nit: singular “endpoint”.This changeset affects a single route (
push.test). Tweak grammar for clarity.-Add OpenAPI support for the Rocket.Chat push.test API endpoints by migrating to a modern chained route definition syntax and utilizing shared AJV schemas for validation to enhance API documentation and ensure type safety through response validation. +Add OpenAPI support for the Rocket.Chat push.test API endpoint by migrating to a modern chained route definition syntax and utilizing shared AJV schemas for validation to enhance API documentation and ensure type safety through response validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/spicy-drinks-carry.md(1 hunks)apps/meteor/app/api/server/v1/push.ts(4 hunks)packages/rest-typings/src/v1/push.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/v1/push.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/push.ts (4)
apps/meteor/app/api/server/api.ts (1)
API(42-77)packages/rest-typings/src/v1/Ajv.ts (3)
ajv(23-23)validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/app/api/server/v1/push.ts (4)
4-4: Good reuse of shared validators.Nice to see centralized AJV and error schemas being used.
13-13: Type helper import looks correct.Matches the pattern used elsewhere with ExtractRoutesFromAPI.
175-176: Confirmthis.useris always defined underauthRequired: true.If there’s any path where
this.usermight be undefined,this.user.usernamecould throw. Likely safe, but worth a quick check.
150-166: ```shell
#!/bin/bash
set -euo pipefailecho "=== Showing apps/meteor/app/api/server/v1/push.ts (up to 320 lines) ==="
if [ -f "apps/meteor/app/api/server/v1/push.ts" ]; then
sed -n '1,320p' apps/meteor/app/api/server/v1/push.ts || true
else
echo "File not found: apps/meteor/app/api/server/v1/push.ts"
fiecho
echo "=== Searching repo for 403/429 related validators / rest-typings ==="
rg -n -C2 --hidden --no-ignore '\bvalidate(?:Forbidden|ForbiddenError|ForbiddenResponse|TooManyRequests|TooMany|TooManyRequestsError|TooManyRequestsResponse)\b' || true
rg -n -C2 --hidden --no-ignore '\b(forbidden|too[-_ ]many|tooManyRequests|rate[-_ ]limit|rateLimiter|permissionsRequired)\b' || true
rg -n -C2 --hidden --no-ignore 'rest-typings' || trueecho
echo "=== If rest-typings directory exists, list files and search inside ==="
if [ -d "rest-typings" ]; then
echo "rest-typings exists; listing:"
ls -R rest-typings | sed -n '1,200p' || true
rg -n --hidden --no-ignore 'Forbidden|TooMany|TooManyRequests|forbidden|tooMany' rest-typings || true
else
echo "No rest-typings directory at repo root"
fi</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/api/server/v1/push.ts (1)
140-166: Add 403 and 429 OpenAPI responses; bound tokensCount to non-negative.
- Document 403 (permission denied) and 429 (rate limited) to match behavior.
tokensCountcan’t be negative; addminimum: 0.Apply this diff:
const pushEndpoints = API.v1.post( 'push.test', { authRequired: true, rateLimiterOptions: { numRequestsAllowed: 1, intervalTimeInMS: 1000, }, permissionsRequired: ['test-push-notifications'], - body: ajv.compile<undefined>({ type: 'object', additionalProperties: false }), + body: ajv.compile<undefined>({ type: 'object', additionalProperties: false }), response: { + 403: validateForbiddenErrorResponse, + 429: validateTooManyRequestsErrorResponse, type: 'object', properties: { - tokensCount: { type: 'integer' }, + tokensCount: { type: 'integer', minimum: 0 }, success: { type: 'boolean', enum: [true], }, }, required: ['tokensCount', 'success'], additionalProperties: false, }), }, }, )
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/push.ts (1)
149-149: Use Record<string, never> for an empty-body schema (fixes generic mismatch).Schema is an empty object, but the generic is
undefined. Align the type for correct inference and stricter typing.Apply this diff:
- body: ajv.compile<undefined>({ type: 'object', additionalProperties: false }), + body: ajv.compile<Record<string, never>>({ type: 'object', additionalProperties: false }),Optional: verify clients sending no body (Content-Length: 0) are treated as
{}by the router, otherwise this will 400.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/push.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/push.ts (4)
apps/meteor/app/api/server/api.ts (1)
API(42-77)packages/rest-typings/src/v1/Ajv.ts (3)
ajv(23-23)validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/app/api/server/v1/push.ts (3)
13-13: LGTM: type-only import for ExtractRoutesFromAPI is correct.
168-177: LGTM: handler logic is preserved and properly guarded by Push_enable.
4-4: Import 403 validator; 429 validator not exported in rest-typings.packages/rest-typings exports validateForbiddenErrorResponse but not validateTooManyRequestsErrorResponse — import the 403 validator and either add a 429 validator to packages/rest-typings or confirm the correct export name.
-import { ajv, validateBadRequestErrorResponse, validateUnauthorizedErrorResponse } from '@rocket.chat/rest-typings'; +import { + ajv, + validateBadRequestErrorResponse, + validateUnauthorizedErrorResponse, + validateForbiddenErrorResponse, +} from '@rocket.chat/rest-typings';
|
@ggazzo 👍 |
ggazzo
added
the
stat: QA assured
ggazzo
changed the title
3 participants
Description:
This PR integrates OpenAPI support into the
Rocket.Chat API, migrate ofRocket.Chat APIendpoints to the new OpenAPI pattern. The update includes improved API documentation, enhanced type safety, and response validation using AJV.Key Changes:
Issue Reference:
Relates to #34983, part of the ongoing OpenAPI integration effort.
Testing:
Verified that the API response schemas are correctly documented in Swagger UI.
All tests passed without any breaking changes
Endpoints:
Test Push Token
Looking forward to your feedback! 🚀
Summary by CodeRabbit
COMM-144