Add declarative resource support for groups#3010
Open
rajithacharith wants to merge 1 commit into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds YAML-backed declarative groups and a file-based store, a compositeGroupStore that merges DB+file reads (DB precedence) and routes writes to DB, service immutability guards for declarative groups, store-mode configuration/initialization, mock updates, and tests covering parsing, store merging, and declarative-mode behavior. ChangesDeclarative Groups Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant GroupService
participant CompositeStore
participant DBStore
participant FileStore
Client->>GroupService: GetGroupList(limit, offset)
GroupService->>CompositeStore: GetGroupList(limit, offset)
CompositeStore->>DBStore: GetGroupListCount / GetGroupList
DBStore-->>CompositeStore: DB results (may be partial)
CompositeStore->>FileStore: GetGroupListCount / GetGroupList (for missing/merge)
FileStore-->>CompositeStore: File results
CompositeStore->>CompositeStore: merge + dedupe (DB precedence, mark IsReadOnly)
CompositeStore-->>GroupService: merged results
GroupService-->>Client: return merged groups
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/group/composite_store.go`:
- Around line 209-217: The DB branch in composite_store.go currently only falls
back to the file store when c.dbStore.GetGroupMembers / GetGroupMemberCount
return an error, which masks declarative groups because DB may return empty
results with nil error; change the logic so that after calling
c.dbStore.GetGroupMembers(ctx, groupID, limit, offset) you return dbMembers only
if dbErr == nil AND len(dbMembers) > 0 (otherwise call
c.fileStore.GetGroupMembers and return its result), and similarly for
GetGroupMemberCount return the DB count only if dbErr == nil AND dbCount > 0
(otherwise call c.fileStore.GetGroupMemberCount and return that); ensure you
still propagate errors from both stores when both fail.
In `@backend/internal/group/config.go`:
- Around line 40-46: The current resolution of cfg.Group.Store silently falls
back to the global/default when a non-empty, misspelled value is provided;
change the resolution function to fail fast by returning an error for invalid
values. Update the function that reads cfg.Group.Store to return
(serverconst.StoreMode, error) instead of only a mode, validate
strings.ToLower(strings.TrimSpace(cfg.Group.Store)) and if non-empty and not one
of serverconst.StoreModeMutable, serverconst.StoreModeDeclarative, or
serverconst.StoreModeComposite return a descriptive error (mentioning the
invalid value and allowed values); keep the existing successful returns for the
three valid modes and preserve existing behavior when cfg.Group.Store is empty
by returning the default mode with nil error. Ensure callers of this resolver
(initialization path) propagate the error and abort startup on error.
In `@backend/internal/group/declarative_resource.go`:
- Around line 255-259: The current branch hides the original error from
resolveGroupOUHandle(grp, ouService) by returning a generic "not found" message;
change the return to wrap the original error (use fmt.Errorf with %w) so callers
see whether the failure was an actual "not found" vs a transport/backend error
while still including the descriptive message that mentions grp.OUHandle and
grp.Name.
- Around line 272-276: The current dbStore.GetGroup call treats any error as
“group not found” and skips duplicate-ID validation; change the logic so that
after calling dbStore.GetGroup(ctx, grp.ID) you only ignore the error when it is
an explicit not-found (use the project's NotFound check/ sentinel or error
type), but for any other non-nil err return/propagate that error. Keep the
existing duplicate check (err == nil && grpDAO.ID != "") to return the
duplicate-ID fmt.Errorf; do not swallow transient DB errors from GetGroup.
- Around line 219-223: The YAML unmarshal currently uses yaml.Unmarshal which
ignores unknown fields; update parseToGroup to use yaml.NewDecoder with
KnownFields(true) to reject unknown keys, decode into the existing
groupDeclarativeResource (grp), and return the error from dec.Decode; after
decoding, iterate grp.Members and convert public member types to the internal
entity type by checking m.Type.IsEntityType() and setting grp.Members[i].Type =
memberTypeEntity to preserve existing normalization behavior.
In `@backend/internal/group/model.go`:
- Line 67: The IsReadOnly boolean added to the group model is not being copied
into API responses; update the DAO→API mapping helpers in
backend/internal/group/model.go to propagate IsReadOnly from the DAO struct into
the API struct (assign DAO.IsReadOnly -> API.IsReadOnly) inside the functions
that build API group objects (the mapping/builder helpers around names like
MapGroupDAOToAPI, BuildGroupResponse, or similar mapper functions in this file),
and ensure the same propagation is added for all variants noted (the other
mapper blocks around the same file sections).
In `@backend/internal/group/service_test.go`:
- Around line 82-90: The test suite initializes a global runtime in SetupSuite
but never resets it; add a TearDownSuite method on GroupServiceTestSuite that
calls config.ResetServerRuntime() to clear global state after tests complete,
ensuring the runtime is reset even if SetupSuite created it; reference the
existing SetupSuite and use the same config.ResetServerRuntime function to
perform the cleanup.
In `@backend/internal/system/config/config.go`:
- Around line 487-495: The PR adds a new GroupConfig.Store ("group.store")
feature and new error codes but lacks docs; update
docs/content/guides/getting-started/configuration.mdx to document group.store
valid values ("mutable","declarative","composite"), fallback behavior tied to
DeclarativeResources.Enabled and include examples, update API docs (e.g.,
docs/content/apis.mdx and relevant guides) to describe group API immutability
constraints in declarative/composite modes (create/update/delete/member mutation
paths), and add the new error semantics/codes GRP-1015 and GRP-1016 to the API
error reference so users can find the behaviors introduced by GroupConfig and
Group store modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed43488a-b597-4d04-8fe7-c96e3518e910
⛔ Files ignored due to path filters (2)
backend/tests/mocks/groupmock/GroupServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/groupmock/groupStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (16)
backend/internal/group/GroupServiceInterface_mock_test.gobackend/internal/group/composite_store.gobackend/internal/group/config.gobackend/internal/group/declarative_resource.gobackend/internal/group/declarative_resource_test.gobackend/internal/group/error_constants.gobackend/internal/group/file_based_store.gobackend/internal/group/groupStoreInterface_mock_test.gobackend/internal/group/init.gobackend/internal/group/model.gobackend/internal/group/service.gobackend/internal/group/service_declarative_mode_test.gobackend/internal/group/service_test.gobackend/internal/group/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
rajithacharith
force-pushed
the
immutable-groups
branch
from
163c186 to
16ac865
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/internal/group/service_test.go (1)
82-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd suite-level runtime cleanup to avoid global state leakage.
SetupSuiteresets and initializes global runtime, but there is no matchingTearDownSuitereset in this suite.Proposed fix
func (suite *GroupServiceTestSuite) SetupSuite() { testConfig := &config.Config{ DeclarativeResources: config.DeclarativeResources{Enabled: false}, } config.ResetServerRuntime() if err := config.InitializeServerRuntime("/tmp/test", testConfig); err != nil { suite.Fail("Failed to initialize runtime", err) } } + +func (suite *GroupServiceTestSuite) TearDownSuite() { + config.ResetServerRuntime() +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/group/service_test.go` around lines 82 - 90, The suite currently calls config.ResetServerRuntime() and config.InitializeServerRuntime() in SetupSuite but never restores global state; add a TearDownSuite method on GroupServiceTestSuite that calls config.ResetServerRuntime() to clean up the runtime after tests. Implement TearDownSuite on the same suite type (GroupServiceTestSuite) and ensure it runs teardown logic to avoid global state leakage from the SetupSuite use of config.InitializeServerRuntime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/internal/group/service_test.go`:
- Around line 82-90: The suite currently calls config.ResetServerRuntime() and
config.InitializeServerRuntime() in SetupSuite but never restores global state;
add a TearDownSuite method on GroupServiceTestSuite that calls
config.ResetServerRuntime() to clean up the runtime after tests. Implement
TearDownSuite on the same suite type (GroupServiceTestSuite) and ensure it runs
teardown logic to avoid global state leakage from the SetupSuite use of
config.InitializeServerRuntime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7fe7edf-d98d-47b3-9423-caef194cb912
⛔ Files ignored due to path filters (2)
backend/tests/mocks/groupmock/GroupServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/groupmock/groupStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (17)
backend/internal/group/GroupServiceInterface_mock_test.gobackend/internal/group/composite_store.gobackend/internal/group/config.gobackend/internal/group/declarative_resource.gobackend/internal/group/declarative_resource_test.gobackend/internal/group/error_constants.gobackend/internal/group/file_based_store.gobackend/internal/group/groupStoreInterface_mock_test.gobackend/internal/group/init.gobackend/internal/group/model.gobackend/internal/group/service.gobackend/internal/group/service_declarative_mode_test.gobackend/internal/group/service_test.gobackend/internal/group/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/internal/system/i18n/core/defaults.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/system/i18n/core/defaults.go
- backend/internal/group/groupStoreInterface_mock_test.go
rajithacharith
force-pushed
the
immutable-groups
branch
from
16ac865 to
366a90f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/internal/group/declarative_resource.go (1)
74-81: ⚡ Quick winUse
GroupBasic.IsReadOnlyhere instead of a second service lookup.
GetGroupListalready returns the read-only bit, so this loop turns export into N+1IsGroupDeclarativecalls and can fail after a successful page fetch. Filter on the page data you already have.♻️ Suggested simplification
for _, g := range groups.Groups { - isDeclarative, svcErr := e.service.IsGroupDeclarative(ctx, g.ID) - if svcErr != nil { - return nil, svcErr - } - if !isDeclarative { + if !g.IsReadOnly { ids = append(ids, g.ID) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/group/declarative_resource.go` around lines 74 - 81, The loop is doing N+1 IsGroupDeclarative calls even though the page already contains the read-only flag; replace the svc call by using the GroupBasic.IsReadOnly field on each g from groups.Groups: remove the IsGroupDeclarative call and svcErr handling, and instead append g.ID to ids only when g.IsReadOnly is false (or !g.IsReadOnly). Keep the rest of the logic unchanged so you no longer call e.service.IsGroupDeclarative for each group.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/group/composite_store.go`:
- Around line 211-224: The current fallback returns fileStore results even when
fileStore returns an empty nil error, which can mask dbStore errors; update the
GetGroupMembers logic to only accept and return fileStore.GetGroupMembers(ctx,
groupID, limit, offset) when fileErr == nil AND len(fileMembers) > 0 (i.e., the
group exists declaratively in the file store); otherwise if dbErr != nil
propagate and return dbErr, and only return fileErr when dbErr is nil and
fileErr != nil (or return the non-empty fileMembers when present). Reference
dbStore.GetGroupMembers, fileStore.GetGroupMembers and the enclosing
GetGroupMembers method in composite_store.go to locate and apply this change.
In `@backend/internal/group/service.go`:
- Around line 461-466: The current early-return on gs.IsGroupDeclarative leaks
declarative/non-declarative distinction before authorization; refactor the flow
in the update path that calls gs.IsGroupDeclarative and returns
&ErrorImmutableGroup so that you first perform the authorized GetGroup (the
existing authorization/read check used by GetGroup) and only after a successful,
authorized GetGroup call check gs.IsGroupDeclarative and then return
&ErrorImmutableGroup. Apply the same change to the other spots where you call
gs.IsGroupDeclarative and return ErrorImmutableGroup early (the other
update/read handlers with the same pattern) so unauthorized callers continue to
receive the standard access error from GetGroup instead of the immutable-group
error.
---
Nitpick comments:
In `@backend/internal/group/declarative_resource.go`:
- Around line 74-81: The loop is doing N+1 IsGroupDeclarative calls even though
the page already contains the read-only flag; replace the svc call by using the
GroupBasic.IsReadOnly field on each g from groups.Groups: remove the
IsGroupDeclarative call and svcErr handling, and instead append g.ID to ids only
when g.IsReadOnly is false (or !g.IsReadOnly). Keep the rest of the logic
unchanged so you no longer call e.service.IsGroupDeclarative for each group.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 97c9c689-31ea-4667-b526-d252b9292626
⛔ Files ignored due to path filters (2)
backend/tests/mocks/groupmock/GroupServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/groupmock/groupStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (17)
backend/internal/group/GroupServiceInterface_mock_test.gobackend/internal/group/composite_store.gobackend/internal/group/config.gobackend/internal/group/declarative_resource.gobackend/internal/group/declarative_resource_test.gobackend/internal/group/error_constants.gobackend/internal/group/file_based_store.gobackend/internal/group/groupStoreInterface_mock_test.gobackend/internal/group/init.gobackend/internal/group/model.gobackend/internal/group/service.gobackend/internal/group/service_declarative_mode_test.gobackend/internal/group/service_test.gobackend/internal/group/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/internal/system/i18n/core/defaults.go
✅ Files skipped from review due to trivial changes (1)
- backend/internal/group/GroupServiceInterface_mock_test.go
Comment on lines
+461
to
+466
| if isDeclarative, svcErr := gs.IsGroupDeclarative(ctx, groupID); svcErr != nil { | ||
| return nil, svcErr | ||
| } else if isDeclarative { | ||
| logger.Debug("Cannot update declarative group", log.String("id", groupID)) | ||
| return nil, &ErrorImmutableGroup | ||
| } |
Contributor
There was a problem hiding this comment.
Authorize before returning ErrorImmutableGroup.
These early exits let callers distinguish declarative IDs from other inaccessible groups, because immutable groups now fail before the normal read/update authorization path. Move the declarative/read-only check after the authorized GetGroup lookup so unauthorized callers still get the usual access response.
Also applies to: 570-575, 819-824
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/group/service.go` around lines 461 - 466, The current
early-return on gs.IsGroupDeclarative leaks declarative/non-declarative
distinction before authorization; refactor the flow in the update path that
calls gs.IsGroupDeclarative and returns &ErrorImmutableGroup so that you first
perform the authorized GetGroup (the existing authorization/read check used by
GetGroup) and only after a successful, authorized GetGroup call check
gs.IsGroupDeclarative and then return &ErrorImmutableGroup. Apply the same
change to the other spots where you call gs.IsGroupDeclarative and return
ErrorImmutableGroup early (the other update/read handlers with the same pattern)
so unauthorized callers continue to receive the standard access error from
GetGroup instead of the immutable-group error.
366a90f to
5c06400
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/group/file_based_store_edge_cases_test.go`:
- Around line 162-185: The test TestGetGroupMembers_PaginationOrder currently
only checks lengths; update it to verify ordering by comparing member IDs across
pages with the full results: after calling suite.seedGroup(...) and retrieving
all := suite.store.GetGroupMembers(..., 3, 0), extract the ordered IDs from all,
then for page1 := suite.store.GetGroupMembers(..., 2, 0) and page2 :=
suite.store.GetGroupMembers(..., 2, 2) extract their IDs and assert that page1
IDs == first two IDs of all and page2 IDs == remaining IDs; use the existing
suite.store.GetGroupMembers, seedGroup and the test assertions
(assert.NoError/assert.Len/assert.Equal) to implement these ID comparisons.
- Around line 309-319: The test TestGetGroupList_SkipsMalformedEntries currently
discards the returned groups; update it so it actually verifies malformed
entries are skipped: after seeding grp1 and creating the malformed file via
suite.store.GenericFileBasedStore.Create("malformed", "not a group"), call
groups, err := suite.store.GetGroupList(...), assert err is nil, assert
len(groups) == 1, and assert the returned group's ID == "grp1" and Name ==
"Valid" (or other expected fields), then remove the unused `_ = groups` line to
avoid dead code; keep references to suite.seedGroup,
GenericFileBasedStore.Create, and GetGroupList to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35001a38-a47e-4698-b420-9932152b5013
⛔ Files ignored due to path filters (2)
backend/tests/mocks/groupmock/GroupServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/groupmock/groupStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (21)
backend/internal/group/GroupServiceInterface_mock_test.gobackend/internal/group/composite_store.gobackend/internal/group/composite_store_edge_cases_test.gobackend/internal/group/composite_store_test.gobackend/internal/group/config.gobackend/internal/group/declarative_resource.gobackend/internal/group/declarative_resource_test.gobackend/internal/group/error_constants.gobackend/internal/group/file_based_store.gobackend/internal/group/file_based_store_edge_cases_test.gobackend/internal/group/file_based_store_test.gobackend/internal/group/groupStoreInterface_mock_test.gobackend/internal/group/init.gobackend/internal/group/model.gobackend/internal/group/service.gobackend/internal/group/service_declarative_mode_test.gobackend/internal/group/service_test.gobackend/internal/group/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/internal/system/i18n/core/defaults.go
✅ Files skipped from review due to trivial changes (1)
- backend/internal/group/groupStoreInterface_mock_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- backend/internal/system/declarative_resource/entity/entity_store.go
- backend/internal/group/store.go
- backend/internal/group/GroupServiceInterface_mock_test.go
- backend/internal/group/error_constants.go
- backend/internal/group/service_test.go
- backend/internal/group/init.go
- backend/internal/system/config/config.go
- backend/internal/group/model.go
- backend/internal/group/config.go
- backend/internal/group/service_declarative_mode_test.go
- backend/internal/group/declarative_resource_test.go
- backend/internal/group/service.go
- backend/internal/group/declarative_resource.go
- backend/internal/group/file_based_store.go
- backend/internal/group/composite_store.go
- backend/internal/system/i18n/core/defaults.go
Comment on lines
+162
to
+185
| func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupMembers_PaginationOrder() { | ||
| suite.seedGroup(groupDeclarativeResource{ | ||
| ID: "grp1", | ||
| Name: "Admins", | ||
| OUID: "ou1", | ||
| Members: []Member{ | ||
| {ID: "user1", Type: memberTypeEntity}, | ||
| {ID: "user2", Type: memberTypeEntity}, | ||
| {ID: "user3", Type: memberTypeEntity}, | ||
| }, | ||
| }) | ||
|
|
||
| all, err := suite.store.GetGroupMembers(context.Background(), "grp1", 3, 0) | ||
| assert.NoError(suite.T(), err) | ||
| assert.Len(suite.T(), all, 3) | ||
|
|
||
| page1, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 0) | ||
| assert.NoError(suite.T(), err) | ||
| assert.Len(suite.T(), page1, 2) | ||
|
|
||
| page2, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 2) | ||
| assert.NoError(suite.T(), err) | ||
| assert.Len(suite.T(), page2, 1) | ||
| } |
Contributor
There was a problem hiding this comment.
TestGetGroupMembers_PaginationOrder doesn’t validate order yet.
This test currently only checks page sizes, so ordering regressions would still pass. Assert member IDs across pages against the full list.
Suggested fix
func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupMembers_PaginationOrder() {
@@
page2, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 2)
assert.NoError(suite.T(), err)
assert.Len(suite.T(), page2, 1)
+
+ assert.Equal(suite.T(), all[0].ID, page1[0].ID)
+ assert.Equal(suite.T(), all[1].ID, page1[1].ID)
+ assert.Equal(suite.T(), all[2].ID, page2[0].ID)
}📝 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.
Suggested change
| func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupMembers_PaginationOrder() { | |
| suite.seedGroup(groupDeclarativeResource{ | |
| ID: "grp1", | |
| Name: "Admins", | |
| OUID: "ou1", | |
| Members: []Member{ | |
| {ID: "user1", Type: memberTypeEntity}, | |
| {ID: "user2", Type: memberTypeEntity}, | |
| {ID: "user3", Type: memberTypeEntity}, | |
| }, | |
| }) | |
| all, err := suite.store.GetGroupMembers(context.Background(), "grp1", 3, 0) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), all, 3) | |
| page1, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 0) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), page1, 2) | |
| page2, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 2) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), page2, 1) | |
| } | |
| func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupMembers_PaginationOrder() { | |
| suite.seedGroup(groupDeclarativeResource{ | |
| ID: "grp1", | |
| Name: "Admins", | |
| OUID: "ou1", | |
| Members: []Member{ | |
| {ID: "user1", Type: memberTypeEntity}, | |
| {ID: "user2", Type: memberTypeEntity}, | |
| {ID: "user3", Type: memberTypeEntity}, | |
| }, | |
| }) | |
| all, err := suite.store.GetGroupMembers(context.Background(), "grp1", 3, 0) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), all, 3) | |
| page1, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 0) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), page1, 2) | |
| page2, err := suite.store.GetGroupMembers(context.Background(), "grp1", 2, 2) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), page2, 1) | |
| assert.Equal(suite.T(), all[0].ID, page1[0].ID) | |
| assert.Equal(suite.T(), all[1].ID, page1[1].ID) | |
| assert.Equal(suite.T(), all[2].ID, page2[0].ID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/group/file_based_store_edge_cases_test.go` around lines 162
- 185, The test TestGetGroupMembers_PaginationOrder currently only checks
lengths; update it to verify ordering by comparing member IDs across pages with
the full results: after calling suite.seedGroup(...) and retrieving all :=
suite.store.GetGroupMembers(..., 3, 0), extract the ordered IDs from all, then
for page1 := suite.store.GetGroupMembers(..., 2, 0) and page2 :=
suite.store.GetGroupMembers(..., 2, 2) extract their IDs and assert that page1
IDs == first two IDs of all and page2 IDs == remaining IDs; use the existing
suite.store.GetGroupMembers, seedGroup and the test assertions
(assert.NoError/assert.Len/assert.Equal) to implement these ID comparisons.
Comment on lines
+309
to
+319
| // Test malformed data in store is skipped gracefully. | ||
| func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupList_SkipsMalformedEntries() { | ||
| suite.seedGroup(groupDeclarativeResource{ID: "grp1", Name: "Valid", OUID: "ou1"}) | ||
|
|
||
| _ = suite.store.GenericFileBasedStore.Create("malformed", "not a group") | ||
|
|
||
| groups, err := suite.store.GetGroupList(context.Background(), 10, 0) | ||
|
|
||
| assert.Nil(suite.T(), err) | ||
| _ = groups | ||
| } |
Contributor
There was a problem hiding this comment.
TestGetGroupList_SkipsMalformedEntries is not asserting the intended behavior.
Line 318 discards groups, so this can pass even if malformed entries break filtering. Assert that the valid record is returned and malformed data is skipped.
Suggested fix
func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupList_SkipsMalformedEntries() {
@@
groups, err := suite.store.GetGroupList(context.Background(), 10, 0)
- assert.Nil(suite.T(), err)
- _ = groups
+ assert.NoError(suite.T(), err)
+ assert.Len(suite.T(), groups, 1)
+ assert.Equal(suite.T(), "grp1", groups[0].ID)
}📝 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.
Suggested change
| // Test malformed data in store is skipped gracefully. | |
| func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupList_SkipsMalformedEntries() { | |
| suite.seedGroup(groupDeclarativeResource{ID: "grp1", Name: "Valid", OUID: "ou1"}) | |
| _ = suite.store.GenericFileBasedStore.Create("malformed", "not a group") | |
| groups, err := suite.store.GetGroupList(context.Background(), 10, 0) | |
| assert.Nil(suite.T(), err) | |
| _ = groups | |
| } | |
| // Test malformed data in store is skipped gracefully. | |
| func (suite *GroupFileBasedStoreEdgeCaseTestSuite) TestGetGroupList_SkipsMalformedEntries() { | |
| suite.seedGroup(groupDeclarativeResource{ID: "grp1", Name: "Valid", OUID: "ou1"}) | |
| _ = suite.store.GenericFileBasedStore.Create("malformed", "not a group") | |
| groups, err := suite.store.GetGroupList(context.Background(), 10, 0) | |
| assert.NoError(suite.T(), err) | |
| assert.Len(suite.T(), groups, 1) | |
| assert.Equal(suite.T(), "grp1", groups[0].ID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/group/file_based_store_edge_cases_test.go` around lines 309
- 319, The test TestGetGroupList_SkipsMalformedEntries currently discards the
returned groups; update it so it actually verifies malformed entries are
skipped: after seeding grp1 and creating the malformed file via
suite.store.GenericFileBasedStore.Create("malformed", "not a group"), call
groups, err := suite.store.GetGroupList(...), assert err is nil, assert
len(groups) == 1, and assert the returned group's ID == "grp1" and Name ==
"Valid" (or other expected fields), then remove the unused `_ = groups` line to
avoid dead code; keep references to suite.seedGroup,
GenericFileBasedStore.Create, and GetGroupList to locate the change.
rajithacharith
force-pushed
the
immutable-groups
branch
from
5c06400 to
f6eee43
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
backend/internal/system/config/config.go (1)
487-495:⚠️ Potential issue | 🟠 Major | ⚡ Quick win🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates underdocs/.
Please update the relevant documentation before merging.Missing documentation:
group.storeconfiguration: document valid values (mutable,declarative,composite), fallback behavior, and examples indocs/content/guides/getting-started/configuration.mdx.- Group API behavior in declarative/composite modes: document create/update/delete immutability behavior and read-only semantics in API/guide docs (for example
docs/content/apis.mdxand/or guides underdocs/content/guides/).- Declarative-mode error responses for group mutations: document the error semantics returned by create/update/delete flows in API error reference (
docs/content/apis.mdx).As per coding guidelines: "If ANY of the above are detected and the PR does NOT include corresponding updates under
docs/, post a single consolidated PR-level comment."Also applies to: 727-727
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/system/config/config.go` around lines 487 - 495, Update the documentation to cover the new GroupConfig.Store behavior: add a section in docs/content/guides/getting-started/configuration.mdx describing the group.store key (valid values "mutable", "declarative", "composite"), its fallback logic to DeclarativeResources.Enabled, and include a short YAML example showing each mode; update docs/content/apis.mdx (and related guides under docs/content/guides/) to describe Group API behavior under declarative and composite modes (immutability/read-only semantics for create/update/delete) and document the exact error responses/semantics returned by group mutation endpoints when in declarative mode so API consumers know expected status codes and messages.backend/internal/group/service.go (1)
461-466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthorize before returning immutable-group errors.
At Line 461 and Line 570, declarative checks run before the authorized group lookup. This lets unauthorized callers distinguish declarative groups from other inaccessible groups.
Move the immutable check to run only after the existing authorized read/access check in each flow, then return
ErrorImmutableGroupfor authorized callers only.Also applies to: 570-575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/group/service.go` around lines 461 - 466, The declarative-group check using gs.IsGroupDeclarative and returning &ErrorImmutableGroup is running before the authorized-group lookup, leaking info to unauthorized callers; move the IsGroupDeclarative check so it executes only after the existing authorized read/access check for the flow that does updates (the block containing the current gs.IsGroupDeclarative at the top) and the flow around lines 570–575, and only return ErrorImmutableGroup to callers who have already been authorized (i.e., after the function that performs the authorized lookup or access check completes successfully); update both locations to call IsGroupDeclarative after authorization and keep returning svcErr for authorization failures as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/internal/group/service.go`:
- Around line 461-466: The declarative-group check using gs.IsGroupDeclarative
and returning &ErrorImmutableGroup is running before the authorized-group
lookup, leaking info to unauthorized callers; move the IsGroupDeclarative check
so it executes only after the existing authorized read/access check for the flow
that does updates (the block containing the current gs.IsGroupDeclarative at the
top) and the flow around lines 570–575, and only return ErrorImmutableGroup to
callers who have already been authorized (i.e., after the function that performs
the authorized lookup or access check completes successfully); update both
locations to call IsGroupDeclarative after authorization and keep returning
svcErr for authorization failures as before.
In `@backend/internal/system/config/config.go`:
- Around line 487-495: Update the documentation to cover the new
GroupConfig.Store behavior: add a section in
docs/content/guides/getting-started/configuration.mdx describing the group.store
key (valid values "mutable", "declarative", "composite"), its fallback logic to
DeclarativeResources.Enabled, and include a short YAML example showing each
mode; update docs/content/apis.mdx (and related guides under
docs/content/guides/) to describe Group API behavior under declarative and
composite modes (immutability/read-only semantics for create/update/delete) and
document the exact error responses/semantics returned by group mutation
endpoints when in declarative mode so API consumers know expected status codes
and messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b0f2538-a3c4-41ea-95e4-1ab546fc1c9e
⛔ Files ignored due to path filters (2)
backend/tests/mocks/groupmock/GroupServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/groupmock/groupStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (21)
backend/internal/group/GroupServiceInterface_mock_test.gobackend/internal/group/composite_store.gobackend/internal/group/composite_store_edge_cases_test.gobackend/internal/group/composite_store_test.gobackend/internal/group/config.gobackend/internal/group/declarative_resource.gobackend/internal/group/declarative_resource_test.gobackend/internal/group/error_constants.gobackend/internal/group/file_based_store.gobackend/internal/group/file_based_store_edge_cases_test.gobackend/internal/group/file_based_store_test.gobackend/internal/group/groupStoreInterface_mock_test.gobackend/internal/group/init.gobackend/internal/group/model.gobackend/internal/group/service.gobackend/internal/group/service_declarative_mode_test.gobackend/internal/group/service_test.gobackend/internal/group/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/internal/system/i18n/core/defaults.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/group/groupStoreInterface_mock_test.go
- backend/internal/group/GroupServiceInterface_mock_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- backend/internal/group/error_constants.go
- backend/internal/system/declarative_resource/entity/entity_store.go
- backend/internal/group/model.go
- backend/internal/group/service_test.go
- backend/internal/system/i18n/core/defaults.go
- backend/internal/group/store.go
- backend/internal/group/file_based_store_edge_cases_test.go
- backend/internal/group/config.go
- backend/internal/group/init.go
- backend/internal/group/composite_store_test.go
- backend/internal/group/file_based_store_test.go
- backend/internal/group/declarative_resource_test.go
- backend/internal/group/declarative_resource.go
- backend/internal/group/file_based_store.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Add declarative resources support for groups
Approach
This pull request introduces several enhancements and refactorings related to group management, especially around declarative resource support and configuration. The main changes include improved handling of declarative groups, new error definitions, refactoring of resource loading and validation logic, and expanded test coverage for these features.
Declarative group support and configuration
config.gofile to encapsulate logic for determining group store mode (mutable, declarative, composite) and checking if declarative-only mode is enabled.IsGroupDeclarativemethod to theGroupServiceInterfaceMock, enabling tests to distinguish between declarative and mutable groups.Declarative resource loading and validation
ou_handle, improved member type translation, and added duplicate ID checks against both file and DB stores. IntroducedloadDeclarativeResources,parseToGroup, andvalidateGroupWrapperfunctions for modularity and testability.GetAllResourceIDsto exclude declarative groups from exports, ensuring only mutable (DB) groups are included. [1] [2]Error handling improvements
Testing enhancements
declarative_resource_test.goto verify parsing, validation, exclusion of declarative groups, error handling, and member type translation. [1] [2] [3] [4]Dependency and import updates
These changes collectively improve the robustness, configurability, and testability of group management with respect to declarative resources.
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Documentation