Conversation
WalkthroughImplements collaborator Grants, simplifies collaborator cache to session-backed reads/writes, threads disableCustomRolesSync through builders, updates several builder and cache signatures, adjusts role grant behavior and adds role-by-name cache, and expands the Workato privilege catalog. Changes
Sequence Diagram(s)sequenceDiagram
participant Syncer as ResourceSyncer
participant Builder as collaboratorBuilder
participant Cache as collaboratorCache (session)
participant Client as WorkatoClient
participant Logger as ContextLogger
Syncer->>Builder: Grants(ctx, resource, attr)
Builder->>Logger: extract logger from ctx
Builder->>Cache: getCollaborator(ctx, session, collaboratorId)
alt collaborator found
Cache-->>Builder: collaborator
Builder->>Client: fetch collaborator roles & privileges
Client-->>Builder: roles, privileges
Builder->>Builder: compute collaboratorRoleGrants(roles)
Builder->>Builder: build collaboratorPrivilegeGrant(privileges)
opt env allows folder grants
Builder->>Builder: build collaboratorFolderGrant(folder)
end
Builder-->>Syncer: []*v2.Grant, results, nil
else not found / error
Cache-->>Builder: error
Builder-->>Syncer: nil, results, error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/role_cache.go (1)
53-67: Same log format issue in getRoleByName.Remove the
%splaceholder from the error message at line 58.- l.Error("failed to get role by name from session storage %s", zap.String("role_name", roleName), zap.Error(err)) + l.Error("failed to get role by name from session storage", zap.String("role_name", roleName), zap.Error(err))pkg/connector/collaborator.go (1)
247-249: DuplicateWithUserLogincall.Line 248 duplicates line 247 - this appears to be a copy-paste error.
resource.WithEmail(collaborator.Email, true), resource.WithUserLogin(collaborator.Email), - resource.WithUserLogin(collaborator.Email), resource.WithCreatedAt(collaborator.CreatedAt),
🧹 Nitpick comments (1)
pkg/connector/privilege.go (1)
68-73: Consider removing the unusedcachefield.The
cachefield is initialized but never used sinceGrantsnow returnsnilimmediately. This is dead code that can be cleaned up.type privilegeBuilder struct { client *client.WorkatoClient - cache *collaboratorCache } func newPrivilegeBuilder(client *client.WorkatoClient, env workato.Environment) *privilegeBuilder { return &privilegeBuilder{ client: client, - cache: newCollaboratorCache(client), } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/collaborator.go(2 hunks)pkg/connector/collaborator_cache.go(1 hunks)pkg/connector/connector.go(1 hunks)pkg/connector/folder.go(2 hunks)pkg/connector/privilege.go(1 hunks)pkg/connector/resource_types.go(2 hunks)pkg/connector/role.go(7 hunks)pkg/connector/role_cache.go(4 hunks)pkg/connector/workato/privileges.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/connector/resource_types.go (1)
pkg/connector/connector.go (1)
New(68-93)
pkg/connector/collaborator_cache.go (2)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/client/models.go (1)
Collaborator(35-49)
pkg/connector/role_cache.go (2)
pkg/connector/client/models.go (1)
Role(65-73)pkg/connector/workato/roles.go (1)
Role(7-10)
pkg/connector/collaborator.go (4)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)pkg/connector/workato/privileges.go (1)
PrivilegeId(21-23)pkg/connector/workato/roles.go (2)
IsBaseRole(18-25)GetBaseRole(27-38)
pkg/connector/role.go (2)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)
🪛 GitHub Actions: ci
pkg/connector/collaborator.go
[error] 97-97: revive: line-length-limit: line is 227 characters, out of limit 200 (revive)
🔇 Additional comments (13)
pkg/connector/collaborator_cache.go (2)
41-51: LGTM!The
getCollaboratormethod correctly retrieves a collaborator from session storage with proper error handling and uses appropriate gRPC status codes for the not-found case.
29-39: Remove unnecessary comment – loop variable semantics are correct in Go 1.25.2This code is correct. The project declares
go 1.25.2in go.mod, which is well beyond Go 1.22. The new for loop semantics apply when the package being compiled is from a module that contains a go line declaring Go 1.22 or later. Since Go 1.25.2 is declared, each loop body effectively starts with k := k and v := v for each range variable, making each&collaboratorpoint to a distinct memory location for each iteration.The workaround suggested in the review (
collaborator := collaborator) is not needed and can be removed. The code at lines 29-39 is already correct as written.Likely an incorrect or invalid review comment.
pkg/connector/resource_types.go (2)
15-19: LGTM!Adding
SkipGrantsannotation toprivilegeResourceTypealigns with the refactoring where privilege grants are now generated from the collaborator builder instead.
32-36: LGTM!Adding
SkipEntitlementsAndGrantsannotation toprojectResourceTypeis appropriate if projects don't have entitlements or grants to sync.pkg/connector/privilege.go (1)
64-66: LGTM!The
Grantsmethod now returnsnilwhich aligns with theSkipGrantsannotation added toprivilegeResourceType. Grant generation has been moved to the collaborator builder.pkg/connector/workato/privileges.go (1)
430-545: LGTM!The expansion of the
Privilegescatalog with new groups (Policies, Dashboard & logs, Manage locked projects, API platform, Collections & endpoints, Data tables, Settings, Client & access profiles) follows the established pattern and provides comprehensive coverage of Workato's privilege system.pkg/connector/connector.go (1)
28-36: LGTM!The
ResourceSyncersmethod correctly propagates thedisableCustomRolesSyncflag to the collaborator and folder builders. The removal ofenvfromnewFolderBuilderaligns with the session rework refactoring.pkg/connector/role_cache.go (1)
92-94: Loop variable pointer capture issue.Storing
&rolecaptures a pointer to the loop variable, which will point to the same memory location for all iterations. In Go 1.22+, this is fixed, but for compatibility with earlier versions, create a copy.Please verify the Go version used in this project. If using Go < 1.22, apply this fix:
+ copyRoleByName := role - mapRolesByName[role.Name] = &role + mapRolesByName[role.Name] = ©RoleByNamepkg/connector/folder.go (1)
173-178: LGTM!The signature simplification removing the
envparameter aligns with the session rework goal. The cache initialization correctly uses the updatednewCollaboratorCache(client)signature.pkg/connector/collaborator.go (1)
68-117: LGTM on Grants implementation.The new Grants method correctly handles environment filtering, role-based grants, privilege grants, and folder grants. The error handling with gRPC status codes for NotFound is appropriate.
pkg/connector/role.go (3)
27-28: LGTM!Interface assertions provide compile-time verification that
roleBuilderimplements the required interfaces. This is a good practice.
139-147: Good refactoring of Grants control flow.The early return for base roles (line 139) cleanly separates the two code paths. The subsequent custom role handling (lines 142-179) is now gated by
disableCustomRolesSync, making the sync behavior explicit and controllable.
257-264: LGTM!The constructor correctly initializes the cache with the simplified
newCollaboratorCache(client)signature, consistent with the session rework changes in other builders.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/connector/folder.go (1)
101-102: Typo fixes applied.The typos in the entitlement descriptions have been corrected as noted in previous review comments.
pkg/connector/collaborator.go (2)
187-187: Remove printf-style placeholder from structured log.The log message uses a
%splaceholder that won't be interpolated with zap's structured logging. The role name is already provided as a structured field.Apply this diff:
- l.Error("failed to get base role %s", zap.String("role_name", role.RoleName), zap.Error(err)) + l.Error("failed to get base role", zap.String("role_name", role.RoleName), zap.Error(err))
208-208: Remove printf-style placeholder from structured log.Same issue: the
%splaceholder won't be interpolated. The role name is already provided as a structured field.Apply this diff:
- l.Debug("skipping role %s because it is not a base role and custom roles sync is disabled", zap.String("role_name", role.RoleName)) + l.Debug("skipping role because it is not a base role and custom roles sync is disabled", zap.String("role_name", role.RoleName))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/connector/collaborator.go(2 hunks)pkg/connector/folder.go(2 hunks)pkg/connector/role_cache.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/connector/folder.go (1)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)
pkg/connector/collaborator.go (4)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)pkg/connector/workato/privileges.go (1)
PrivilegeId(21-23)pkg/connector/workato/roles.go (2)
IsBaseRole(18-25)GetBaseRole(27-38)
pkg/connector/role_cache.go (2)
pkg/connector/client/models.go (1)
Role(65-73)pkg/connector/workato/roles.go (1)
Role(7-10)
🔇 Additional comments (13)
pkg/connector/folder.go (2)
173-178: Constructor signature simplified correctly.The removal of the
envparameter and the simplified cache initialization align with the PR's objective to reduce cache complexity. The environment context has been appropriately moved to the collaborator level.
110-171: Grants method correctly refactored to role-based grants.The simplified grants logic appropriately handles only role-based grants for folders, with proper pagination and expandable grant annotations. The move of collaborator-specific grants to the collaborator builder reduces complexity as intended.
pkg/connector/collaborator.go (5)
68-121: Grants implementation looks solid.The new Grants method correctly:
- Parses and validates the collaborator ID
- Retrieves role-based grants
- Handles the NotFound case gracefully for collaborator privileges
- Filters grants by environment type
- Generates both privilege and folder grants appropriately
123-139: Folder grant helper correctly marks grants as immutable.The immutable annotation is appropriate since folder access for collaborators is determined by their assigned roles. The grant construction is correct.
141-158: Privilege grant helper implemented correctly.The function properly constructs privilege grants using the workato.PrivilegeId helper. The logic aligns with the role-based privilege model.
196-206: Consider handling missing custom roles gracefully.Line 199 returns an error if a custom role is not found. This could cause sync failures if a role is deleted after being assigned to a collaborator. If this is intentional (to surface data inconsistencies), the current behavior is fine. Otherwise, consider logging a warning and skipping the role instead.
Would you like this to fail fast on missing custom roles, or should it skip them with a warning?
225-231: Constructor correctly updated with new parameters.The signature properly threads through the
envanddisableCustomRolesSyncparameters needed for the grants logic. The cache initialization is consistent with the simplified cache API.pkg/connector/role_cache.go (6)
15-19: LGTM: Cache prefix constants are well-organized.The new
rolesByNameCachePrefixconstant follows the established pattern and improves maintainability.
26-26: Log format issue resolved.The printf-style
%splaceholder has been correctly removed, addressing the previous review comment.
37-51: Improved error handling and log format.The explicit
nilreturns for both error and not-found cases improve clarity. The log format issue has also been resolved.
53-67: LGTM: New function follows established patterns.The
getRoleByNamefunction correctly mirrorsgetRoleByIdwith appropriate cache prefix and error handling.
71-71: LGTM: Cache prefix constants improve maintainability.Good use of constants instead of hard-coded strings for cache prefixes. The new roles-by-name cache follows the same pattern as existing caches.
Also applies to: 97-97, 103-109
78-78: Document or enforce the assumption that role names are unique.The
mapRolesByNamemap usesrole.Nameas the key without validating uniqueness. If duplicate role names exist in Workato, this would silently overwrite previous entries, causinggetRoleByNameto return an incorrect role. Either add validation to detect and handle duplicates, document that Workato guarantees unique role names per workspace, or userole.Idas the primary cache key instead.
|
|
||
| users, found, err := session.GetJSON[[]*CompoundUser](ctx, sessionStorage, folderId, sessions.WithPrefix(folderToUserCachePrefix)) | ||
| // getCollaborator gets a collaborator from the cache by id. | ||
| // It is assumed that the cache is populated when listing resources and is only used when listing grants. |
There was a problem hiding this comment.
I don't like this assumption, but I was trying to avoid a bunch of additional session store calls to remove the assumption. We make the same assumptions with the role_cache.
I'm open to suggestions here.
There was a problem hiding this comment.
This is an ok assumption as long as we don't do incremental sync and don't enable parallel sync (which will be off by default whenever it gets shipped in baton-sdk).
There was a problem hiding this comment.
I agree. Seems ok to make this assumption with the current process, since this function will be called when creating Grants, and that will be executed only after the List process of the resource (when the cache gets populated)
| var privilegeResourceType = &v2.ResourceType{ | ||
| Id: "privilege", | ||
| DisplayName: "Privilege", | ||
| Annotations: annotations.New(&v2.SkipGrants{}), |
There was a problem hiding this comment.
I'm glad we added this annotation and that people are using it.
Ideally we should skip grants/entitlements based on whether these methods exist, but that would require some changes to interfaces. :/
| } | ||
|
|
||
| // Grants returns collaborator-has grants and derived privilege grants for base and custom roles. | ||
| // Grants always returns an empty slice for users since they don't have any entitlements. |
There was a problem hiding this comment.
nit: should remove entirely since we are emitting grants here
|
|
||
| // Entitlements always returns an empty slice for users. | ||
| func (o *roleBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { | ||
| func (o *roleBuilder) Entitlements(_ context.Context, resource *v2.Resource, attr rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { |
There was a problem hiding this comment.
nit: we can leave attr as _ since i dont see it being used
| // List returns all the users from the database as resource objects. | ||
| // Users include a UserTrait because they are the 'shape' of a standard user. | ||
| func (o *roleBuilder) List(ctx context.Context, _ *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { | ||
| func (o *roleBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { |
|
|
||
| collaboratorRoles, err := o.client.GetCollaboratorPrivileges(ctx, collaboratorId) | ||
| if err != nil { | ||
| if status.Code(err) != codes.NotFound { |
There was a problem hiding this comment.
nit: I understand that this is evaluating the case where response.Data len is 0, but could it be confused with the actual response returning a 404 (which I guess it would mean the collabolator wasn't found)? I guess in any case is probably safe to ignore and continue
|
|
||
| users, found, err := session.GetJSON[[]*CompoundUser](ctx, sessionStorage, folderId, sessions.WithPrefix(folderToUserCachePrefix)) | ||
| // getCollaborator gets a collaborator from the cache by id. | ||
| // It is assumed that the cache is populated when listing resources and is only used when listing grants. |
There was a problem hiding this comment.
I agree. Seems ok to make this assumption with the current process, since this function will be called when creating Grants, and that will be executed only after the List process of the resource (when the cache gets populated)
| @@ -63,38 +62,13 @@ func (o *privilegeBuilder) Entitlements(_ context.Context, resource *v2.Resource | |||
|
|
|||
| // Grants always returns an empty slice for users since they don't have any entitlements. | |||
There was a problem hiding this comment.
Nit: This is not important at all but this comment is misplaced
| @@ -36,7 +40,7 @@ func (o *roleBuilder) ResourceType(ctx context.Context) *v2.ResourceType { | |||
|
|
|||
| // List returns all the users from the database as resource objects. | |||
There was a problem hiding this comment.
Nit: Another absolutely not important thing - commentary misplaced
5 participants
Description
Summary by CodeRabbit
Bug Fixes
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.