[BB-1148] Wrap Errors With gRPC - Google Workspace Connector #56
Conversation
mateoHernandez123
requested review from
btipling and
laurenleach
as code owners
WalkthroughAdds two Google-API-aware error helpers and systematically replaces many raw errors with context-rich, typed wrappers across connector setup, action handlers, feeds, and resource handlers; one method signature in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-05T20:15:11.628ZApplied to files:
🔇 Additional comments (20)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/connector/role.go (4)
115-118: List role assignments: decide on NotFound semanticsIf the role can be deleted between discovery and listing assignments, Directory API may return 404. Previously this was treated as empty. Confirm desired behavior; otherwise, convert 404 to an empty page to keep reconciliation resilient.
- if err != nil { - return nil, "", nil, wrapError(err, fmt.Sprintf("google-workspace: failed to list role assignments for role %s", resource.Id.Resource)) - } + if err != nil { + if gerr := new(googleapi.Error); errors.As(err, &gerr) && gerr.Code == http.StatusNotFound { + // Role disappeared; treat as empty. + return nil, "", nil, nil + } + return nil, "", nil, wrapError(wrapGoogleApiError(err), fmt.Sprintf("google-workspace-v2: failed to list role assignments for role %s", resource.Id.Resource)) + }Please confirm whether returning empty on 404 matches the connector’s contract for other list endpoints.
175-186: Grant: preserve idempotency and attach gRPC/rate‑limit detailsKeeping 409 as a no‑op is fine for idempotency. For other errors, route through wrapGoogleApiError before wrapError so callers get gRPC codes and rate‑limit details.
- return nil, nil, wrapError(err, "google-workspace-v2: failed to grant role assignment") + return nil, nil, wrapError(wrapGoogleApiError(err), "google-workspace-v2: failed to grant role assignment")If possible, consider returning an existing grant on 409 by attempting a lookup; if not feasible, current behavior is acceptable.
205-216: Revoke: attach gRPC/rate‑limit details and minor log nit
- Wrap via wrapGoogleApiError to surface codes/details.
- Log fields use "group_id" in a role path; consider "role_id" for clarity.
- return nil, wrapError(err, "google-workspace-v2: failed to revoke role assignment") + return nil, wrapError(wrapGoogleApiError(err), "google-workspace-v2: failed to revoke role assignment")Rename log key from "group_id" to "role_id" (Lines 209–211).
225-228: Get: attach gRPC/rate‑limit details and align prefixAlign prefix with other messages and route through wrapGoogleApiError.
- return nil, nil, wrapError(err, fmt.Sprintf("google-workspace: failed to retrieve role: %s", resourceId.Resource)) + return nil, nil, wrapError(wrapGoogleApiError(err), fmt.Sprintf("google-workspace-v2: failed to retrieve role: %s", resourceId.Resource))pkg/connector/error_helpers.go (2)
23-36: Map 429 to ResourceExhausted and add common client‑error mappingsImprove signal to callers and retry logic by using more specific gRPC codes.
switch e.Code { case http.StatusRequestTimeout: return wrapGoogleApiErrorWithRateLimitInfo(codes.DeadlineExceeded, e, err) -case http.StatusTooManyRequests, http.StatusServiceUnavailable: - return wrapGoogleApiErrorWithRateLimitInfo(codes.Unavailable, e, err) +case http.StatusTooManyRequests: + return wrapGoogleApiErrorWithRateLimitInfo(codes.ResourceExhausted, e, err) +case http.StatusServiceUnavailable, http.StatusBadGateway: + return wrapGoogleApiErrorWithRateLimitInfo(codes.Unavailable, e, err) case http.StatusNotFound: return wrapGoogleApiErrorWithRateLimitInfo(codes.NotFound, e, err) case http.StatusUnauthorized: return wrapGoogleApiErrorWithRateLimitInfo(codes.Unauthenticated, e, err) case http.StatusForbidden: return wrapGoogleApiErrorWithRateLimitInfo(codes.PermissionDenied, e, err) +case http.StatusBadRequest: + return wrapGoogleApiErrorWithRateLimitInfo(codes.InvalidArgument, e, err) +case http.StatusConflict: + return wrapGoogleApiErrorWithRateLimitInfo(codes.AlreadyExists, e, err) +case http.StatusPreconditionFailed: + return wrapGoogleApiErrorWithRateLimitInfo(codes.FailedPrecondition, e, err) +case 422: // Unprocessable Entity + return wrapGoogleApiErrorWithRateLimitInfo(codes.InvalidArgument, e, err) case http.StatusNotImplemented: return wrapGoogleApiErrorWithRateLimitInfo(codes.Unimplemented, e, err) }
42-49: Avoid nested errors.Join and keep status first in the chainPass component errors directly rather than pre‑joining to reduce nesting depth.
- return wrapGoogleApiErrorWithRateLimitInfo(codes.Unknown, e, errors.Join(fmt.Errorf("unexpected status code: %d", e.Code), err)) + return wrapGoogleApiErrorWithRateLimitInfo(codes.Unknown, e, fmt.Errorf("unexpected status code: %d", e.Code), err)Also consider including e.Message/e.Details in msg construction for Unknown to aid debugging.
📜 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 (10)
go.mod(1 hunks)pkg/connector/actions.go(1 hunks)pkg/connector/admin_event_feed.go(3 hunks)pkg/connector/connector.go(1 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(5 hunks)pkg/connector/helpers.go(1 hunks)pkg/connector/role.go(5 hunks)pkg/connector/usage_event_feed.go(1 hunks)pkg/connector/user.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/error_helpers.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/ratelimit/http.go (1)
ExtractRateLimitData(66-130)
🔇 Additional comments (21)
go.mod (1)
9-9: Benign dependency declaration change.The addition of
googleapis/gax-go/v2to the require block with its subsequent removal from indirect dependencies results in no net functional change. This appears to be a dependency resolution reordering.pkg/connector/usage_event_feed.go (1)
149-149: Good contextual error wrapping.The error from the Google API request is now wrapped with operation-specific context, making it easier to identify the failure point. The message clearly indicates the operation that failed.
pkg/connector/connector.go (1)
218-218: Consistent error wrapping for domain listing.The error is now wrapped with clear context about the failed operation, following the same pattern established across the codebase.
pkg/connector/actions.go (1)
37-37: Excellent error context with resource identifier.The error message includes the specific user ID, making it easy to trace which operation failed. This is valuable for debugging and operations.
pkg/connector/helpers.go (2)
54-65: Clean nil-check and error wrapping logic.The implementation correctly handles the nil case and provides flexible message prefixing. The pattern of wrapping first, then optionally prefixing with context, ensures consistent error handling throughout the codebase.
52-65: No action required —error_helpers.goexists andwrapGoogleApiErroris properly defined.The verification confirms that
error_helpers.gois present in the repository atpkg/connector/error_helpers.goand contains thewrapGoogleApiErrorfunction (lines 17-50) with the correct signature. The call inhelpers.go(line 58) is valid and will compile without errors.Likely an incorrect or invalid review comment.
pkg/connector/group.go (5)
68-68: Consistent error wrapping for group listing.The error is now wrapped with clear operation context, replacing the previous raw error return.
137-137: Good context with resource identifier.The error message includes the specific group ID, making it easy to identify which group's member listing failed.
196-209: Clear error differentiation in grant flow.The three error paths (get existing member, insert member, unexpected error) each have distinct, descriptive messages. This makes troubleshooting grant operations much easier.
239-239: Clear revoke failure context.The error message clearly indicates a membership revocation failure, making it easy to understand the operation that failed.
251-251: Error message includes resource identifier.Including the specific group resource ID in the error message aids in debugging and troubleshooting.
pkg/connector/admin_event_feed.go (3)
69-69: Clear context for activity listing failures.The error is now wrapped with specific operation context, making it easier to identify failures in the admin event feed.
384-384: User lookup error includes email for traceability.Including the email address in the error message makes it easy to identify which user lookup failed during event feed processing.
427-427: Group lookup error includes email for traceability.Including the email address in the error message makes it easy to identify which group lookup failed during event feed processing.
pkg/connector/user.go (5)
82-82: Clear error context for user listing.The error is now wrapped with operation-specific context, making it easier to identify failures in user listing operations.
230-230: Get user error includes resource identifier.Including the specific user resource ID in the error message aids in debugging and makes it easy to identify which user retrieval failed.
390-390: User fetch error includes identifier for deletion flow.The error message clearly indicates which user fetch failed during the deletion process, making troubleshooting easier.
403-403: Suspend operation error includes user identifier.The error message clearly indicates which user suspension failed, providing good context for debugging.
406-406: Correct return value for successful deletion.Returning
nil, nilon successful user suspension (deletion) is correct and matches the function signature(annotations.Annotations, error).pkg/connector/error_helpers.go (1)
52-71: Looks solid: attaches rate‑limit details as grpc-status detailsThe helper builds a status with details and preserves the original error via errors.Join. Good ordering (status first) for errors.As on status.
Would you like a small unit test table for HTTP→gRPC mapping and presence of RateLimitDescription on 429/503? I can generate it quickly.
pkg/connector/role.go (1)
55-58: Update error message prefix for consistencyThe prefix standardization concern is valid, but the suggestion to wrap with
wrapGoogleApiErroris incorrect. ThewrapErrorfunction already callswrapGoogleApiErrorinternally (line 58 in helpers.go), so adding it again would be redundant.Only update the prefix from "google-workspace" to "google-workspace-v2" to match the pattern used at nearby lines 185 and 215:
- return nil, "", nil, wrapError(err, "google-workspace: failed to list roles") + return nil, "", nil, wrapError(err, "google-workspace-v2: failed to list roles")Likely an incorrect or invalid review comment.
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
ae979bc to
0778452
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/connector/group.go (1)
206-216: Bug: using the wrong service for Members.Get after 409On conflict, code calls o.groupService.Members.Get(...). This likely lacks member read scope — should use o.groupMemberService.
- assignment, err = o.groupService.Members.Get(entitlement.Resource.Id.Resource, principal.GetId().GetResource()).Context(ctx).Do() + assignment, err = o.groupMemberService.Members.Get(entitlement.Resource.Id.Resource, principal.GetId().GetResource()).Context(ctx).Do()pkg/connector/actions.go (2)
291-299: Fix status comparison: "IN_PROGRESS" not matched by "inProgress".API returns "IN_PROGRESS"; current check misses underscore variant and can create duplicate transfers.
Apply robust normalization:
- if strings.EqualFold(t.OverallTransferStatusCode, "new") || strings.EqualFold(t.OverallTransferStatusCode, "inProgress") { + status := strings.ToUpper(strings.ReplaceAll(t.OverallTransferStatusCode, " ", "_")) + if status == "NEW" || status == "IN_PROGRESS" {
236-236: Use trimmed IDs when inserting transfers.You validate trimmed src/dst but pass the untrimmed field values to Insert. This can cause API errors when inputs contain whitespace.
- return c.dataTransferInsert(ctx, appIdGoogleDocsAndGoogleDrive, sourceField.StringValue, targetField.StringValue, params) + return c.dataTransferInsert(ctx, appIdGoogleDocsAndGoogleDrive, src, dst, params)- return c.dataTransferInsert(ctx, appIdGoogleCalendar, sourceField.StringValue, targetField.StringValue, params) + return c.dataTransferInsert(ctx, appIdGoogleCalendar, src, dst, params)Also applies to: 267-267
🧹 Nitpick comments (20)
pkg/connector/role.go (2)
44-45: Consider InvalidArgument for bad pagination tokensDecoding a client-supplied pagination token failing is typically an InvalidArgument, not Internal. Aligning here and in similar paths improves signal to callers.
Also applies to: 102-103
59-60: Map googleapi HTTP errors to precise gRPC codes (avoid blanket Unknown)Instead of codes.Unknown, map common HTTP statuses:
- 404 → NotFound
- 401/403 → PermissionDenied
- 429 → ResourceExhausted
- 500/502/503/504 → Unavailable
Recommend a small helper and use it at these sites.
- return nil, "", nil, uhttp.WrapErrors(codes.Unknown, "google-workspace: failed to list roles", err) + return nil, "", nil, uhttp.WrapErrors(mapGoogleToGRPC(err), "google-workspace: failed to list roles", err)Add once in this package:
func mapGoogleToGRPC(err error) codes.Code { var gerr *googleapi.Error if errors.As(err, &gerr) { switch gerr.Code { case http.StatusNotFound: return codes.NotFound case http.StatusUnauthorized, http.StatusForbidden: return codes.PermissionDenied case http.StatusTooManyRequests: return codes.ResourceExhausted case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout, http.StatusInternalServerError: return codes.Unavailable default: return codes.Unknown } } return codes.Unknown }Also applies to: 126-127, 194-195, 224-225, 236-237
pkg/connector/admin_event_feed.go (4)
57-60: Token marshal/unmarshal: InvalidArgument may fit betterThese are client-cursor issues; consider codes.InvalidArgument instead of Internal.
Also applies to: 128-131
69-72: List activities: map googleapi HTTP to gRPC codesSame mapping guidance as elsewhere; avoid blanket Unknown on API failures.
268-271: Avoid wrapping an already wrapped error againlookupGroup/lookupUser already wrap with uhttp; re-wrapping here produces joined statuses with duplicated context. Prefer propagating the original (or add context via fmt.Errorf("%w")).
- return nil, uhttp.WrapErrors(codes.Internal, fmt.Sprintf("google-workspace: failed to lookup group: %s", groupEmail), err) + return nil, fmt.Errorf("google-workspace: failed to lookup group: %s: %w", groupEmail, err)Also applies to: 282-285
371-374: Directory service and get failures: map HTTP->gRPC codesFor getDirectoryService()/Users.Get()/Groups.Get() failures, apply the centralized HTTP→gRPC mapping to produce PermissionDenied/NotFound/Unavailable where appropriate.
Also applies to: 416-417, 386-387, 429-430
pkg/connector/group.go (2)
222-224: Use groupMemberEntitlement for clarityBoth constants resolve to "member", but using roleMemberEntitlement here is confusing. Swap to groupMemberEntitlement.
- grant := sdkGrant.NewGrant(entitlement.Resource, roleMemberEntitlement, principal.GetId()) + grant := sdkGrant.NewGrant(entitlement.Resource, groupMemberEntitlement, principal.GetId())
70-71: Adopt centralized HTTP→gRPC mappingReplace several codes.Unknown returns with a helper that maps googleapi.Error HTTP statuses to precise gRPC codes (NotFound/PermissionDenied/ResourceExhausted/Unavailable). See proposed helper in role.go comment.
Also applies to: 147-148, 212-219, 249-250, 259-262
pkg/connector/usage_event_feed.go (4)
175-177: Don’t abort the entire feed on userTrait build failureThis is per-event data; log and continue like you already do for newV2Event errors.
- if err != nil { - return nil, nil, nil, uhttp.WrapErrors(codes.Internal, "google-workspace: failed to create user trait", err) - } + if err != nil { + l.Error("google-workspace-event-feed: failed to create user trait", zap.Error(err)) + continue + }
209-215: Missing parameters should skip, not raise InternalEvents lacking client_id or app_name are malformed upstream; prefer skipping with debug log to keep the stream healthy.
- return nil, uhttp.WrapErrors(codes.Internal, "google-workspace: no client_id in event parameters", nil) + return nil, nilApply similarly for app_name. Optionally add a debug log at the call site.
86-91: Pagination token decode/marshal: InvalidArgument fits betterSame guidance as other files: treat malformed cursors as InvalidArgument for clearer client feedback.
Also applies to: 115-116
149-151: ListEvents failures: map HTTP errors; parsing errors already OKMap req.Do() errors via HTTP→gRPC; keep internal for time parse errors.
Also applies to: 156-156, 129-130
pkg/connector/user.go (3)
58-59: Consider InvalidArgument for pagination token unmarshalConsistent with other resources.
84-85: Replace codes.Unknown with mapped gRPC codes on API errorsAdopt the shared HTTP→gRPC mapping helper noted earlier for list/get/update failures.
Also applies to: 232-233, 392-393, 405-406
158-190: Optional: dedupe helpers via extractFromInterfaceextractRelations/extractOrganizations duplicate extractFromInterface logic. Consider refactoring to use the generic helper for consistency.
pkg/connector/actions.go (4)
26-26: Call WrapErrors without a trailing nil.Passing a literal nil to a variadic errs parameter is unnecessary and confusing; WrapErrors already handles zero errs.
Example change (apply consistently across the listed lines):
- return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing resource ID", nil) + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing resource ID")Also applies to: 31-31, 67-67, 110-110, 153-153, 157-157, 211-211, 215-215, 222-222, 225-225, 243-243, 247-247, 254-254, 257-257, 345-345, 366-366, 373-373, 380-380, 388-388
78-81: Map Google HTTP errors to precise gRPC codes (e.g., 404 → NotFound).Currently these paths wrap as Unknown. Improve UX by translating googleapi.Error HTTP codes to gRPC (NotFound, PermissionDenied, DeadlineExceeded, etc.).
Minimal example for one call (repeat for similar sites):
- u, err := userService.Users.Get(userId).Context(ctx).Do() - if err != nil { - return nil, nil, uhttp.WrapErrors(codes.Unknown, fmt.Sprintf("google-workspace: failed to get user %s for disableUser", userId), err) - } + u, err := userService.Users.Get(userId).Context(ctx).Do() + if err != nil { + var gerr *googleapi.Error + if errors.As(err, &gerr) && gerr.Code == http.StatusNotFound { + return nil, nil, uhttp.WrapErrors(codes.NotFound, fmt.Sprintf("google-workspace: user not found: %s", userId), err) + } + return nil, nil, uhttp.WrapErrors(codes.Unknown, fmt.Sprintf("google-workspace: failed to get user %s for disableUser", userId), err) + }Add imports:
+ "net/http" + "google.golang.org/api/googleapi"Also applies to: 121-124, 196-197, 325-326
163-171: Validate new_primary_email is within verified domains.Schema says "must be within a verified domain", but code only checks RFC format. Enforce domain membership to fail fast with InvalidArgument.
newPrimary := newEmailField.StringValue // Validate that newPrimary is a valid email address if _, err := mail.ParseAddress(newPrimary); err != nil { return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, fmt.Sprintf("google-workspace: invalid email address: %s", newPrimary), err) } + // Enforce verified domain + domains, derr := c.getDomains(ctx) + if derr != nil { + return nil, nil, uhttp.WrapErrors(codes.Unknown, "google-workspace: failed to get domains", derr) + } + parts := strings.SplitN(strings.ToLower(newPrimary), "@", 2) + if len(parts) != 2 { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: invalid email address format", nil) + } + nd := parts[1] + okDomain := false + for _, d := range domains { + if strings.EqualFold(nd, d) { + okDomain = true + break + } + } + if !okDomain { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, fmt.Sprintf("google-workspace: new_primary_email domain '%s' is not among verified domains", nd)) + }
136-136: Typo in comment.- // This is needed becasuse the SDK would omit any field that has the field type default value (false). + // This is needed because the SDK omits fields with their type's default value (false).pkg/connector/connector.go (1)
503-504: Improve cache type-mismatch message and drop nil arg.Include the expected type to aid debugging and avoid passing nil to WrapErrors.
- return nil, uhttp.WrapErrors(codes.Internal, fmt.Sprintf("google-workspace: cache entry for scope %s exists, but is not of type %s", scope, reflect.TypeOf(service)), nil) + var expected *T + return nil, uhttp.WrapErrors( + codes.Internal, + fmt.Sprintf("google-workspace: cache entry for scope %s exists, but is %T; expected %T", scope, service, expected), + )
📜 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 (7)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(19 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/group.go(12 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/connector/actions.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/group.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryGroupMemberScope(144-144)
pkg/connector/user.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/role.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/admin_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/connector.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryDomainReadonlyScope(138-138)
pkg/connector/usage_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
🔇 Additional comments (11)
pkg/connector/role.go (2)
123-124: Grants NotFound vs empty result — confirm contractComment says “Return no grants if the role no longer exists,” but this now returns NotFound. Ensure the caller expects an error (to prune the resource) rather than an empty page. If empty page is preferred, return nils instead.
75-76: LGTM on internal error wrapping and validationsResource-build, next-token generation, precondition checks, and arg validation are wrapped consistently.
Also applies to: 81-82, 136-137, 145-146, 169-170, 172-173, 178-178, 250-251
pkg/connector/admin_event_feed.go (1)
74-77: LGTM: parsing latest event time is correctly wrapped and guardedpkg/connector/group.go (2)
143-147: Grants NotFound vs empty result — confirm contractSame note as roles: decide whether a missing group should yield NotFound (to prune) or an empty page. Align across connectors.
95-96: LGTM: internal failures are wrapped with contextResource creation, pagination, and Get build paths look consistent.
Also applies to: 101-102, 165-166, 277-278
pkg/connector/usage_event_feed.go (1)
198-206: LGTM: stream state management and cursor advancementpkg/connector/user.go (2)
235-239: LGTM: wrapping extraction failure improves diagnosability
302-318: LGTM: posix/external IDs extraction now error-awareInternal wrap is appropriate; calling sites already wrap with context.
pkg/connector/connector.go (2)
302-308: Good use of structured wrapping in service init.Also applies to: 333-334
575-607: Nice: action registrations wrapped with context and logs.Consistent Internal wrapping and logging per action. No changes requested.
pkg/connector/actions.go (1)
350-350: No changes required — RELEASE_RESOURCES value is correct.The API expects "TRUE" (string) for RELEASE_RESOURCES, which matches the current implementation. The code already uses the correct uppercase literal.
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
0778452 to
25aaae7
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/connector/usage_event_feed.go (1)
132-139: Guard against nil pToken before using pToken.SizeListEvents dereferences pToken (Line 133) before unmarshal; if a caller passes nil, this will panic.
Apply:
-req := s.Activities.List("all", "token") -req.MaxResults(int64(pToken.Size)) +req := s.Activities.List("all", "token") +if pToken != nil { + req.MaxResults(int64(pToken.Size)) +}pkg/connector/admin_event_feed.go (2)
376-387: Add .Context(ctx) to user lookupEnsure requests honor cancellation/timeouts.
- user, err := userService.Users.Get(email).Do() + user, err := userService.Users.Get(email).Context(ctx).Do()
419-430: Add .Context(ctx) to group lookupSame rationale as user lookup.
- group, err := groupService.Groups.Get(email).Do() + group, err := groupService.Groups.Get(email).Context(ctx).Do()
🧹 Nitpick comments (9)
pkg/connector/usage_event_feed.go (4)
84-91: Use InvalidArgument for malformed client cursorsCorrupt/invalid cursors are client-provided artifacts; returning codes.InvalidArgument is clearer than Internal.
- return nil, uhttp.WrapErrors(codes.Internal, "google-workspace: failed to decode page token", err) + return nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: failed to decode page token", err) @@ - return nil, uhttp.WrapErrors(codes.Internal, "google-workspace: failed to unmarshal page token JSON", err) + return nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: failed to unmarshal page token JSON", err)Also applies to: 89-91
23-23: Anchor privateAppIDRegex to exact 21 digitsAvoid false positives on longer strings containing 21 digits.
-var privateAppIDRegex = regexp.MustCompile("[0-9]{21}") +var privateAppIDRegex = regexp.MustCompile(`^\d{21}$`)Also applies to: 220-223
55-63: Remove unnecessary shadowing in parameter helpersp := p inside the loops is redundant.
- for _, p := range parameters { - p := p + for _, p := range parameters { if p.Name == name { return p.Value } }(and similarly in hasParameter)
Also applies to: 65-72
118-121: Nit: variable name typobasedToken likely meant base64Token for clarity.
- basedToken := base64.StdEncoding.EncodeToString(data) - return basedToken, nil + base64Token := base64.StdEncoding.EncodeToString(data) + return base64Token, nilpkg/connector/actions.go (1)
339-346: Input parsing is solid; consider a small helper to DRY field extractionYou repeatedly parse string/bool fields from args. A tiny helper reduces duplication and centralizes type errors.
Example helper:
func mustStringArg(args *structpb.Struct, key, msg string) (string, error) { v, ok := args.Fields[key] if !ok || v == nil { return "", uhttp.WrapErrors(codes.InvalidArgument, fmt.Sprintf("google-workspace: missing %s", key)) } sv, ok := v.GetKind().(*structpb.Value_StringValue) if !ok || strings.TrimSpace(sv.StringValue) == "" { return "", uhttp.WrapErrors(codes.InvalidArgument, fmt.Sprintf("google-workspace: %s must be a non-empty string", key)) } return sv.StringValue, nil }Use in update/enable/disable/transfer handlers.
Also applies to: 356-389
pkg/connector/role.go (1)
216-223: Logging key nit: use role_id instead of group_idThis log refers to role member deletion but logs group_id.
- l.Info("google-workspace-v2: role member is being deleted but doesn't exist", - zap.String("group_id", grant.Entitlement.Resource.Id.Resource), + l.Info("google-workspace-v2: role member is being deleted but doesn't exist", + zap.String("role_id", grant.Entitlement.Resource.Id.Resource), zap.String("user_id", grant.Principal.GetId().GetResource()))pkg/connector/connector.go (2)
313-324: Wrap non-401 token fetch failures for consistencyUnauthorized is handled specially; other token errors should be wrapped for uniform error surface.
- return nil, err + return nil, uhttp.WrapErrors(codes.Unknown, "google-workspace: failed to fetch OAuth token", err)
298-336: Optional: add short per-call deadlinesExternal Google Admin/Reports/DataTransfer calls rely on upstream timeouts. Consider context.WithTimeout around high-latency calls (e.g., 10–30s) to fail fast and improve resilience.
Also applies to: 511-555
pkg/connector/admin_event_feed.go (1)
109-111: Prefer evt.Name in debug logsType is generic; Name is more actionable when triaging.
- l.Debug("google-workspace-event-feed: skipping event", zap.String("event", evt.Name), zap.String("type", evt.Type)) + l.Debug("google-workspace-event-feed: skipping event", zap.String("name", evt.Name), zap.String("type", evt.Type))(and similarly for the USER branch)
Also applies to: 214-215
📜 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 (7)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(19 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/group.go(12 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/group.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/connector/usage_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/user.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
UserOrganization(5072-5114)
pkg/connector/admin_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/connector.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryDomainReadonlyScope(138-138)
pkg/connector/actions.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/role.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryRolemanagementScope(165-165)
🔇 Additional comments (10)
pkg/connector/actions.go (1)
229-235: Good validation UX and messagesNice job on clear, actionable validation errors for privacy_levels and release_resources.
Also applies to: 260-265
pkg/connector/role.go (2)
117-127: Graceful NotFound handling looks goodReturning codes.NotFound for missing roles prevents hard failures during sync.
39-84: Consistent wrapping and pagination handlingList/Get paths now wrap errors consistently with context; pagination token handling is solid.
Also applies to: 230-251
pkg/connector/connector.go (1)
338-347: Consistent error wrapping across connector paths looks goodService creation, metadata, domain fetch, and action registration now return contextual, coded errors. Nice.
Also applies to: 389-399, 425-436, 575-607
pkg/connector/admin_event_feed.go (1)
146-167: Solid contextual wrappingEvent construction errors are wrapped with precise messages; continuing on per-event errors is appropriate here.
Also applies to: 172-190, 202-216
pkg/connector/user.go (5)
13-13: LGTM! Imports added correctly.The new imports support the standardized error-wrapping pattern being introduced across the connector.
Also applies to: 17-17
53-107: LGTM! Error wrapping is consistent and well-structured.The error handling in the
Listmethod properly distinguishes between internal processing errors (codes.Internalfor pagination/marshaling) and external API errors (codes.Unknownfor Google API calls). All messages provide clear context.
225-270: LGTM! Error handling is appropriate.The
Getmethod correctly appliescodes.Unknownfor external API calls andcodes.Internalfor data processing errors. Including the resource ID in error messages aids debugging.
302-343: LGTM! Data extraction errors properly wrapped.The error handling for extracting POSIX accounts and external IDs consistently uses
codes.Internal, which is appropriate for internal data processing failures.
387-409: LGTM! Error wrapping is correct, with a minor note.The error handling in the
Deletemethod is consistent with the rest of the file. The change at line 408 fromreturn nil, errtoreturn nil, nilis a minor improvement in code clarity—at that point,erris guaranteed to benil.Note: The comment on line 395 says "return true" but the method signature returns
(annotations.Annotations, error). This is a pre-existing documentation issue unrelated to this PR.
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
25aaae7 to
095b619
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pkg/connector/actions.go (3)
236-236: Pass the trimmed user IDs into dataTransferInsertAfter trimming and validating
src/dst, Lines 236 and 267 still forward the rawStringValues (which may include leading/trailing whitespace) todataTransferInsert. That defeats the validation and can make the API reject the IDs. Use the trimmedsrcanddstvalues instead in both Drive and Calendar transfers.- return c.dataTransferInsert(ctx, appIdGoogleDocsAndGoogleDrive, sourceField.StringValue, targetField.StringValue, params) + return c.dataTransferInsert(ctx, appIdGoogleDocsAndGoogleDrive, src, dst, params)- return c.dataTransferInsert(ctx, appIdGoogleCalendar, sourceField.StringValue, targetField.StringValue, params) + return c.dataTransferInsert(ctx, appIdGoogleCalendar, src, dst, params)Also applies to: 267-267
305-309: Handle nil transfers before dereferencing NextPageToken
Transfers.List().Do()may succeed with a nil response body; Line 305 dereferencestransfers.NextPageTokenregardless, which panics in that scenario. Add an explicit nil check before readingNextPageToken(and skip assigning the page token when nil) so the loop exits safely.- if transfers.NextPageToken == "" { + if transfers == nil || transfers.NextPageToken == "" { break } pageToken = transfers.NextPageToken
24-32: Guard structpb field lookups before calling GetKindLine 24 (and again on Line 29) dereferences
args.Fields["…"]before checking if the map contained a value, so a missing argument yields a nil*structpb.Value,GetKind()panics, and the new Wrapped error is never returned. Please grab the map value first, ensure it’s present and non-nil, and only then callGetKind(). The same pattern needs to be applied touser_id,new_primary_email, and the drive/calendar target fields in the other action handlers.- guidField, ok := args.Fields["resource_id"].GetKind().(*structpb.Value_StringValue) - if !ok { + resourceIDValue, present := args.Fields["resource_id"] + if !present || resourceIDValue == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing resource ID") + } + guidField, ok := resourceIDValue.GetKind().(*structpb.Value_StringValue) + if !ok { return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing resource ID") } - isSuspendedField, ok := args.Fields["is_suspended"].GetKind().(*structpb.Value_BoolValue) - if !ok { + isSuspendedValue, present := args.Fields["is_suspended"] + if !present || isSuspendedValue == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing is_suspended") + } + isSuspendedField, ok := isSuspendedValue.GetKind().(*structpb.Value_BoolValue) + if !ok { return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing is_suspended") }pkg/connector/admin_event_feed.go (1)
55-55: GuardpTokenbefore callingSize
ListEventsstill dereferencespTokenwithout a nil check, so the first page (where callers pass a nil token) will panic. Please guard theMaxResultscall before touchingpToken.Size(), mirroring the earlier fix fromusage_event_feed.req := s.Activities.List("all", "admin") -req.MaxResults(int64(pToken.Size)) +if pToken != nil { + req.MaxResults(int64(pToken.Size)) +}
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(19 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(11 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(10 hunks)pkg/connector/user.go(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T18:24:02.512Z
Learnt from: agustin-conductor
Repo: ConductorOne/baton-google-workspace PR: 55
File: pkg/connector/actions.go:232-233
Timestamp: 2025-10-16T18:24:02.512Z
Learning: In Google Workspace Data Transfer API (admin/datatransfer/v1), the PRIVACY_LEVEL parameter values must be lowercase ("private", "shared"). Using uppercase values causes the API to ignore the parameter and transfer all privacy levels regardless of what was specified.
Applied to files:
pkg/connector/actions.go
🧬 Code graph analysis (8)
pkg/connector/connector.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryDomainReadonlyScope(138-138)
pkg/connector/group.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (3)
AdminDirectoryGroupMemberScope(144-144)Members(3259-3282)Member(3211-3252)
pkg/connector/actions.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/admin_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/user.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
UserOrganization(5072-5114)
pkg/connector/usage_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/error_helpers.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/ratelimit/http.go (1)
ExtractRateLimitData(66-130)
pkg/connector/role.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryRolemanagementScope(165-165)
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
095b619 to
44b5d37
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(19 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(11 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(10 hunks)pkg/connector/user.go(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T18:24:02.512Z
Learnt from: agustin-conductor
Repo: ConductorOne/baton-google-workspace PR: 55
File: pkg/connector/actions.go:232-233
Timestamp: 2025-10-16T18:24:02.512Z
Learning: In Google Workspace Data Transfer API (admin/datatransfer/v1), the PRIVACY_LEVEL parameter values must be lowercase ("private", "shared"). Using uppercase values causes the API to ignore the parameter and transfer all privacy levels regardless of what was specified.
Applied to files:
pkg/connector/actions.go
🧬 Code graph analysis (8)
pkg/connector/usage_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/user.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/role.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryRolemanagementScope(165-165)
pkg/connector/admin_event_feed.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/actions.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/error_helpers.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/ratelimit/http.go (1)
ExtractRateLimitData(66-130)
pkg/connector/connector.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryDomainReadonlyScope(138-138)
pkg/connector/group.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (3)
AdminDirectoryGroupMemberScope(144-144)Members(3259-3282)Member(3211-3252)
pkg/connector/group.go
Outdated
| err := bag.Unmarshal(pt.Token) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| return nil, "", nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: failed to unmarshal pagination token in group Grants", err) |
There was a problem hiding this comment.
Let's not wrap any of these pagination bag errors, as per Luisina's guidelines we should just do it when returning erros from an http request, sdk library, or custom errors regarding permissions for example where a GRPC makes sense. The point of wrapping these errors it to alert less. A pagination bag error should 100% alert us as it is a connector bug.
I also noticed that we're being inconsistent here codes.InvalidArgument here and codes.Internal, but this 100% should not be a codes.InvalidArgument as this has nothing to do with how the customer configured the connector. Please just remove all these wrapped errors that don't align with Luisina's guidance. Also be sure you are syncing the connector after making changes to make sure this doesn't impact the connector's ability to sync.
Wrap custom errors into uhttp errors using uhttp.WrapErrors (with status code), then the SDK is able to check status and decide how to proceed with the given error. This support is needed when returning errors from an HTTP request (no uhttp), SDK library, or custom errors inferred by the developer
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
44b5d37 to
f872b97
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
pkg/connector/usage_event_feed.go (1)
147-150: Preserve the gRPC status when listing usage activities failsWrapping
wrapGoogleApiError(err)withfmt.Errorfagain hides the helper’sGRPCStatus, so callers only seecodes.Unknown. Emit any context via logging, then return the helper error directly (e.g., log withzapandreturn wrapGoogleApiError(err)) so the original status propagates.pkg/connector/role.go (1)
57-60: Keep role-related Google API errors returning their gRPC statusAcross these role paths you’re still wrapping
wrapGoogleApiError(err)withfmt.Errorf, which hides the helper’sGRPCStatusimplementation and collapses everything tocodes.Unknown. Return the helper error directly (log context separately or extend the helper to format it) so callers receive the correct gRPC code and can react appropriately.Also applies to: 117-120, 185-188, 205-218, 227-230
pkg/connector/admin_event_feed.go (1)
68-69: Preserve the gRPC status returned bywrapGoogleApiError.Wrapping the helper’s result with
fmt.Errorfdrops the embeddedGRPCStatus, so callers now only seecodes.Unknowninstead of the Google→gRPC mapping. Return the helper’s error directly (after adding context before calling it) and update the otherfmt.Errorf(... wrapGoogleApiError(err))sites in this file accordingly.- return nil, nil, nil, fmt.Errorf("google-workspace: failed to list admin activities: %w", wrapGoogleApiError(err)) + return nil, nil, nil, wrapGoogleApiError(fmt.Errorf("google-workspace: failed to list admin activities: %w", err))
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(19 hunks)pkg/connector/connector.go(12 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(11 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/actions.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
📚 Learning: 2025-11-05T20:15:11.628Z
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
Applied to files:
pkg/connector/error_helpers.go
🧬 Code graph analysis (6)
pkg/connector/role.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryRolemanagementScope(165-165)
pkg/connector/connector.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/user.go (1)
vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
UserOrganization(5072-5114)
pkg/connector/admin_event_feed.go (1)
vendor/google.golang.org/api/admin/reports/v1/admin-gen.go (1)
Activities(223-248)
pkg/connector/group.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (3)
AdminDirectoryGroupMemberScope(144-144)Members(3259-3282)Member(3211-3252)
pkg/connector/error_helpers.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/ratelimit/http.go (1)
ExtractRateLimitData(66-130)
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
2 times, most recently
from
4ed160e to
10d8a0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/connector/admin_event_feed.go (3)
141-151: Preserve gRPC status: don’t re-wrap helper errors in handleGroupEventErrors from newGroupChangedEvent/newGroupMemberGrantEvent may already carry GRPCStatus. Wrapping with fmt.Errorf masks it as codes.Unknown. Return err directly here.
Apply:
- if err != nil { - return nil, fmt.Errorf("google-workspace: failed to create group changed event: %w", err) - } + if err != nil { + return nil, err + } ... - if err != nil { - return nil, fmt.Errorf("google-workspace: failed to create group changed event (group email): %w", err) - } + if err != nil { + return nil, err + } ... - if err != nil { - return nil, fmt.Errorf("google-workspace: failed to create group changed event (new value): %w", err) - } + if err != nil { + return nil, err + } ... - if err != nil { - return nil, fmt.Errorf("google-workspace: failed to create group member grant event: %w", err) - } + if err != nil { + return nil, err + } ... - if err != nil { - return nil, fmt.Errorf("google-workspace: failed to create group changed event (update member): %w", err) - } + if err != nil { + return nil, err + }Also applies to: 151-161, 161-169, 169-178, 178-187
199-206: Preserve gRPC status in handleUserEventSame masking via fmt.Errorf; return err directly so the mapped code propagates.
- if err != nil { - return nil, fmt.Errorf("google-workspace: failed to create user changed event: %w", err) - } + if err != nil { + return nil, err + }
265-283: Avoid masking helper errors in newGroupMemberGrantEvent / newUserChangedEventlookupGroup/lookupUser already wrap API errors with GRPCStatus; re-wrapping masks it. Return err directly. Keep fmt.Errorf for local resource-build errors.
- group, err := f.lookupGroup(ctx, groupEmail) + group, err := f.lookupGroup(ctx, groupEmail) if err != nil { - return nil, fmt.Errorf("google-workspace: failed to lookup group %s: %w", groupEmail, err) + return nil, err } ... - user, err := f.lookupUser(ctx, userEmail) + user, err := f.lookupUser(ctx, userEmail) if err != nil { - return nil, fmt.Errorf("google-workspace: failed to lookup user %s in newGroupMemberGrantEvent: %w", userEmail, err) + return nil, err }- user, err := f.lookupUser(ctx, userEmail) + user, err := f.lookupUser(ctx, userEmail) if err != nil { - return nil, fmt.Errorf("google-workspace: failed to lookup user %s in newUserChangedEvent: %w", userEmail, err) + return nil, err }Also applies to: 333-338
pkg/connector/group.go (1)
209-214: Build break: undefined identifier roleMemberEntitlementConstant defined here is groupMemberEntitlement; replace to fix compile and keep naming consistent with Entitlements/Grants.
- grant := sdkGrant.NewGrant(entitlement.Resource, roleMemberEntitlement, principal.GetId()) + grant := sdkGrant.NewGrant(entitlement.Resource, groupMemberEntitlement, principal.GetId())
♻️ Duplicate comments (2)
pkg/connector/actions.go (1)
24-32: Defend against nilstructpb.Valueentries before callingGetKind()These handlers still call
args.Fields["…"].GetKind()directly. If the key is absent (or explicitly set to nil), that expression dereferences a nil pointer and panics the action execution path. We previously flagged this; it’s still a release blocker. Please first retrieve the*structpb.Value, confirm it’s present and non-nil, and only then callGetKind(). Apply the same guard to each touched handler (resource_id,is_suspended,user_id,new_primary_email,target_resource_id, etc.).- guidField, ok := args.Fields["resource_id"].GetKind().(*structpb.Value_StringValue) - if !ok { + resourceVal, present := args.Fields["resource_id"] + if !present || resourceVal == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing resource ID") + } + guidField, ok := resourceVal.GetKind().(*structpb.Value_StringValue) + if !ok { return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing resource ID") } @@ - isSuspendedField, ok := args.Fields["is_suspended"].GetKind().(*structpb.Value_BoolValue) - if !ok { + suspendedVal, present := args.Fields["is_suspended"] + if !present || suspendedVal == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing is_suspended") + } + isSuspendedField, ok := suspendedVal.GetKind().(*structpb.Value_BoolValue) + if !ok { return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing is_suspended") }Replicate this pattern for the other argument lookups called out above.
Also applies to: 65-68, 108-111, 151-158, 209-216, 241-248
pkg/connector/admin_event_feed.go (1)
52-53: pToken nil guard was previously flagged (acknowledging prior thread)Dereferencing pToken.Size can panic if pToken is nil. Not blocking this PR per earlier scope decision. Tagging as duplicate for tracking.
🧹 Nitpick comments (2)
pkg/connector/error_helpers.go (1)
66-87: Preserve the original errors when returning the status
wrapGoogleApiErrorWithRateLimitInfoacceptserrs ...error, but the current implementation drops them. That strips out the originalgoogleapi.Error(and any additional context) from the chain, making troubleshooting anderrors.Is/Aschecks impossible once the helper returns. Please join the base status error with the extras so the full context survives.func wrapGoogleApiErrorWithRateLimitInfo(preferredCode codes.Code, contextMsg string, e *googleapi.Error, errs ...error) error { @@ - return st.Err() + baseErr := st.Err() + if len(errs) == 0 { + return baseErr + } + + combined := []error{baseErr} + for _, extra := range errs { + if extra != nil { + combined = append(combined, extra) + } + } + + return errors.Join(combined...) }pkg/connector/admin_event_feed.go (1)
358-399: Reduce lock scope to avoid holding mutex across network callslookupUser/lookupGroup hold mutex while calling Google APIs. Narrow the critical section: check cache under lock; on miss, release lock, fetch from API, then re-acquire to write. Improves concurrency and avoids head-of-line blocking.
Also applies to: 401-442
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(19 hunks)pkg/connector/connector.go(12 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(11 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
📚 Learning: 2025-10-16T18:24:02.512Z
Learnt from: agustin-conductor
Repo: ConductorOne/baton-google-workspace PR: 55
File: pkg/connector/actions.go:232-233
Timestamp: 2025-10-16T18:24:02.512Z
Learning: In Google Workspace Data Transfer API (admin/datatransfer/v1), the PRIVACY_LEVEL parameter values must be lowercase ("private", "shared"). Using uppercase values causes the API to ignore the parameter and transfer all privacy levels regardless of what was specified.
Applied to files:
pkg/connector/actions.go
📚 Learning: 2025-11-05T20:15:11.628Z
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
Applied to files:
pkg/connector/error_helpers.go
🧬 Code graph analysis (7)
pkg/connector/role.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
AdminDirectoryRolemanagementScope(165-165)
pkg/connector/user.go (1)
vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (1)
UserOrganization(5072-5114)
pkg/connector/actions.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/error_helpers.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/ratelimit/http.go (1)
ExtractRateLimitData(66-130)
pkg/connector/connector.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/group.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)vendor/google.golang.org/api/admin/directory/v1/admin-gen.go (3)
AdminDirectoryGroupMemberScope(144-144)Members(3259-3282)Member(3211-3252)
pkg/connector/admin_event_feed.go (1)
vendor/google.golang.org/api/admin/reports/v1/admin-gen.go (1)
Activities(223-248)
🔇 Additional comments (5)
pkg/connector/user.go (2)
300-316: Internal extraction errors: context is fineUsing fmt.Errorf for posix/external ID extraction is appropriate (non-HTTP). No changes requested.
80-84: No improper error wrapping found
Scan results show no occurrences offmt.ErrorfwrappingwrapGoogleApiErrororwrapGoogleApiErrorWithContext. Please manually verify that no masked error helpers remain elsewhere.pkg/connector/group.go (2)
68-71: Good: helper used for Google API errors; uhttp for precondition/argYou’re returning helper/uhttp errors directly without fmt.Errorf re-wrapping, preserving GRPCStatus. LGTM.
Also applies to: 137-141, 247-251, 216-223
119-123: Pagination token errors: plain errors are fine per guidanceUsing fmt.Errorf (not gRPC-wrapped) for Bag/NextToken errors aligns with “wrap only HTTP/SDK/custom permission errors.” No change requested. As per coding guidelines.
Also applies to: 154-159, 98-102, 40-45
pkg/connector/admin_event_feed.go (1)
66-69: No action needed: HTTP 429 correctly maps to codes.Unavailable
pkg/connector/actions.go
Outdated
| userService, err := c.getDirectoryService(ctx, directoryAdmin.AdminDirectoryUserScope) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return nil, nil, fmt.Errorf("google-workspace: failed to get directory service for updateUserStatus: %w", err) |
There was a problem hiding this comment.
nit: connector name in error (again, fine in logs, but imo not in errors)
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
10d8a0b to
4d3e538
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/connector/actions.go (2)
218-226: Validation uses trimmed values but API call uses untrimmed values.In
transferUserDriveFiles(line 236) andtransferUserCalendar(line 267), you validate the trimmedsrcanddstbut passsourceField.StringValueandtargetField.StringValuetodataTransferInsert. If the original values contain leading/trailing whitespace, the validation passes but the API receives untrimmed IDs.This was flagged as out of scope for this PR, but it's worth noting for a follow-up fix:
-return c.dataTransferInsert(ctx, appIdGoogleDocsAndGoogleDrive, sourceField.StringValue, targetField.StringValue, params) +return c.dataTransferInsert(ctx, appIdGoogleDocsAndGoogleDrive, src, dst, params)Also applies to: 250-258
23-32: Nil pointer dereference risk confirmed, but commit reference is incorrect.The concern is valid: when
args.Fields["key"]returns nil (missing key), calling.GetKind().(*structpb.Value_StringValue)will panic during type assertion. However, commit095b619does not exist in this repository's history—the last two commits are4d3e538and2ee36ee.The unsafe pattern exists in 10 locations across 6 functions (lines 24, 29, 65, 108, 151, 155, 209, 213, 241, 245), while the correct pattern is already used in
buildReleaseResourcesParam(line 339-340) andparseDrivePrivacyLevels(line 360-361), confirming inconsistent application.Apply the safe pattern to all
updateUserStatus,disableUserActionHandler,enableUserActionHandler,changeUserPrimaryEmail,transferUserDriveFiles, andtransferUserCalendarfunctions:-guidField, ok := args.Fields["resource_id"].GetKind().(*structpb.Value_StringValue) -if !ok { +v, present := args.Fields["resource_id"] +if !present || v == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "missing resource ID") +} +guidField, ok := v.GetKind().(*structpb.Value_StringValue) +if !ok { return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "missing resource ID") }
🧹 Nitpick comments (2)
pkg/connector/error_helpers.go (1)
52-64: Unreachable code path for 2xx status codes.If
e.Codeis in the 200-299 range, the condition on line 52 (e.Code < 200 || e.Code >= 300) is false, so control falls through to lines 56-63. However, Google API errors with 2xx status codes are extremely unlikely—googleapi.Erroris typically only returned for non-success responses. This dead code path could confuse future maintainers.Consider removing the unreachable fallthrough or adding a comment explaining this is defensive coding:
if e.Code < 200 || e.Code >= 300 { return wrapGoogleApiErrorWithRateLimitInfo(codes.Unknown, contextMsg, e, errors.Join(fmt.Errorf("unexpected status code: %d", e.Code), err)) } - contextErr := err - if contextMsg != "" { - contextErr = errors.Join(fmt.Errorf("%s", contextMsg), err) - } - return errors.Join( - fmt.Errorf("unexpected status code: %d", e.Code), - contextErr, - ) + // Defensive: googleapi.Error with 2xx should not occur, but handle gracefully + return wrapGoogleApiErrorWithRateLimitInfo(codes.OK, contextMsg, e, err)pkg/connector/usage_event_feed.go (1)
208-214: Consider returning nil instead of error for missing parameters.These errors indicate malformed event data from Google's API rather than a connector failure. Returning an error causes the entire event to be logged as an error (line 179), but the function already returns
nil, nilfor private apps (lines 219-222). Consider returningnil, nilhere too for consistency, or change the log level in the caller.
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(20 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(12 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/connector/user.go
- pkg/connector/connector.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T20:15:11.628Z
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
Applied to files:
pkg/connector/error_helpers.go
🔇 Additional comments (17)
pkg/connector/role.go (3)
39-84: LGTM!Error handling in the
Listfunction is well-structured:
- Pagination unmarshalling errors are wrapped with context
- Google API errors use
wrapGoogleApiErrorWithContextto preserve gRPC status- Internal resource creation errors use
fmt.Errorfappropriately
160-193: LGTM!The
Grantfunction properly uses:
uhttp.WrapErrorswithcodes.FailedPreconditionfor missing service scopeuhttp.WrapErrorswithcodes.InvalidArgumentfor invalid principal type and conversion errorswrapGoogleApiErrorWithContextfor Google API errors- Correct handling of HTTP 409 Conflict (existing assignment) returning nil
195-221: LGTM!The
Revokefunction correctly:
- Uses
uhttp.WrapErrorsfor precondition and validation errors- Handles HTTP 404 gracefully with logging (idempotent delete)
- Wraps other Google API errors with context
pkg/connector/usage_event_feed.go (1)
122-206: LGTM!The
ListEventsfunction demonstrates good error handling:
- Internal service/parsing errors use
fmt.Errorfwith context- Google API error at line 149 correctly uses
wrapGoogleApiErrorWithContext- Individual event creation failures are logged but don't fail the entire feed (lines 178-182)
- Cursor marshalling errors are properly wrapped
pkg/connector/actions.go (2)
270-334: LGTM!The
dataTransferInsertfunction has proper error handling:
- Service retrieval errors wrapped with
fmt.Errorf- Google API errors use
wrapGoogleApiErrorWithContext- Idempotency check for existing transfers is well-implemented
353-391: LGTM!The
parseDrivePrivacyLevelsfunction has thorough validation:
- Type checks for list and string values
- Validation against allowed values
- Deduplication of normalized values
- Clear error messages with
uhttp.WrapErrorspkg/connector/admin_event_feed.go (3)
42-135: LGTM!The
ListEventsfunction has well-structured error handling:
- Service and token errors use
fmt.Errorfwith descriptive context- Google API error at line 68 correctly uses
wrapGoogleApiErrorWithContext- Event handler errors are logged but don't fail the entire feed (lines 93-96, 100-103)
- Cursor marshalling errors are properly wrapped
358-399: LGTM!The
lookupUserfunction properly:
- Wraps service retrieval errors with context
- Handles HTTP 404 gracefully by returning nil (user no longer exists)
- Uses
wrapGoogleApiErrorWithContextfor other Google API errors- Maintains cache integrity by deleting entries for non-existent users
401-442: LGTM!The
lookupGroupfunction mirrorslookupUserwith consistent error handling patterns and proper cache management.pkg/connector/group.go (8)
40-45: LGTM - Pagination error handling is appropriate.Plain
fmt.Errorfwrapping (without gRPC codes) is correct here per guidance that pagination bag errors should alert as connector bugs rather than being suppressed with gRPC status codes.
68-71: LGTM - API error wrapping is correct.Using
wrapGoogleApiErrorWithContextfor the Google API call properly maps HTTP errors to gRPC codes while preserving context.
93-96: LGTM - SDK error handling is appropriate.Plain error wrapping for SDK resource creation failures is correct since these are not external API calls requiring gRPC status code mapping.
99-102: Consistent pagination error handling.
118-160: LGTM - Grants method error handling follows correct patterns.
- Pagination errors use plain
fmt.Errorf(lines 122, 157)- API call uses
wrapGoogleApiErrorWithContext(line 139)- SDK resource creation uses plain
fmt.Errorf(line 149)This correctly distinguishes external API errors from internal processing errors.
188-214: LGTM - Grant method error handling is well-structured.
FailedPreconditionfor missing service scope (line 190) andInvalidArgumentfor wrong principal type (line 193) are appropriate gRPC codes- Conflict handling (lines 200-208) correctly implements idempotency by fetching existing member
- Uses
groupMemberProvisioningServicefor the member lookup (line 202), which has the correct scope
216-242: LGTM - Revoke method maintains consistency with Grant.Error handling mirrors Grant method patterns appropriately:
- Same precondition checks with correct gRPC codes
- 404/NotFound case logs and returns nil for idempotent behavior
- Other API errors properly wrapped
244-270: LGTM - Get method error handling is correct.
- API retrieval error properly wrapped with resource ID context (line 250)
- SDK resource creation uses plain error wrapping (line 266)
- Edge case of group with no ID is logged and handled gracefully
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Has conflicts and this connector needs to be containerized. The ci.yaml is out of date.
| ) | ||
| } | ||
|
|
||
| func wrapGoogleApiErrorWithRateLimitInfo(preferredCode codes.Code, contextMsg string, e *googleapi.Error, errs ...error) error { |
There was a problem hiding this comment.
we should follow baton-sdk implementation for this function here, and use it from baton-sdk in case it's possible, otherwise, this should be refactored here
func WrapErrorsWithRateLimitInfo(preferredCode codes.Code, resp *http.Response, errs ...error) error {
st := status.New(preferredCode, resp.Status)
description, err := ratelimit.ExtractRateLimitData(resp.StatusCode, &resp.Header)
// Ignore any error extracting rate limit data
if err == nil {
st, _ = st.WithDetails(description)
}
if len(errs) == 0 {
return st.Err()
}
allErrs := append([]error{st.Err()}, errs...)
return errors.Join(allErrs...)
}
https://github.com/ConductorOne/baton-sdk/blob/main/pkg/uhttp/wrapper.go#L327-L342
| } | ||
| } | ||
| return nil, "", nil, fmt.Errorf("google-workspace: can't get role assignment: %w", err) | ||
| return nil, "", nil, wrapGoogleApiErrorWithContext(err, "failed to list role assignments") |
There was a problem hiding this comment.
we should still check by status not found here
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
4d3e538 to
c4adebc
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)
pkg/connector/actions.go (1)
285-309: Potential nil dereference when checkingtransfers.NextPageToken.If the API returns
nilfortransfers, the code at line 305 will panic when accessingtransfers.NextPageToken. The nil check at line 289 only guards the iteration loop, not the pagination check.- if transfers != nil { + if transfers == nil { + break + } + { for _, t := range transfers.DataTransfers { // ... existing logic ... } } if transfers.NextPageToken == "" { break }Or more simply:
+ if transfers == nil || transfers.NextPageToken == "" { - if transfers.NextPageToken == "" { break }
♻️ Duplicate comments (1)
pkg/connector/error_helpers.go (1)
66-88: Theerrsvariadic parameter is accepted but never used.The function signature accepts
errs ...errorbut the implementation discards them entirely, returning onlyst.Err(). This breaks the error chain—callers passing the original error won't be able to useerrors.Is/errors.Asfor introspection.Per the past review comment from luisina-santos, this should follow the baton-sdk pattern:
- return st.Err() + if len(errs) == 0 { + return st.Err() + } + allErrs := append([]error{st.Err()}, errs...) + return errors.Join(allErrs...)This preserves the gRPC status at the top level while maintaining the original error chain.
🧹 Nitpick comments (1)
pkg/connector/connector.go (1)
491-498: Consider usinguhttp.WrapErrorsfor invalid domain configuration.The domain validation error at line 497 indicates a configuration issue. Using
codes.InvalidArgumentwould provide better gRPC semantics:- return nil, fmt.Errorf("domain '%s' is not a valid domain for customer '%s'", c.domain, c.customerID) + return nil, uhttp.WrapErrors(codes.InvalidArgument, fmt.Sprintf("domain '%s' is not a valid domain for customer '%s'", c.domain, c.customerID))This aligns with similar validation errors elsewhere in the codebase.
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(20 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(12 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/usage_event_feed.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T20:15:11.628Z
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
Applied to files:
pkg/connector/error_helpers.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: connector-test
🔇 Additional comments (16)
pkg/connector/error_helpers.go (1)
40-41: HTTP 429 correctly maps tocodes.Unavailable.This aligns with the baton-sdk convention where HTTP 429 (Too Many Requests) is mapped to
codes.Unavailablerather thancodes.ResourceExhausted. Based on learnings from this repository.pkg/connector/role.go (2)
39-84: Error handling inListfollows correct patterns.
- Pagination errors use
fmt.Errorf(internal connector issues that should alert)- API errors use
wrapGoogleApiErrorWithContext(preserves gRPC codes)- SDK resource creation errors use
fmt.Errorf(internal issues)This aligns with the guidance that only HTTP request errors need gRPC wrapping.
160-193: Grant method error handling is well-structured.The implementation correctly:
- Uses
uhttp.WrapErrorswith appropriate codes for validation/precondition failures- Handles HTTP 409 (conflict) specially for idempotency
- Falls back to
wrapGoogleApiErrorWithContextfor other API errorspkg/connector/connector.go (1)
298-336: Service initialization error handling is well-structured.The function appropriately maps errors to gRPC codes:
codes.Internalfor HTTP client creation failurescodes.InvalidArgumentfor credential parsing issuescodes.Unavailablefor service creation failuresThis provides meaningful error semantics to callers.
pkg/connector/group.go (2)
40-109: Error handling inListcorrectly follows project guidelines.Per btipling's guidance, pagination bag errors are left as plain errors (should alert as connector bugs), while API errors use
wrapGoogleApiErrorWithContext. This is the correct pattern.
193-219: Grant method correctly handles conflict case with proper service.The implementation uses
groupMemberProvisioningServicefor both insert and conflict-recovery get operations, addressing the previous review concern about scope mismatches.pkg/connector/actions.go (2)
23-61: Error handling inupdateUserStatusfollows correct patterns.
- Validation errors use
uhttp.WrapErrors(codes.InvalidArgument, ...)- Service retrieval uses
fmt.Errorf(internal error)- API errors use
wrapGoogleApiErrorWithContextThis is consistent with the PR's error wrapping approach.
353-391: Privacy levels validation is thorough and well-structured.The function handles:
- Missing argument (returns defaults)
- Type validation (must be list)
- Value validation (only "private" and "shared" allowed)
- Deduplication
- Empty list after filtering
All validation errors use
codes.InvalidArgumentappropriately.pkg/connector/admin_event_feed.go (5)
17-18: LGTM on imports.The added
ctxzapandzapimports support the structured logging changes throughout this file.
48-68: Error wrapping patterns look correct.The implementation correctly differentiates between internal errors (using
fmt.Errorffor service initialization, token handling) and Google API errors (usingwrapGoogleApiErrorWithContextat line 68). This preserves gRPC status codes for API failures while still providing context for internal errors.
265-268: Note: Double-wrapping may mask gRPC status, but impact is limited.
lookupGroupreturns a gRPC-wrapped error fromwrapGoogleApiErrorWithContextat line 434. Wrapping it again withfmt.Errorfhere could strip theGRPCStatus. However, since this error is ultimately only logged at line 94 (and not propagated to gRPC clients), the impact is minimal for this use case.
378-391: Error handling in lookupUser is well-structured.Good pattern: internal errors (directory service initialization) use
fmt.Errorf, while Google API errors usewrapGoogleApiErrorWithContext. The 404 handling correctly avoids wrapping the not-found case as an error.
419-434: Consistent error handling in lookupGroup.Mirrors the
lookupUserpattern appropriately.pkg/connector/user.go (3)
54-107: LGTM on List method error handling.Error wrapping is consistent:
wrapGoogleApiErrorWithContextfor the Google API call (line 85),fmt.Errorffor internal operations (pagination, resource building). This correctly preserves gRPC status codes for API errors.
227-269: LGTM on Get method error handling.Correctly uses
wrapGoogleApiErrorWithContextfor the API call at line 234 andfmt.Errorffor internal operations.
304-319: LGTM on extraction error handling.Appropriate use of
fmt.Errorffor internal JSON extraction errors.
| err := o.userProvisioningService.Users.Delete(resourceId.Resource).Context(ctx).Do() | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to get user in Delete: %s", resourceId.Resource)) | ||
| } | ||
|
|
||
| _, err = o.userService.Users.Update(resourceId.Resource, &admin.User{ | ||
| Suspended: true, | ||
| }).Context(ctx).Do() | ||
|
|
||
| if err != nil { | ||
| return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to suspend user in Delete: %s", resourceId.Resource)) | ||
| } |
There was a problem hiding this comment.
Incorrect error message and suspicious delete-then-suspend logic.
Two issues in the Delete method:
-
Wrong error message (line 488): The message says "failed to get user" but the operation is
Delete(). Should say "failed to delete user". -
Suspicious logic (lines 491-496): After successfully deleting a user, the code attempts to suspend them. Suspending a deleted user will likely fail or is a no-op. Was the intended order suspend-then-delete, or should the suspend call be removed entirely?
Apply this diff to fix the error message (the logic question requires clarification):
err := o.userProvisioningService.Users.Delete(resourceId.Resource).Context(ctx).Do()
if err != nil {
- return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to get user in Delete: %s", resourceId.Resource))
+ return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to delete user: %s", resourceId.Resource))
}Please clarify the intended behavior: should the user be suspended before deletion, or is the suspend call unnecessary?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := o.userProvisioningService.Users.Delete(resourceId.Resource).Context(ctx).Do() | |
| if err != nil { | |
| return nil, err | |
| return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to get user in Delete: %s", resourceId.Resource)) | |
| } | |
| _, err = o.userService.Users.Update(resourceId.Resource, &admin.User{ | |
| Suspended: true, | |
| }).Context(ctx).Do() | |
| if err != nil { | |
| return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to suspend user in Delete: %s", resourceId.Resource)) | |
| } | |
| err := o.userProvisioningService.Users.Delete(resourceId.Resource).Context(ctx).Do() | |
| if err != nil { | |
| return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to delete user: %s", resourceId.Resource)) | |
| } | |
| _, err = o.userService.Users.Update(resourceId.Resource, &admin.User{ | |
| Suspended: true, | |
| }).Context(ctx).Do() | |
| if err != nil { | |
| return nil, wrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to suspend user in Delete: %s", resourceId.Resource)) | |
| } |
🤖 Prompt for AI Agents
In pkg/connector/user.go around lines 486 to 497, the Delete method has an
incorrect error message and questionable suspend-after-delete logic: change the
error returned when the Delete() call fails to read "failed to delete user"
(including the resourceId) instead of "failed to get user"; then decide and
apply one of two fixes for the suspend call — either (A) if the intent is to
suspend before removing, move the Users.Update(...) suspend call to run before
Users.Delete(...) and keep its error handling (adjust messages to "failed to
suspend user in Delete"), or (B) if suspension is unnecessary prior to deletion,
remove the suspend call entirely; update corresponding error messages to match
the actual operations and ensure ctx is used consistently; confirm which
behavior (suspend-then-delete or no suspend) is intended.
c4adebc to
17f4658
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 (2)
pkg/connector/connector.go (1)
338-347: Return the original error fromnewGWSAdminServiceForScopeswithout wrapping
newGWSAdminServiceForScopesreturns status-bearing errors (viauhttp.WrapErrors). Wrapping withfmt.Errorf("failed to create report service: %w", err)masks the gRPC status code, causing callers to receivecodes.Unknowninstead of the intended status. ThegetServicefunction already follows the correct pattern of returning such errors directly.Change line 344 from:
return nil, fmt.Errorf("failed to create report service: %w", err)to:
return nil, errThis preserves the status code information while still propagating the error message from
newGWSAdminServiceForScopes.pkg/connector/group.go (1)
193-219: Fix entitlement constant inGrantmethod (line 216)The grant is constructed with
roleMemberEntitlement, but should usegroupMemberEntitlementto match the entitlement defined at line 24, and to align with the entitlements used inGroupMembers(line 116) andRevoke(line 156). ReplaceroleMemberEntitlementwithgroupMemberEntitlementon line 216.
♻️ Duplicate comments (1)
pkg/connector/actions.go (1)
304-309: Previous nil-check concern remains.The check at line 289 (
if transfers != nil) protects the iteration, but line 305 accessestransfers.NextPageTokenoutside that guard. This was flagged in a previous review and marked out of scope for this PR.
🧹 Nitpick comments (3)
pkg/connector/usage_event_feed.go (1)
208-214: Consider usinguhttp.WrapErrorsfor parameter validation.For consistency with the pattern in
actions.go, these validation errors could useuhttp.WrapErrors(codes.InvalidArgument, ...)to provide structured gRPC codes. However, since these errors are logged and don't halt the feed (line 179-181), the current approach is acceptable.pkg/connector/error_helpers.go (1)
56-64: Defensive handling for unexpected 2xx codes.This fallback handles the edge case where a
googleapi.Errorhas a 2xx status code (which shouldn't normally occur). The "unexpected status code" message clearly signals this abnormal condition. Consider logging this case for debugging.+// This path should rarely be hit - a googleapi.Error with a 2xx code is unusual contextErr := err if contextMsg != "" { contextErr = errors.Join(fmt.Errorf("%s", contextMsg), err) }pkg/connector/user.go (1)
396-479:CreateAccount: consider mapping obvious input/precondition errors to specific gRPC codesThe structure here is good overall:
- The Google API
Users.Insertfailure is wrapped withwrapGoogleApiErrorWithContext(err, "failed to create account").userResourceconstruction failures arefmt.Errorf("failed to create user resource: %w", err), correctly treated as connector bugs.A few of the earlier failures are clearly client/configuration errors but are currently plain
fmt.Errorf:
- Missing
given_name, orfamily_nameinaccountInfo.Profile.credentialOptions == nil.o.userProvisioningService == nil(missing required scope).If you want the SDK to distinguish these from internal errors and treat them as non-alerting client/config problems, you might consider using
uhttp.WrapErrorswith more specific codes:@@ email, ok := pMap["email"].(string) if !ok || email == "" { - return nil, nil, nil, fmt.Errorf("email not found in profile") + return nil, nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "email not found in profile", nil) } @@ givenName, ok := pMap["given_name"].(string) if !ok || givenName == "" { - return nil, nil, nil, fmt.Errorf("given_name not found in profile") + return nil, nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "given_name not found in profile", nil) } @@ familyName, ok := pMap["family_name"].(string) if !ok || familyName == "" { - return nil, nil, nil, fmt.Errorf("family_name not found in profile") + return nil, nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "family_name not found in profile", nil) } @@ if credentialOptions == nil { - return nil, nil, nil, fmt.Errorf("credentialOptions cannot be nil") + return nil, nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "credentialOptions cannot be nil", nil) } @@ if o.userProvisioningService == nil { - return nil, nil, nil, fmt.Errorf("user provisioning service not available - requires %s scope", admin.AdminDirectoryUserScope) + return nil, nil, nil, uhttp.WrapErrors( + codes.FailedPrecondition, + fmt.Sprintf("user provisioning service not available - requires %s scope", admin.AdminDirectoryUserScope), + nil, + ) }This would align CreateAccount’s behavior with the patterns you’ve already established in
group.Grant/Revokefor similar precondition and argument issues.
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(20 hunks)pkg/connector/connector.go(13 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(12 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T20:15:11.628Z
Learnt from: btipling
Repo: ConductorOne/baton-google-workspace PR: 56
File: pkg/connector/error_helpers.go:40-41
Timestamp: 2025-11-05T20:15:11.628Z
Learning: In the baton-google-workspace repository and baton-sdk, HTTP 429 (Too Many Requests) should be mapped to codes.Unavailable, not codes.ResourceExhausted, as per the project's established error handling conventions.
Applied to files:
pkg/connector/error_helpers.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: connector-test
🔇 Additional comments (23)
pkg/connector/usage_event_feed.go (1)
147-155: LGTM on Google API error wrapping.The
wrapGoogleApiErrorWithContextis correctly used here without additionalfmt.Errorfwrapping, preserving the gRPC status code as intended by this PR.pkg/connector/admin_event_feed.go (2)
66-69: LGTM on Google API error wrapping.The
wrapGoogleApiErrorWithContextis correctly used for the admin activities API call, preserving the gRPC status code.
381-392: Appropriate handling of not-found vs other errors.The pattern correctly distinguishes between expected 404s (user/group deleted) and actual API errors, using
wrapGoogleApiErrorWithContextonly for unexpected failures.pkg/connector/actions.go (2)
23-32: Consistent use ofuhttp.WrapErrorsfor input validation.The structured error wrapping with
codes.InvalidArgumentprovides proper gRPC semantics for validation failures.Note: The defensive nil check for
args.Fields["resource_id"]before callingGetKind()was discussed in a previous review and marked as out of scope for this PR.
47-50: LGTM on Google API error wrapping.The
wrapGoogleApiErrorWithContextcorrectly wraps the API error with operation-specific context.pkg/connector/error_helpers.go (3)
14-21: Well-designed error helper with proper gRPC status preservation.The helper correctly:
- Uses
errors.Asto safely extractgoogleapi.Error- Returns original error unchanged for non-Google API errors
- Maps HTTP codes to appropriate gRPC codes
40-41: 429 mapping follows project conventions.The mapping of HTTP 429 to
codes.Unavailablealigns with the baton-sdk's established error handling pattern. Based on learnings from this PR.
82-93: Correctly follows baton-sdk pattern for rate limit handling.The implementation properly:
- Guards against nil
descriptionbefore callingWithDetails- Uses
errors.Jointo preserve the original error chain (addressing the past review concern)- Returns
st.Err()directly when no additional errors are providedpkg/connector/role.go (3)
117-127: Appropriate handling of NotFound in role assignments.The code correctly returns an empty list for 404 (role not found) and wraps other errors with
wrapGoogleApiErrorWithContext. This aligns with the expected behavior for listing grants on potentially deleted roles.
167-178: Good validation pattern with proper gRPC codes.The use of
uhttp.WrapErrorswithcodes.FailedPreconditionfor service availability andcodes.InvalidArgumentfor principal validation provides clear, actionable error semantics.
234-237: LGTM on role retrieval error wrapping.The
wrapGoogleApiErrorWithContextcorrectly wraps the API error with the role ID context for debugging.pkg/connector/connector.go (4)
298-336: Good use ofuhttp.WrapErrorswith explicit gRPC codes innewGWSAdminServiceForScopesThe three failure branches here (HTTP client creation, JWT config parsing, service construction) are now consistently wrapped with
uhttp.WrapErrorsand mapped tocodes.Internal,codes.InvalidArgument, andcodes.Unavailable, which matches the intent of surfacing HTTP/SDK failures to the SDK with machine-checkable status while still carrying the underlying error.No changes needed from my side.
552-562: Cache type-mismatch error messaging looks goodThe new error returned when a cached entry exists but has the wrong type:
return nil, fmt.Errorf("cache entry for scope %s exists, but is not of type %s", scope, reflect.TypeOf(service))is appropriate as a plain error (not uhttp-wrapped) since it indicates an internal connector bug. It should surface loudly and not be down-classified as a client or transient issue. No changes needed.
568-612: Service cache error wrapping is appropriateThe new wrappers in
getService:
fmt.Errorf("failed to get service from cache: %w", err)fmt.Errorf("failed to get upgraded service from cache: %w", err)only wrap the internal cache-type errors produced by
getFromCache. These are connector bugs rather than HTTP/SDK failures, so it’s correct to keep them as plain errors (no gRPC code mapping) and let them alert.The recursive upgrade path (
getService→upgradeScope→getServicewith upgraded scope) still terminates safely and doesn’t introduce new concurrency issues beyond what already existed.
632-665: Action registration error context improvementsWrapping
RegisterActionfailures with contextual messages like:fmt.Errorf("failed to register update_user_status action: %w", err)for each action manager registration point makes startup failures much easier to trace, while the original error is preserved via
%w. Since these are not HTTP/SDK errors surfaced to gRPC clients but rather initialization failures, treating them as plain errors is consistent with the rest of the file.Looks good.
pkg/connector/group.go (4)
40-109:List: clean separation between connector bugs and Google API failures
- Pagination token unmarshalling and next-token generation now return plain
fmt.Errorferrors with clear context, which is appropriate since those indicate connector bugs.- The Google API call error is returned via
wrapGoogleApiErrorWithContext(err, "failed to list groups")without an outerfmt.Errorfwrapper, so the status-bearing helper remains the top-level error and gRPC sees the intended code.- Logging for groups with missing IDs is kept at log level (no error returned), which avoids spurious sync failures on bad items.
This matches the error-handling guidelines you’ve been applying elsewhere.
123-166:Grants: consistent error classification and Google API wrapping
- Pagination token unmarshal and next-page token errors now use descriptive
fmt.Errorfmessages, correctly treated as connector issues.- Member listing failures are wrapped with
wrapGoogleApiErrorWithContext(err, "failed to list group members"), keeping the helper’s gRPC status outermost.- Failures to build user resource IDs (
NewResourceID) are surfaced as plain internal errors, which is appropriate.Overall, this function now cleanly distinguishes between Google API failures and internal pagination/ID-building bugs.
221-247:Revoke: appropriate precondition handling and Google API wrapping
- Missing provisioning service and wrong principal type now return
uhttp.WrapErrorswithcodes.FailedPreconditionandcodes.InvalidArgument, respectively, which is consistent withGrant.- For delete failures, non-404 Google API errors are surfaced via
wrapGoogleApiErrorWithContext(err, "failed to delete group member"), keeping the Google API-aware status intact.- The 404 path still logs and returns success to maintain idempotent semantics on double-revokes.
This is a solid improvement in both correctness and observability.
249-279:Get: preserves Google API status while isolating connector bugs
- Failures from
o.groupService.Groups.Get(...).Do()are now wrapped withwrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to retrieve group: %s", resourceId.Resource)), so callers see a status-aware error.- A group with an empty ID is logged and treated as non-existent, which avoids returning malformed resources.
NewGroupResourcefailures are surfaced via plainfmt.Errorf("failed to create group resource in Get: %w", err), correctly indicating an internal connector issue rather than an API problem.No changes needed here.
pkg/connector/user.go (4)
54-108:List: correct split between connector bugs and Google API failures
- Pagination token unmarshal and next-page token failures now use
fmt.Errorfwith clear context, correctly treating them as connector bugs.- The user list API call failure is returned via
wrapGoogleApiErrorWithContext(err, "failed to list users")without additional wrapping, preserving the helper’s gRPC status.- Logging for users with missing IDs is kept as a log-and-skip behavior instead of failing the whole page.
This matches the intended error-handling pattern.
227-272:Get: status-preserving API errors and reasonable filter behavior
- Failures from
Users.Get(...).Do()now go throughwrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to retrieve user: %s", resourceId.Resource)), so gRPC-aware status codes are preserved.- Domain and customerId mismatches are handled by logging an info message and returning
(nil, nil, nil), which is a reasonable way to treat out-of-scope users.- JSON-shape issues when extracting organizations and user resource build failures are surfaced via
fmt.Errorf, correctly indicating internal connector issues rather than API problems.Looks solid.
274-383:userResource: internal extraction failures correctly treated as connector bugsFailures when extracting POSIX accounts or external IDs now return plain
fmt.Errorfwith clear messages:
"failed to extract posix accounts: %w""failed to extract external IDs: %w"Given these come from local JSON marshal/unmarshal of
user.PosixAccountsanduser.ExternalIds, treating them as hard connector errors (rather than downgrading them via uhttp) is appropriate—they indicate a schema/assumption mismatch, not a user or transient API issue.No changes needed.
481-492:Delete: improved error semantics and status preservation
- The missing provisioning service case now returns a clear
fmt.Errorf("user provisioning service not available - requires %s scope", ...), correctly marking it as a configuration/precondition failure.- The Google API
Users.Deletefailure is routed throughwrapGoogleApiErrorWithContext(err, fmt.Sprintf("failed to delete user: %s", resourceId.Resource)), with a corrected and specific message, so gRPC sees the appropriate status code for API errors.This is a clear improvement over the prior “failed to get user” wording and non-status-aware error.
| func (c *GoogleWorkspace) Metadata(ctx context.Context) (*v2.ConnectorMetadata, error) { | ||
| _, err := c.Validate(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to validate connector: %w", err) | ||
| } | ||
| primaryDomain, err := c.getPrimaryDomain(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to get primary domain: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the connector.go file
fd -t f "connector.go" pkg/connector/
# Get line count to determine readability
wc -l pkg/connector/connector.goRepository: ConductorOne/baton-google-workspace
Length of output: 136
🏁 Script executed:
# Read the specific line ranges mentioned in the review
echo "=== Lines 369-377 (Metadata function) ==="
sed -n '369,377p' pkg/connector/connector.go
echo -e "\n=== Lines 433-445 ==="
sed -n '433,445p' pkg/connector/connector.go
echo -e "\n=== Lines 447-457 ==="
sed -n '447,457p' pkg/connector/connector.go
echo -e "\n=== Lines 469-483 ==="
sed -n '469,483p' pkg/connector/connector.go
echo -e "\n=== Lines 485-501 ==="
sed -n '485,501p' pkg/connector/connector.go
# Also look for context around these functions
echo -e "\n=== Searching for function definitions ==="
rg -n "func \(c \*GoogleWorkspace\)" pkg/connector/connector.go | head -20Repository: ConductorOne/baton-google-workspace
Length of output: 3403
🏁 Script executed:
# Search for wrapGoogleApiErrorWithContext definition
echo "=== Searching for wrapGoogleApiErrorWithContext ==="
rg -n "wrapGoogleApiErrorWithContext|func wrapGoogleApiError" pkg/connector/
# Search for getDirectoryService implementation
echo -e "\n=== getDirectoryService implementation ==="
sed -n '350,365p' pkg/connector/connector.go
# Search for newGWSAdminServiceForScopes
echo -e "\n=== Searching for newGWSAdminServiceForScopes ==="
rg -n "newGWSAdminServiceForScopes" pkg/connector/
# Search for uhttp usage
echo -e "\n=== Searching for uhttp package ==="
rg -n "uhttp" pkg/connector/Repository: ConductorOne/baton-google-workspace
Length of output: 10526
🏁 Script executed:
# Read error_helpers.go to see wrapGoogleApiErrorWithContext
echo "=== error_helpers.go content ==="
cat -n pkg/connector/error_helpers.go
# Read newGWSAdminServiceForScopes implementation
echo -e "\n=== newGWSAdminServiceForScopes (lines 298-335) ==="
sed -n '298,335p' pkg/connector/connector.goRepository: ConductorOne/baton-google-workspace
Length of output: 5583
🏁 Script executed:
# Search for how errors are handled in gRPC context
echo "=== Searching for error handling patterns ==="
rg -n "status.FromError|GRPCStatus" pkg/
# Look for similar error wrapping patterns in other connectors or SDK
echo -e "\n=== Searching for error chain unwrapping ==="
rg -n "errors.Unwrap|errors.As.*Code|errors.Is" pkg/connector/ | head -20
# Check if there's a test file that shows the error wrapping behavior
echo -e "\n=== Looking for test files ==="
fd -t f "*_test.go" pkg/connector/Repository: ConductorOne/baton-google-workspace
Length of output: 600
🏁 Script executed:
# Let's understand the error flow better by looking at how errors are returned
echo "=== Checking fetchDomains full implementation ==="
sed -n '447,468p' pkg/connector/connector.go
# Check if there are any comments or documentation about error handling
echo -e "\n=== Searching for comments about error wrapping ==="
rg -n "fmt.Errorf|error.*wrap" pkg/connector/connector.go | head -30Repository: ConductorOne/baton-google-workspace
Length of output: 2345
🌐 Web query:
gRPC Go status.FromError fmt.Errorf wrapping error chain preservation
💡 Result:
Short answer: status.FromError does not break Go's %w wrapping — use fmt.Errorf("%w", err) to preserve the error chain (so errors.Is / errors.As work). However, FromError treats wrapped gRPC status-bearing errors specially: if any error in the chain implements GRPCStatus() *status.Status it returns that Status and the message it reports will include the full err.Error() (i.e., the whole wrapped message chain), not only the inner Status message. [1][2]
Example (preserves chain):
- wrap: wrapped := fmt.Errorf("handler: %w", grpcErr)
- convert: st, ok := status.FromError(wrapped) // ok true if grpcErr (or a wrapped error) had GRPCStatus()
- errors.Is(wrapped, grpcErr) and errors.As(wrapped, &SomeType) still work because %w preserves Unwrap(). [2][1]
References
- google.golang.org/grpc/status package docs (FromError behavior). [1]
- Go 1.13 errors blog (fmt.Errorf %w and errors.Is/As / Unwrap). [2]
🏁 Script executed:
# Search for how errors are handled at the gRPC boundary
echo "=== Looking for gRPC error handling ==="
rg -n "status\.|codes\." pkg/connector/ | head -20
# Check if there are integration tests
echo -e "\n=== Looking for test or integration files ==="
fd -type f -name "*.go" | grep -i test | head -10
# Check the actual error flow - look for where errors are returned to gRPC
echo -e "\n=== Checking ResourceSyncers and other entry points ==="
sed -n '507,530p' pkg/connector/connector.goRepository: ConductorOne/baton-google-workspace
Length of output: 4189
🏁 Script executed:
# Check the full context around the error wrapping pattern
echo "=== Lines 14-20 in error_helpers.go with context ==="
sed -n '14,20p' pkg/connector/error_helpers.go
# Check lines 74-78 where the comment about "ONLY place" appears
echo -e "\n=== Lines 74-78 (critical comment) ==="
sed -n '74,78p' pkg/connector/error_helpers.go
# Search for any README or documentation
echo -e "\n=== Looking for README or docs ==="
fd -type f \( -name "README*" -o -name "*.md" \) | head -10
# Check git history to understand intent
echo -e "\n=== Recent commits mentioning error or status ==="
git log --oneline --all -20 2>/dev/null || echo "Git history unavailable"Repository: ConductorOne/baton-google-workspace
Length of output: 2091
🏁 Script executed:
# Verify the comment about "ONLY place" context
echo "=== Full error_helpers.go relevant sections ==="
sed -n '66,94p' pkg/connector/error_helpers.go
# Check if there are other connectors in baton-sdk that follow this pattern
echo -e "\n=== Searching for similar patterns in other files ==="
rg -n "This is the ONLY place" pkg/
# Look at how uhttp.WrapErrors is designed
echo -e "\n=== Checking uhttp usage pattern ==="
rg -B2 "uhttp.WrapErrors" pkg/connector/connector.go | head -20Repository: ConductorOne/baton-google-workspace
Length of output: 1818
Remove unnecessary fmt.Errorf wrapping layers that bypass designed error context points
The codebase explicitly documents in error_helpers.go line 75: "This is the ONLY place we should add context, to ensure the gRPC status is preserved." However, several functions in Metadata, getPrimaryDomain, getDomains, fetchDomains, and Validate re-wrap errors from wrapGoogleApiErrorWithContext and uhttp.WrapErrors (which already carry gRPC status codes) with additional fmt.Errorf layers. This violates the established pattern and creates redundant error message nesting.
Since wrapGoogleApiErrorWithContext and uhttp.WrapErrors are designed as the single context-adding points where gRPC status is properly preserved, subsequent layers should propagate errors directly:
Metadata(lines 372, 376): Removefmt.Errorfwrapping; return errors directly fromValidateandgetPrimaryDomaingetPrimaryDomain(line 442): Return error directly fromfetchDomainsfetchDomains(line 450): Return error directly fromgetDirectoryServicegetDomains(line 479): Return error directly fromfetchDomainsValidate(line 488): Return error directly fromgetDomainsValidate(line 497): The domain validation error may benefit fromuhttp.WrapErrors(codes.InvalidArgument, ...)for consistency
Applies to: 369-377, 433-445, 447-457, 469-483, 485-501
🤖 Prompt for AI Agents
In pkg/connector/connector.go around lines 369-377, 433-445, 447-457, 469-483,
and 485-501: the code wraps errors returned from wrapGoogleApiErrorWithContext
and uhttp.WrapErrors with additional fmt.Errorf layers which breaks the
single-point-of-context rule in error_helpers.go; remove the fmt.Errorf wrapping
and return errors directly from Validate and getPrimaryDomain (lines ~372,
~376), from getPrimaryDomain returning fetchDomains (line ~442), from
fetchDomains returning getDirectoryService (line ~450), from getDomains
returning fetchDomains (lines ~479), and from Validate returning getDomains
(lines ~488); keep the single context-addition points only
(wrapGoogleApiErrorWithContext / uhttp.WrapErrors) and, where domain validation
is performed (around line ~497), replace the current error creation with
uhttp.WrapErrors(codes.InvalidArgument, ...) to preserve consistent gRPC status
semantics.
| gerr := &googleapi.Error{} | ||
| if errors.As(err, &gerr) { | ||
| // Return no grants if the group no longer exists. This might happen if the group is deleted during a sync. | ||
| if gerr.Code == http.StatusNotFound { |
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
17f4658 to
15b397a
Compare
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-errors-with-grpc
branch
from
15b397a to
a2491e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/connector/actions.go (1)
23-32: Defensively guardargs.Fields[...]before callingGetKind()Across these handlers you still call
args.Fields["key"].GetKind()directly; if the key is absent or the value is nil, this will panic instead of returning a cleanInvalidArgumenterror. This is the same issue previously noted and is worth fixing here.A safe pattern (example for
"resource_id") would be:- guidField, ok := args.Fields["resource_id"].GetKind().(*structpb.Value_StringValue) - if !ok { - return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "missing resource ID") - } + v, present := args.Fields["resource_id"] + if !present || v == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "missing resource ID") + } + guidField, ok := v.GetKind().(*structpb.Value_StringValue) + if !ok { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "missing resource ID") + }and similarly for
is_suspended,user_id,new_primary_email,target_resource_id.Also applies to: 63-68, 107-111, 151-158, 209-216, 241-248
🧹 Nitpick comments (2)
pkg/connector/connector.go (1)
447-501: Consider surfacing invalid configured domain as a typed InvalidArgument
fetchDomainscorrectly propagates directory‑service errors and wraps the Google APIListfailure viawrapGoogleApiErrorWithContext, andgetDomains/caching logic look fine. For theValidatefailure case:return nil, fmt.Errorf("domain '%s' is not a valid domain for customer '%s'", c.domain, c.customerID)you might consider using
uhttp.WrapErrors(codes.InvalidArgument, ...)so misconfigured domains are distinguishable from transient/internal failures at the gRPC layer, similar to how other configuration/permission issues are surfaced elsewhere in this PR. The new cache/type‑mismatch and action‑registration error messages viafmt.Errorflook good as plain connector errors.Also applies to: 552-584, 632-662
pkg/connector/user.go (1)
396-479: Optional: surface CreateAccount profile validation issues as InvalidArgumentIn
CreateAccount, missing or emptygiven_name, andfamily_namecurrently return plainfmt.Errorf(...)errors, which will likely surface as generic gRPC failures rather than structuredInvalidArgument. Given these are client/profile issues rather than connector bugs, you may want to switch them touhttp.WrapErrors(codes.InvalidArgument, "...")(and similarly for the “credentialOptions cannot be nil” case if that can be triggered by callers) to match how other validation errors are handled inactions.go.
📜 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 (8)
pkg/connector/actions.go(16 hunks)pkg/connector/admin_event_feed.go(20 hunks)pkg/connector/connector.go(10 hunks)pkg/connector/error_helpers.go(1 hunks)pkg/connector/group.go(12 hunks)pkg/connector/role.go(13 hunks)pkg/connector/usage_event_feed.go(9 hunks)pkg/connector/user.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/error_helpers.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: connector-test
🔇 Additional comments (7)
pkg/connector/usage_event_feed.go (1)
7-7: Error wrapping and context in usage event feed look consistent and safeThe added
fmt.Errorfcontexts for token (un)marshalling, latest-event parsing, user trait creation, and cursor marshaling are appropriate for internal bugs, while the Google APIDo()path correctly useswrapGoogleApiErrorWithContextso external call failures carry the expected status and context;newV2Event’s parameter checks also fail per‑event rather than taking down the whole feed. No issues spotted here.Also applies to: 80-115, 122-200, 208-214
pkg/connector/admin_event_feed.go (1)
17-23: Admin event feed error handling is consistent with the new helpersListEvents now cleanly distinguishes between internal issues (token parsing, cursor marshaling, time parsing) using contextual
fmt.Errorf, and Google API failures usingwrapGoogleApiErrorWithContext; lookup helpers correctly special‑case 404s while routing other API errors through the helper. Group/user event constructors add context with plainfmt.Errorfwhile callers log and continue, so a single bad event doesn’t break the feed. This all looks aligned with the desired error‑wrapping strategy.Also applies to: 42-135, 137-214, 251-328, 366-450
pkg/connector/actions.go (1)
3-15: uhttp / Google API error wrapping in actions is well‑structuredArgument validation errors for IDs, booleans, and privacy levels now use
uhttp.WrapErrorswithcodes.InvalidArgument, and Data Transfer API failures are routed throughwrapGoogleApiErrorWithContext, while non‑HTTP failures (password generation, internal building) stay as plainfmt.Errorf. That separation matches the guidance on when to attach gRPC codes vs. leaving connector bugs as generic errors.Also applies to: 149-205, 207-268, 270-276, 336-391
pkg/connector/connector.go (1)
298-336: Service construction errors are mapped to gRPC codes in a sensible way
newGWSAdminServiceForScopesnow classifies failures asInternal(HTTP client),InvalidArgument(JWT config/credentials), andUnavailable(service construction), which is a good fit for how these failures surface.getReportServiceadds user‑facing context withfmt.Errorfwhile still preserving the underlying status fromuhttp.WrapErrors. No issues here.Also applies to: 338-348
pkg/connector/role.go (1)
39-45: Role CRUD error handling and gRPC mapping look solidRole listing and grants now treat pagination and resource‑construction failures as plain connector errors, while Google API failures are wrapped with
wrapGoogleApiErrorWithContext;Grants,Grant, andRevokecorrectly special‑case 404/409 conditions and useuhttp.WrapErrorsfor precondition and argument issues. The Get path is similarly consistent. This matches the intended error‑wrapping and idempotency behavior.Also applies to: 57-82, 98-149, 167-200, 202-225, 230-251
pkg/connector/group.go (1)
40-45: Group listing and membership error paths align with the new strategyGroup List/Grants now leave pagination and construction problems as plain
fmt.Errorfconnector bugs, while group/membership Google API failures consistently go throughwrapGoogleApiErrorWithContextand, where appropriate,uhttp.WrapErrors(e.g., NotFound, FailedPrecondition, InvalidArgument). The conflict‑retry path inGrantuses the member‑scoped service and cleanly distinguishes “already a member” from real failures. Looks good.Also applies to: 68-71, 83-107, 123-169, 199-225, 227-253, 255-283
pkg/connector/user.go (1)
54-60: User List/Get/Delete and trait building use the helpers appropriatelyUser List and Get now distinguish pagination/trait‑building problems (plain
fmt.Errorfwith context) from Google Admin API failures, which go throughwrapGoogleApiErrorWithContext; domain/customer‑mismatch cases correctly log and return nil, and Delete uses the helper for API errors while keeping provisioning‑service absence as a plain configuration error. The newuserResourceextraction errors for POSIX accounts and external IDs are also appropriately treated as internal connector bugs.Also applies to: 83-105, 230-269, 304-320, 346-359, 481-489
Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.