refactor: standardize error response format in auth and oauth handlers#53
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR standardizes API error responses across auth and OAuth handlers by introducing a centralized ChangesError Response Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@roshankumar0036singh I have added some changes. These are not final changes i just want to know is the modified helper function good or i need to change. I will fix the duplicates that is right now coming. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/utils/response.go (1)
84-103: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
BadRequestResponseandInternalServerErrorResponseare not refactored to useStructuredError.These two functions still emit the old response shape (
"message"at the top level, no"error"object). This creates inconsistency in the API—some error responses will have{"success":false,"error":{...}}while these will have{"success":false,"message":"..."}.Consider refactoring these to also use
StructuredErrorwith appropriate codes (ErrValidation/ErrInternalServer).♻️ Suggested refactor
// BadRequestResponse returns a 400 Bad Request response func BadRequestResponse(c interface{}, message string) { // Type assertion to *gin.Context if ctx, ok := c.(*gin.Context); ok { - ctx.JSON(400, Response{ - Success: false, - Message: message, - }) + ctx.JSON(400, StructuredError(ErrValidation, message, nil)) } } // InternalServerErrorResponse returns a 500 Internal Server Error response func InternalServerErrorResponse(c interface{}, message string) { // Type assertion to *gin.Context if ctx, ok := c.(*gin.Context); ok { - ctx.JSON(500, Response{ - Success: false, - Message: message, - }) + ctx.JSON(500, StructuredError(ErrInternalServer, message, nil)) } }🤖 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 `@internal/utils/response.go` around lines 84 - 103, BadRequestResponse and InternalServerErrorResponse still emit the old top-level "message" shape; update both to build and return a StructuredError (instead of the legacy Message field) with Success:false and Error:StructuredError{Code: ErrValidation / ErrInternalServer, Message: message} and send that via ctx.JSON, ensuring you use the StructuredError type and the ErrValidation and ErrInternalServer codes so the API error shape matches other handlers.internal/handler/auth_handler.go (1)
141-154:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
returnafter error response causes double response.When
ResetPasswordfails, the handler sends the error JSON but then falls through to also send the success response on line 153. This will cause a runtime panic or corrupted response.🐛 Proposed fix
if err := h.authService.ResetPassword(req.Token, req.Password); err != nil { c.JSON(http.StatusBadRequest, utils.StructuredError("PASSWORD_RESET_FAILED", "Password reset failed", err.Error())) + return }🤖 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 `@internal/handler/auth_handler.go` around lines 141 - 154, In ResetPassword (func ResetPassword on AuthHandler) you send an error response when h.authService.ResetPassword(req.Token, req.Password) returns an error but do not return, causing the success response to be sent as well; fix by exiting the handler immediately after writing the error JSON (i.e., add a return right after the c.JSON call that uses utils.StructuredError for "PASSWORD_RESET_FAILED") so the subsequent c.JSON success response is not executed.
🧹 Nitpick comments (4)
internal/handler/oauth_handler.go (1)
233-238: ⚡ Quick win
extractBearerTokenerror response not migrated toStructuredError.When the bearer token is missing or malformed, the handler still returns
gin.H{"error": err.Error()}(lines 236). This deviates from the standardized response shape used elsewhere in the same function.♻️ Suggested fix
token, err := extractBearerToken(c) if err != nil { - c.JSON(http.StatusUnauthorized, gin.H{"error": err.Error()}) + c.JSON(http.StatusUnauthorized, utils.StructuredError("INVALID_TOKEN", "Token extraction failed", err.Error())) return }🤖 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 `@internal/handler/oauth_handler.go` around lines 233 - 238, The error path in UserInfo still returns gin.H{"error": err.Error()} after calling extractBearerToken; replace that response with the standardized StructuredError payload used elsewhere in this handler (construct a StructuredError containing the error message and appropriate fields/status) so the handler returns the same response shape on token-extraction failures; update the code in the UserInfo function immediately after extractBearerToken to build and return a StructuredError instead of gin.H.internal/utils/response.go (2)
38-54: ⚡ Quick winUse
ErrGenericconstant instead of hardcoded"ERROR"string.
ErrorResponseuses"ERROR"directly while you've definedErrGeneric = "ERROR"above. For consistency and maintainability, reference the constant.♻️ Suggested fix
return Response{ Success: false, Error: &ErrorDetail{ - Code: "ERROR", + Code: ErrGeneric, Message: errMsg, }, }🤖 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 `@internal/utils/response.go` around lines 38 - 54, Replace the hardcoded error code string in ErrorResponse with the ErrGeneric constant: locate the ErrorResponse function and change the Code field of the returned ErrorDetail from "ERROR" to ErrGeneric so the function uses the shared constant for consistency and maintainability.
57-63: 💤 Low valueClarify the semantics of
ValidationErrorResponse.The refactored function now places the caller's
messageintoDetailswhile using a fixed"Invalid request"as the message. This changes the API contract—previously themessageparameter was the primary message.If this is intentional (i.e., details provide more granular info), consider renaming the parameter to
detailsfor clarity. Otherwise, if the caller's message should remain the primary message, swap the arguments.🤖 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 `@internal/utils/response.go` around lines 57 - 63, ValidationErrorResponse currently passes the caller's message into the Details slot of StructuredError while using a fixed "Invalid request" as the primary message, changing the previous API semantics; either rename the parameter in ValidationErrorResponse from message to details and update call sites to reflect that semantics, or keep the original semantics by swapping the arguments so the caller's message is passed as the primary message to StructuredError (use ErrValidation, callerMessage, "Invalid request" or similar); locate the function ValidationErrorResponse and the StructuredError invocation to apply the change and update any callers accordingly.internal/handler/auth_handler.go (1)
508-595: ⚖️ Poor tradeoffGoogle/GitHub OAuth callbacks and MFA endpoints still use old error response patterns.
These handlers continue to use
utils.ErrorResponseandutils.UnauthorizedResponse/utils.ValidationErrorResponserather than the newStructuredErrorpattern. This creates inconsistent error response shapes across the API.Consider migrating these in a follow-up to maintain a unified error contract.
Also applies to: 601-689, 698-769
🤖 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 `@internal/handler/auth_handler.go` around lines 508 - 595, Handlers GoogleLogin and GoogleCallback (and other OAuth/MFA handlers referenced) still return errors using utils.ErrorResponse / UnauthorizedResponse / ValidationErrorResponse; migrate them to the new StructuredError pattern so error responses have a consistent shape. For each error branch in GoogleLogin, GoogleCallback and the GitHub/MFA handlers, construct a StructuredError instance with the same HTTP status, human message and underlying error/details, and return that via c.JSON(status, structuredError) (or the app's StructuredError-to-response helper) instead of calling utils.ErrorResponse/UnauthorizedResponse/ValidationErrorResponse; update all occurrences (including state validation, token exchange, user fetch, login failures, and cookie errors) to use StructuredError while preserving the original status codes and messages.
🤖 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.
Outside diff comments:
In `@internal/handler/auth_handler.go`:
- Around line 141-154: In ResetPassword (func ResetPassword on AuthHandler) you
send an error response when h.authService.ResetPassword(req.Token, req.Password)
returns an error but do not return, causing the success response to be sent as
well; fix by exiting the handler immediately after writing the error JSON (i.e.,
add a return right after the c.JSON call that uses utils.StructuredError for
"PASSWORD_RESET_FAILED") so the subsequent c.JSON success response is not
executed.
In `@internal/utils/response.go`:
- Around line 84-103: BadRequestResponse and InternalServerErrorResponse still
emit the old top-level "message" shape; update both to build and return a
StructuredError (instead of the legacy Message field) with Success:false and
Error:StructuredError{Code: ErrValidation / ErrInternalServer, Message: message}
and send that via ctx.JSON, ensuring you use the StructuredError type and the
ErrValidation and ErrInternalServer codes so the API error shape matches other
handlers.
---
Nitpick comments:
In `@internal/handler/auth_handler.go`:
- Around line 508-595: Handlers GoogleLogin and GoogleCallback (and other
OAuth/MFA handlers referenced) still return errors using utils.ErrorResponse /
UnauthorizedResponse / ValidationErrorResponse; migrate them to the new
StructuredError pattern so error responses have a consistent shape. For each
error branch in GoogleLogin, GoogleCallback and the GitHub/MFA handlers,
construct a StructuredError instance with the same HTTP status, human message
and underlying error/details, and return that via c.JSON(status,
structuredError) (or the app's StructuredError-to-response helper) instead of
calling utils.ErrorResponse/UnauthorizedResponse/ValidationErrorResponse; update
all occurrences (including state validation, token exchange, user fetch, login
failures, and cookie errors) to use StructuredError while preserving the
original status codes and messages.
In `@internal/handler/oauth_handler.go`:
- Around line 233-238: The error path in UserInfo still returns gin.H{"error":
err.Error()} after calling extractBearerToken; replace that response with the
standardized StructuredError payload used elsewhere in this handler (construct a
StructuredError containing the error message and appropriate fields/status) so
the handler returns the same response shape on token-extraction failures; update
the code in the UserInfo function immediately after extractBearerToken to build
and return a StructuredError instead of gin.H.
In `@internal/utils/response.go`:
- Around line 38-54: Replace the hardcoded error code string in ErrorResponse
with the ErrGeneric constant: locate the ErrorResponse function and change the
Code field of the returned ErrorDetail from "ERROR" to ErrGeneric so the
function uses the shared constant for consistency and maintainability.
- Around line 57-63: ValidationErrorResponse currently passes the caller's
message into the Details slot of StructuredError while using a fixed "Invalid
request" as the primary message, changing the previous API semantics; either
rename the parameter in ValidationErrorResponse from message to details and
update call sites to reflect that semantics, or keep the original semantics by
swapping the arguments so the caller's message is passed as the primary message
to StructuredError (use ErrValidation, callerMessage, "Invalid request" or
similar); locate the function ValidationErrorResponse and the StructuredError
invocation to apply the change and update any callers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4deed7f3-ef4c-4820-8810-49c39e37a4ad
📒 Files selected for processing (3)
internal/handler/auth_handler.gointernal/handler/oauth_handler.gointernal/utils/response.go
|
handle the nitpick comments by coderabbit and check the cause of failure for the ci |
|
@roshankumar0036singh changing the error response format is causing error |
|
@NirmalMishra08 ResetPassword missing return after error response |
|
@NirmalMishra08 Two tests are failing, bcz the tests were written against the old response shape and haven't been updated to match the new StructuredError format |
|
|
@NirmalMishra08 do check the issues flagged by sonar and the exsitence of duplicate code blocks in the pr |
|
@NirmalMishra08 are you there resolev the sonar issues and check this once UnauthorizedResponse now returns a structured error but still sets Message on the root The old implementation set Message at the root level without an error object. The new one wraps it correctly via StructuredError, but any client relying on the old shape will break. |
2 participants
Checklist
I have read and signed the CLA by commenting
I have read the CLA and agree to its terms.on this PR.My changes follow the project's coding style.
I have tested my changes.
Added structured error helper for consistent API error responses
Standardized error codes, messages, and details fields
Migrated auth_handler.go and oauth_handler.go to use the new error format
Removed inconsistent JSON error response shapes
Summary by CodeRabbit