From 01ff85b60e8fa69d26bcb89f854a395906d59f60 Mon Sep 17 00:00:00 2001 From: Amruthan Manjula Anandan <157590530+rbiamru@users.noreply.github.com> Date: Wed, 3 Sep 2025 23:45:48 -0400 Subject: [PATCH 1/4] Fix: normalize group names to be case-insensitive in manager.go --- pkg/groups/group_test.go | 71 ++++++++++++++++++++++++++++++++++++++++ pkg/groups/manager.go | 8 +++++ 2 files changed, 79 insertions(+) diff --git a/pkg/groups/group_test.go b/pkg/groups/group_test.go index 8866c2fcf..734516147 100644 --- a/pkg/groups/group_test.go +++ b/pkg/groups/group_test.go @@ -635,3 +635,74 @@ func (m *mockWriteCloser) Write(p []byte) (n int, err error) { func (*mockWriteCloser) Close() error { return nil } + + +// TestManager_GroupNameCaseInsensitive tests that group names are normalized +// and treated case-insensitively. +func TestManager_GroupNameCaseInsensitive(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + inputName string + existing string + setupMock func(*mocks.MockStore, string) + expectError bool + }{ + { + name: "exists with different case", + inputName: "MyGroup", + existing: "mygroup", + setupMock: func(mock *mocks.MockStore, existing string) { + // Manager should lowercase input, so Exists is called with "mygroup" + mock.EXPECT(). + Exists(gomock.Any(), existing). + Return(true, nil) + }, + expectError: false, + }, + { + name: "create duplicate with different case", + inputName: "MyGroup", + existing: "mygroup", + setupMock: func(mock *mocks.MockStore, existing string) { + // Manager should lowercase input, so Exists is called with "mygroup" + mock.EXPECT(). + Exists(gomock.Any(), existing). + Return(true, nil) + }, + expectError: true, // expect duplicate error + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := mocks.NewMockStore(ctrl) + manager := &manager{groupStore: mockStore} + + // set up the mock for this test + tt.setupMock(mockStore, tt.existing) + + if tt.expectError { + err := manager.Create(context.Background(), tt.inputName) + if err == nil { + t.Errorf("expected error when creating duplicate with case-insensitive name, got nil") + } + } else { + exists, err := manager.Exists(context.Background(), tt.inputName) + if err != nil { + t.Fatalf("exists check failed: %v", err) + } + if !exists { + t.Errorf("expected group %q to exist regardless of case", tt.inputName) + } + } + }) + } +} + diff --git a/pkg/groups/manager.go b/pkg/groups/manager.go index 5d9ec4069..6b6d3b4cd 100644 --- a/pkg/groups/manager.go +++ b/pkg/groups/manager.go @@ -34,6 +34,8 @@ func NewManager() (Manager, error) { // Create creates a new group with the given name func (m *manager) Create(ctx context.Context, name string) error { + // Lowercase the group name before checking for existence + name = strings.ToLower(name) // Check if group already exists exists, err := m.groupStore.Exists(ctx, name) if err != nil { @@ -52,6 +54,8 @@ func (m *manager) Create(ctx context.Context, name string) error { // Get retrieves a group by name func (m *manager) Get(ctx context.Context, name string) (*Group, error) { + // + name = strings.ToLower(name) reader, err := m.groupStore.GetReader(ctx, name) if err != nil { return nil, fmt.Errorf("failed to get reader for group: %w", err) @@ -92,11 +96,15 @@ func (m *manager) List(ctx context.Context) ([]*Group, error) { // Delete removes a group by name func (m *manager) Delete(ctx context.Context, name string) error { + // + name = strings.ToLower(name) return m.groupStore.Delete(ctx, name) } // Exists checks if a group exists func (m *manager) Exists(ctx context.Context, name string) (bool, error) { + // + name = strings.ToLower(name) return m.groupStore.Exists(ctx, name) } From 2e35eb651962406a077969a3beeec9a29f1b8c0a Mon Sep 17 00:00:00 2001 From: Amruthan Manjula Anandan <157590530+rbiamru@users.noreply.github.com> Date: Thu, 4 Sep 2025 00:10:03 -0400 Subject: [PATCH 2/4] Fixed comments --- pkg/groups/group_test.go | 3 +-- pkg/groups/manager.go | 3 --- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/groups/group_test.go b/pkg/groups/group_test.go index 734516147..53898a447 100644 --- a/pkg/groups/group_test.go +++ b/pkg/groups/group_test.go @@ -637,8 +637,7 @@ func (*mockWriteCloser) Close() error { } -// TestManager_GroupNameCaseInsensitive tests that group names are normalized -// and treated case-insensitively. +// TestManager_GroupNameCaseInsensitive tests that group names are normalized and treated case-insensitively. func TestManager_GroupNameCaseInsensitive(t *testing.T) { t.Parallel() diff --git a/pkg/groups/manager.go b/pkg/groups/manager.go index 6b6d3b4cd..8a8113991 100644 --- a/pkg/groups/manager.go +++ b/pkg/groups/manager.go @@ -54,7 +54,6 @@ func (m *manager) Create(ctx context.Context, name string) error { // Get retrieves a group by name func (m *manager) Get(ctx context.Context, name string) (*Group, error) { - // name = strings.ToLower(name) reader, err := m.groupStore.GetReader(ctx, name) if err != nil { @@ -96,14 +95,12 @@ func (m *manager) List(ctx context.Context) ([]*Group, error) { // Delete removes a group by name func (m *manager) Delete(ctx context.Context, name string) error { - // name = strings.ToLower(name) return m.groupStore.Delete(ctx, name) } // Exists checks if a group exists func (m *manager) Exists(ctx context.Context, name string) (bool, error) { - // name = strings.ToLower(name) return m.groupStore.Exists(ctx, name) } From d33020b09da1b4e97a22f2c16e874a3cd2010a5a Mon Sep 17 00:00:00 2001 From: Amruthan Manjula Anandan <157590530+rbiamru@users.noreply.github.com> Date: Thu, 4 Sep 2025 10:49:35 -0400 Subject: [PATCH 3/4] Enforce lowercase validation for group names with error message --- pkg/groups/group_test.go | 88 +++++++++------------------------------- pkg/groups/manager.go | 28 ++++++++++--- 2 files changed, 43 insertions(+), 73 deletions(-) diff --git a/pkg/groups/group_test.go b/pkg/groups/group_test.go index 53898a447..1e57e0ba8 100644 --- a/pkg/groups/group_test.go +++ b/pkg/groups/group_test.go @@ -82,6 +82,26 @@ func TestManager_Create(t *testing.T) { expectError: true, errorMsg: "failed to get writer for group", }, + { + name: "invalid name - uppercase", + groupName: "MyGroup", + setupMock: func(mock *mocks.MockStore) { + // validation should fail before touching the store, + // so no expectations needed + }, + expectError: true, + errorMsg: "invalid group name", + }, + { + name: "invalid name - mixed case", + groupName: "DefAult", + setupMock: func(mock *mocks.MockStore) { + // same as above: no store calls + }, + expectError: true, + errorMsg: "invalid group name", + }, + } for _, tt := range tests { @@ -637,71 +657,3 @@ func (*mockWriteCloser) Close() error { } -// TestManager_GroupNameCaseInsensitive tests that group names are normalized and treated case-insensitively. -func TestManager_GroupNameCaseInsensitive(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - inputName string - existing string - setupMock func(*mocks.MockStore, string) - expectError bool - }{ - { - name: "exists with different case", - inputName: "MyGroup", - existing: "mygroup", - setupMock: func(mock *mocks.MockStore, existing string) { - // Manager should lowercase input, so Exists is called with "mygroup" - mock.EXPECT(). - Exists(gomock.Any(), existing). - Return(true, nil) - }, - expectError: false, - }, - { - name: "create duplicate with different case", - inputName: "MyGroup", - existing: "mygroup", - setupMock: func(mock *mocks.MockStore, existing string) { - // Manager should lowercase input, so Exists is called with "mygroup" - mock.EXPECT(). - Exists(gomock.Any(), existing). - Return(true, nil) - }, - expectError: true, // expect duplicate error - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockStore := mocks.NewMockStore(ctrl) - manager := &manager{groupStore: mockStore} - - // set up the mock for this test - tt.setupMock(mockStore, tt.existing) - - if tt.expectError { - err := manager.Create(context.Background(), tt.inputName) - if err == nil { - t.Errorf("expected error when creating duplicate with case-insensitive name, got nil") - } - } else { - exists, err := manager.Exists(context.Background(), tt.inputName) - if err != nil { - t.Fatalf("exists check failed: %v", err) - } - if !exists { - t.Errorf("expected group %q to exist regardless of case", tt.inputName) - } - } - }) - } -} - diff --git a/pkg/groups/manager.go b/pkg/groups/manager.go index 8a8113991..bd0843047 100644 --- a/pkg/groups/manager.go +++ b/pkg/groups/manager.go @@ -17,6 +17,16 @@ const ( DefaultGroupName = "default" ) +// ValidateGroupName enforces lowercase-only group names. +func ValidateGroupName(name string) error { + if name != strings.ToLower(name) { + return fmt.Errorf("invalid group name: %q (must be lowercase)", name) + } + return nil +} + + + // manager implements the Manager interface type manager struct { groupStore state.Store @@ -34,8 +44,10 @@ func NewManager() (Manager, error) { // Create creates a new group with the given name func (m *manager) Create(ctx context.Context, name string) error { - // Lowercase the group name before checking for existence - name = strings.ToLower(name) + // Enforce lowercase group names + if err := ValidateGroupName(name); err != nil { + return err + } // Check if group already exists exists, err := m.groupStore.Exists(ctx, name) if err != nil { @@ -54,7 +66,9 @@ func (m *manager) Create(ctx context.Context, name string) error { // Get retrieves a group by name func (m *manager) Get(ctx context.Context, name string) (*Group, error) { - name = strings.ToLower(name) + if err := ValidateGroupName(name); err != nil { + return nil, err + } reader, err := m.groupStore.GetReader(ctx, name) if err != nil { return nil, fmt.Errorf("failed to get reader for group: %w", err) @@ -95,13 +109,17 @@ func (m *manager) List(ctx context.Context) ([]*Group, error) { // Delete removes a group by name func (m *manager) Delete(ctx context.Context, name string) error { - name = strings.ToLower(name) + if err := ValidateGroupName(name); err != nil { + return err + } return m.groupStore.Delete(ctx, name) } // Exists checks if a group exists func (m *manager) Exists(ctx context.Context, name string) (bool, error) { - name = strings.ToLower(name) + if err := ValidateGroupName(name); err != nil { + return false, err + } return m.groupStore.Exists(ctx, name) } From 000b73dd132b1d1eaab39827381be1f245e71972 Mon Sep 17 00:00:00 2001 From: Amruthan Manjula Anandan <157590530+rbiamru@users.noreply.github.com> Date: Mon, 8 Sep 2025 21:25:51 -0400 Subject: [PATCH 4/4] Nitpicks, tests: clean up invalid-name cases; format imports --- pkg/groups/group_test.go | 20 ++++++-------------- pkg/groups/manager.go | 2 -- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/pkg/groups/group_test.go b/pkg/groups/group_test.go index 1e57e0ba8..17ca7d860 100644 --- a/pkg/groups/group_test.go +++ b/pkg/groups/group_test.go @@ -83,25 +83,19 @@ func TestManager_Create(t *testing.T) { errorMsg: "failed to get writer for group", }, { - name: "invalid name - uppercase", - groupName: "MyGroup", - setupMock: func(mock *mocks.MockStore) { - // validation should fail before touching the store, - // so no expectations needed - }, + name: "invalid name - uppercase", + groupName: "MyGroup", + setupMock: func(_ *mocks.MockStore) {}, // validation fails before store access expectError: true, errorMsg: "invalid group name", }, { - name: "invalid name - mixed case", - groupName: "DefAult", - setupMock: func(mock *mocks.MockStore) { - // same as above: no store calls - }, + name: "invalid name - mixed case", + groupName: "DefAult", + setupMock: func(_ *mocks.MockStore) {}, // validation fails before store access expectError: true, errorMsg: "invalid group name", }, - } for _, tt := range tests { @@ -655,5 +649,3 @@ func (m *mockWriteCloser) Write(p []byte) (n int, err error) { func (*mockWriteCloser) Close() error { return nil } - - diff --git a/pkg/groups/manager.go b/pkg/groups/manager.go index bd0843047..b01718c1c 100644 --- a/pkg/groups/manager.go +++ b/pkg/groups/manager.go @@ -25,8 +25,6 @@ func ValidateGroupName(name string) error { return nil } - - // manager implements the Manager interface type manager struct { groupStore state.Store