[BB-1555] Wrap Expensify API errors with gRPC Codes #21
Conversation
mateoHernandez123
requested review from
btipling and
laurenleach
as code owners
WalkthroughAdded a direct gRPC dependency and upgraded Expensify client error handling: JSON error bodies (often returned with HTTP 200) are parsed and mapped to gRPC codes with priority for "authentication error", errors are wrapped with prefix "expensify-connector: ", and response decoding moved after error handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant doRequest
participant ExpensifyAPI as "Expensify API"
participant Classifier as "Error Classifier"
participant gRPCCodes as "gRPC Codes"
Client->>doRequest: invoke doRequest
doRequest->>ExpensifyAPI: send HTTP request
ExpensifyAPI-->>doRequest: HTTP 200 + JSON (may include error fields)
doRequest->>doRequest: buffer response
doRequest->>Classifier: decode JSON and check for error fields
alt message contains "authentication error" (case-insensitive)
Classifier->>gRPCCodes: Unauthenticated
else if responseCode present
Classifier->>gRPCCodes: map responseCode (401,403,404,410,429,500,503,...)
else
Classifier->>gRPCCodes: Unknown
end
gRPCCodes-->>doRequest: wrapped error ("expensify-connector: ...")
alt error detected
doRequest-->>Client: return wrapped error (with cause)
else success
doRequest->>doRequest: decode expected response type
doRequest-->>Client: return success value
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/expensify/helper.go (2)
15-38: Minor polish: use errors.New and expand common phrases.Nit-level improvements; behavior unchanged.
Apply this diff:
- apiErr := fmt.Errorf("%s", message) + apiErr := errors.New(message)Optionally recognize more rate‑limit/auth phrases:
@@ - if strings.Contains(msgLower, "invalid credentials") || + if strings.Contains(msgLower, "invalid credentials") || strings.Contains(msgLower, "invalid partneruserid") || strings.Contains(msgLower, "invalid partnerusersecret") || - strings.Contains(msgLower, "authentication failed") { + strings.Contains(msgLower, "authentication failed") || + strings.Contains(msgLower, "auth failed") { return uhttp.WrapErrors(codes.Unauthenticated, message, apiErr) } @@ - if strings.Contains(msgLower, "invalid policyid") || + if strings.Contains(msgLower, "invalid policyid") || strings.Contains(msgLower, "invalid email") || strings.Contains(msgLower, "invalid tag") { return uhttp.WrapErrors(codes.InvalidArgument, message, apiErr) } + // Common rate-limit text (in addition to 429) + if strings.Contains(msgLower, "rate limit") || strings.Contains(msgLower, "too many requests") { + return uhttp.WrapErrors(codes.ResourceExhausted, message, apiErr) + }Remember to
import "errors".
41-66: Consider mapping 409 to Aborted (optional).If Expensify uses 409 for conflicts/partial failures, mapping it to codes.Aborted can be useful.
Apply this diff if applicable:
switch statusCode { +case 409: + return uhttp.WrapErrors(codes.Aborted, message, apiErr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (5)
go.mod(1 hunks)pkg/connector/policy.go(5 hunks)pkg/connector/user.go(3 hunks)pkg/expensify/client.go(6 hunks)pkg/expensify/helper.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/expensify/client.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)pkg/expensify/models.go (1)
User(3-7)
pkg/connector/user.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)
pkg/expensify/helper.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)
pkg/connector/policy.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)
🔇 Additional comments (8)
go.mod (1)
13-13: Direct gRPC dep addition looks right.Promoting google.golang.org/grpc to a direct require aligns with new codes usage across the repo.
pkg/connector/user.go (3)
47-47: Good use of uhttp.WrapErrors for internal construction failure.Attaches codes.Internal and preserves the original error.
60-60: Contextual wrap preserves underlying gRPC status.Using fmt.Errorf with %w keeps lower-layer status from expensify client; no need to re-wrap here.
68-68: Per-user resource error context is clear and preserves status.Message includes email and policy; underlying status is retained.
pkg/connector/policy.go (2)
53-53: Right place to assign codes.Internal.Resource construction failures are internal; wrapping with uhttp.WrapErrors(codes.Internal, ...) is appropriate.
64-64: Consistent contextual errors are fine here.Upstream client already classifies API failures; these fmt.Errorf contexts keep original gRPC status discoverable via %w.
Also applies to: 70-70, 97-97, 113-113
pkg/expensify/client.go (2)
109-112: Good guard for empty policyId.Returning InvalidArgument early avoids a round trip to Expensify.
141-142: Error classification and wrapping are solid.Internal/Unavailable codes for marshal, request creation, transport, and JSON decode, plus API-level classification, give actionable statuses upstream.
Also applies to: 149-150, 155-156, 166-170
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
6635f75 to
d5a7275
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/connector/user.go (1)
47-47: Emails in returned error messages (PII)Consider logging emails/policy IDs with ctxzap fields and returning a generic message to the client. Helps reduce PII exposure in surfaced errors while retaining diagnostics in logs.
Example:
ctxzap.Extract(ctx).Error("error creating user resource", zap.String("email", user.Email), zap.String("policy", parentId.Resource), zap.Error(err)) return nil, "", nil, wrapWithPreservedCode(codes.Internal, "expensify-connector: error creating user resource", err)Also applies to: 60-60, 68-68
pkg/connector/policy.go (1)
53-53: Minimize PII in error messagesAvoid returning emails and policy IDs in surfaced errors; log with ctxzap fields and use a generic client-facing message.
Pattern:
ctxzap.Extract(ctx).Error("error creating user resource", zap.String("policy_id", resource.Id.Resource), zap.String("email", policyEmployee.Email), zap.Error(err), ) return nil, "", nil, wrapWithPreservedCode(codes.Internal, "expensify-connector: error creating user resource", err)Also applies to: 64-64, 70-70, 97-97, 113-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (5)
go.mod(1 hunks)pkg/connector/policy.go(5 hunks)pkg/connector/user.go(3 hunks)pkg/expensify/client.go(6 hunks)pkg/expensify/helper.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/expensify/helper.go
- pkg/expensify/client.go
- go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/policy.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)
pkg/connector/user.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
please revisit doRequest function. I think we should improve the error handling there, instead of wrapping every error with status code Internal, since the real error response could be missed from doRequest
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
d5a7275 to
d8f8878
Compare
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
d8f8878 to
9f8c80c
Compare
pkg/expensify/client.go
Outdated
| return uhttp.WrapErrors(codes.ResourceExhausted, fmt.Sprintf("expensify-connector: %s", errResp.Message), apiErr) | ||
| case 500: | ||
| // Internal server error | ||
| return uhttp.WrapErrors(codes.Internal, fmt.Sprintf("expensify-connector: %s", errResp.Message), apiErr) |
There was a problem hiding this comment.
No I don't think we should return this error as internal as internal to us means we have an internal error, but this is the service having an internal error, let return Unavailable as that's what uhttp does
https://github.com/ConductorOne/baton-sdk/blob/main/pkg/uhttp/wrapper.go#L461-L464
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
9f8c80c to
e982154
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/expensify/client.go (1)
168-169: Consider simplifying the error construction.The
fmt.Errorf("%s", errResp.Message)is essentially just wrapping a string in an error. Since you're already passing the message in the status string toWrapErrors, you could simplify this toerrors.New(errResp.Message)or construct it inline.Apply this diff to simplify:
- apiErr := fmt.Errorf("%s", errResp.Message) - msgLower := strings.ToLower(errResp.Message) + msgLower := strings.ToLower(errResp.Message) + apiErr := errors.New(errResp.Message)Or even more concise, create
apiErrinline in eachWrapErrorscall if preferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (2)
go.mod(1 hunks)pkg/expensify/client.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/expensify/client.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(311-320)
🔇 Additional comments (4)
pkg/expensify/client.go (4)
15-15: LGTM: gRPC codes import is necessary for error mapping.The import is correctly added to support the error classification logic below.
207-209: LGTM: JSON decode correctly uses resType.The decode correctly passes
resTypeto populate the caller's destination, addressing the issue flagged in the previous review. Moving this decode after error handling is also the right approach—no need to decode the success response if an error was detected.
171-175: String matching for authentication errors may not align with documented API format and is fragile to changes.The code checks for
"authentication error"in the response message, but Expensify's Integration Server documents authentication errors with format like"Not authorized to authenticate as user [email protected]". The documented message does not contain the string "authentication error", suggesting the check may not catch the API's actual authentication error responses.Additionally, if Expensify changes the wording (e.g., "auth error", "authentication failed"), this check will break. Verify:
- Whether the documented format ("Not authorized to authenticate as user...") is what the code actually receives
- Whether there are other authentication error message formats Expensify returns that contain "authentication error"
- Whether a more robust approach (e.g., checking
responseCodevalues) would be more reliable than message content matching
177-204: I need to gather more detailed information about the Integration Server API error codes, particularly to verify if the codes mapped in the snippet (401, 404, 429) are actually documented.Approved — error code mappings verified against Expensify documentation.
The code correctly maps 429 to ResourceExhausted, which is documented as the rate limit error code. Code 401 is documented for authentication errors, and 404 is a documented error code. The mappings for 403, 410, 500, and 503 align with standard Expensify Integration Server API responses. The gRPC code translations appropriately match HTTP semantics to gRPC error types.
mateoHernandez123
deleted the
mateoHernandez123/wrap-errors-with-grpc
branch
Summary by CodeRabbit
Bug Fixes
Chores