fix(overrides): address gaps in attribute override system (#345)

webapp/backend/pkg/database/interface.go

@@ -103,6 +103,11 @@ type DeviceRepo interface {
 
 	// Attribute Override operations
 	GetAttributeOverrides(ctx context.Context) ([]models.AttributeOverride, error)
+	// GetAllOverridesForDisplay returns all overrides for display in the settings UI.
+	// It merges DB overrides (source: "ui") with config file overrides (source: "config"),
+	// so users can see everything that is active. Config overrides have ID=0 and cannot
+	// be deleted via the UI.
+	GetAllOverridesForDisplay(ctx context.Context) ([]models.AttributeOverride, error)
 	GetAttributeOverrideByID(ctx context.Context, id uint) (*models.AttributeOverride, error)
 	SaveAttributeOverride(ctx context.Context, override *models.AttributeOverride) error
 	DeleteAttributeOverride(ctx context.Context, id uint) error

webapp/backend/pkg/database/migrations/m20260411000000/attribute_override.go

@@ -0,0 +1,8 @@
+package m20260411000000
+
+// AttributeOverride is the migration-scoped struct after adding a unique
+// constraint on (protocol, attribute_id, wwn). The actual migration uses raw
+// SQL to remove any pre-existing duplicates before creating the unique index,
+// which GORM AutoMigrate cannot do safely on its own.
+// This struct is kept for reference only; the migration logic is in the
+// registered Migrate function in scrutiny_repository_migrations.go.

webapp/backend/pkg/database/scrutiny_repository_migrations.go

@@ -836,6 +836,32 @@ func (sr *scrutinyRepository) Migrate(ctx context.Context) error {
 				return tx.Create(&defaultSettings).Error
 			},
 		},
+		{
+			ID: "m20260411000000", // enforce unique constraint on (protocol, attribute_id, wwn) in attribute_overrides
+			Migrate: func(tx *gorm.DB) error {
+				// Remove any duplicate overrides, keeping the row with the lowest id
+				// for each (protocol, attribute_id, wwn) combination.
+				if err := tx.Exec(`
+					DELETE FROM attribute_overrides
+					WHERE id NOT IN (
+						SELECT MIN(id)
+						FROM attribute_overrides
+						GROUP BY protocol, attribute_id, wwn
+					)
+				`).Error; err != nil {
+					return fmt.Errorf("failed to remove duplicate attribute overrides: %w", err)
+				}
+				// Drop the existing non-unique composite index so we can replace it.
+				if err := tx.Exec("DROP INDEX IF EXISTS idx_override_lookup").Error; err != nil {
+					return fmt.Errorf("failed to drop old attribute_overrides index: %w", err)
+				}
+				// Create a unique composite index to prevent future duplicates.
+				if err := tx.Exec("CREATE UNIQUE INDEX idx_override_lookup ON attribute_overrides (protocol, attribute_id, wwn)").Error; err != nil {
+					return fmt.Errorf("failed to create unique attribute_overrides index: %w", err)
+				}
+				return nil
+			},
+		},
 	})
 
 	if err := m.Migrate(); err != nil {

webapp/backend/pkg/database/scrutiny_repository_overrides.go

@@ -25,6 +25,60 @@ func (sr *scrutinyRepository) GetAttributeOverrideByID(ctx context.Context, id u
 	return &override, nil
 }
 
+// GetAllOverridesForDisplay returns all active overrides for display in the settings UI.
+// DB overrides (source: "ui") are returned as-is. Config file overrides (source: "config")
+// are synthesized into models.AttributeOverride with ID=0, so the UI can show them as
+// read-only entries. DB overrides take precedence: if a DB override matches the same
+// (protocol, attribute_id, wwn) as a config override, only the DB version is returned.
+func (sr *scrutinyRepository) GetAllOverridesForDisplay(ctx context.Context) ([]models.AttributeOverride, error) {
+	dbOverrides, err := sr.GetAttributeOverrides(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	configOverrides := overrides.ParseOverrides(sr.appConfig)
+
+	// Build a set of (protocol|attribute_id|wwn) keys already covered by DB overrides.
+	dbKeys := make(map[string]struct{}, len(dbOverrides))
+	for i := range dbOverrides {
+		key := dbOverrides[i].Protocol + "|" + dbOverrides[i].AttributeId + "|" + dbOverrides[i].WWN
+		dbKeys[key] = struct{}{}
+	}
+
+	// Append config overrides that are not shadowed by a DB override.
+	result := make([]models.AttributeOverride, 0, len(dbOverrides)+len(configOverrides))
+	result = append(result, dbOverrides...)
+
+	for _, co := range configOverrides {
+		key := co.Protocol + "|" + co.AttributeId + "|" + co.WWN
+		if _, exists := dbKeys[key]; exists {
+			continue // DB override takes precedence; skip the config version
+		}
+		var warnAbove *int64
+		var failAbove *int64
+		if co.WarnAbove != nil {
+			v := *co.WarnAbove
+			warnAbove = &v
+		}
+		if co.FailAbove != nil {
+			v := *co.FailAbove
+			failAbove = &v
+		}
+		result = append(result, models.AttributeOverride{
+			Protocol:    co.Protocol,
+			AttributeId: co.AttributeId,
+			WWN:         co.WWN,
+			Action:      string(co.Action),
+			Status:      co.Status,
+			WarnAbove:   warnAbove,
+			FailAbove:   failAbove,
+			Source:      "config",
+		})
+	}
+
+	return result, nil
+}
+
 // GetMergedOverrides retrieves overrides from both config file and database,
 // merging them with database overrides taking precedence over config overrides.
 func (sr *scrutinyRepository) GetMergedOverrides(ctx context.Context) []overrides.AttributeOverride {

webapp/backend/pkg/models/attribute_override.go

@@ -18,18 +18,18 @@ type AttributeOverride struct {
 	DeletedAt gorm.DeletedAt `json:"deleted_at" gorm:"index"`
 
 	// Required: Protocol type (ATA, NVMe, SCSI)
-	Protocol string `json:"protocol" gorm:"not null;index:idx_override_lookup"`
+	Protocol string `json:"protocol" gorm:"not null;uniqueIndex:idx_override_lookup"`
 
 	// Required: Attribute ID (string for all protocols)
 	// ATA: "5", "187", etc.
 	// ATA DevStats: "devstat_7_8"
 	// NVMe: "media_errors", "percentage_used"
 	// SCSI: "scsi_grown_defect_list"
-	AttributeId string `json:"attribute_id" gorm:"not null;index:idx_override_lookup"`
+	AttributeId string `json:"attribute_id" gorm:"not null;uniqueIndex:idx_override_lookup"`
 
 	// Optional: Limit override to specific device by WWN
 	// If empty, override applies to all devices
-	WWN string `json:"wwn,omitempty" gorm:"index:idx_override_lookup"`
+	WWN string `json:"wwn,omitempty" gorm:"uniqueIndex:idx_override_lookup"`
 
 	// Optional: Action to take (ignore or force_status)
 	// If not set, custom thresholds are applied

webapp/backend/pkg/overrides/applier.go

@@ -141,7 +141,9 @@ func Apply(cfg config.Interface, protocol, attributeId, wwn string) *Result {
 		result.StatusReason = "Status forced by user configuration"
 	}
 
-	// Custom thresholds (can be combined with force_status or standalone)
+	// Custom thresholds are only evaluated when action is empty (see smart.go).
+	// When action is force_status, these fields are populated but callers use
+	// mutually exclusive else-if branches, so thresholds are effectively ignored.
 	result.WarnAbove = override.WarnAbove
 	result.FailAbove = override.FailAbove
 
@@ -228,7 +230,9 @@ func ApplyWithOverrides(overrideList []AttributeOverride, protocol, attributeId,
 		result.StatusReason = "Status forced by user configuration"
 	}
 
-	// Custom thresholds (can be combined with force_status or standalone)
+	// Custom thresholds are only evaluated when action is empty (see smart.go).
+	// When action is force_status, these fields are populated but callers use
+	// mutually exclusive else-if branches, so thresholds are effectively ignored.
 	result.WarnAbove = override.WarnAbove
 	result.FailAbove = override.FailAbove
 

webapp/backend/pkg/web/handler/attribute_overrides.go

@@ -2,6 +2,7 @@ package handler
 
 import (
 	"net/http"
+	"regexp"
 	"strconv"
 
 	"github.com/analogj/scrutiny/webapp/backend/pkg/database"
@@ -10,6 +11,9 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+// wwnPattern matches a valid WWN: optional 0x prefix followed by 1-16 hex digits.
+var wwnPattern = regexp.MustCompile(`(?i)^(0x)?[0-9a-f]{1,16}$`)
+
 // validProtocols defines the allowed protocol values
 var validProtocols = map[string]bool{
 	"ATA":  true,
@@ -31,12 +35,14 @@ var validStatuses = map[string]bool{
 	"failed": true,
 }
 
-// GetAttributeOverrides retrieves all attribute overrides from the database
+// GetAttributeOverrides retrieves all active attribute overrides for display.
+// Includes both UI-created overrides (source: "ui") and config file overrides
+// (source: "config"), so users can see everything that is currently active.
 func GetAttributeOverrides(c *gin.Context) {
 	logger := c.MustGet("LOGGER").(*logrus.Entry)
 	deviceRepo := c.MustGet("DEVICE_REPOSITORY").(database.DeviceRepo)
 
-	overrides, err := deviceRepo.GetAttributeOverrides(c)
+	allOverrides, err := deviceRepo.GetAllOverridesForDisplay(c)
 	if err != nil {
 		logger.Errorln("Error retrieving attribute overrides:", err)
 		c.JSON(http.StatusInternalServerError, gin.H{"success": false, "error": "Failed to retrieve overrides"})
@@ -45,10 +51,63 @@ func GetAttributeOverrides(c *gin.Context) {
 
 	c.JSON(http.StatusOK, gin.H{
 		"success": true,
-		"data":    overrides,
+		"data":    allOverrides,
 	})
 }
 
+// validateAttributeOverride checks all fields of an override and returns a
+// human-readable error string, or an empty string if the override is valid.
+func validateAttributeOverride(o *models.AttributeOverride) string {
+	if o.Protocol == "" {
+		return "Protocol is required"
+	}
+	if o.AttributeId == "" {
+		return "AttributeId is required"
+	}
+	if !validProtocols[o.Protocol] {
+		return "Invalid protocol. Must be ATA, NVMe, or SCSI"
+	}
+	if !validActions[o.Action] {
+		return "Invalid action. Must be empty, 'ignore', or 'force_status'"
+	}
+	if o.WWN != "" && !wwnPattern.MatchString(o.WWN) {
+		return "Invalid WWN format. Must be a hex value (e.g. 0x5000cca264eb01d7)"
+	}
+	if o.Action == "force_status" {
+		return validateForceStatus(o)
+	}
+	if o.Action == "" {
+		return validateThresholds(o)
+	}
+	return ""
+}
+
+func validateForceStatus(o *models.AttributeOverride) string {
+	if o.Status == "" {
+		return "Status is required when action is 'force_status'"
+	}
+	if !validStatuses[o.Status] {
+		return "Invalid status. Must be 'passed', 'warn', or 'failed'"
+	}
+	return ""
+}
+
+func validateThresholds(o *models.AttributeOverride) string {
+	if o.WarnAbove == nil && o.FailAbove == nil {
+		return "At least one of warn_above or fail_above is required for custom threshold overrides"
+	}
+	if o.WarnAbove != nil && *o.WarnAbove < 0 {
+		return "warn_above must be a non-negative value"
+	}
+	if o.FailAbove != nil && *o.FailAbove < 0 {
+		return "fail_above must be a non-negative value"
+	}
+	if o.WarnAbove != nil && o.FailAbove != nil && *o.WarnAbove >= *o.FailAbove {
+		return "warn_above must be less than fail_above"
+	}
+	return ""
+}
+
 // SaveAttributeOverride creates or updates an attribute override
 func SaveAttributeOverride(c *gin.Context) {
 	logger := c.MustGet("LOGGER").(*logrus.Entry)
@@ -61,40 +120,11 @@ func SaveAttributeOverride(c *gin.Context) {
 		return
 	}
 
-	// Validate required fields
-	if override.Protocol == "" {
-		c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": "Protocol is required"})
-		return
-	}
-	if override.AttributeId == "" {
-		c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": "AttributeId is required"})
-		return
-	}
-
-	// Validate protocol
-	if !validProtocols[override.Protocol] {
-		c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": "Invalid protocol. Must be ATA, NVMe, or SCSI"})
-		return
-	}
-
-	// Validate action
-	if !validActions[override.Action] {
-		c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": "Invalid action. Must be empty, 'ignore', or 'force_status'"})
+	if errMsg := validateAttributeOverride(&override); errMsg != "" {
+		c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": errMsg})
 		return
 	}
 
-	// Validate status if force_status action is used
-	if override.Action == "force_status" {
-		if override.Status == "" {
-			c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": "Status is required when action is 'force_status'"})
-			return
-		}
-		if !validStatuses[override.Status] {
-			c.JSON(http.StatusBadRequest, gin.H{"success": false, "error": "Invalid status. Must be 'passed', 'warn', or 'failed'"})
-			return
-		}
-	}
-
 	// Source is always "ui" for API-created overrides
 	override.Source = "ui"