[BB-1788] Add containerization support implementing ResourceSyncerV2 and SessionStore cache#29
Conversation
luisina-santos
requested review from
btipling and
laurenleach
as code owners
WalkthroughAdds a generated Workato config type with reflection-backed accessors; renames package conf→config and exposes a field-driven Config; migrates builder APIs from pagination tokens/annotations to rs.SyncOpAttrs/SyncOpResults; and replaces in-memory caches with session-backed read/write helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Connector
participant Builder as Resource Builder
participant Session as Session Cache
participant API as Workato API
rect rgba(220, 235, 255, 0.35)
Note over Client,Connector: Old flow (pagination + in-memory cache)
Client->>Connector: ResourceSyncers()
Connector->>Builder: List(parentID, paginationToken)
Builder->>API: Fetch resources (paged)
API-->>Builder: resources, nextToken
Builder->>Builder: populate in-memory cache (mutex)
Builder-->>Connector: resources, nextToken, annotations
end
rect rgba(220, 255, 230, 0.35)
Note over Client,Connector: New flow (SyncOpAttrs + session cache)
Client->>Connector: ResourceSyncers()
Connector->>Builder: List(_, SyncOpAttrs{Session, PageToken})
Builder->>Session: getUsers/getRoles... (read)
Session-->>Builder: cached entries or nil
alt cache miss
Builder->>API: Fetch resources
API-->>Builder: resources
Builder->>Session: setRolesCache / setCollaboratorsCache (write)
Session-->>Builder: stored
end
Builder-->>Connector: resources, SyncOpResults{NextPageToken}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/connector/collaborator_cache.go (1)
93-151: FixCompoundUser.Userpointer aliasing insetCollaboratorsCache
CompoundUseris built withUser: &collaborator, wherecollaboratoris the range variable. This means every cachedCompoundUserends up pointing to the same collaborator (the last one in the slice).Use a per-iteration copy before taking the address:
func (c *collaboratorCache) setCollaboratorsCache(ctx context.Context, sessionStorage sessions.SessionStore, collaborators []client.Collaborator) error { @@ - for _, collaborator := range collaborators { + for _, collaborator := range collaborators { collaboratorRoles, err := c.client.GetCollaboratorPrivileges(ctx, collaborator.Id) @@ - compoundUser := &CompoundUser{ - User: &collaborator, + collaboratorCopy := collaborator + compoundUser := &CompoundUser{ + User: &collaboratorCopy, UserDetail: collaboratorRoles, }This preserves the rest of the logic while ensuring each
CompoundUserreferences the correct collaborator.pkg/connector/privilege.go (1)
50-61: Fix entitlement display-name typo and update outdated comment
- Line 50: The doc comment still mentions “users” and an empty slice, but this builder now returns a real entitlement for privileges. Consider updating the comment to reflect the current behavior.
- Line 56: The display name format string has a trailing backtick, which will show up in UI and looks like a typo.
You can minimally fix both with something like:
-// Entitlements always returns an empty slice for users. +// Entitlements returns the assignment entitlement for a privilege. - entitlement.WithDisplayName(fmt.Sprintf("%s have %s`", collaboratorResourceType.DisplayName, resource.DisplayName)), + entitlement.WithDisplayName(fmt.Sprintf("%s have %s", collaboratorResourceType.DisplayName, resource.DisplayName)),pkg/connector/folder.go (1)
95-107: Fix entitlement description/display-name typos and comment wording
- Line 95: The comment still claims this returns an empty slice “for users”, but this builder now emits a folder access entitlement. Updating the wording would avoid confusion.
- Lines 101–102: Both strings use “acess” instead of “access”, which will surface in UI.
A minimal fix:
-// Entitlements always returns an empty slice for users. +// Entitlements returns the collaborator-access entitlement for a folder. - entitlement.WithDescription(fmt.Sprintf("%s can acess %s", collaboratorResourceType.DisplayName, resource.DisplayName)), - entitlement.WithDisplayName(fmt.Sprintf("%s acess %s", collaboratorResourceType.DisplayName, resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("%s can access %s", collaboratorResourceType.DisplayName, resource.DisplayName)), + entitlement.WithDisplayName(fmt.Sprintf("%s access %s", collaboratorResourceType.DisplayName, resource.DisplayName)),(or tweak the grammar to whatever product copy you prefer).
🧹 Nitpick comments (10)
pkg/connector/role_cache.go (1)
15-18: Reuse cache prefix constants insetRolesCache
setRolesCachehard-codes"roles"and"folder_roles"instead of usingrolesCachePrefixandfolderRolesCachePrefix, which is easy to desync later.Consider this small cleanup:
const ( - rolesCachePrefix = "roles" - folderRolesCachePrefix = "folder_roles" + rolesCachePrefix = "roles" + folderRolesCachePrefix = "folder_roles" ) @@ func setRolesCache(ctx context.Context, sessionStorage sessions.SessionStore, roles []client.Role) error { - err := session.SetManyJSON(ctx, sessionStorage, parseJSONRolesCache(roles), sessions.WithPrefix("roles")) + err := session.SetManyJSON(ctx, sessionStorage, parseJSONRolesCache(roles), sessions.WithPrefix(rolesCachePrefix)) @@ - folderRoles, found, err := session.GetJSON[[]*client.Role](ctx, sessionStorage, folderIDStr, sessions.WithPrefix("folder_roles")) + folderRoles, found, err := session.GetJSON[[]*client.Role](ctx, sessionStorage, folderIDStr, sessions.WithPrefix(folderRolesCachePrefix)) @@ - err = session.SetJSON(ctx, sessionStorage, folderIDStr, folderRoles, sessions.WithPrefix("folder_roles")) + err = session.SetJSON(ctx, sessionStorage, folderIDStr, folderRoles, sessions.WithPrefix(folderRolesCachePrefix))Also applies to: 52-76
pkg/connector/collaborator_cache.go (1)
154-172: Clarify generic cache helper error message
appendCachedValueis used for privilege, folder, and role caches, but the error message is hard-coded as “failed to set folder roles…”, which can be misleading when debugging.Consider generalizing the error text:
func appendCachedValue[T any](ctx context.Context, sessionStorage sessions.SessionStore, cachePrefix string, cacheKey string, cacheValue T) error { @@ - err = session.SetJSON(ctx, sessionStorage, cacheKey, values, sessions.WithPrefix(cachePrefix)) - if err != nil { - return fmt.Errorf("failed to set folder roles in session storage: %w", err) - } + err = session.SetJSON(ctx, sessionStorage, cacheKey, values, sessions.WithPrefix(cachePrefix)) + if err != nil { + return fmt.Errorf("failed to set cached values in session storage (prefix=%s, key=%s): %w", cachePrefix, cacheKey, err) + }pkg/config/conf.gen.go (2)
13-25: Defensive check for nil receiver infindFieldByTag
reflect.ValueOf(c).Elem()will panic ifcisnil. Even though this is generated code and callers should always have a non-nil*Workato, a quick nil guard (or a clearer panic message includingtagValue) would make failures easier to debug and slightly safer if misused in the future.
28-88: Generated accessors usepanicand may trigger lintersAll the getters (
GetStringSlice,GetString,GetInt,GetBool,GetStringMap)panic("wrong type")on mismatches. That’s reasonable for generated config access, but:
- The panic message is very opaque.
- golangci-lint may complain about panics or other patterns in generated code (as hinted by the pipeline failure).
Consider either:
- Enriching panic messages with field/tag and concrete/expected types, or
- Tweaking golangci-lint config to ignore
*.gen.gofiles if you don’t want to change generated output.pkg/connector/project.go (1)
21-45: List implementation aligns with SyncOpAttrs/SyncOpResults; update commentThe pagination wiring via
attr.PageToken.Tokenand returning&rs.SyncOpResults{NextPageToken: nextToken}looks consistent with the new V2 pattern, and error paths returning(nil, nil, err)are correct.Minor: the comment still refers to “users” even though this is a project builder; consider updating it to avoid confusion.
pkg/connector/role.go (1)
95-199: Grants logic and cache usage look good; clarify behavior on missing custom roleThe Grants implementation:
- Uses
getUsersByRole(ctx, attr.Session, resource.DisplayName)to derive collaborator grants.- Handles base roles by expanding to privilege grants with the expected annotations.
- For custom roles, uses
getRoleById(ctx, attr.Session, resource.Id.Resource)andworkato.FindRelatedPrivilegesErrto derive privilege grants.This is a solid migration to session-backed caches and SyncOpAttrs/Results. One nuance: when
getRoleByIdreturnsnil, you log a warning and returnrv, nil, NotFound error. Sincervis initially empty, that effectively yields “no grants + error”, which is reasonable but slightly asymmetric with other error paths that return(nil, nil, err).If you want stricter consistency, consider returning
nil, nil, errhere as well; otherwise, the current behavior is acceptable as long as callers are aware thatrvmay be non-nil even when an error is returned.pkg/connector/collaborator.go (1)
68-89: DuplicateWithUserLogincall incollaboratorResource
collaboratorResourcecallsresource.WithUserLogin(collaborator.Email)twice in thetraitsslice. This is harmless but redundant and slightly noisy.You can drop one of them:
- resource.WithUserLogin(collaborator.Email), - resource.WithUserLogin(collaborator.Email), + resource.WithUserLogin(collaborator.Email),pkg/connector/privilege.go (1)
93-97: Consider dropping the unused client from privilegeBuilder
newPrivilegeBuilderstores a*client.WorkatoClientonprivilegeBuilder, but that field is not used anywhere in this file (listing, entitlements, and grants all work off static data and session helpers). If it’s not used elsewhere, consider removing the field and constructor parameter to avoid confusion about unused dependencies.pkg/connector/folder.go (2)
110-198: Bag-based Grants pagination is valid but can be simplifiedThe cpagination-based Grants flow is structurally sound—
GenBagFromTokenis used to reconstruct state,bag.Current()seeds initial work with collaborator/role entries, andNextPageTokenis driven bybag.Marshal(), matching the generic bag helper pattern (seepkg/connector/cpagination/generic_bag.go). Based on relevant snippets.A couple of cleanups you might consider:
- The first call where
bag.Current() == nilreturns no grants and only a token. You could instead initialize the bag when empty and immediately pop/process the first state, avoiding an extra “empty” round trip.- The
Bag.Pagefield is currently unused; unless you plan to add per-type pagination soon, dropping it would reduce noise.- When
o.disableCustomRolesSyncis true, you still push a role state on initialization that will never produce grants; you could conditionally omit that push to avoid an extra iteration.These are all behavior-preserving refactors and can be deferred if you prefer.
200-205: folderBuilder.cache field is unused and not initialized
folderBuilderstill declares acache *collaboratorCachefield, butnewFolderBuilderonly setsclientanddisableCustomRolesSync, and this file never referenceso.cache. If there are no external uses, consider removing the field (and any dead code around collaboratorCache) to make it clear that folder grants are purely session-based now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Makefileis excluded by none and included by nonecmd/baton-workato/main.gois excluded by none and included by nonepkg/config/gen/gen.gois excluded by!**/gen/**and included bypkg/**
📒 Files selected for processing (12)
go.mod(1 hunks)pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(2 hunks)pkg/config/config_test.go(1 hunks)pkg/connector/collaborator.go(2 hunks)pkg/connector/collaborator_cache.go(4 hunks)pkg/connector/connector.go(3 hunks)pkg/connector/folder.go(5 hunks)pkg/connector/privilege.go(4 hunks)pkg/connector/project.go(1 hunks)pkg/connector/role.go(7 hunks)pkg/connector/role_cache.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/connector/role_cache.go (1)
pkg/connector/client/models.go (1)
Role(65-73)
pkg/connector/collaborator_cache.go (3)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)pkg/connector/client/models.go (1)
Collaborator(35-49)
pkg/config/conf.gen.go (1)
pkg/config/config.go (2)
WorkatoEnv(23-28)DisableCustomRolesSync(30-35)
pkg/connector/role.go (1)
pkg/connector/workato/privileges.go (2)
Privileges(31-430)FindRelatedPrivilegesErr(448-480)
pkg/connector/connector.go (5)
pkg/connector/client/client.go (2)
WorkatoClient(28-33)NewWorkatoClient(35-61)pkg/connector/workato/env.go (2)
Environment(10-10)EnvFromString(22-33)pkg/config/conf.gen.go (1)
Workato(6-11)pkg/config/config.go (3)
Config(55-61)WorkatoEnv(23-28)DisableCustomRolesSync(30-35)pkg/connector/client/helpers.go (1)
WorkatoDataCenters(18-24)
pkg/connector/collaborator.go (2)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)
pkg/connector/folder.go (1)
pkg/connector/cpagination/generic_bag.go (1)
GenBagFromToken(23-30)
🪛 GitHub Actions: ci
pkg/config/config_test.go
[error] 20-20: undefined: ValidateConfig (typecheck) encountered during tests.
pkg/config/conf.gen.go
[error] 1-1: golangci-lint config issue reported for generated file: '# github.com/conductorone/baton-workato/pkg/config [github.com/conductorone/baton-workato/pkg/config.test]'.
🔇 Additional comments (12)
go.mod (1)
86-86: Viper moved to indirect looks fineMarking
github.com/spf13/viper v1.19.0as indirect is consistent with removing direct config usage; no issues from this change alone.pkg/config/config_test.go (1)
1-21: Fix config tests:ValidateConfigis gone, causing CI failure
config.goremovedValidateConfig, but this test still references it, leading to the reportedundefined: ValidateConfigtypecheck failure.Assuming you no longer have custom validation logic beyond the schema, the fix is:
- Reuse the exported
Configinstead of rebuilding it- Pass
nilas the validator totest.ExerciseTestCases(which accepts a function ornil)func TestConfigs(t *testing.T) { - configurationSchema := field.NewConfiguration( - ConfigurationFields, - field.WithConstraints(FieldRelationships...), - ) + configurationSchema := Config testCases := []test.TestCase{ // Add test cases here. } - test.ExerciseTestCases(t, configurationSchema, ValidateConfig, testCases) + test.ExerciseTestCases(t, configurationSchema, nil, testCases) }Verify this matches your pinned
github.com/conductorone/baton-sdkv0.5.21 test API documentation.pkg/connector/project.go (1)
47-55: No-op Entitlements/Grants for projects are fineReturning
(nil, nil, nil)for both Entitlements and Grants makes it explicit that projects have no entitlements/grants. This is consistent with the previous behavior and the new V2 signatures.pkg/connector/connector.go (1)
27-35: ResourceSyncers V2 wiring looks consistentThe updated
ResourceSyncersreturning[]connectorbuilder.ResourceSyncerV2and usingnewCollaboratorBuilder(d.client, d.env),newPrivilegeBuilder(d.client),newRoleBuilder(d.client, d.env, d.disableCustomRolesSync),newFolderBuilder(d.client, d.disableCustomRolesSync), andnewProjectBuilder(d.client)is consistent with the new builder signatures and the broader V2 refactor.pkg/connector/role.go (2)
38-79: Role listing + session cache integration looks correctUsing
attr.PageToken.TokenwithGetRoles, caching roles into the session viasetRolesCache(ctx, attr.Session, roles), and returning&rs.SyncOpResults{NextPageToken: nextToken}is consistent with the new SyncOpAttrs/Results pattern and the role cache design. Error paths correctly return(nil, nil, err).No functional issues spotted here; just ensure callers always supply a non-nil
attr.Sessionwhen custom role sync is enabled so caching works as intended.
82-92: Entitlements signature migration is cleanThe updated
Entitlementsmethod correctly adoptsrs.SyncOpAttrsand returns(rv, nil, nil). The entitlement construction itself is unchanged aside from the signature shape, so behavior should be preserved.pkg/connector/collaborator.go (3)
26-49: List migration + collaborator cache integration look goodThe List method now:
- Uses
resource.SyncOpAttrsand returns([]*v2.Resource, *resource.SyncOpResults, error).- Populates the collaborator cache via
o.cache.setCollaboratorsCache(ctx, attr.Session, collaborators).- Returns
(rv, nil, nil)on success and(nil, nil, err)on failure.This is consistent with the new SyncOp pattern and the session-based cache design. Just ensure
collaboratorBuilderinstances are always constructed vianewCollaboratorBuildersoo.cacheis never nil.
52-59: No-op Entitlements/Grants for collaborators are explicit and fineHaving collaborators expose no entitlements or grants and returning
(nil, nil, nil)matches the prior semantics and aligns with the V2 signatures.
61-65: All call sites verified; constructor changes are correctThe search confirms only one external call site in
./pkg/connector/connector.go:30, which correctly uses the new signaturenewCollaboratorBuilder(d.client, d.env). No remaining calls to an old signature exist.pkg/connector/privilege.go (2)
31-47: Privilege List implementation aligns with new SyncOp APIStatic privilege enumeration via
workato.AllCompoundPrivileges()withSyncOpResultsset tonil(no pagination) is consistent and error handling is correct. No changes needed here.
64-91: Grants use session-based privilege lookup correctlyThe
Grantsmethod’s use ofgetUsersByPrivilege(ctx, attr.Session, privilegeId)and construction of immutable collaborator grants looks consistent with the session-backed cache pattern and the newSyncOpAttrs/SyncOpResultsshapes. Error propagation is straightforward and there’s no pagination state to maintain here, which is fine if the privilege→user fanout is expected to be small/medium.pkg/connector/folder.go (1)
36-93: Folder List pagination and parent-type handling look correctThe List implementation branches correctly on
parentResourceID.ResourceType, passesattr.PageToken.Tokenthrough to the client, and wraps the client’snextTokeninSyncOpResults. Early returns fornilparent and unknown types (with a warning log) are consistent with a “no children” semantics. No changes needed here.
luisina-santos
force-pushed
the
luisinasantos/add-containerization-support
branch
from
4351b56 to
0d666e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
-
pkg/config/config_test.go(2 hunks) -
pkg/connector/collaborator.go(2 hunks) -
pkg/connector/folder.go(5 hunks) -
pkg/connector/role.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/config_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/collaborator.go (2)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)
pkg/connector/role.go (1)
pkg/connector/workato/privileges.go (2)
Privileges(31-430)FindRelatedPrivilegesErr(448-480)
🔇 Additional comments (10)
pkg/connector/folder.go (4)
35-92: LGTM! Migration to SyncOpAttrs/SyncOpResults is consistent.The List method has been successfully migrated to use
SyncOpAttrsfor input andSyncOpResultsfor output. The pagination token is correctly accessed viaattr.PageToken.Token, and the next page token is properly returned inSyncOpResults.NextPageToken.
199-204: I'll help you verify that all call sites tonewFolderBuilderhave been updated to match the new signature. Let me search the codebase for all occurrences of this function call.
<function_calls>
#!/bin/bashSearch for all calls to newFolderBuilder to verify signature changes
rg -nP 'newFolderBuilder(' --type=go -C3
</function_calls>
<function_calls>
#!/bin/bashAlso search for newFolderBuilder in comments or other contexts
rg -nP 'newFolderBuilder' --type=go -C2
</function_calls>
169-169: I'll help you verify the review comment about thegetRoleByFolderfunction. Let me start by searching for its definition and analyzing its error handling.
<function_calls>
#!/bin/bashSearch for getRoleByFolder function definition
ast-grep --pattern
$'func getRoleByFolder($ $$) {
$$$
}'
</function_calls>Now let me also search for the function with rg to get more context:
<function_calls>
#!/bin/bashSearch for getRoleByFolder definition and usages with context
rg -n "getRoleByFolder" -A 15 -B 2
</function_calls>
147-147: Verify that getUsersByFolder helper exists and handles session errors.The function is properly defined at
pkg/connector/collaborator_cache.go:61and implements error handling:
- Catches errors from
session.GetJSONand logs them (line 66), returningnilon error- Handles cache misses by checking the
foundflag (line 70), returningnilwhen empty- The function correctly accepts
sessions.SessionStorefromattr.Sessionand delegates session operations to the SDK'ssession.GetJSON, which implicitly handles nil or invalid session storage scenariospkg/connector/collaborator.go (3)
26-52: LGTM! Cache population integrated correctly.The List method has been successfully migrated to use
SyncOpAttrsandSyncOpResults. The cache population viasetCollaboratorsCache(ctx, attr.Session, collaborators)is correctly placed after fetching collaborators and before building the resource list. Error handling is consistent throughout.
35-39: The implementation properly handles session persistence with appropriate error handling.The
setCollaboratorsCachemethod (pkg/connector/collaborator_cache.go:93) correctly persists collaborators to the session through theappendCachedValuehelper function. This helper:
- Retrieves existing cached values using
session.GetJSON()- Appends new collaborators to the cache
- Persists the updated data using
session.SetJSON()- Returns wrapped errors if serialization or storage operations fail
Error handling covers both retrieval failures ("failed to get cached value from session storage") and persistence failures ("failed to set [privilege/folder/role] to user cache in session storage"), ensuring any serialization or storage errors are properly caught and propagated.
64-69: Perfect! I've completed the verification. Let me trace the complete env parameter propagation flow:Verification Results:
- Configuration to Initialization: In the
New()function (line 77),envis derived fromconfig.WorkatoEnvviaworkato.EnvFromString()- Connector Creation: The
envis passed toNewConnector()(line 86)- Struct Assignment: In
NewConnector()(line 62), theenvparameter is correctly assigned to the Connector struct field- Usage in ResourceSyncers: In
ResourceSyncers()(line 30),d.envis correctly passed tonewCollaboratorBuilder()- Cache Initialization: In
newCollaboratorBuilder()(line 67), theenvis passed tonewCollaboratorCache()Summary: The env parameter is correctly propagated from connector initialization through all call sites. There is only one call site to
newCollaboratorBuilder(), and it properly passes the environment value from the Connector instance.
The env parameter is correctly propagated through the connector initialization flow. The environment value originates from configuration validation, is properly stored in the Connector struct, and is correctly passed to
newCollaboratorBuilderin the only call site.pkg/connector/role.go (3)
38-82: LGTM! Role caching integrated correctly.The List method has been successfully migrated to use
SyncOpAttrsandSyncOpResults. The cache population viasetRolesCache(ctx, attr.Session, roles)is correctly placed after fetching roles from the API. The method properly handles both custom roles and base roles.
102-102:getUsersByRolehelper function exists and properly handles session retrieval and error cases.The function is defined in
pkg/connector/collaborator_cache.go(lines 77-91) and includes comprehensive error handling:
- Errors from session storage retrieval are caught and logged (lines 81-83), returning
nil- When the cache entry is not found, the function returns
nil(lines 86-87)- The function returns the retrieved users when successfully found (line 90)
The implementation safely handles both nil sessions and empty cache scenarios by delegating session management to the
session.GetJSON()helper and returningnilappropriately in all error paths.
54-58: setRolesCache properly handles session persistence with comprehensive error handling.Verified: The function uses
session.SetManyJSON()to persist roles to session storage with a "roles" prefix (line 53, pkg/connector/role_cache.go), wraps storage errors with descriptive context usingfmt.Errorf(), and also creates folder-level indices for efficient lookups. All session operations include proper error handling and are passed to the underlyingsessionStorageparameter, ensuring session state is correctly persisted.
|
|
||
| // Entitlements always returns an empty slice for users. | ||
| func (o *folderBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { | ||
| func (o *folderBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { |
There was a problem hiding this comment.
This can be a static entitlements now
There was a problem hiding this comment.
we are experiencing issues with skipping entitlements and use StaticEntitlements, so for now I'll keep this as regular entitlements
|
|
||
| // Entitlements always returns an empty slice for users. | ||
| func (o *roleBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { | ||
| func (o *roleBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { |
There was a problem hiding this comment.
Can be static entitlements
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/collaborator_cache.go (1)
93-171: Fix pointer aliasing insetCollaboratorsCache(allCompoundUser.Userpoint to last collaborator)Inside the
for _, collaborator := range collaboratorsloop,CompoundUser.Useris set to&collaborator, which is the address of the loop variable. In Go, this means every cachedCompoundUserends up pointing at the same memory location and will all reference the last collaborator processed.Adjust the loop to take the address of the slice element instead of the loop variable so each
CompoundUserpoints to the correct collaborator.-func (c *collaboratorCache) setCollaboratorsCache(ctx context.Context, sessionStorage sessions.SessionStore, collaborators []client.Collaborator) error { +func (c *collaboratorCache) setCollaboratorsCache(ctx context.Context, sessionStorage sessions.SessionStore, collaborators []client.Collaborator) error { l := ctxzap.Extract(ctx) @@ - for _, collaborator := range collaborators { - collaboratorRoles, err := c.client.GetCollaboratorPrivileges(ctx, collaborator.Id) + for i := range collaborators { + collaborator := &collaborators[i] + + collaboratorRoles, err := c.client.GetCollaboratorPrivileges(ctx, collaborator.Id) @@ - compoundUser := &CompoundUser{ - User: &collaborator, - UserDetail: collaboratorRoles, - } + compoundUser := &CompoundUser{ + User: collaborator, + UserDetail: collaboratorRoles, + } @@ - // Build for roles - for _, role := range collaborator.Roles { + // Build for roles + for _, role := range collaborator.Roles { if role.EnvironmentType != c.env.String() { continue } appendCachedValue(roleToUsers, role.RoleName, compoundUser) } }
♻️ Duplicate comments (2)
pkg/connector/role.go (1)
38-58: Defensively require a non‑nilattr.Sessionbefore using session-backed cachesBoth
ListandGrantsassumeattr.Sessionis non‑nil (forsetRolesCache,getUsersByRole, andgetRoleById). If a caller ever passes a SyncOpAttrs without a SessionStore, these calls will fail at runtime instead of returning a clear error.Add a defensive check near the top of each method that touches
attr.Sessionso callers get a structuredInvalidArgumenterror instead of a panic or opaque failure.func (o *roleBuilder) List(ctx context.Context, _ *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { l := ctxzap.Extract(ctx) l.Debug("Listing roles") rv := make([]*v2.Resource, 0) var nextToken string + if attr.Session == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "baton-workato: session storage is required") + } + if !o.disableCustomRolesSync { @@ func (o *roleBuilder) Grants(ctx context.Context, resource *v2.Resource, attr rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { l := ctxzap.Extract(ctx) + if attr.Session == nil { + return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "baton-workato: session storage is required") + } + // Since roles names are unique, we can use the role name as the key to get all the users that have that role. collaborators := getUsersByRole(ctx, attr.Session, resource.DisplayName)This also ensures the subsequent use of
getRoleById(ctx, attr.Session, ...)in the custom roles path is always safe.Also applies to: 97-199
pkg/connector/connector.go (1)
68-92: GuardWorkatoDataCentersmap lookup to avoid invalid/empty base URLs
dataCenterUrl := client.WorkatoDataCenters[config.WorkatoDataCenter]assumes the key is always valid. A bad or unexpectedWorkatoDataCentervalue will yield the zero value (""), which then flows intoNewWorkatoClientand can produce confusing runtime errors instead of a clear configuration failure.Add an existence check and return a descriptive error when the configured data center is not known.
func New(ctx context.Context, config *cfg.Workato, _ *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { l := ctxzap.Extract(ctx) err := field.Validate(cfg.Config, config) if err != nil { return nil, nil, err } - dataCenterUrl := client.WorkatoDataCenters[config.WorkatoDataCenter] + dataCenterUrl, ok := client.WorkatoDataCenters[config.WorkatoDataCenter] + if !ok || dataCenterUrl == "" { + return nil, nil, fmt.Errorf("baton-workato: invalid data center %q", config.WorkatoDataCenter) + }
🧹 Nitpick comments (1)
pkg/connector/role_cache.go (1)
52-93: Reuse cache-prefix constants insetRolesCacheand thanks for fixing pointer aliasing
parseJSONRolesCachenow uses a per-iterationroleCopy, so each cached*client.Rolepoints to a distinct instance—this fixes the earlier pointer alias bug. The folder-role map also usescopyRole := role, which is correct.To keep readers and writers in lockstep, it would be clearer to reuse the prefix constants instead of string literals in
setRolesCache:func setRolesCache(ctx context.Context, sessionStorage sessions.SessionStore, roles []client.Role) error { - err := session.SetManyJSON(ctx, sessionStorage, parseJSONRolesCache(roles), sessions.WithPrefix("roles")) + err := session.SetManyJSON(ctx, sessionStorage, parseJSONRolesCache(roles), sessions.WithPrefix(rolesCachePrefix)) @@ - if (len(mapRoles)) > 0 { - err = session.SetManyJSON(ctx, sessionStorage, mapRoles, sessions.WithPrefix("folder_roles")) + if len(mapRoles) > 0 { + err = session.SetManyJSON(ctx, sessionStorage, mapRoles, sessions.WithPrefix(folderRolesCachePrefix))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/config/config.go(2 hunks)pkg/connector/collaborator_cache.go(4 hunks)pkg/connector/connector.go(3 hunks)pkg/connector/role.go(7 hunks)pkg/connector/role_cache.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/config.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/connector/collaborator_cache.go (3)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)pkg/connector/client/models.go (1)
Collaborator(35-49)
pkg/connector/connector.go (5)
pkg/connector/client/client.go (2)
WorkatoClient(28-33)NewWorkatoClient(35-61)pkg/connector/workato/env.go (2)
Environment(10-10)EnvFromString(22-33)pkg/config/conf.gen.go (1)
Workato(6-11)pkg/config/config.go (3)
Config(55-61)WorkatoEnv(23-28)DisableCustomRolesSync(30-35)pkg/connector/client/helpers.go (1)
WorkatoDataCenters(18-24)
pkg/connector/role.go (1)
pkg/connector/workato/privileges.go (2)
Privileges(31-430)FindRelatedPrivilegesErr(448-480)
🔇 Additional comments (3)
pkg/connector/collaborator_cache.go (1)
45-91: Session-backed collaborator lookup helpers look goodThe
getUsersByPrivilege,getUsersByFolder, andgetUsersByRolehelpers correctly scope keys by prefix, log failures, and safely treat cache misses as empty (nil) slices, which works fine with range semantics at call sites.pkg/connector/role_cache.go (1)
20-50: Role lookup helpers are consistent with session caching pattern
getRoleByFolderandgetRoleByIdcorrectly scope by prefixes, log retrieval errors, and return nil on cache miss, which matches how the callers treat “no cached roles”.pkg/connector/connector.go (1)
28-35: ResourceSyncers migration to V2 builders looks consistentThe
ResourceSyncersimplementation cleanly migrates toResourceSyncerV2and wires each builder with the expected dependencies (env,disableCustomRolesSync), so the connector surface matches the updated Baton builder API.
johnallers
left a comment
johnallers
left a comment
There was a problem hiding this comment.
Tested locally and it worked.
I have some concerns for how the caches are initialized. It will break down if we ever implement parallel sync, but that's a problem for the future.
luisina-santos
changed the title
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/folder.go (1)
96-112: Fix folder entitlement/grant comments and typos in descriptionsThe comments and messages are now misleading:
// Entitlements always returns an empty slice for users.— this builder returns real entitlements for folder resources.// Grants always returns an empty slice for users since they don't have any entitlements.— grants are being emitted for collaborators and roles.- User-facing strings use
"acess"instead of"access".Consider updating to something like:
-// Entitlements always returns an empty slice for users. +// Entitlements returns collaborator-access entitlements for folder resources. @@ - entitlement.WithDescription(fmt.Sprintf("%s can acess %s", collaboratorResourceType.DisplayName, resource.DisplayName)), - entitlement.WithDisplayName(fmt.Sprintf("%s acess %s", collaboratorResourceType.DisplayName, resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("%s can access %s", collaboratorResourceType.DisplayName, resource.DisplayName)), + entitlement.WithDisplayName(fmt.Sprintf("%s access %s", collaboratorResourceType.DisplayName, resource.DisplayName)), @@ -// Grants always returns an empty slice for users since they don't have any entitlements. +// Grants returns collaborator and role grants for folder resources.to keep docs and UX strings accurate.
pkg/connector/role.go (2)
85-96: Update Entitlements doc comment to reflect actual behaviorThe comment
// Entitlements always returns an empty slice for users.is now incorrect: this method emits a collaborator-has entitlement for each role.Consider updating it to something like:
-// Entitlements always returns an empty slice for users. +// Entitlements returns a collaborator-has entitlement for each role resource.to keep the docs aligned with the implementation.
39-59: Propagate errors from cache operations inGrantsand ensure session availabilityThe cache read helpers (
getUsersByRole,getRoleById,getUsersByFolder,getUsersByPrivilege) silently fail by logging errors and returning nil/empty results. This masks session storage problems:
roleBuilder.Grants(line 103, 165):o.cache.getUsersByRole()andgetRoleById()return nil without errorfolderBuilder.Grants:o.cache.getUsersByFolder()andgetRoleByFolder()return nil without errorprivilegeBuilder.Grants:o.cache.getUsersByPrivilege()returns nil without errorWhen
attr.Sessionis unavailable, these methods silently return empty results instead of signaling the failure, making the code return incorrect grant lists. UnlikesetRolesCache()which properly propagates errors (line 56), read methods should also return errors to callers. Consider updating cache helpers to return(results, error)or at minimum validatingattr.Session != nilat method entry to fail fast with a clear error rather than silent empty results.Also fix the doc comment for
Entitlements(line 85): it claims the method "always returns an empty slice for users" but the implementation creates a single entitlement.pkg/connector/privilege.go (1)
52-62: Fix privilege Entitlements/Grants comments and user-facing textA few small but user-visible issues:
// Entitlements always returns an empty slice for users.— this emits a real entitlement per privilege.// Grants always returns an empty slice for users since they don't have any entitlements.— this method returns collaborator grants for the privilege.WithDisplayName(fmt.Sprintf("%s have %s", ...))` has a stray backtick and “have”/grammar issues.Suggested cleanup:
-// Entitlements always returns an empty slice for users. +// Entitlements returns assignment entitlements for each privilege resource. @@ - entitlement.WithDisplayName(fmt.Sprintf("%s have %s`", collaboratorResourceType.DisplayName, resource.DisplayName)), + entitlement.WithDisplayName(fmt.Sprintf("%s has %s", collaboratorResourceType.DisplayName, resource.DisplayName)), @@ -// Grants always returns an empty slice for users since they don't have any entitlements. +// Grants returns collaborator grants for the given privilege; privilege grants for roles are handled on the role resource.This keeps docs and UI text coherent with the implementation.
Also applies to: 64-65
♻️ Duplicate comments (1)
pkg/connector/connector.go (1)
68-82: ValidateWorkatoDataCenterbefore indexingWorkatoDataCenters
dataCenterUrl := client.WorkatoDataCenters[config.WorkatoDataCenter]assumes the key is valid; a typo or bad config yields"", whichNewWorkatoClientaccepts and only fails later in surprising ways.Recommend guarding the lookup:
- dataCenterUrl := client.WorkatoDataCenters[config.WorkatoDataCenter] + dataCenterUrl, ok := client.WorkatoDataCenters[config.WorkatoDataCenter] + if !ok || dataCenterUrl == "" { + return nil, nil, fmt.Errorf("baton-workato invalid data center %q", config.WorkatoDataCenter) + }(and ensure
fmtis imported). This gives a clear, early error on misconfiguration.
🧹 Nitpick comments (5)
pkg/connector/collaborator_cache.go (2)
87-151: Consider consolidating the three getter functions into a generic helper.The
getUsersByPrivilege,getUsersByFolder, andgetUsersByRolefunctions follow an identical pattern (initialize cache, retrieve from session storage with a specific prefix, handle errors/not-found). This duplication could be eliminated with a single generic helper function.Example refactor:
+func (c *collaboratorCache) getUsersByKey(ctx context.Context, sessionStorage sessions.SessionStore, key string, prefix string) []*CompoundUser { + l := ctxzap.Extract(ctx) + + // Ensure cache is initialized before attempting to retrieve + if err := c.initializeCache(ctx, sessionStorage); err != nil { + l.Error("failed to ensure cache is initialized", zap.Error(err)) + return nil + } + + users, found, err := session.GetJSON[[]*CompoundUser](ctx, sessionStorage, key, sessions.WithPrefix(prefix)) + if err != nil { + l.Error("failed to get users from session storage", zap.String("prefix", prefix), zap.Error(err)) + return nil + } + + if !found { + return nil + } + + return users +} + func (c *collaboratorCache) getUsersByPrivilege(ctx context.Context, sessionStorage sessions.SessionStore, privilegeKey string) []*CompoundUser { - l := ctxzap.Extract(ctx) - - // Ensure cache is initialized before attempting to retrieve - if err := c.initializeCache(ctx, sessionStorage); err != nil { - l.Error("failed to ensure cache is initialized", zap.Error(err)) - return nil - } - - users, found, err := session.GetJSON[[]*CompoundUser](ctx, sessionStorage, privilegeKey, sessions.WithPrefix(privilegeToUserCachePrefix)) - if err != nil { - l.Error("failed to get users by privilege from session storage", zap.Error(err)) - return nil - } - - if !found { - return nil - } - - return users + return c.getUsersByKey(ctx, sessionStorage, privilegeKey, privilegeToUserCachePrefix) } func (c *collaboratorCache) getUsersByFolder(ctx context.Context, sessionStorage sessions.SessionStore, folderId string) []*CompoundUser { - l := ctxzap.Extract(ctx) - - // Ensure cache is initialized before attempting to retrieve - if err := c.initializeCache(ctx, sessionStorage); err != nil { - l.Error("failed to ensure cache is initialized", zap.Error(err)) - return nil - } - - users, found, err := session.GetJSON[[]*CompoundUser](ctx, sessionStorage, folderId, sessions.WithPrefix(folderToUserCachePrefix)) - if err != nil { - l.Error("failed to get users by folder from session storage", zap.Error(err)) - return nil - } - - if !found { - return nil - } - - return users + return c.getUsersByKey(ctx, sessionStorage, folderId, folderToUserCachePrefix) } func (c *collaboratorCache) getUsersByRole(ctx context.Context, sessionStorage sessions.SessionStore, roleName string) []*CompoundUser { - l := ctxzap.Extract(ctx) - - // Ensure cache is initialized before attempting to retrieve - if err := c.initializeCache(ctx, sessionStorage); err != nil { - l.Error("failed to ensure cache is initialized", zap.Error(err)) - return nil - } - - users, found, err := session.GetJSON[[]*CompoundUser](ctx, sessionStorage, roleName, sessions.WithPrefix(roleToUserCachePrefix)) - if err != nil { - l.Error("failed to get users by role from session storage", zap.Error(err)) - return nil - } - - if !found { - return nil - } - - return users + return c.getUsersByKey(ctx, sessionStorage, roleName, roleToUserCachePrefix) }
208-225: Remove redundant parentheses in length checks.The parentheses around
len(...)in the conditional checks are unnecessary.Apply this diff:
- if (len(privilegeToUsers)) > 0 { + if len(privilegeToUsers) > 0 { err := session.SetManyJSON(ctx, sessionStorage, privilegeToUsers, sessions.WithPrefix(privilegeToUserCachePrefix)) if err != nil { return fmt.Errorf("failed to set privilege to user cache in session storage: %w", err) } } - if (len(folderToUsers)) > 0 { + if len(folderToUsers) > 0 { err := session.SetManyJSON(ctx, sessionStorage, folderToUsers, sessions.WithPrefix(folderToUserCachePrefix)) if err != nil { return fmt.Errorf("failed to set folder to user cache in session storage: %w", err) } } - if (len(roleToUsers)) > 0 { + if len(roleToUsers) > 0 { err := session.SetManyJSON(ctx, sessionStorage, roleToUsers, sessions.WithPrefix(roleToUserCachePrefix)) if err != nil { return fmt.Errorf("failed to set role to user cache in session storage: %w", err) } }pkg/connector/role_cache.go (2)
20-50: Distinguish session errors from cache misses in role helpersBoth
getRoleByFolderandgetRoleByIdtreat anysession.GetJSONerror the same as a cache miss (log then returnnil). Callers (e.g.,roleBuilder.Grants) then convertnilinto a NotFound, which can mask real storage or serialization issues.Consider changing these helpers to return either
(value, found bool, err error)or(value, error)so callers can differentiate “role not cached/not found” from “session backend error” and surface a more appropriate status.
52-95: Minor: reuse cache prefix constants in setters
setRolesCachecurrently hardcodes"roles"and"folder_roles"instead of using the defined constants, which can drift if prefixes change.You can tighten this up with:
- err := session.SetManyJSON(ctx, sessionStorage, parseJSONRolesCache(roles), sessions.WithPrefix("roles")) + err := session.SetManyJSON(ctx, sessionStorage, parseJSONRolesCache(roles), sessions.WithPrefix(rolesCachePrefix)) ... - err := session.SetManyJSON(ctx, sessionStorage, mapRoles, sessions.WithPrefix("folder_roles")) + err := session.SetManyJSON(ctx, sessionStorage, mapRoles, sessions.WithPrefix(folderRolesCachePrefix))Also, the
roleCopypattern inparseJSONRolesCachecorrectly avoids the pointer-aliasing bug from the previous revision.pkg/connector/privilege.go (1)
65-92: Note on session usage in privilege Grants
privilegeBuilder.Grantsnow relies ono.cache.getUsersByPrivilege(ctx, attr.Session, privilegeId), mirroring the session-backed approach inroleBuilderandfolderBuilder. Once you add a defensiveattr.Sessionnil check inroleBuilderas suggested, it would be good to apply the same pattern here (or factor out a shared helper) to prevent nil-session panics across all collaborators/privilege grant paths.Also applies to: 94-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/baton-workato/main.gois excluded by none and included by none
📒 Files selected for processing (6)
pkg/connector/collaborator_cache.go(4 hunks)pkg/connector/connector.go(3 hunks)pkg/connector/folder.go(6 hunks)pkg/connector/privilege.go(4 hunks)pkg/connector/role.go(9 hunks)pkg/connector/role_cache.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/connector/collaborator_cache.go (3)
pkg/connector/client/client.go (1)
WorkatoClient(28-33)pkg/connector/workato/env.go (1)
Environment(10-10)pkg/connector/client/models.go (1)
Collaborator(35-49)
pkg/connector/connector.go (4)
pkg/connector/client/client.go (2)
WorkatoClient(28-33)NewWorkatoClient(35-61)pkg/connector/workato/env.go (2)
Environment(10-10)EnvFromString(22-33)pkg/config/conf.gen.go (1)
Workato(6-11)pkg/connector/client/helpers.go (1)
WorkatoDataCenters(18-24)
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/role.go (1)
pkg/connector/workato/privileges.go (2)
Privileges(31-430)FindRelatedPrivilegesErr(448-480)
pkg/connector/folder.go (1)
pkg/connector/cpagination/generic_bag.go (1)
GenBagFromToken(23-30)
🔇 Additional comments (10)
pkg/connector/collaborator_cache.go (7)
3-16: LGTM!The new imports for session storage (
session,sessions) and error formatting (fmt) are appropriate for the migration from in-memory to session-backed caching.
27-37: LGTM!The struct has been properly simplified to remove in-memory cache state and mutex, keeping only the client and environment fields needed for session-backed operations.
39-45: LGTM!The cache prefix constants provide clear namespacing for different cache types and the initialization marker, ensuring no key collisions in session storage.
47-54: LGTM!The cache initialization check is straightforward and properly handles errors from session storage operations.
56-85: LGTM!The initialization flow correctly checks for existing cache, fetches collaborators only when needed, and properly delegates to the cache-setting function with appropriate error handling.
208-225: Good use of bulk writes with SetManyJSON.The implementation now correctly uses
SetManyJSONto write all cache entries in bulk operations, addressing the concern from previous reviews about reducing network requests to session storage.Based on past review comments, this approach is more efficient than writing in a loop.
236-238: LGTM!The generic helper function provides a clean, type-safe way to append values to the cache maps.
pkg/connector/folder.go (1)
37-90: List: SyncOpAttrs/SyncOpResults migration looks consistentThe updated
Listimplementation usesattr.PageToken.TokenforGetProjects/GetFoldersand returnsNextPageTokenviars.SyncOpResults, which aligns with the new V2 patterns. TreatingparentResourceID == nilas a no-op forfolderBuilder(projects own the roots) also makes sense.pkg/connector/role.go (1)
26-31: Collaborator cache wiring for roles looks consistentAdding
cache *collaboratorCachetoroleBuilderand initializing it vianewCollaboratorCache(client, env)innewRoleBuildermatches how folder and privilege builders are using the shared collaborator cache. This keeps collaborator lookups centralized and session-backed.Also applies to: 278-283
pkg/connector/privilege.go (1)
32-49: Privilege List: SyncOpAttrs migration looks fine
Listignores the parent ID (as before), materializes all compound privileges, and now returns(*rs.SyncOpResults)(nil)which is appropriate for a non-paginated list. Usingrs.SyncOpAttrshere is consistent with the other builders.
| ) | ||
|
|
||
| DisableCustomRolesSync = field.BoolField( | ||
| "disable-custom-roles-sync", |
There was a problem hiding this comment.
just a heads up, this config name is not the same in c1 It'll need mapping
4 participants
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.