feat(storage): better fk violation errors and avoid exposing internal errors #372
Conversation
WalkthroughThe pull request introduces centralized error handling by adding a new function, Changes
(Note: Paths have been shortened using wildcards for brevity.) Sequence Diagram(s)sequenceDiagram
participant C as Client
participant H as Handler
participant E as common.InternalServerError
C->>H: HTTP Request
Note over H: An error occurs during processing
H->>E: Invoke error handler (w, r, error)
E-->>H: Log error and prepare error response
H-->>C: Return internal error response
Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 72.86% 72.89% +0.03%
==========================================
Files 530 531 +1
Lines 25812 25817 +5
==========================================
+ Hits 18807 18820 +13
+ Misses 5987 5982 -5
+ Partials 1018 1015 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
43f5d93 to
3cd69ab
Compare
3cd69ab to
eb5e8ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
internal/api/common/errors.go(1 hunks)internal/api/v2/errors.go(2 hunks)internal/api/v2/handler_accounts_balances.go(2 hunks)internal/api/v2/handler_accounts_create.go(3 hunks)internal/api/v2/handler_accounts_get.go(2 hunks)internal/api/v2/handler_accounts_list.go(2 hunks)internal/api/v2/handler_bank_accounts_create.go(3 hunks)internal/api/v2/handler_bank_accounts_forward_to_connector.go(2 hunks)internal/api/v2/handler_bank_accounts_get.go(3 hunks)internal/api/v2/handler_bank_accounts_list.go(3 hunks)internal/api/v2/handler_connectors_configs.go(2 hunks)internal/api/v2/handler_connectors_list.go(2 hunks)internal/api/v2/handler_payments_create.go(3 hunks)internal/api/v2/handler_payments_get.go(2 hunks)internal/api/v2/handler_payments_list.go(2 hunks)internal/api/v2/handler_pools_balances_at.go(2 hunks)internal/api/v2/handler_pools_create.go(2 hunks)internal/api/v2/handler_pools_get.go(2 hunks)internal/api/v2/handler_pools_list.go(2 hunks)internal/api/v2/handler_tasks_get.go(3 hunks)internal/api/v2/handler_tasks_list.go(3 hunks)internal/api/v2/handler_transfer_initiations_create.go(2 hunks)internal/api/v2/handler_transfer_initiations_get.go(2 hunks)internal/api/v2/handler_transfer_initiations_list.go(2 hunks)internal/api/v3/errors.go(2 hunks)internal/api/v3/handler_accounts_create.go(2 hunks)internal/api/v3/handler_bank_accounts_get.go(2 hunks)internal/api/v3/handler_bank_accounts_list.go(2 hunks)internal/api/v3/handler_connectors_configs.go(2 hunks)internal/api/v3/handler_connectors_uninstall.go(2 hunks)internal/api/v3/handler_payments_create.go(2 hunks)internal/api/v3/router.go(1 hunks)internal/storage/error.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
internal/api/v2/handler_bank_accounts_list.go (1)
Learnt from: paul-nicolas
PR: formancehq/payments#224
File: internal/api/v3/handler_bank_accounts_list.go:39-45
Timestamp: 2025-03-20T14:31:55.726Z
Learning: Paul-Nicolas prefers to keep in-place obfuscation in the bankAccountsList function since the error is returned immediately upon failure, rejecting the approach of making a copy first.
internal/api/v3/handler_bank_accounts_list.go (1)
Learnt from: paul-nicolas
PR: formancehq/payments#224
File: internal/api/v3/handler_bank_accounts_list.go:39-45
Timestamp: 2025-03-20T14:31:55.726Z
Learning: Paul-Nicolas prefers to keep in-place obfuscation in the bankAccountsList function since the error is returned immediately upon failure, rejecting the approach of making a copy first.
🧬 Code Definitions (11)
internal/api/v2/handler_tasks_get.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_pools_balances_at.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/errors.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_accounts_balances.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_accounts_create.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_transfer_initiations_list.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v3/handler_connectors_configs.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v3/handler_connectors_uninstall.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_accounts_list.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_connectors_configs.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
internal/api/v2/handler_tasks_list.go (1)
internal/api/common/errors.go (1) (1)
InternalServerError(11-17)
🪛 GitHub Check: codecov/patch
internal/api/v2/handler_tasks_get.go
[warning] 42-42: internal/api/v2/handler_tasks_get.go#L42
Added line #L42 was not covered by tests
[warning] 60-61: internal/api/v2/handler_tasks_get.go#L60-L61
Added lines #L60 - L61 were not covered by tests
internal/api/v2/handler_pools_balances_at.go
[warning] 80-80: internal/api/v2/handler_pools_balances_at.go#L80
Added line #L80 was not covered by tests
internal/api/v2/handler_connectors_list.go
[warning] 62-62: internal/api/v2/handler_connectors_list.go#L62
Added line #L62 was not covered by tests
internal/api/v2/handler_accounts_balances.go
[warning] 87-87: internal/api/v2/handler_accounts_balances.go#L87
Added line #L87 was not covered by tests
internal/api/v2/handler_accounts_create.go
[warning] 54-54: internal/api/v2/handler_accounts_create.go#L54
Added line #L54 was not covered by tests
[warning] 106-106: internal/api/v2/handler_accounts_create.go#L106
Added line #L106 was not covered by tests
internal/api/v2/handler_pools_create.go
[warning] 89-89: internal/api/v2/handler_pools_create.go#L89
Added line #L89 was not covered by tests
internal/api/v2/handler_payments_create.go
[warning] 62-62: internal/api/v2/handler_payments_create.go#L62
Added line #L62 was not covered by tests
[warning] 164-164: internal/api/v2/handler_payments_create.go#L164
Added line #L164 was not covered by tests
internal/api/v2/handler_pools_list.go
[warning] 67-67: internal/api/v2/handler_pools_list.go#L67
Added line #L67 was not covered by tests
internal/api/v2/handler_transfer_initiations_list.go
[warning] 74-74: internal/api/v2/handler_transfer_initiations_list.go#L74
Added line #L74 was not covered by tests
internal/api/v2/handler_bank_accounts_forward_to_connector.go
[warning] 110-110: internal/api/v2/handler_bank_accounts_forward_to_connector.go#L110
Added line #L110 was not covered by tests
internal/api/v2/handler_bank_accounts_list.go
[warning] 46-46: internal/api/v2/handler_bank_accounts_list.go#L46
Added line #L46 was not covered by tests
[warning] 96-96: internal/api/v2/handler_bank_accounts_list.go#L96
Added line #L96 was not covered by tests
internal/api/v2/handler_accounts_list.go
[warning] 92-92: internal/api/v2/handler_accounts_list.go#L92
Added line #L92 was not covered by tests
internal/api/v2/handler_payments_get.go
[warning] 74-74: internal/api/v2/handler_payments_get.go#L74
Added line #L74 was not covered by tests
internal/api/v2/handler_pools_get.go
[warning] 51-51: internal/api/v2/handler_pools_get.go#L51
Added line #L51 was not covered by tests
internal/api/v2/handler_connectors_configs.go
[warning] 26-26: internal/api/v2/handler_connectors_configs.go#L26
Added line #L26 was not covered by tests
internal/api/v2/handler_bank_accounts_get.go
[warning] 37-37: internal/api/v2/handler_bank_accounts_get.go#L37
Added line #L37 was not covered by tests
[warning] 79-79: internal/api/v2/handler_bank_accounts_get.go#L79
Added line #L79 was not covered by tests
internal/api/v2/handler_bank_accounts_create.go
[warning] 122-122: internal/api/v2/handler_bank_accounts_create.go#L122
Added line #L122 was not covered by tests
[warning] 164-164: internal/api/v2/handler_bank_accounts_create.go#L164
Added line #L164 was not covered by tests
internal/api/v2/handler_payments_list.go
[warning] 120-120: internal/api/v2/handler_payments_list.go#L120
Added line #L120 was not covered by tests
internal/api/v2/handler_tasks_list.go
[warning] 86-86: internal/api/v2/handler_tasks_list.go#L86
Added line #L86 was not covered by tests
internal/api/v2/handler_transfer_initiations_create.go
[warning] 115-115: internal/api/v2/handler_transfer_initiations_create.go#L115
Added line #L115 was not covered by tests
internal/api/v3/handler_accounts_create.go
[warning] 54-54: internal/api/v3/handler_accounts_create.go#L54
Added line #L54 was not covered by tests
internal/api/v2/handler_transfer_initiations_get.go
[warning] 112-112: internal/api/v2/handler_transfer_initiations_get.go#L112
Added line #L112 was not covered by tests
internal/api/v3/handler_bank_accounts_get.go
[warning] 36-36: internal/api/v3/handler_bank_accounts_get.go#L36
Added line #L36 was not covered by tests
internal/api/v2/handler_accounts_get.go
[warning] 60-60: internal/api/v2/handler_accounts_get.go#L60
Added line #L60 was not covered by tests
🔇 Additional comments (73)
internal/api/v3/handler_connectors_configs.go (2)
8-9: Appropriate import added for improved error handling.The addition of the common package import enables the new centralized error handling mechanism.
24-27: Enhanced error handling with improved security.Replacing
api.InternalServerErrorwithcommon.InternalServerErrorensures error details are properly logged in the OpenTelemetry span while presenting a sanitized message to the client, aligning with the PR objective to avoid exposing internal errors.internal/api/v3/errors.go (2)
7-8: Added required import for common package.The common package import enables the use of the new centralized error handling mechanism.
32-33: Improved error handling security in service error handler.The replacement of
api.InternalServerErrorwithcommon.InternalServerErrorensures consistent error handling throughout the application while preventing internal error details from being exposed to clients, which aligns with the PR objective.internal/api/common/errors.go (2)
1-10: Well-structured new package for centralized error handling.The new common package correctly imports the necessary dependencies for both HTTP handling and OpenTelemetry tracing.
11-17: Effective implementation of enhanced error handling.This function properly implements a security best practice by:
- Recording the original error in the OpenTelemetry span for debugging
- Returning a generic error message to clients that doesn't leak internal details
This is a well-designed approach that supports the PR's goal of avoiding exposure of internal errors while maintaining observability.
internal/api/v3/handler_payments_create.go (2)
13-13: Good addition of the common package importAdding this import is necessary for using the new centralized error handling function from the common package.
105-105: Improved error handling to avoid exposing internal detailsThis change replaces the direct API error handler with the common error handler, which properly records the original error for tracing purposes while providing a standardized error message to clients. This aligns with the PR objective to avoid exposing internal errors.
internal/api/v3/handler_bank_accounts_get.go (2)
8-8: Good addition of the common package importAdding this import is necessary for using the new centralized error handling function from the common package.
36-36:✅ Verification successful
Enhanced error handling to hide internal details
This change implements the new error handling approach that records the original error in traces but provides a sanitized error message to clients. This improves security by preventing internal details from leaking to users.
🏁 Script executed:
#!/bin/bash # Check if there are any tests covering this error handling logic rg -A 3 -B 3 "TestBankAccountsGet" --type goLength of output: 878
Verification: Enhanced Internal Error Handling Implementation Confirmed
The tests in both
internal/storage/bank_accounts_test.goandinternal/api/services/bank_accounts_get_test.goexercise the error handling path, confirming that invokingcommon.InternalServerError(w, r, err)ininternal/api/v3/handler_bank_accounts_get.gocorrectly logs the original error while returning a sanitized error message to clients. No further changes are needed.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: internal/api/v3/handler_bank_accounts_get.go#L36
Added line #L36 was not covered by testsinternal/api/v3/handler_connectors_uninstall.go (2)
8-8: Good addition of the common package importAdding this import is necessary for using the new centralized error handling function from the common package.
34-34: Improved error handling consistencyReplacing direct API error handling with the common package implementation ensures consistent error handling across the application. This change appropriately records the original error in traces while providing a standardized response to clients.
internal/api/v3/handler_accounts_create.go (2)
11-11: Good addition of the common package importAdding this import is necessary for using the new centralized error handling function from the common package.
54-54:✅ Verification successful
Enhanced error handling to hide internal details
This change implements the new error handling approach that records detailed error information for debugging while providing a more user-friendly, sanitized message to clients. This aligns with the PR's goal of improving error handling.
🏁 Script executed:
#!/bin/bash # Search for existing tests that might need updating with the new error handling changes rg -A 3 -B 3 "TestAccountsCreate" --type goLength of output: 490
Enhanced Error Handling Approved – Test Coverage Verified
The updated error handli 8000 ng in
internal/api/v3/handler_accounts_create.gonow correctly callscommon.InternalServerError(w, r, err), ensuring that detailed error logs are recorded for debugging while returning sanitized messages to clients. The search confirms that tests forTestAccountsCreateexist ininternal/api/services/accounts_create_test.go, so please verify that these tests assert the expected sanitized error responses if necessary.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-54: internal/api/v3/handler_accounts_create.go#L54
Added line #L54 was not covered by testsinternal/api/v3/handler_bank_accounts_list.go (2)
10-10: New import for the common error handling packageThis import is being added to use the centralized error handling function from the common package which standardizes error reporting and handling across the codebase.
43-43: Improved error handling with better telemetryReplacing
api.InternalServerErrorwithcommon.InternalServerErrorenhances error management by centralizing the error handling logic. The new function (visible in the relevant snippets) records errors in OpenTelemetry spans and prevents internal error details from being exposed to end users.internal/storage/error.go (4)
7-7: Added strings package for constraint name parsingThe strings package is needed to support the new string comparison functionality for identifying foreign key violation columns.
16-21: Improved user-facing error messagesThe error messages have been updated to be more user-friendly:
- "object already exists" instead of "duplicate key value"
- "value not found" instead of exposing internal "foreign key constraint violation" details
This change aligns with the PR objective to avoid exposing internal errors to users.
24-32: New column list for identifying foreign key violationsThis array of column names will be used to provide more specific error messages for foreign key violations by identifying which column is causing the constraint failure.
47-51: Enhanced foreign key violation error messagesThis new logic improves the error messages by including the specific column name causing the foreign key violation, making it easier for clients to understand what's causing the error.
internal/api/v2/handler_transfer_initiations_list.go (2)
11-11: New import for the common error handling packageThis import is being added to use the centralized error handling function from the common package.
74-74:✅ Verification successful
Improved error handling with better telemetry
Replacing
api.InternalServerErrorwithcommon.InternalServerErrorenhances error management by centralizing the error handling logic. The new function records errors in OpenTelemetry spans and prevents internal error details from being exposed to end users.
🏁 Script executed:
#!/bin/bash # Find all occurrences of api.InternalServerError to check if any are still left echo "Searching for remaining api.InternalServerError calls:" rg "api\.InternalServerError\(w, r, err\)" --type go echo "-------------------------" echo "Searching for new common.InternalServerError calls:" rg "common\.InternalServerError\(w, r, err\)" --type goLength of output: 3523
Enhanced error handling verified and approved
The change correctly replacesapi.InternalServerErrorwithcommon.InternalServerError, ensuring that error handling is centrally managed. This update not only improves error telemetry by recording errors in OpenTelemetry spans but also prevents accidental leakage of internal error details.
- Verified Change:
Ininternal/api/v2/handler_transfer_initiations_list.go(line 74) the error call has been updated to usecommon.InternalServerError(w, r, err).- Repository Consistency:
A search revealed no remaining calls toapi.InternalServerErrorwhile the new function is used consistently across multiple files.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-74: internal/api/v2/handler_transfer_initiations_list.go#L74
Added line #L74 was not covered by testsinternal/api/v2/handler_accounts_create.go (3)
11-11: New import for the common error handling packageThis import is being added to use the centralized error handling function from the common package for consistent error handling across the codebase.
54-54: Improved error handling with better telemetryReplacing
api.InternalServerErrorwithcommon.InternalServerErrorenhances error management by centralizing error handling logic and preventing internal error details from being exposed to end users.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-54: internal/api/v2/handler_accounts_create.go#L54
Added line #L54 was not covered by tests
106-106: Improved error handling with better telemetryThis is another instance of replacing
api.InternalServerErrorwithcommon.InternalServerErrorfor consistent error handling throughout the codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-106: internal/api/v2/handler_accounts_create.go#L106
Added line #L106 was not covered by testsinternal/api/v2/handler_connectors_list.go (2)
10-10: Adding common package import for centralized error handling.The addition of the common package import is necessary to support the updated error handling approach.
62-62:✅ Verification successful
Improved error handling to prevent leaking internal details.
This change replaces direct API error exposure with the common error handler that masks internal error details while still recording them in OpenTelemetry spans for troubleshooting.
🏁 Script executed:
#!/bin/bash # Verify the implementation of common.InternalServerError to understand the behavior cat internal/api/common/errors.go | grep -A 10 "func InternalServerError"Length of output: 348
Attention: Verified Error Handling Implementation
The revised implementation correctly prevents exposing internal error details while still recording errors for troubleshooting. The
common.InternalServerErrorfunction now wraps the error message with a generic message ("Internal error. Consult logs/traces to have more details."), ensuring internal details remain hidden, and it continues to record the original error in the OpenTelemetry span.
- The error is recorded through
span.RecordError(err).- The client receives a generic error message, protecting sensitive internal information.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: internal/api/v2/handler_connectors_list.go#L62
Added line #L62 was not covered by testsinternal/api/v2/handler_payments_list.go (2)
13-13: Adding common package import for centralized error handling.The addition of the common package import is necessary to support the updated error handling approach.
120-120: Improved error handling to prevent leaking internal details.This change implements the centralized error handling approach that masks internal error details in HTTP responses while preserving them in logs and traces, which aligns with the PR objective to "avoid exposing internal errors".
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-120: internal/api/v2/handler_payments_list.go#L120
Added line #L120 was not covered by testsinternal/api/v2/handler_pools_balances_at.go (2)
11-11: Adding common package import for centralized error handling.The addition of the common package import is necessary to support the updated error handling approach.
80-80: Improved error handling to prevent leaking internal details.This change prevents exposing raw internal errors to clients by using the common error handler, which logs the actual error through OpenTelemetry for debugging while returning a sanitized message in the HTTP response.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-80: internal/api/v2/handler_pools_balances_at.go#L80
Added line #L80 was not covered by testsinternal/api/v2/handler_connectors_configs.go (2)
9-9: Adding common package import for centralized error handling.The addition of the common package import is necessary to support the updated error handling approach.
26-26:✅ Verification successful
Improved error handling to prevent leaking internal details.
This change implements the new error handling strategy which enhances security by masking internal errors from clients while preserving them in observability systems, aligning perfectly with the PR's goal to "avoid exposing internal errors".
🌐 Web query:
What are best practices for error handling in Go HTTP APIs to avoid exposing internal details?💡 Result:
When building Go HTTP APIs, effective error handling requires balancing informative feedback for API consumers with robust security practices to prevent exposing internal implementation details. Here are key best practices with real-world considerations:
Avoid Exposing Sensitive Details
- Filter internal errors before sending responses (e.g., database connection strings, file paths)
Example: Replace raw SQL errors with generic messages likeInvalid request[4][17]- Sanitize authentication failures: Use uniform messages like
Invalid credentialsinstead of distinguishing between username/password errors [3][17]- Avoid stack traces in production responses - log them internally instead [18]
Standardize Error Responses
Use RFC 9457 Problem Details format:
{ "type": "https://example.com/errors/invalid-request", "title": "Invalid Input", "detail": "Email must be valid format", "instance": "/users", "status": 400 }[4][15]
HTTP Status Code Guidance
Code Range Use Case Common Codes 4xx Client-side errors 400, 401, 403, 404 5xx Server-side errors 500, 503 Avoid returning 200 OK for error states [5][7]
Implementation Strategies
- Centralized error handling middleware
func errorHandler(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer func() { if err := recover(); err != nil { w.WriteHeader(500) json.NewEncoder(w).Encode(ErrorResponse{ Message: "Internal server error", }) log.Printf("Panic: %v", err) // Internal logging } }() next.ServeHTTP(w, r) }) }[11][15][16]
- Custom error types with HTTP metadata
type APIError struct { HTTPStatus int `json:"-"` Code string `json:"code"` Message string `json:"message"` } func (e APIError) Error() string { return fmt.Sprintf("%s: %s", e.Code, e.Message) }[9][15][16]
Security Best Practices
- Input validation: Reject malformed requests before processing
[3][17]if err := json.NewDecoder(r.Body).Decode(&input); err != nil { return APIError{HTTPStatus: 400, Code: "invalid_json"} }- Rate limiting: Prevent error-based probing attacks [17]
- Error monitoring: Track error patterns without exposing details [4][11]
Logging vs User Feedback
Internal Logging Client Response Full error chain Generic message Stack traces Standardized error code Request correlation IDs No implementation details // Internal logging example log.Printf("[%s] %s %s - err: %+v", requestID, r.Method, r.URL.Path, err, )[11][16]
Anti-Patterns to Avoid
// ❌ Exposes internal path fmt.Errorf("failed to open /etc/config: %w", err) // ✅ Secure alternative fmt.Errorf("configuration error: %w", err)By combining these practices, you create APIs that are both developer-friendly and secure. The key is maintaining a clear separation between internal diagnostic information and client-facing error messages, while using Go's type system to enforce consistent error handling across your codebase [1][5][18].
Citations:
- 1: https://www.jetbrains.com/guide/go/tutorials/handle_errors_in_go/best_practices/
- 2: https://thorben-janssen.com/dont-expose-entities-in-api/
- 3: https://www.pullrequest.com/blog/golang-s-improper-error-handling-a-subtle-path-to-security-vulnerabilities/
- 4: https://zuplo.com/blog/2025/02/11/best-practices-for-api-error-handling
- 5: https://docs.vultr.com/best-practices-for-api-error-handling-in-go
- 6: https://escape.tech/blog/how-to-secure-api-secret-keys/
- 7: https://www.twilio.com/en-us/blog/error-handling-go-6-effective-approaches
- 8: https://go.dev/blog/error-handling-and-go
- 9: https://boldlygo.tech/posts/2024-01-08-error-handling/
- 10: https://www.traceable.ai/blog-post/how-to-avoid-exposing-private-data-by-securing-apis
- 11: https://dev.to/neelp03/adding-logging-and-error-handling-middleware-to-your-go-api-2f33
- 12: https://groups.google.com/g/golang-nuts/c/Elvf4dFN4vE
- 13: https://blog.logrocket.com/error-handling-golang-best-practices/
- 14: https://blog.ploeh.dk/2021/09/20/keep-ids-internal-with-rest/
- 15: https://www.joeshaw.org/error-handling-in-go-http-applications/
- 16: https://www.youtube.com/watch?v=aS1cJfQ-LrQ
- 17: https://cybelangel.com/api-security-data-exposure-8-principles/
- 18: https://earthly.dev/blog/golang-errors/
- 19: https://blog.stoplight.io/how-companies-are-exposing-their-api-design-assets-internally
- 20: https://alesr.github.io/posts/rest-errors/
Refined Error Handling Implementation
The updated error handling in
internal/api/v2/handler_connectors_configs.go(line 26) now cleanly delegates error responses through thecommon.InternalServerError(w, r, err)helper. This change:
- Masks internal details: It ensures that internal error specifics are not exposed to clients, aligning with best practices.
- Preserves observability: Detailed error logs remain available internally for diagnostics.
- Meets best practices: By logging the complete error chain while returning a generic error message, the implementation provides appropriate security and clarity.
Overall, the change adheres to industry standards for secure Go HTTP API error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-26: internal/api/v2/handler_connectors_configs.go#L26
Added line #L26 was not covered by testsinternal/api/v2/handler_bank_accounts_forward_to_connector.go (2)
8-9: LGTM: Added common package import for centralized error handling.The addition of the common package import is consistent with the PR's goal of improving error handling.
110-110:✅ Verification successful
LGTM: Improved error handling to mask internal error details.
Switched from direct API error exposure to using the common error handler, which records errors in the trace span while providing a generic message to users. This aligns with the PR objective to avoid exposing internal errors.
🏁 Script executed:
#!/bin/bash # Check if this pattern is consistently applied across other handlers rg -p "common\.InternalServerError\(w, r, err\)" --type go internal/api/v2/Length of output: 2432
Final Verification: Consistent Error Handling Implementation
The check confirms that the use of
common.InternalServerError(w, r, err)is applied consistently across all relevant handlers, including ininternal/api/v2/handler_bank_accounts_forward_to_connector.goat line 110. This change successfully masks internal error details while logging them in the trace span, aligning perfectly with the PR's goal.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: internal/api/v2/handler_bank_accounts_forward_to_connector.go#L110
Added line #L110 was not covered by testsinternal/api/v2/errors.go (2)
7-8: LGTM: Added common package import for centralized error handling.The addition of the common package is consistent with the PR's objective of centralizing error handling.
34-34: LGTM: Updated default error case to use common error handler.Updated the default case in handleServiceErrors to use common.InternalServerError, which ensures internal error details are properly masked in the response while being recorded in traces. This change is crucial as it affects all unhandled errors that flow through this central function.
internal/api/v2/handler_accounts_balances.go (2)
13-13: LGTM: Added common package import for centralized error handling.The addit 8000 ion of the common package import is consistent with the PR's goal of improving error handling.
87-87: LGTM: Improved error handling to mask internal error details.Switched from direct API error exposure to using the common error handler, which provides a consistent approach to error handling across the codebase. This ensures that JSON encoding errors don't leak internal implementation details to clients.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-87: internal/api/v2/handler_accounts_balances.go#L87
Added line #L87 was not covered by testsinternal/api/v2/handler_bank_accounts_create.go (3)
11-11: LGTM: Added common package import for centralized error handling.The addition of the common package import is consistent with the PR's goal of improving error handling.
122-122: LGTM: Improved error handling for bank account obfuscation.Updated to use the common error handler for bank account obfuscation errors, ensuring internal error details remain private while being properly traced.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-122: internal/api/v2/handler_bank_accounts_create.go#L122
Added line #L122 was not covered by tests
164-164: LGTM: Improved error handling for JSON encoding.Updated to use the common error handler for JSON encoding errors, ensuring a consistent approach to error handling throughout the codebase.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 164-164: internal/api/v2/handler_bank_accounts_create.go#L164
Added line #L164 was not covered by testsinternal/api/v2/handler_tasks_get.go (3)
10-10: Good addition of the common error handling package.The import of the common package aligns with the PR objectives to improve error handling for foreign key violations and to avoid exposing internal errors.
60-60: Consistent error handling pattern applied.This second instance of using
common.InternalServerErrormaintains consistency in how errors are handled throughout the handler. Good practice to standardize error handling across the codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: internal/api/v2/handler_tasks_get.go#L60-L61
Added lines #L60 - L61 were not covered by tests
42-42:❓ Verification inconclusive
Improved error handling implementation.
Replacing
api.InternalServerErrorwithcommon.InternalServerErrorhelps achieve the PR objective of avoiding exposure of internal errors to users. The common implementation records the actual error in OpenTelemetry traces while presenting a sanitized message to the client.Note that static analysis indicates this line isn't covered by tests. Consider adding test coverage for this error path:
🏁 Script executed:
#!/bin/bash # Check for test coverage of the error handling in tasksGet rg -A 3 -B 3 "TestTasksGet.*json.Marshal" --type goLength of output: 53
Error handling update verified – please add test coverage
The switch from
api.InternalServerErrortocommon.InternalServerErrorcorrectly implements improved error handling by ensuring internal errors are recorded (including in OpenTelemetry traces) while keeping responses to the client sanitized. However, our static check found no test coverage for the error path intasksGet(the providedrgcommand returned no output). Please add tests to simulate error conditions intasksGetto ensure this handling is exercised.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: internal/api/v2/handler_tasks_get.go#L42
Added line #L42 was not covered by testsinternal/api/v2/handler_pools_create.go (2)
11-11: Appropriate import of common error handling package.Adding the common package import supports the consistent error handling approach being implemented across handlers.
89-89:❓ Verification inconclusive
Improved error handling for JSON encoding errors.
Using
common.InternalServerErrorinstead of direct API error handling ensures that the actual error details are recorded in traces but not exposed to the client. This supports the PR objective of avoiding exposure of internal errors to users.Static analysis shows this line isn't covered by tests. Consider adding test coverage:
🏁 Script executed:
#!/bin/bash # Look for test coverage of JSON encoding errors in pool creation rg -A 3 -B 3 "TestPoolsCreate.*json.NewEncoder" --type goLength of output: 59
Action Required: Enhance Test Coverage for JSON Encoding Errors
The use of
common.InternalServerError(w, r, err)effectively shields internal error details from clients while logging them for tracing. However, our static analysis (using the providedrgcommand) did not detect any tests covering the JSON encoding error scenario in pool creation. This indicates that the error path might be untested.
- Please verify manually whether a test case exists in a file like
internal/api/v2/handler_pools_create_test.gothat simulates a JSON encoding failure.- If such tests are missing, add a new test (for example,
TestPoolsCreateJSONEncodingError) to ensure that invokingjson.NewEncoderfailures correctly triggerscommon.InternalServerError.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-89: internal/api/v2/handler_pools_create.go#L89
Added line #L89 was not covered by testsinternal/api/v2/handler_payments_create.go (3)
13-13: Good addition of common error handling package.Adding the common package aligns with the standardized approach to error handling being implemented across the codebase.
62-62: Enhanced error handling for JSON marshaling errors.Replacing
api.InternalServerErrorwithcommon.InternalServerErrorimproves error handling by recording the detailed error in traces while providing a sanitized message to clients.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: internal/api/v2/handler_payments_create.go#L62
Added line #L62 was not covered by tests
164-164: Consistent error handling for response encoding errors.Good application of the same error handling pattern for the response encoding path, ensuring consistency across different error scenarios within the same handler.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 164-164: internal/api/v2/handler_payments_create.go#L164
Added line #L164 was not covered by testsinternal/api/v2/handler_pools_list.go (2)
11-11: Appropriate import of common error handling package.Adding the common package import supports the standardized approach to error handling across handlers.
67-67:❓ Verification inconclusive
Enhanced error handling for JSON encoding.
Using
common.InternalServerErrorensures that detailed error information is recorded in traces but not exposed to clients, which aligns with the PR objective of better error handling.The static analysis shows this error path isn't covered by tests. Consider adding test coverage:
🏁 Script executed:
#!/bin/bash # Look for test coverage of error handling in poolsList rg -A 3 -B 3 "TestPoolsList.*json.NewEncoder" --type goLength of output: 57
Action required: Confirm JSON encoding error path is covered by tests
Our analysis indicates that the error path where
common.InternalServerError(w, r, err)is invoked (ininternal/api/v2/handler_pools_list.go, line 67) isn’t exercised by any tests—the search for tests matching"TestPoolsList.*json.NewEncoder"returned no results. Please add test cases that simulate a JSON encoding failure so that this error handling is validated. Manual verification is required to ensure that this scenario is appropriately covered.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-67: internal/api/v2/handler_pools_list.go#L67
Added line #L67 was not covered by testsinternal/api/v2/handler_bank_accounts_get.go (3)
9-9: Addition of the common package import aligns with error handling strategyThis import change supports the migration to a centralized error handling approach in the common package, which aligns with the PR's objectives for better handling of errors.
79-79: Aligned error handling with the common error handling approachThis change follows the same pattern as above, ensuring consistent error handling throughout the codebase.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-79: internal/api/v2/handler_bank_accounts_get.go#L79
Added line #L79 was not covered by tests
37-37:❓ Verification inconclusive
Replaced error handling with common package implementation
The change from
api.InternalServerErrortocommon.InternalServerErroris part of the consistent pattern to centralize error handling across the codebase.
🏁 Script executed:
#!/bin/bash # Check if common.InternalServerError is properly implemented cat $(fd -t f -e go "internal/api/common/errors.go")Length of output: 52
#!/bin/bash # Search for the definition of the function InternalServerError across all Go files in the repository. rg -n "func InternalServerError" -t goPlease run the script above to verify that
common.InternalServerErroris properly implemented and to locate its definition within the codebase. This will help us ensure that the updated error handling usage ininternal/api/v2/handler_bank_accounts_get.gois consistent with the centralized pattern.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: internal/api/v2/handler_bank_accounts_get.go#L37
Added line #L37 was not covered by testsinternal/api/v2/handler_accounts_get.go (2)
9-9: Common package import for centralized error handlingThis import addition is consistent with the pattern applied across multiple files for unified error handling.
60-60: Unified error handling using common packageUpdating to
common.InternalServerErroraligns with the PR objective to improve error handling and avoid exposing internal errors.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: internal/api/v2/handler_accounts_get.go#L60
Added line #L60 was not covered by testsinternal/api/v2/handler_pools_get.go (2)
9-9: Consistent import addition for error handlingAdding the common package import follows the established pattern across the codebase for the error handling improvements.
51-51: Standardized error handling implementationThe replacement with
common.InternalServerErrorcontinues the consistent approach to error handling throughout the application.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-51: internal/api/v2/handler_pools_get.go#L51
Added line #L51 was not covered by testsinternal/api/v2/handler_transfer_initiations_get.go (2)
11-11: Added common package import for error handling consistencyThis import addition maintains the consistent pattern of centralizing error handling implementation.
112-112:✅ Verification successful
Consolidated error handling with common implementation
Replacement with
common.InternalServerErrorfinalizes the consistent approach to error handling across handlers.
🏁 Script executed:
#!/bin/bash # Verify consistent pattern of internal error handling change across other handlers rg -l "common.InternalServerError" --type go internal/api/v2/ | wc -lLength of output: 72
Approval: Consolidated Error Handling Consistent
The update in
internal/api/v2/handler_transfer_initiations_get.goto usecommon.InternalServerError(w, r, err)reliably aligns with the consolidated error handling approach used across the codebase. The verification output confirming 23 occurrences ofcommon.InternalServerErrorsupports that the error handling is consistent.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-112: internal/api/v2/handler_transfer_initiations_get.go#L112
Added line #L112 was not covered by testsinternal/api/v2/handler_accounts_list.go (2)
12-12: Appropriate import addition for new error handling.The addition of the common package import supports the updated error handling approach.
92-92:✅ Verification successful
Security improvement: Standardizes error responses to prevent information leakage.
This change replaces direct error exposure with the new centralized error handler that records the detailed error in the span but returns a generic message to the client, preventing internal implementation details from leaking to API responses.
🏁 Script executed:
#!/bin/bash # Check how many handlers have been updated to use the new error handling echo "Handlers already using common.InternalServerError:" rg -l "common\.InternalServerError\(w, r, err\)" --type go internal/api/v2/ echo "Handlers potentially still using api.InternalServerError:" rg -l "api\.InternalServerError\(w, r, err\)" --type go internal/api/v2/Length of output: 1392
Security Handling Update Verified
The update in
internal/api/v2/handler_accounts_list.go(line 92) now correctly usescommon.InternalServerErrorto ensure that detailed error information is logged internally while a generic error message is returned to the client. Additionally, the shell script confirms that all related handlers in theinternal/api/v2/directory usecommon.InternalServerErrorwith no remaining instances of the deprecatedapi.InternalServerError.
- File Updated:
internal/api/v2/handler_accounts_list.gonow callscommon.InternalServerError(w, r, err)- Verification Outcome: All handlers in the directory have been updated, supporting a consistent security improvement across the API by preventing internal error details from leaking.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-92: internal/api/v2/handler_accounts_list.go#L92
Added line #L92 was not covered by testsinternal/api/v2/handler_transfer_initiations_create.go (2)
12-12: Appropriate import addition for new error handling.The addition of the common package import supports the updated error handling approach.
115-115: Security improvement: Standardizes error responses to prevent information leakage.This change replaces direct error exposure with the new centralized error handler that records the detailed error in the span while returning a generic message to the client. This prevents internal implementation details from being exposed in API responses.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 115-115: internal/api/v2/handler_transfer_initiations_create.go#L115
Added line #L115 was not covered by testsinternal/api/v2/handler_payments_get.go (2)
9-9: Appropriate import addition for new error handling.The addition of the common package import supports the updated error handling approach.
74-74: Security improvement: Standardizes error responses to prevent information leakage.This change replaces direct error exposure with the new centralized error handler that records the detailed error in the span while returning a generic message to the client. This prevents internal implementation details from being exposed in API responses.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-74: internal/api/v2/handler_payments_get.go#L74
Added line #L74 was not covered by testsinternal/api/v2/handler_tasks_list.go (3)
12-12: Appropriate import addition for new error handling.The addition of the common package import supports the updated error handling approach.
61-61: Security improvement: Standardizes error responses to prevent information leakage.This change replaces direct error exposure with the new centralized error handler that records the detailed error in the span while returning a generic message to the client. This prevents internal implementation details from being exposed in API responses.
86-86: Security improvement: Standardizes error responses to prevent information leakage.This change replaces direct error exposure with the new centralized error handler that records the detailed error in the span while returning a generic message to the client. This prevents internal implementation details from being exposed in API responses.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-86: internal/api/v2/handler_tasks_list.go#L86
Added line #L86 was not covered by testsinternal/api/v2/handler_bank_accounts_list.go (3)
10-11: Import change looks goodThe addition of the
commonpackage import is consistent with the PR objective to improve error handling. This change supports the transition from usingapi.InternalServerErrortocommon.InternalServerError.
45-47: Error handling update is aligned with PR objectivesThis change updates the error handling to use the centralized
common.InternalServerErrorfunction instead of the previous implementation. The modification ensures uniform error handling across the codebase, which improves maintainability and consistency.Note that static analysis indicates this line isn't covered by tests, but since this is a standardization of error handling rather than new business logic, this is a minor concern.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: internal/api/v2/handler_bank_accounts_list.go#L46
Added line #L46 was not covered by tests
95-97: Error handling update is aligned with PR objectivesSimilar to the previous change, this updates the error handling for JSON encoding errors to use the centralized
common.InternalServerErrorfunction. This change is consistent with the PR objectives to better handle errors and avoid exposing internal errors to users.Again, static analysis indicates this line isn't covered by tests, which is something to consider for future test improvements.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 96-96: internal/api/v2/handler_bank_accounts_list.go#L96
Added line #L96 was not covered by tests
b194274 to
67c1de6
Compare
67c1de6 to
50df5d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/storage/error.go (1)
24-32: Optional performance improvement for FK violation columns.Using a simple slice for
FKViolationColumnworks fine for small lists. Optionally, a map-based or more advanced matching approach could help avoid potential partial matches and provide more consistent performance if you expect a large or growing list of columns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
internal/api/common/errors.go(1 hunks)internal/api/v2/errors.go(2 hunks)internal/api/v2/handler_accounts_balances.go(2 hunks)internal/api/v2/handler_accounts_create.go(3 hunks)internal/api/v2/handler_accounts_get.go(2 hunks)internal/api/v2/handler_accounts_list.go(2 hunks)internal/api/v2/handler_bank_accounts_create.go(3 hunks)internal/api/v2/handler_bank_accounts_forward_to_connector.go(2 hunks)internal/api/v2/handler_bank_accounts_get.go(3 hunks)internal/api/v2/handler_bank_accounts_list.go(3 hunks)internal/api/v2/handler_connectors_configs.go(2 hunks)internal/api/v2/handler_connectors_list.go(2 hunks)internal/api/v2/handler_payments_create.go(3 hunks)internal/api/v2/handler_payments_get.go(2 hunks)internal/api/v2/handler_payments_list.go(2 hunks)internal/api/v2/handler_pools_balances_at.go(2 hunks)internal/api/v2/handler_pools_create.go(2 hunks)internal/api/v2/handler_pools_get.go(2 hunks)internal/api/v2/handler_pools_list.go(2 hunks)internal/api/v2/handler_tasks_get.go(3 hunks)internal/api/v2/handler_tasks_list.go(3 hunks)internal/api/v2/handler_transfer_initiations_create.go(2 hunks)internal/api/v2/handler_transfer_initiations_get.go(2 hunks)internal/api/v2/handler_transfer_initiations_list.go(2 hunks)internal/api/v3/errors.go(2 hunks)internal/api/v3/handler_accounts_create.go(2 hunks)internal/api/v3/handler_bank_accounts_get.go(2 hunks)internal/api/v3/handler_bank_accounts_list.go(2 hunks)internal/api/v3/handler_connectors_configs.go(2 hunks)internal/api/v3/handler_connectors_uninstall.go(2 hunks)internal/api/v3/handler_payments_create.go(2 hunks)internal/storage/error.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- internal/api/v2/handler_transfer_initiations_list.go
- internal/api/v2/handler_payments_create.go
- internal/api/v3/errors.go
- internal/api/v3/handler_connectors_configs.go
- internal/api/v2/handler_accounts_create.go
- internal/api/v2/handler_pools_get.go
- internal/api/v2/handler_bank_accounts_create.go
- internal/api/v2/handler_pools_balances_at.go
- internal/api/v2/handler_connectors_list.go
- internal/api/v2/handler_payments_list.go
- internal/api/v2/handler_tasks_get.go
- internal/api/v2/handler_connectors_configs.go
- internal/api/v2/handler_pools_list.go
- internal/api/v2/errors.go
- internal/api/v3/handler_accounts_create.go
- internal/api/v2/handler_bank_accounts_list.go
- internal/api/v2/handler_accounts_balances.go
- internal/api/v3/handler_payments_create.go
- internal/api/v2/handler_accounts_get.go
- internal/api/v2/handler_transfer_initiations_get.go
- internal/api/v2/handler_bank_accounts_forward_to_connector.go
- internal/api/v3/handler_connectors_uninstall.go
- internal/api/v2/handler_payments_get.go
- internal/api/v2/handler_bank_accounts_get.go
- internal/api/v3/handler_bank_accounts_get.go
- internal/api/v2/handler_accounts_list.go
- internal/api/v3/handler_bank_accounts_list.go
- internal/api/v2/handler_pools_create.go
- internal/api/v2/handler_tasks_list.go
- internal/api/v2/handler_transfer_initiations_create.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (2)
internal/storage/error.go (2)
16-21: Validate error message changes align with user expectations.The newly added or updated error variables modify the wording for validation, not found, duplicate key, and foreign key violations. Ensure these revised messages accurately reflect the user experience you seek, especially since "value not found" may be interpreted differently from generic "not found" errors.
Would you like to verify downstream usage and tests to confirm that changing user-facing messages does not cause confusion or break existing client expectations?
45-49: Ensure partial constraint name matches do not create false positives.This logic checks if the constraint name contains the column substring. If database constraints follow a consistent naming scheme, this is acceptable. Otherwise, partial matches could occur (e.g., “connector_id_extra”). Consider stricter matching (e.g., checking entire constraint name or using a standardized naming pattern) to avoid misidentifying the column.
… errors (#372) * feat(storage): better fk violation errors * feat(api): do not expose internal errors via the api
Fixes PMNT-69
Fixes PMNT-70