refactor(api): pass real errors in CodeInternal instead of generic sentinel#1689
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHandlers and unit tests across v1beta1connect now return connect.CodeInternal that wraps the original error via fmt.Errorf(": %w", err); per-request ErrorLogger usage and generic ErrInternalServerError mapping were removed and tests updated accordingly. ChangesInternal error propagation cleanup
Estimated code review effort Possibly related PRs
Suggested reviewers
|
Coverage Report for CI Build 27191405123Coverage decreased (-0.2%) to 43.271%Details
Uncovered Changes
Coverage Regressions7 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/api/v1beta1connect/audit_record.go (1)
269-283:⚠️ Potential issue | 🟠 MajorFix potential truncation when
io.Reader.Readreturnsn>0witherr==io.EOFinstreamReaderInChunks.
streamReaderInChunksbreaks onio.EOFbefore it sends thenbytes from that sameReadcall (seeinternal/api/v1beta1connect/audit_record.goloop around lines 269-272). Per Go’sio.Readercontract,(n>0, io.EOF)is allowed, so the current ordering can drop the last chunk.💡 Suggested fix
func streamReaderInChunks(reader io.Reader, contentType string, stream *connect.ServerStream[httpbody.HttpBody]) error { buffer := make([]byte, HttpChunkSize) for { n, err := reader.Read(buffer) - if err == io.EOF { - break - } - if err != nil { - return connect.NewError(connect.CodeInternal, err) - } if n > 0 { msg := &httpbody.HttpBody{ ContentType: contentType, Data: buffer[:n], } if sendErr := stream.Send(msg); sendErr != nil { return connect.NewError(connect.CodeInternal, sendErr) } } + if err == io.EOF { + break + } + if err != nil { + return connect.NewError(connect.CodeInternal, err) + } } return nil }internal/api/v1beta1connect/authenticate.go (1)
165-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not re-wrap
GetLoggedInPrincipalerrors as internal.
GetLoggedInPrincipalalready returns mapped transport errors; wrapping it again withCodeInternalhides those statuses forAuthToken.Proposed fix
principal, err := h.GetLoggedInPrincipal(ctx, authenticate.SessionClientAssertion, authenticate.ClientCredentialsClientAssertion, authenticate.JWTGrantClientAssertion) if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, err }internal/api/v1beta1connect/billing_invoice_test.go (1)
299-303:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
wantErrmessage in the failure branches.Line 299 and Line 515 only check code, so the updated
wantErrvalues are never validated. This makes the new error-message expectations non-binding.💡 Suggested patch
if tt.wantErr != nil { assert.Error(t, err) assert.Equal(t, tt.errCode, connect.CodeOf(err)) + assert.Contains(t, err.Error(), tt.wantErr.Error()) assert.Nil(t, got) } else {if tt.wantErr != nil { assert.Error(t, err) assert.Equal(t, tt.errCode, connect.CodeOf(err)) + assert.Contains(t, err.Error(), tt.wantErr.Error()) assert.Nil(t, got) } else {Also applies to: 515-519
internal/api/v1beta1connect/billing_webhook_test.go (1)
124-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the expected underlying error message in failure-path tests.
The table now sets
wantErr, but the error branch never checks it. This makes the updated expectations effectively unused.Proposed test assertion fix
if tt.wantErr != nil { assert.Error(t, err) assert.Equal(t, tt.errCode, connect.CodeOf(err)) + assert.Contains(t, err.Error(), tt.wantErr.Error()) assert.Nil(t, got) } else {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b670f155-9428-4e62-8bdf-6550078c7b15
📒 Files selected for processing (75)
internal/api/v1beta1connect/audit_record.gointernal/api/v1beta1connect/audit_record_test.gointernal/api/v1beta1connect/authenticate.gointernal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/billing_check.gointernal/api/v1beta1connect/billing_check_test.gointernal/api/v1beta1connect/billing_checkout.gointernal/api/v1beta1connect/billing_checkout_test.gointernal/api/v1beta1connect/billing_customer.gointernal/api/v1beta1connect/billing_customer_test.gointernal/api/v1beta1connect/billing_invoice.gointernal/api/v1beta1connect/billing_invoice_test.gointernal/api/v1beta1connect/billing_plan.gointernal/api/v1beta1connect/billing_plan_test.gointernal/api/v1beta1connect/billing_product.gointernal/api/v1beta1connect/billing_product_test.gointernal/api/v1beta1connect/billing_subscription.gointernal/api/v1beta1connect/billing_usage.gointernal/api/v1beta1connect/billing_usage_test.gointernal/api/v1beta1connect/billing_webhook.gointernal/api/v1beta1connect/billing_webhook_test.gointernal/api/v1beta1connect/deleter.gointernal/api/v1beta1connect/deleter_test.gointernal/api/v1beta1connect/domain.gointernal/api/v1beta1connect/domain_test.gointernal/api/v1beta1connect/group.gointernal/api/v1beta1connect/group_test.gointernal/api/v1beta1connect/invitations.gointernal/api/v1beta1connect/invitations_test.gointernal/api/v1beta1connect/kyc.gointernal/api/v1beta1connect/kyc_test.gointernal/api/v1beta1connect/metaschema.gointernal/api/v1beta1connect/metaschema_test.gointernal/api/v1beta1connect/namespace.gointernal/api/v1beta1connect/namespace_test.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_billing.gointernal/api/v1beta1connect/organization_invoices.gointernal/api/v1beta1connect/organization_pats.gointernal/api/v1beta1connect/organization_pats_test.gointernal/api/v1beta1connect/organization_projects.gointernal/api/v1beta1connect/organization_serviceuser.gointernal/api/v1beta1connect/organization_serviceuser_credentials.gointernal/api/v1beta1connect/organization_test.gointernal/api/v1beta1connect/organization_tokens.gointernal/api/v1beta1connect/organization_users.gointernal/api/v1beta1connect/permission.gointernal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/permission_check_test.gointernal/api/v1beta1connect/permission_test.gointernal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/policy.gointernal/api/v1beta1connect/preferences.gointernal/api/v1beta1connect/preferences_test.gointernal/api/v1beta1connect/project.gointernal/api/v1beta1connect/project_test.gointernal/api/v1beta1connect/project_users.gointernal/api/v1beta1connect/prospect.gointernal/api/v1beta1connect/prospect_test.gointernal/api/v1beta1connect/relation.gointernal/api/v1beta1connect/relation_test.gointernal/api/v1beta1connect/resource.gointernal/api/v1beta1connect/resource_test.gointernal/api/v1beta1connect/role.gointernal/api/v1beta1connect/serviceuser.gointernal/api/v1beta1connect/serviceuser_test.gointernal/api/v1beta1connect/session.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_orgs.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_pat_test.gointernal/api/v1beta1connect/user_projects.gointernal/api/v1beta1connect/user_test.gointernal/api/v1beta1connect/v1beta1connect.gointernal/api/v1beta1connect/webhook.go
dbc0a9a to
4940bd1
Compare
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)
internal/api/v1beta1connect/billing_invoice_test.go (1)
299-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
wantErris never asserted, so wrapped-error behavior is currently untested.Line 299 and Line 515 branches only check
connect.CodeInternalandgot == nil. The newly addedwantErrvalues at Line 50/277/338/493 are not validated, so these tests can pass even if handlers regress back to generic internal errors.Suggested assertion fix
got, err := handler.ListInvoices(context.Background(), tt.request) if tt.wantErr != nil { assert.Error(t, err) assert.Equal(t, tt.errCode, connect.CodeOf(err)) + assert.ErrorContains(t, err, tt.wantErr.Error()) assert.Nil(t, got) } else { @@ got, err := handler.GetUpcomingInvoice(context.Background(), tt.request) if tt.wantErr != nil { assert.Error(t, err) assert.Equal(t, tt.errCode, connect.CodeOf(err)) + assert.ErrorContains(t, err, tt.wantErr.Error()) assert.Nil(t, got) } else {Also applies to: 515-518
🧹 Nitpick comments (2)
internal/api/v1beta1connect/billing_invoice.go (1)
43-60: ⚡ Quick winRemove the remaining pre-return
errorLoggercalls onCodeInternalpaths.Line 43, Line 91, and Line 169 still initialize
NewErrorLogger(), and Lines 58-59, 106-107, 188-190 still log immediately before returningconnect.CodeInternal. This leaves duplicate logging behavior in this file while the rest of the PR standardizes on wrapped errors + interceptor logging.Suggested cleanup
func (h *ConnectHandler) ListInvoices(ctx context.Context, request *connect.Request[frontierv1beta1.ListInvoicesRequest]) (*connect.Response[frontierv1beta1.ListInvoicesResponse], error) { - errorLogger := NewErrorLogger() @@ - errorLogger.LogServiceError(ctx, request, "ListInvoices.GetByOrgID", err, - "org_id", request.Msg.GetOrgId()) return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("ListInvoices.GetByOrgID: org_id=%s: %w", request.Msg.GetOrgId(), err)) } @@ func (h *ConnectHandler) GetUpcomingInvoice(ctx context.Context, request *connect.Request[frontierv1beta1.GetUpcomingInvoiceRequest]) (*connect.Response[frontierv1beta1.GetUpcomingInvoiceResponse], error) { - errorLogger := NewErrorLogger() @@ - errorLogger.LogServiceError(ctx, request, "GetUpcomingInvoice.GetByOrgID", err, - "org_id", request.Msg.GetOrgId()) return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("GetUpcomingInvoice.GetByOrgID: org_id=%s: %w", request.Msg.GetOrgId(), err)) } @@ func (h *ConnectHandler) SearchInvoices(ctx context.Context, request *connect.Request[frontierv1beta1.SearchInvoicesRequest]) (*connect.Response[frontierv1beta1.SearchInvoicesResponse], error) { - errorLogger := NewErrorLogger() @@ - errorLogger.LogServiceError(ctx, request, "SearchInvoices.SearchInvoices", err, - "query_offset", rqlQuery.Offset, - "query_limit", rqlQuery.Limit) return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("SearchInvoices.SearchInvoices: query_offset=%d query_limit=%d: %w", rqlQuery.Offset, rqlQuery.Limit, err)) }Also applies to: 91-109, 169-192
internal/api/v1beta1connect/role_test.go (1)
207-219: ⚡ Quick winUse non-sentinel service errors in these internal-error tests.
Both test setups still return
ErrInternalServerError, so these cases remain coupled to the sentinel instead of validating arbitrary internal failures.Suggested change
import ( "context" + "errors" "fmt" "testing" "time" @@ { name: "should return internal error if role service fails", setup: func(rs *mocks.RoleService) { + serviceErr := errors.New("test error") rs.EXPECT().List(mock.AnythingOfType("context.backgroundCtx"), role.Filter{ OrgID: testRoleMap[instanceLevelRoleID].OrgID, Scopes: []string{}, - }).Return(nil, ErrInternalServerError) + }).Return(nil, serviceErr) }, @@ - wantErr: connect.NewError(connect.CodeInternal, fmt.Errorf("ListRoles: scopes=%v: %w", []string{}, ErrInternalServerError)), + wantErr: connect.NewError(connect.CodeInternal, fmt.Errorf("ListRoles: scopes=%v: %w", []string{}, errors.New("test error"))), }, @@ { name: "should return internal error if role service gives unknown error", setup: func(rs *mocks.RoleService) { - rs.EXPECT().Delete(mock.AnythingOfType("context.backgroundCtx"), testRoleID).Return(ErrInternalServerError) + rs.EXPECT().Delete(mock.AnythingOfType("context.backgroundCtx"), testRoleID).Return(errors.New("test error")) }, @@ - wantErr: connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteRole: role_id=%s: %w", testRoleID, ErrInternalServerError)), + wantErr: connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteRole: role_id=%s: %w", testRoleID, errors.New("test error"))), },Also applies to: 264-273
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ec1dabd-77c9-4dc2-9120-58d0c50c8e5f
📒 Files selected for processing (76)
internal/api/v1beta1connect/audit_record.gointernal/api/v1beta1connect/audit_record_test.gointernal/api/v1beta1connect/authenticate.gointernal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/billing_check.gointernal/api/v1beta1connect/billing_check_test.gointernal/api/v1beta1connect/billing_checkout.gointernal/api/v1beta1connect/billing_checkout_test.gointernal/api/v1beta1connect/billing_customer.gointernal/api/v1beta1connect/billing_customer_test.gointernal/api/v1beta1connect/billing_invoice.gointernal/api/v1beta1connect/billing_invoice_test.gointernal/api/v1beta1connect/billing_plan.gointernal/api/v1beta1connect/billing_plan_test.gointernal/api/v1beta1connect/billing_product.gointernal/api/v1beta1connect/billing_product_test.gointernal/api/v1beta1connect/billing_subscription.gointernal/api/v1beta1connect/billing_usage.gointernal/api/v1beta1connect/billing_usage_test.gointernal/api/v1beta1connect/billing_webhook.gointernal/api/v1beta1connect/billing_webhook_test.gointernal/api/v1beta1connect/deleter.gointernal/api/v1beta1connect/deleter_test.gointernal/api/v1beta1connect/domain.gointernal/api/v1beta1connect/domain_test.gointernal/api/v1beta1connect/group.gointernal/api/v1beta1connect/group_test.gointernal/api/v1beta1connect/invitations.gointernal/api/v1beta1connect/invitations_test.gointernal/api/v1beta1connect/kyc.gointernal/api/v1beta1connect/kyc_test.gointernal/api/v1beta1connect/metaschema.gointernal/api/v1beta1connect/metaschema_test.gointernal/api/v1beta1connect/namespace.gointernal/api/v1beta1connect/namespace_test.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_billing.gointernal/api/v1beta1connect/organization_invoices.gointernal/api/v1beta1connect/organization_pats.gointernal/api/v1beta1connect/organization_pats_test.gointernal/api/v1beta1connect/organization_projects.gointernal/api/v1beta1connect/organization_serviceuser.gointernal/api/v1beta1connect/organization_serviceuser_credentials.gointernal/api/v1beta1connect/organization_test.gointernal/api/v1beta1connect/organization_tokens.gointernal/api/v1beta1connect/organization_users.gointernal/api/v1beta1connect/permission.gointernal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/permission_check_test.gointernal/api/v1beta1connect/permission_test.gointernal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/policy.gointernal/api/v1beta1connect/preferences.gointernal/api/v1beta1connect/preferences_test.gointernal/api/v1beta1connect/project.gointernal/api/v1beta1connect/project_test.gointernal/api/v1beta1connect/project_users.gointernal/api/v1beta1connect/prospect.gointernal/api/v1beta1connect/prospect_test.gointernal/api/v1beta1connect/relation.gointernal/api/v1beta1connect/relation_test.gointernal/api/v1beta1connect/resource.gointernal/api/v1beta1connect/resource_test.gointernal/api/v1beta1connect/role.gointernal/api/v1beta1connect/role_test.gointernal/api/v1beta1connect/serviceuser.gointernal/api/v1beta1connect/serviceuser_test.gointernal/api/v1beta1connect/session.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_orgs.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_pat_test.gointernal/api/v1beta1connect/user_projects.gointernal/api/v1beta1connect/user_test.gointernal/api/v1beta1connect/v1beta1connect.gointernal/api/v1beta1connect/webhook.go
✅ Files skipped from review due to trivial changes (3)
- internal/api/v1beta1connect/billing_webhook_test.go
- internal/api/v1beta1connect/permission_check_test.go
- internal/api/v1beta1connect/serviceuser_test.go
🚧 Files skipped from review as they are similar to previous changes (59)
- internal/api/v1beta1connect/billing_usage_test.go
- internal/api/v1beta1connect/organization_serviceuser_credentials.go
- internal/api/v1beta1connect/user_test.go
- internal/api/v1beta1connect/user_projects.go
- internal/api/v1beta1connect/organization_serviceuser.go
- internal/api/v1beta1connect/audit_record_test.go
- internal/api/v1beta1connect/organization_invoices.go
- internal/api/v1beta1connect/namespace.go
- internal/api/v1beta1connect/preferences.go
- internal/api/v1beta1connect/billing_customer_test.go
- internal/api/v1beta1connect/relation_test.go
- internal/api/v1beta1connect/prospect_test.go
- internal/api/v1beta1connect/v1beta1connect.go
- internal/api/v1beta1connect/organization_billing.go
- internal/api/v1beta1connect/organization_pats_test.go
- internal/api/v1beta1connect/billing_check_test.go
- internal/api/v1beta1connect/invitations_test.go
- internal/api/v1beta1connect/billing_checkout_test.go
- internal/api/v1beta1connect/organization_users.go
- internal/api/v1beta1connect/user_pat_test.go
- internal/api/v1beta1connect/deleter_test.go
- internal/api/v1beta1connect/permission_test.go
- internal/api/v1beta1connect/billing_plan_test.go
- internal/api/v1beta1connect/billing_webhook.go
- internal/api/v1beta1connect/organization_pats.go
- internal/api/v1beta1connect/metaschema_test.go
- internal/api/v1beta1connect/billing_check.go
- internal/api/v1beta1connect/policy.go
- internal/api/v1beta1connect/authorize.go
- internal/api/v1beta1connect/billing_plan.go
- internal/api/v1beta1connect/kyc.go
- internal/api/v1beta1connect/organization_tokens.go
- internal/api/v1beta1connect/billing_checkout.go
- internal/api/v1beta1connect/billing_product_test.go
- internal/api/v1beta1connect/group_test.go
- internal/api/v1beta1connect/audit_record.go
- internal/api/v1beta1connect/metaschema.go
- internal/api/v1beta1connect/billing_subscription.go
- internal/api/v1beta1connect/billing_customer.go
- internal/api/v1beta1connect/invitations.go
- internal/api/v1beta1connect/organization_projects.go
- internal/api/v1beta1connect/session.go
- internal/api/v1beta1connect/deleter.go
- internal/api/v1beta1connect/permission.go
- internal/api/v1beta1connect/webhook.go
- internal/api/v1beta1connect/permission_check.go
- internal/api/v1beta1connect/organization_test.go
- internal/api/v1beta1connect/user_pat.go
- internal/api/v1beta1connect/project_users.go
- internal/api/v1beta1connect/prospect.go
- internal/api/v1beta1connect/resource.go
- internal/api/v1beta1connect/role.go
- internal/api/v1beta1connect/project.go
- internal/api/v1beta1connect/authenticate.go
- internal/api/v1beta1connect/platform.go
- internal/api/v1beta1connect/domain.go
- internal/api/v1beta1connect/organization.go
- internal/api/v1beta1connect/user.go
- internal/api/v1beta1connect/group.go
9c6218f to
715c7c7
Compare
Summary
Replace
connect.NewError(connect.CodeInternal, ErrInternalServerError)withconnect.NewError(connect.CodeInternal, err)across all handlers, and remove the now-redundanterrorLogger.Log*calls that preceded those returns.Why
The error sanitizer interceptor (#1680) ensures
CodeInternal/CodeUnknownerrors are sanitized to a generic "internal server error" before reaching the client. This means handlers can safely pass the real error — the logger interceptor captures it for debugging, and the sanitizer strips it from the response.Previously, handlers used
ErrInternalServerErrorsentinel which meant the real error was lost in both logs and responses. Now:"internal: pq: connection refused")"internal server error"(sanitized by interceptor)The
errorLogger.Log*calls beforeCodeInternalreturns were also removed since the logger interceptor already logs the error withrequest_idandmethodcontext.Changes
ErrInternalServerError→ realerrreplacementserrorLogger.LogServiceError/LogUnexpectedError/LogTransformErrorcalls beforeCodeInternalreturnserrorLogger := NewErrorLogger()declarationsTest plan
go test ./internal/api/v1beta1connect/...— all tests passgo build ./...— full build passes