fix: Use GraphQL-compliant response payload on error #972
Conversation
📝 WalkthroughWalkthroughThis update standardizes JSON response handling across multiple services. The changes replace direct construction of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as API Service
participant RespUtil as Response Utility
Client->>Service: Send request
Service->>Service: Validate request & process data
alt Request is valid
Service->>RespUtil: Call jsonOk({ data..., headers })
else Request is invalid
Service->>RespUtil: Call jsonError({ status, message, headers })
end
RespUtil-->>Service: Return structured JSON response
Service-->>Client: Respond with JSON formatted data
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/typegate/src/services/responses.ts (1)
16-18: Consider consolidating headers initialization logic.The headers initialization logic is duplicated between
jsonOkandjsonError.Consider extracting to a shared utility function:
+const initHeaders = (headersInit?: Headers | HeadersInit) => { + const headers = headersInit != null + ? new Headers(headersInit) + : new Headers(); + headers.set("content-type", "application/json"); + return headers; +}; export const jsonOk = ( { status = 200, data, headers: headersInit }: JsonOkConfig, ) => { - const headers = headersInit != null - ? new Headers(headersInit) - : new Headers(); - headers.set("content-type", "application/json"); + const headers = initHeaders(headersInit); return new Response(JSON.stringify({ data }), { status, headers, }); };Also applies to: 35-37
src/typegate/src/services/auth/routes/take.ts (1)
31-31: Consider adding error code to extensions.For better error handling on the client side, consider adding an error code to the extensions field.
return jsonError({ status: 401, message: "not authorized", + extensions: { code: "UNAUTHORIZED" }, headers: resHeaders, });Also applies to: 34-38
src/typegate/src/services/auth/routes/validate.ts (1)
7-10: Address the TODO comment about using BaseError.The comment indicates that
BaseErrorshould be used instead of directjsonErrorcalls.Would you like me to help implement the
BaseErrorintegration? This would align with error handling in other parts of the codebase.src/typegate/src/services/artifact_service.ts (2)
12-13: Consolidate jsonError and jsonOk imports.The utilities are imported separately from the same module.
-import { jsonError } from "./responses.ts"; -import { jsonOk } from "./responses.ts"; +import { jsonError, jsonOk } from "./responses.ts";
51-55: Consider standardizing error handling.Some error paths use
BaseError.toResponse()while others usejsonError. Consider standardizing the approach for consistency.Either:
- Convert all error handling to use
jsonError, or- Use
BaseError.toResponse()consistently throughout the codebase} catch (e) { if (e instanceof BaseError) { - return e.toResponse(); + return jsonError({ + status: e.status, + message: e.message, + extensions: { code: e.type }, + }); } return new UnknownError(e).toResponse(); }Also applies to: 101-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/typegate/src/services/artifact_service.ts(6 hunks)src/typegate/src/services/auth/mod.ts(1 hunks)src/typegate/src/services/auth/protocols/oauth2.ts(3 hunks)src/typegate/src/services/auth/routes/take.ts(2 hunks)src/typegate/src/services/auth/routes/validate.ts(2 hunks)src/typegate/src/services/graphql_service.ts(1 hunks)src/typegate/src/services/responses.ts(1 hunks)src/typegate/src/typegate/mod.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (12)
src/typegate/src/services/responses.ts (2)
7-11: LGTM! Well-structured type definitions.The type definitions for
JsonOkConfigandJsonErrorConfigare clear and properly typed. Good use of optional properties where appropriate.Also applies to: 26-30
39-54: LGTM! GraphQL-compliant error response structure.The error response format correctly follows the GraphQL specification with:
- Array of error objects
- Required fields: message, locations, path
- Extensions field with timestamp
src/typegate/src/services/auth/routes/take.ts (1)
24-29: LGTM! Clear error message for domain mismatch.The error response is well-structured with a descriptive message and proper status code.
src/typegate/src/services/auth/routes/validate.ts (1)
71-77: LGTM! Well-structured success response.The response correctly uses
jsonOkwith proper headers handling.src/typegate/src/services/auth/mod.ts (1)
135-135: LGTM! Standardized JSON response handling.The change improves consistency by using the
jsonOkutility function instead of directly constructing a Response object.src/typegate/src/services/graphql_service.ts (4)
122-122: LGTM! GraphQL-compliant success response.The change wraps the response in a
dataproperty, aligning with GraphQL specification.
130-130: LGTM! Standardized resolver error response.The change includes a status code in the error response, improving error handling consistency.
133-137: LGTM! Improved authentication error handling.The change dynamically sets the status code based on the context:
- 401 for missing authentication
- 403 for insufficient permissions
145-145: LGTM! Standardized generic error response.The change includes a status code in the generic error response, maintaining consistency.
src/typegate/src/services/auth/protocols/oauth2.ts (2)
209-213: LGTM! Standardized OAuth2 credential error response.The change improves error handling by:
- Including a status code
- Maintaining the error message
- Preserving the headers for cookie clearing
242-245: LGTM! Standardized missing parameter error response.The change improves error handling by including a status code for missing origin/redirect_uri parameters.
src/typegate/src/typegate/mod.ts (1)
288-288: LGTM! Standardized JWT error response.The change improves consistency by using the structured
jsonErrorformat for JWT validation failures.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #972 +/- ##
==========================================
+ Coverage 77.77% 77.85% +0.07%
==========================================
Files 160 160
Lines 19657 19641 -16
Branches 1964 1969 +5
==========================================
+ Hits 15289 15291 +2
+ Misses 4347 4329 -18
Partials 21 21 ☔ View full report in Codecov by Sentry. |
Migration notes
Summary by CodeRabbit