Fix/add probe uniqueness check#5519
Conversation
Signed-off-by: Yoonseo Han <yooncer00@gmail.com>
…robe Signed-off-by: Yoonseo Han <yooncer00@gmail.com>
…dateUniqueProbe Signed-off-by: Yoonseo Han <yooncer00@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes missing application-level probe name uniqueness enforcement in the GraphQL server’s probe handler by wiring the existing ValidateUniqueProbe check into AddProbe, and introduces initial unit coverage for the probe handler layer to prevent regressions.
Changes:
- Add an early uniqueness check in
AddProbeviaValidateUniqueProbe, returning a clear"probe already exists"error before attempting any DB write. - Add first unit tests for probe handler uniqueness behavior (
ValidateUniqueProbetrue/false/error cases, andAddProbeduplicate rejection).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
chaoscenter/graphql/server/pkg/probe/handler/handler.go |
Adds early uniqueness validation in AddProbe to avoid surfacing raw Mongo duplicate key errors in common duplicate-name cases. |
chaoscenter/graphql/server/pkg/probe/handler/handler_test.go |
Adds initial unit tests for probe handler uniqueness logic and duplicate rejection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.NoError(t, err) | ||
| assert.True(t, unique) | ||
| mockOp.AssertExpectations(t) | ||
| } |
| assert.Error(t, err) | ||
| assert.Equal(t, "probe already exists", err.Error()) | ||
| mockOp.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything) | ||
| mockOp.AssertExpectations(t) |
|
Thanks for reviewing this PR. I have read through the comments that copilot has left. I believe the comments are pointing out a real issue in the shared mongo mock implementation where methods are currently defined with value receivers instead of pointer receivers. This can make testify call-history based assertions unreliable when the mock is used via the mongodb.MongoOperator interface. So the issue raised by Copilot seems valid as a test-infra issue. For this PR however, the probe handler test intent looks correct for validating unique probe and duplicate handling. The root cause appears to be in the shared mock type rather than the probe handler test logic itself. Hence I wanted to confirm the preferred scope:
Or any other ideas would be very much appreciated. Thanks |
|
Hi @yoonseo-han , |
Signed-off-by: Yoonseo Han <yooncer00@gmail.com>
|
Thanks @ispeakc0de Included the mongo mock fix in this PR. Updated the MongoOperator mock methods to pointer receivers so that testify assertions work correctly Ready for another look when you are free |
Proposed changes
Fixes #5518
AddProbemissed server-side uniqueness enforcement. Uniqueness was only guaranteed by the MongoDB partial compound index on (name, project_id) where is_removed: false, which prevented data corruption but surfaced raw E11000 duplicate key errors to non-UI callers instead of a clear application-level message.Two entry points were affected:
Contribution points:
ValidateUniqueProbemethod intoAddProbeas an early return before any database write is attempted, making all entry points consistent.pkg/probe/handler/had no test coverage covering both the duplicate rejection behavior and the ValidateUniqueProbe method directly.Types of changes
What types of changes does your code introduce to Litmus? Put an
xin the boxes that applyChanges
pkg/probe/handler/handler.go: Added uniqueness check at the top ofAddProbeusing the existingValidateUniqueProbemethod, returning a clear "probe already exists" error before any object construction or DB write occurs.ValidateUniqueProberuns aCountDocumentsquery filtered onname,project_id, andis_removed= false : Filter is backed by the existing partial compound index on (name, project_id), so no collection scan occurs.pkg/probe/handler/handler_test.go— Added unit tests covering:TestAddProbe_DuplicateName): verifies Create is never called on duplicateValidateUniqueProbereturns true when name is unique (TestValidateUniqueProbe_Unique)ValidateUniqueProbereturns false when duplicate exists (TestValidateUniqueProbe_Duplicate)ValidateUniqueProbepropagates DB errors correctly (TestValidateUniqueProbe_DBError)Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Dependency
NA
Special notes for your reviewer: