From 533580e70fd77cf4d56a58fe29800e147958aa75 Mon Sep 17 00:00:00 2001 From: madhavilosetty-intel Date: Tue, 8 Sep 2026 15:35:49 -0700 Subject: [PATCH] fix(db): match SQL constraint behavior on MongoDB 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) Claude-Session: https://claude.ai/code/session_01UCV2vuSwoz7J5VV2hZxcWe --- internal/repoerrors/foreignkeyviolation.go | 23 ++++++ internal/usecase/nosqldb/mongo/ciraconfig.go | 4 + .../usecase/nosqldb/mongo/ciraconfig_test.go | 23 ++++++ internal/usecase/nosqldb/mongo/device.go | 4 + internal/usecase/nosqldb/mongo/device_test.go | 23 ++++++ internal/usecase/nosqldb/mongo/domain.go | 4 + internal/usecase/nosqldb/mongo/domain_test.go | 24 ++++++ internal/usecase/nosqldb/mongo/errors.go | 4 + .../usecase/nosqldb/mongo/ieee8021xconfig.go | 4 + .../nosqldb/mongo/ieee8021xconfig_test.go | 23 ++++++ internal/usecase/nosqldb/mongo/profile.go | 4 + .../usecase/nosqldb/mongo/profile_test.go | 23 ++++++ internal/usecase/nosqldb/mongo/wificonfig.go | 29 +++++-- .../usecase/nosqldb/mongo/wificonfig_test.go | 77 ++++++++++++++++++- internal/usecase/sqldb/foreignkeyviolation.go | 22 +----- 15 files changed, 266 insertions(+), 25 deletions(-) create mode 100644 internal/repoerrors/foreignkeyviolation.go diff --git a/internal/repoerrors/foreignkeyviolation.go b/internal/repoerrors/foreignkeyviolation.go new file mode 100644 index 000000000..2fe84faea --- /dev/null +++ b/internal/repoerrors/foreignkeyviolation.go @@ -0,0 +1,23 @@ +package repoerrors + +import "github.com/device-management-toolkit/console/pkg/consoleerrors" + +// ForeignKeyViolationError reports a delete or insert that would break a +// relationship between records. Postgres and SQLite raise it from the foreign +// key constraint itself; MongoDB has no constraints, so its repositories check +// the referencing collection first and raise the same error — the way RPS did +// it (src/data/postgres/tables/wirelessProfiles.ts queries +// profiles_wirelessconfigs before deleting). Controllers map it to 400. +type ForeignKeyViolationError struct { + Console consoleerrors.InternalError +} + +func (e ForeignKeyViolationError) Error() string { + return e.Console.Error() +} + +func (e ForeignKeyViolationError) Wrap(details string) error { + e.Console.Message = "foreign key violation: " + details + + return e +} diff --git a/internal/usecase/nosqldb/mongo/ciraconfig.go b/internal/usecase/nosqldb/mongo/ciraconfig.go index 4e3859c31..92afdc271 100644 --- a/internal/usecase/nosqldb/mongo/ciraconfig.go +++ b/internal/usecase/nosqldb/mongo/ciraconfig.go @@ -130,6 +130,10 @@ func (r *CIRARepo) Update(ctx context.Context, c *entity.CIRAConfig) (bool, erro }}, ) if err != nil { + if isDuplicateKey(err) { + return false, errCIRANotUnique.Wrap(err.Error()) + } + return false, errCIRADatabase.Wrap("Update", "UpdateOne", err) } diff --git a/internal/usecase/nosqldb/mongo/ciraconfig_test.go b/internal/usecase/nosqldb/mongo/ciraconfig_test.go index 0e58cb22a..ea2654849 100644 --- a/internal/usecase/nosqldb/mongo/ciraconfig_test.go +++ b/internal/usecase/nosqldb/mongo/ciraconfig_test.go @@ -105,6 +105,29 @@ func TestCIRARepo_Insert_DuplicateReturnsNotUniqueError(t *testing.T) { require.True(t, errors.As(err, &nu), "expected NotUniqueError, got %T: %v", err, err) } +// A unique-index collision on update has to reach the handler as a +// NotUniqueError so it answers 409, the way the SQL backends do. +func TestCIRARepo_Update_DuplicateReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(duplicateKeyResponse()) + + repo := mongo.NewCIRARepo(db) + + ok, err := repo.Update(context.Background(), &entity.CIRAConfig{ + ConfigName: "cira1", + TenantID: "t1", + }) + require.False(t, ok) + require.Error(t, err) + + var notUnique repoerrors.NotUniqueError + + require.ErrorAs(t, err, ¬Unique) +} + func TestCIRARepo_Update_Matched(t *testing.T) { t.Parallel() diff --git a/internal/usecase/nosqldb/mongo/device.go b/internal/usecase/nosqldb/mongo/device.go index c1e5a45f3..2a0a23706 100644 --- a/internal/usecase/nosqldb/mongo/device.go +++ b/internal/usecase/nosqldb/mongo/device.go @@ -250,6 +250,10 @@ func (r *DeviceRepo) Update(ctx context.Context, d *entity.Device) (bool, error) }}, ) if err != nil { + if isDuplicateKey(err) { + return false, errDeviceNotUnique.Wrap(err.Error()) + } + return false, errDeviceDatabase.Wrap("Update", "UpdateOne", err) } diff --git a/internal/usecase/nosqldb/mongo/device_test.go b/internal/usecase/nosqldb/mongo/device_test.go index 8fedd41ee..c602f6510 100644 --- a/internal/usecase/nosqldb/mongo/device_test.go +++ b/internal/usecase/nosqldb/mongo/device_test.go @@ -200,6 +200,29 @@ func TestDeviceRepo_Insert_DuplicateReturnsNotUniqueError(t *testing.T) { require.True(t, errors.As(err, &nu)) } +// A unique-index collision on update has to reach the handler as a +// NotUniqueError so it answers 409, the way the SQL backends do. +func TestDeviceRepo_Update_DuplicateReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(duplicateKeyResponse()) + + repo := mongo.NewDeviceRepo(db) + + ok, err := repo.Update(context.Background(), &entity.Device{ + GUID: "g1", + TenantID: "t1", + }) + require.False(t, ok) + require.Error(t, err) + + var notUnique repoerrors.NotUniqueError + + require.ErrorAs(t, err, ¬Unique) +} + func TestDeviceRepo_Update_Matched(t *testing.T) { t.Parallel() diff --git a/internal/usecase/nosqldb/mongo/domain.go b/internal/usecase/nosqldb/mongo/domain.go index 6f8b3a2f7..cad55ac38 100644 --- a/internal/usecase/nosqldb/mongo/domain.go +++ b/internal/usecase/nosqldb/mongo/domain.go @@ -159,6 +159,10 @@ func (r *DomainRepo) Update(ctx context.Context, d *entity.Domain) (bool, error) }}, ) if err != nil { + if isDuplicateKey(err) { + return false, errDomainNotUnique.Wrap(err.Error()) + } + return false, errDomainDatabase.Wrap("Update", "UpdateOne", err) } diff --git a/internal/usecase/nosqldb/mongo/domain_test.go b/internal/usecase/nosqldb/mongo/domain_test.go index d91ea9227..6b0115308 100644 --- a/internal/usecase/nosqldb/mongo/domain_test.go +++ b/internal/usecase/nosqldb/mongo/domain_test.go @@ -162,6 +162,30 @@ func TestDomainRepo_Update_Matched(t *testing.T) { require.True(t, ok) } +// A suffix collision on update has to reach the handler as a NotUniqueError so +// it answers 409, the way the SQL backends do from their unique index. +func TestDomainRepo_Update_DuplicateReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(duplicateKeyResponse()) + + repo := mongo.NewDomainRepo(db) + + ok, err := repo.Update(context.Background(), &entity.Domain{ + ProfileName: "Acme", + DomainSuffix: "taken.com", + TenantID: "t1", + }) + require.False(t, ok) + require.Error(t, err) + + var notUnique repoerrors.NotUniqueError + + require.ErrorAs(t, err, ¬Unique) +} + func TestDomainRepo_Update_NoMatch(t *testing.T) { t.Parallel() diff --git a/internal/usecase/nosqldb/mongo/errors.go b/internal/usecase/nosqldb/mongo/errors.go index f17cc6b5c..d7af55a3d 100644 --- a/internal/usecase/nosqldb/mongo/errors.go +++ b/internal/usecase/nosqldb/mongo/errors.go @@ -23,6 +23,10 @@ var ( errWiFiNotUnique = repoerrors.NotUniqueError{Console: consoleerrors.CreateConsoleError("MongoWirelessRepo")} errProfileWiFiConfigsDatabase = repoerrors.DatabaseError{Console: consoleerrors.CreateConsoleError("MongoProfileWiFiConfigsRepo")} errProfileWiFiConfigsNotUnique = repoerrors.NotUniqueError{Console: consoleerrors.CreateConsoleError("MongoProfileWiFiConfigsRepo")} + + // Mongo has no foreign keys, so the repositories check the referencing + // collection themselves and raise the error SQL gets from its constraint. + errWiFiForeignKeyViolation = repoerrors.ForeignKeyViolationError{Console: consoleerrors.CreateConsoleError("MongoWirelessRepo")} ) // isDuplicateKey matches Mongo E11000 errors (mapped to NotUniqueError, mirroring SQL). diff --git a/internal/usecase/nosqldb/mongo/ieee8021xconfig.go b/internal/usecase/nosqldb/mongo/ieee8021xconfig.go index 0f54ace07..bfe5c837e 100644 --- a/internal/usecase/nosqldb/mongo/ieee8021xconfig.go +++ b/internal/usecase/nosqldb/mongo/ieee8021xconfig.go @@ -141,6 +141,10 @@ func (r *IEEE8021xRepo) Update(ctx context.Context, c *entity.IEEE8021xConfig) ( }}, ) if err != nil { + if isDuplicateKey(err) { + return false, errIEEENotUnique.Wrap(err.Error()) + } + return false, errIEEEDatabase.Wrap("Update", "UpdateOne", err) } diff --git a/internal/usecase/nosqldb/mongo/ieee8021xconfig_test.go b/internal/usecase/nosqldb/mongo/ieee8021xconfig_test.go index 4ca3eb60e..4a13652f4 100644 --- a/internal/usecase/nosqldb/mongo/ieee8021xconfig_test.go +++ b/internal/usecase/nosqldb/mongo/ieee8021xconfig_test.go @@ -156,6 +156,29 @@ func TestIEEE8021xRepo_Insert_DuplicateReturnsNotUniqueError(t *testing.T) { require.True(t, errors.As(err, &nu)) } +// A unique-index collision on update has to reach the handler as a +// NotUniqueError so it answers 409, the way the SQL backends do. +func TestIEEE8021xRepo_Update_DuplicateReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(duplicateKeyResponse()) + + repo := mongo.NewIEEE8021xRepo(db) + + ok, err := repo.Update(context.Background(), &entity.IEEE8021xConfig{ + ProfileName: "ieee1", + TenantID: "t1", + }) + require.False(t, ok) + require.Error(t, err) + + var notUnique repoerrors.NotUniqueError + + require.ErrorAs(t, err, ¬Unique) +} + func TestIEEE8021xRepo_Update(t *testing.T) { t.Parallel() diff --git a/internal/usecase/nosqldb/mongo/profile.go b/internal/usecase/nosqldb/mongo/profile.go index b5a5a85c6..c480332d9 100644 --- a/internal/usecase/nosqldb/mongo/profile.go +++ b/internal/usecase/nosqldb/mongo/profile.go @@ -187,6 +187,10 @@ func (r *ProfileRepo) Update(ctx context.Context, p *entity.Profile) (bool, erro bson.M{opSet: set}, ) if err != nil { + if isDuplicateKey(err) { + return false, errProfileNotUnique.Wrap(err.Error()) + } + return false, errProfileDatabase.Wrap("Update", "UpdateOne", err) } diff --git a/internal/usecase/nosqldb/mongo/profile_test.go b/internal/usecase/nosqldb/mongo/profile_test.go index b8170f8cb..aff0390e1 100644 --- a/internal/usecase/nosqldb/mongo/profile_test.go +++ b/internal/usecase/nosqldb/mongo/profile_test.go @@ -146,6 +146,29 @@ func TestProfileRepo_Insert_DuplicateReturnsNotUniqueError(t *testing.T) { require.True(t, errors.As(err, &nu)) } +// A unique-index collision on update has to reach the handler as a +// NotUniqueError so it answers 409, the way the SQL backends do. +func TestProfileRepo_Update_DuplicateReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(duplicateKeyResponse()) + + repo := mongo.NewProfileRepo(db, logger.New("error")) + + ok, err := repo.Update(context.Background(), &entity.Profile{ + ProfileName: "p1", + TenantID: "t1", + }) + require.False(t, ok) + require.Error(t, err) + + var notUnique repoerrors.NotUniqueError + + require.ErrorAs(t, err, ¬Unique) +} + func TestProfileRepo_Update(t *testing.T) { t.Parallel() diff --git a/internal/usecase/nosqldb/mongo/wificonfig.go b/internal/usecase/nosqldb/mongo/wificonfig.go index fecb7b845..de9a8da62 100644 --- a/internal/usecase/nosqldb/mongo/wificonfig.go +++ b/internal/usecase/nosqldb/mongo/wificonfig.go @@ -15,18 +15,20 @@ import ( ) type WirelessRepo struct { - col *mongo.Collection - ieee8021xCol *mongo.Collection - log logger.Interface + col *mongo.Collection + ieee8021xCol *mongo.Collection + profileWiFiCol *mongo.Collection + log logger.Interface } var _ wificonfigs.Repository = (*WirelessRepo)(nil) func NewWirelessRepo(db *mongo.Database, log logger.Interface) *WirelessRepo { return &WirelessRepo{ - col: db.Collection(CollectionWirelessConfigs), - ieee8021xCol: db.Collection(CollectionIEEE8021xConfigs), - log: log, + col: db.Collection(CollectionWirelessConfigs), + ieee8021xCol: db.Collection(CollectionIEEE8021xConfigs), + profileWiFiCol: db.Collection(CollectionProfileWiFiConfigs), + log: log, } } @@ -162,6 +164,17 @@ func (r *WirelessRepo) Delete(ctx context.Context, profileName, tenantID string) return false, nil } + // 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() + + switch { + case err == nil: + return false, errWiFiForeignKeyViolation.Wrap("wireless profile " + profileName + " is associated with an AMT profile") + case !errors.Is(err, mongo.ErrNoDocuments): + return false, errWiFiDatabase.Wrap("Delete", "FindOne", err) + } + res, err := r.col.DeleteOne(ctx, bson.M{fieldProfileName: profileName, fieldTenantID: tenantID}) if err != nil { return false, errWiFiDatabase.Wrap("Delete", "DeleteOne", err) @@ -192,6 +205,10 @@ func (r *WirelessRepo) Update(ctx context.Context, w *entity.WirelessConfig) (bo }}, ) if err != nil { + if isDuplicateKey(err) { + return false, errWiFiNotUnique.Wrap(err.Error()) + } + return false, errWiFiDatabase.Wrap("Update", "UpdateOne", err) } diff --git a/internal/usecase/nosqldb/mongo/wificonfig_test.go b/internal/usecase/nosqldb/mongo/wificonfig_test.go index 0bb3766dc..ad5e203c5 100644 --- a/internal/usecase/nosqldb/mongo/wificonfig_test.go +++ b/internal/usecase/nosqldb/mongo/wificonfig_test.go @@ -164,6 +164,29 @@ func TestWirelessRepo_Insert_DuplicateReturnsNotUniqueError(t *testing.T) { require.True(t, errors.As(err, &nu)) } +// A unique-index collision on update has to reach the handler as a +// NotUniqueError so it answers 409, the way the SQL backends do. +func TestWirelessRepo_Update_DuplicateReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(duplicateKeyResponse()) + + repo := mongo.NewWirelessRepo(db, logger.New("error")) + + ok, err := repo.Update(context.Background(), &entity.WirelessConfig{ + ProfileName: "wifi1", + TenantID: "t1", + }) + require.False(t, ok) + require.Error(t, err) + + var notUnique repoerrors.NotUniqueError + + require.ErrorAs(t, err, ¬Unique) +} + func TestWirelessRepo_Update(t *testing.T) { t.Parallel() @@ -186,7 +209,8 @@ func TestWirelessRepo_Delete(t *testing.T) { db, md := newMockedDB(t) - md.AddResponses(deleteResponse(1)) + // No referencing profiles_wirelessconfigs row, then the delete itself. + md.AddResponses(findResponse("consoledb.profiles_wirelessconfigs"), deleteResponse(1)) repo := mongo.NewWirelessRepo(db, logger.New("error")) @@ -194,3 +218,54 @@ func TestWirelessRepo_Delete(t *testing.T) { require.NoError(t, err) require.True(t, ok) } + +// A failed reference lookup must not fall through to the delete: the repo cannot +// tell whether the wireless profile is still in use, so it reports the error. +func TestWirelessRepo_Delete_ReferenceLookupFailurePreventsDelete(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + // Only one queued response: a delete would need a second, and reaching it + // would hang rather than silently pass. + md.AddResponses(bson.D{ + {Key: "ok", Value: 0}, + {Key: "code", Value: int32(13)}, + {Key: "errmsg", Value: "not authorized"}, + }) + + repo := mongo.NewWirelessRepo(db, logger.New("error")) + + ok, err := repo.Delete(context.Background(), "wifi1", "t1") + require.False(t, ok) + require.Error(t, err) + + var dbErr repoerrors.DatabaseError + + require.ErrorAs(t, err, &dbErr) +} + +// SQL gets this from the profiles_wirelessconfigs foreign key; Mongo has to look +// for the referencing row itself, and must raise the same error so the handler +// still answers 400. +func TestWirelessRepo_Delete_ReferencedByProfileIsRejected(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(findResponse("consoledb.profiles_wirelessconfigs", + bson.D{{Key: "profilename", Value: "amt-profile"}, {Key: "wirelessprofilename", Value: "wifi1"}}, + )) + + repo := mongo.NewWirelessRepo(db, logger.New("error")) + + ok, err := repo.Delete(context.Background(), "wifi1", "t1") + require.False(t, ok) + require.Error(t, err) + + var fkErr repoerrors.ForeignKeyViolationError + + require.ErrorAs(t, err, &fkErr) + // FriendlyMessage is what the handler puts in the 400 body. + require.Contains(t, fkErr.Console.FriendlyMessage(), "foreign key violation") +} diff --git a/internal/usecase/sqldb/foreignkeyviolation.go b/internal/usecase/sqldb/foreignkeyviolation.go index 77d9f06ba..364ac0d9c 100644 --- a/internal/usecase/sqldb/foreignkeyviolation.go +++ b/internal/usecase/sqldb/foreignkeyviolation.go @@ -1,21 +1,7 @@ package sqldb -import "github.com/device-management-toolkit/console/pkg/consoleerrors" +import "github.com/device-management-toolkit/console/internal/repoerrors" -// ForeignKeyViolationError lives in sqldb (not internal/repoerrors) because -// foreign key constraints are a relational-only concept. Backends without -// referential integrity (e.g. MongoDB) do not produce this error, so it has -// no place in the cross-backend error vocabulary. -type ForeignKeyViolationError struct { - Console consoleerrors.InternalError -} - -func (e ForeignKeyViolationError) Error() string { - return e.Console.Error() -} - -func (e ForeignKeyViolationError) Wrap(details string) error { - e.Console.Message = "foreign key violation: " + details - - return e -} +// ForeignKeyViolationError is an alias so both repository packages raise the +// same type and the controllers' errors.As checks work for either backend. +type ForeignKeyViolationError = repoerrors.ForeignKeyViolationError