fix(db): match SQL constraint behavior on MongoDB - #1260
Open
madhavilosetty-intel wants to merge 1 commit into
Open
fix(db): match SQL constraint behavior on MongoDB#1260madhavilosetty-intel wants to merge 1 commit into
madhavilosetty-intel wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // SQL leaves this to the profiles_wirelessconfigs foreign key. Mongo has no | ||
| // constraints, so look for a referencing row the way RPS did before deleting. | ||
| err := r.profileWiFiCol.FindOne(ctx, bson.M{fieldWirelessProfileName: profileName, fieldTenantID: tenantID}).Err() |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1260 +/- ##
==========================================
+ Coverage 56.71% 56.74% +0.03%
==========================================
Files 149 149
Lines 12154 12171 +17
==========================================
+ Hits 6893 6907 +14
- Misses 5260 5263 +3
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Two repository behaviors differed between the SQL backends and Mongo, so the same request returned a different status depending on which database Console was configured with. Every Mongo Insert mapped a duplicate-key error to NotUniqueError, but no Mongo Update did, so a unique-index collision on update surfaced as a generic DatabaseError and the handler answered 400 instead of 409. Updating a domain to a suffix another domain already owns is the case the API tests cover; the mapping is added to all six repositories. Deleting a wireless profile that an AMT profile still references is rejected by the profiles_wirelessconfigs foreign key on Postgres and SQLite. Mongo has no constraints, so the delete succeeded and left the AMT profile pointing at a wireless profile that no longer exists. The repository now looks for a referencing document first, which is what RPS itself did: src/data/postgres/tables/wirelessProfiles.ts queries profiles_wirelessconfigs before deleting rather than relying on the constraint. ForeignKeyViolationError moves from sqldb to repoerrors so both backends raise one type and the controller's errors.As check works for either; sqldb keeps the name as an alias, so no other package changes. Its doc comment said the error had no place in the cross-backend vocabulary because Mongo lacks constraints - that reasoning is what left the divergence in place, so the comment is corrected rather than worked around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UCV2vuSwoz7J5VV2hZxcWe
madhavilosetty-intel
force-pushed
the
fix/mongo-constraint-parity
branch
from
September 8, 2026 22:49
8bc04eb to
533580e
Compare
Contributor
|
@madhavilosetty-intel : Few initial comments
🤖 Generated with Claude Code
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The Mongo drivertest responses in wificonfig_test.go hardcode a consoledb.* namespace even though the shared mock helper uses testdb, making the new tests inconsistent and potentially brittle.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns MongoDB repository behavior with the SQL backends so identical API requests return consistent HTTP status codes regardless of the configured storage provider.
Changes:
- Map MongoDB duplicate-key errors on
Updatetorepoerrors.NotUniqueError(matching SQL behavior and enabling HTTP 409). - Add a pre-delete reference check in Mongo
WirelessRepo.Deleteto emulate SQL’s foreign key constraint behavior (reject deletes that would orphan profile references). - Move
ForeignKeyViolationErrorintointernal/repoerrors(with ansqldbalias) and add targeted Mongo repo tests for the new behaviors.
File summaries
| File | Description |
|---|---|
| internal/usecase/sqldb/foreignkeyviolation.go | Re-exports ForeignKeyViolationError as an alias to the shared repoerrors type. |
| internal/repoerrors/foreignkeyviolation.go | Introduces cross-backend ForeignKeyViolationError for consistent controller handling. |
| internal/usecase/nosqldb/mongo/errors.go | Adds a Mongo-side ForeignKeyViolationError instance for wireless delete referential checks. |
| internal/usecase/nosqldb/mongo/wificonfig.go | Adds a reference lookup before delete; maps duplicate key on update to NotUniqueError. |
| internal/usecase/nosqldb/mongo/wificonfig_test.go | Adds tests for duplicate-key-on-update and referential-delete behavior (mocked Mongo wire protocol). |
| internal/usecase/nosqldb/mongo/profile.go | Maps duplicate key on update to NotUniqueError. |
| internal/usecase/nosqldb/mongo/profile_test.go | Adds test coverage for duplicate-key-on-update mapping. |
| internal/usecase/nosqldb/mongo/ieee8021xconfig.go | Maps duplicate key on update to NotUniqueError. |
| internal/usecase/nosqldb/mongo/ieee8021xconfig_test.go | Adds test coverage for duplicate-key-on-update mapping. |
| internal/usecase/nosqldb/mongo/domain.go | Maps duplicate key on update to NotUniqueError. |
| internal/usecase/nosqldb/mongo/domain_test.go | Adds test coverage for duplicate-key-on-update mapping. |
| internal/usecase/nosqldb/mongo/device.go | Maps duplicate key on update to NotUniqueError. |
| internal/usecase/nosqldb/mongo/device_test.go | Adds test coverage for duplicate-key-on-update mapping. |
| internal/usecase/nosqldb/mongo/ciraconfig.go | Maps duplicate key on update to NotUniqueError. |
| internal/usecase/nosqldb/mongo/ciraconfig_test.go | Adds test coverage for duplicate-key-on-update mapping. |
Review details
Suppressed comments (1)
internal/usecase/nosqldb/mongo/wificonfig_test.go:258
- This test hardcodes the mock cursor namespace as "consoledb.profiles_wirelessconfigs", but newMockedDB() returns client.Database("testdb"). Use the same "testdb." namespace used elsewhere in this test suite to keep the drivertest responses consistent.
md.AddResponses(findResponse("consoledb.profiles_wirelessconfigs",
bson.D{{Key: "profilename", Value: "amt-profile"}, {Key: "wirelessprofilename", Value: "wifi1"}},
))
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+212
to
214
| // No referencing profiles_wirelessconfigs row, then the delete itself. | ||
| md.AddResponses(findResponse("consoledb.profiles_wirelessconfigs"), deleteResponse(1)) | ||
|
|
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
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.
What
Two repository behaviors differed between the SQL backends and MongoDB, so the same request returned a different status depending on which database Console was configured with. CLAUDE.md treats that as a bug: the use case has no way to know which backend it got.
1. Duplicate key on update. Every Mongo
Insertmapped a duplicate-key error toNotUniqueError; no MongoUpdatedid. A unique-index collision on update surfaced as a genericDatabaseError, so the handler answered 400 where Postgres and SQLite answer 409.2. Referential integrity on wireless-profile delete. The
profiles_wirelessconfigsforeign key rejects this on Postgres and SQLite. Mongo has no constraints, so the delete succeeded, leaving the AMT profile pointing at a wireless profile that no longer exists.How
Updatein all six Mongo repositories, matching what eachInsertalready does.WirelessRepo.Deletelooks for a referencingprofiles_wirelessconfigsdocument before deleting. This is what RPS itself did —src/data/postgres/tables/wirelessProfiles.tsrunsSELECT 1 FROM profiles_wirelessconfigs ...and throws'Foreign key violation'rather than relying on the constraint — so this is parity with the service Console replaces, not a new invention.ForeignKeyViolationErrormoves fromsqldbtorepoerrorsso both backends raise one type and the controller'serrors.Ascheck works for either.sqldbkeeps the name as a type alias, so no other package changes.Its doc comment previously said the error "has no place in the cross-backend error vocabulary" because Mongo lacks constraints. That reasoning is what left the divergence in place, so the comment is corrected rather than worked around. Flagging it explicitly for review — it reverses a documented design decision.
Note for the reviewer: CodeQL
CodeQL raises
go/sql-injection(high) on the newFindOneinwificonfig.go. It is the same pattern as every other query in this package: the value is passed as a BSON value, not concatenated into a query string, so it cannot alter the query structure — andDeletealready rejects anything that failsidentifierRegexbefore the query runs.This repository has 20+ alerts for this exact rule in this exact package already dismissed as "false positive", including two in
wificonfig.goitself (alerts #166 and #167, on the neighbouringDeleteandUpdate). This one is alert #238 and needs the same disposition — I have not dismissed it, since that is a maintainer call.Testing
go build ./...,go vet ./...,gofmt -s -l— cleango test -count=1 ./...andgo test -race -count=1 ./...— passdrivertestwire-level mock:Test{CIRA,Device,Domain,IEEE8021x,Profile,Wireless}Repo_Update_DuplicateReturnsNotUniqueError— one per repository, so every branch this PR adds is coveredTestWirelessRepo_Delete_ReferencedByProfileIsRejectedandTestWirelessRepo_Delete_ReferenceLookupFailurePreventsDeleteFound while re-enabling the RPS Postman collection, which has not run in CI since #217. The CI change that turns it back on follows in a separate PR and depends on this one.
No route or response-shape changes on the SQL backends, so
internal/controller/openapi/and the Postman collections are untouched here.🤖 Generated with Claude Code
https://claude.ai/code/session_01UCV2vuSwoz7J5VV2hZxcWe