feat: Add OpenAPI Support to emoji-custom.create API #36523
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: 0492e84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
|
hey @cardoso the POST method lacks support for handling multipart/form-data payloads |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36523 +/- ##
===========================================
- Coverage 66.40% 65.61% -0.80%
===========================================
Files 3325 3222 -103
Lines 111301 110516 -785
Branches 21113 20723 -390
===========================================
- Hits 73913 72517 -1396
- Misses 34702 35389 +687
+ Partials 2686 2610 -76
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@ahmed-n-abdeltwab can you fix that in this PR? |
ahmed-n-abdeltwab
force-pushed
the
feat/openapi-emoji-custom-create
branch
from
121790a to
9eb457f
Compare
WalkthroughAdds OpenAPI/AJV-backed route handling for emoji-custom.create, integrates explicit 200/400 response schemas and error handling, exposes request on handler this-context, and adjusts/restores public typings by relocating the create endpoint’s typings from rest-typings into server route definitions with module augmentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API Router
participant emoji-custom.create Handler
participant Validator (AJV)
participant Emoji Service
participant Storage/DB
Client->>API Router: POST /v1/emoji-custom.create (payload)
API Router->>emoji-custom.create Handler: Invoke with this.request
emoji-custom.create Handler->>Validator (AJV): Validate request + prepare response schemas
alt Valid
emoji-custom.create Handler->>Emoji Service: insertOrUpdateEmoji()
Emoji Service->>Storage/DB: Upsert emoji metadata
Storage/DB-->>Emoji Service: OK / Data
emoji-custom.create Handler->>Emoji Service: uploadEmojiCustomWithBuffer()
Emoji Service->>Storage/DB: Store asset
Storage/DB-->>Emoji Service: OK
Emoji Service-->>emoji-custom.create Handler: Result
emoji-custom.create Handler-->>API Router: 200 { structured response }
API Router-->>Client: 200
else Invalid or Error
note over emoji-custom.create Handler: try/catch logs error
emoji-custom.create Handler-->>API Router: 400 { failure payload }
API Router-->>Client: 400
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![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 (2)
apps/meteor/app/api/server/v1/emoji-custom.ts (2)
158-171: Consider including error details in the failure response.The catch block logs the error but returns a generic
API.v1.failure()without any error message. This makes debugging difficult for API consumers. Consider passing error details to the failure response.Apply this diff to include error context:
} catch (e) { SystemLogger.error(e); - return API.v1.failure(); + return API.v1.failure(e instanceof Error ? e.message : 'Failed to create emoji'); }
252-254: Consider the purpose of the EmojiCustomEndpoints alias.The
EmojiCustomEndpointstype is an alias toEmojiCustomCreateEndpoints, which only includes the create endpoint. This naming might be misleading since it suggests it covers all emoji-custom endpoints when it only covers one.If the intent is to eventually include all emoji-custom endpoints in this type, consider:
- Using a more specific name like
EmojiCustomCreateEndpoint(singular), or- Documenting the planned expansion, or
- Directly exporting
EmojiCustomCreateEndpointsif the alias doesn't add clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/weak-terms-shave.md(1 hunks)apps/meteor/app/api/server/definition.ts(1 hunks)apps/meteor/app/api/server/v1/emoji-custom.ts(4 hunks)packages/rest-typings/src/v1/emojiCustom.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/v1/emojiCustom.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/emoji-custom.ts (5)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
getUploadFormData(55-195)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/v1/emojiCustom.ts (1)
EmojiCustomEndpoints(53-73)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
🔇 Additional comments (6)
.changeset/weak-terms-shave.md (1)
1-7: LGTM!The changeset correctly documents the OpenAPI support addition as patch-level changes for both affected packages. The description accurately summarizes the enhancement.
apps/meteor/app/api/server/v1/emoji-custom.ts (4)
4-4: LGTM!The import additions for
ajvandisEmojiCustomListare appropriate for the OpenAPI schema validation implementation.
14-14: LGTM!The
ExtractRoutesFromAPIimport is correctly used for deriving route types from the endpoint specification.
138-146: Verify multipart/form-data Content-Type handling.Per the PR comments, there's a concern about POST method support for multipart/form-data payloads. While
getUploadFormDatahandles multipart parsing, verify that the OpenAPI specification documents the expectedContent-Type: multipart/form-dataheader and that no middleware blocks it.Consider adding explicit Content-Type validation or documentation in the OpenAPI schema to address the maintainer's concern about multipart/form-data handling.
256-259: LGTM!The module augmentation correctly extends the
@rocket.chat/rest-typingsEndpoints interface. The eslint-disable comments are appropriate for this declaration merging pattern.apps/meteor/app/api/server/definition.ts (1)
304-304: LGTM!Adding the
request: Requestfield toTypedThisaligns it with the existingActionThisinterface and enables typed access to the HTTP request in OpenAPI-style handlers. This change directly supports the emoji-custom.create endpoint implementation.
| response: { | ||
| 400: ajv.compile({ | ||
| type: 'object', | ||
| properties: { | ||
| success: { type: 'boolean', enum: [false] }, | ||
| stack: { type: 'string' }, | ||
| error: { type: 'string' }, | ||
| errorType: { type: 'string' }, | ||
| details: { type: 'string' }, | ||
| }, | ||
| { field: 'emoji', sizeLimit: settings.get('FileUpload_MaxFileSize') }, | ||
| ); | ||
| required: ['success'], | ||
| additionalProperties: false, | ||
| }), | ||
| 200: ajv.compile<void>({ | ||
| type: 'object', | ||
| properties: { | ||
| success: { | ||
| type: 'boolean', | ||
| enum: [true], | ||
| }, | ||
| }, | ||
| required: ['success'], | ||
| additionalProperties: false, | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify 400 response schema matches API.v1.failure() output.
The 400 response schema defines optional fields (stack, error, errorType, details), but API.v1.failure() at line 170 is called without arguments. Ensure the schema accurately reflects all possible 400 responses from this endpoint.
Run this script to check the API.v1.failure() implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check API.v1.failure() signature and return shape
# Search for the failure method definition
ast-grep --pattern $'failure($$$) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate API.v1.failure definition and its return shape
echo "Searching for .failure( occurrences across .ts files"
rg -nP '\.failure\(' -C3 -g '*.ts' || echo "No .failure calls found"
echo -e "\nSearching for class API definition"
rg -nP 'class\s+API\b' -C3 -g '*.ts' || echo "No class API found"
echo -e "\nSearching for exported failure helper"
rg -nP 'export\s+(const|function)\s+failure' -C3 -g '*.ts' || echo "No exported failure helper found"Length of output: 201931
🏁 Script executed:
#!/bin/bash
# Locate ApiClass declaration and failure method definition
echo "ApiClass declaration:"
rg -nP 'class\s+ApiClass\b' -n apps/meteor/app/api/server/ApiClass.ts || echo "Class ApiClass not found"
echo -e "\nFailure method signature and context:"
rg -nP '^\s*(public|private|protected)\s+failure\s*\(' -n -C3 apps/meteor/app/api/server/ApiClass.ts || echo "failure method definition not found"Length of output: 121
🏁 Script executed:
#!/bin/bash
# Locate the ApiClass.ts file
fd --extension ts ApiClass.ts
# Search for the ApiClass declaration
rg -nP 'class\s+ApiClass\b' -g 'apps/meteor/app/api/server/ApiClass.ts'
# Extract the failure method implementation from ApiClass.ts
sed -n '900,950p' apps/meteor/app/api/server/ApiClass.ts | rg -C3 'failure\('Length of output: 472
🏁 Script executed:
# Print the first 200 lines of ApiClass.ts to locate the failure method definition
sed -n '1,200p' apps/meteor/app/api/server/ApiClass.tsLength of output: 6409
🏁 Script executed:
#!/bin/bash
# Search for public success and failure method definitions
echo "-- public success occurrences --"
rg -nP 'public\s+success\(' -C3 apps/meteor/app/api/server/ApiClass.ts || echo "No public success found"
echo -e "\n-- public failure occurrences --"
rg -nP 'public\s+failure\(' -C3 apps/meteor/app/api/server/ApiClass.ts || echo "No public failure found"
echo -e "\n-- Context around where success and failure are called --"
sed -n '600,700p' apps/meteor/app/api/server/ApiClass.ts | rg -C3 '\.failure\(' || echo "No failure calls in this segment"
sed -n '600,700p' apps/meteor/app/api/server/ApiClass.ts | rg -C3 '\.success\(' || echo "No success calls in this segment"Length of output: 343
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' apps/meteor/app/api/server/definition.tsLength of output: 6259
Add missing message field to 400 schema
The default FailureResult body may include an optional message?: string, but your AJV schema forbids any properties beyond success, stack, error, errorType, and details. In apps/meteor/app/api/server/v1/emoji-custom.ts (lines 111–135), under the 400 schema’s properties, add
message: { type: 'string' },so the schema aligns with API.v1.failure()’s possible output.
🤖 Prompt for AI Agents
In apps/meteor/app/api/server/v1/emoji-custom.ts around lines 111 to 135 the 400
AJV response schema omits the optional message field present in the default
FailureResult, causing valid error responses to be rejected; add a message
property with type 'string' to the 400 schema's properties so the schema allows
the optional message (i.e., include message: { type: 'string' } alongside stack,
error, errorType, details) while keeping required/additionalProperties as-is.
2 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:
Endpoints: (In progress)
Looking forward to your feedback! 🚀
Summary by CodeRabbit