From 34ecd8b47a560b15226e6d8596a6b520ca778c23 Mon Sep 17 00:00:00 2001 From: Eder <34795193+Starosdev@users.noreply.github.com> Date: Mon, 9 Mar 2026 23:12:26 -0400 Subject: [PATCH 1/7] fix(detect): preserve device path case for /dev/disk/by-id paths (#335) (#340) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Remove `strings.ToLower` from scanned and config-override device file paths in `TransformDetectedDevices` - `/dev/disk/by-id/` paths containing uppercase manufacturer names and serial numbers are now passed verbatim to `smartctl` - Config override map lookups use `strings.EqualFold` so comparisons remain case-insensitive without mutating stored paths - WWN normalization to lowercase is unchanged - Add `groupedDeviceKey` helper for case-insensitive map key lookup - Add two tests covering uppercase by-id paths via config override and via scan ## Linked Issues Closes #335 ## Test plan - [x] `TestDetect_TransformDetectedDevices_UppercaseByIDPath` — config-only uppercase path preserved verbatim - [x] `TestDetect_TransformDetectedDevices_UppercaseByIDPathScanned` — scanned uppercase path preserved verbatim - [x] All existing detect package tests pass (`go test ./collector/pkg/detect/...`) --- collector/pkg/detect/detect.go | 49 ++++++++++++++++++++++------- collector/pkg/detect/detect_test.go | 46 +++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/collector/pkg/detect/detect.go b/collector/pkg/detect/detect.go index 8e026279..870dfe03 100644 --- a/collector/pkg/detect/detect.go +++ b/collector/pkg/detect/detect.go @@ -181,12 +181,10 @@ func (d *Detect) TransformDetectedDevices(detectedDeviceConns models.Scan) []mod for _, scannedDevice := range detectedDeviceConns.Devices { - // Preserve case for IOService/IODeviceTree paths; they are case-sensitive - // macOS identifiers that must be passed verbatim to smartctl. + // Preserve case for all device paths — filesystem paths are case-sensitive. + // /dev/disk/by-id/ paths may contain uppercase characters from manufacturer + // names and serial numbers that must be passed verbatim to smartctl. deviceFile := scannedDevice.Name - if !isIOPath(deviceFile) { - deviceFile = strings.ToLower(deviceFile) - } // If the user has defined a device allow list, and this device isnt there, then ignore it if !d.Config.IsAllowlistedDevice(deviceFile) { @@ -209,14 +207,21 @@ func (d *Detect) TransformDetectedDevices(detectedDeviceConns models.Scan) []mod groupedDevices[deviceFile] = append(groupedDevices[deviceFile], detectedDevice) } - //now tha we've "grouped" all the devices, lets override any groups specified in the config file. + // now that we've "grouped" all the devices, lets override any groups specified in the config file. for _, overrideDevice := range d.Config.GetDeviceOverrides() { - overrideDeviceFile := strings.ToLower(overrideDevice.Device) + // Preserve case for the override device path — filesystem paths are case-sensitive. + // Map lookups use case-insensitive comparison to match scanned devices without mutating paths. + overrideDeviceFile := overrideDevice.Device if overrideDevice.Ignore { - // this device file should be deleted if it exists - delete(groupedDevices, overrideDeviceFile) + // case-insensitive delete: the scanned entry may have been stored with different case + for key := range groupedDevices { + if strings.EqualFold(key, overrideDeviceFile) { + delete(groupedDevices, key) + break + } + } } else { //create a new device group, and replace the one generated by smartctl --scan overrideDeviceGroup := []models.Device{} @@ -234,16 +239,17 @@ func (d *Detect) TransformDetectedDevices(detectedDeviceConns models.Scan) []mod } else { //user may have specified device in config file without device type (default to scanned device type) - //check if the device file was detected by the scanner + // check if the device file was detected by the scanner (case-insensitive) var deviceType string - if scannedDevice, foundScannedDevice := groupedDevices[overrideDeviceFile]; foundScannedDevice { + scannedKey := groupedDeviceKey(groupedDevices, overrideDeviceFile) + if scannedKey != "" { + scannedDevice := groupedDevices[scannedKey] if len(scannedDevice) > 0 { //take the device type from the first grouped device deviceType = scannedDevice[0].DeviceType } else { deviceType = "ata" } - } else { //fallback to ata if no scanned device detected deviceType = "ata" @@ -258,6 +264,13 @@ func (d *Detect) TransformDetectedDevices(detectedDeviceConns models.Scan) []mod }) } + // Remove any scanned entry stored under a different case to prevent duplicates. + for key := range groupedDevices { + if strings.EqualFold(key, overrideDeviceFile) && key != overrideDeviceFile { + delete(groupedDevices, key) + break + } + } groupedDevices[overrideDeviceFile] = overrideDeviceGroup } } @@ -270,3 +283,15 @@ func (d *Detect) TransformDetectedDevices(detectedDeviceConns models.Scan) []mod return detectedDevices } + +// groupedDeviceKey returns the key in groupedDevices whose lowercased form matches the +// lowercased target, enabling case-insensitive lookups without mutating stored paths. +// Returns an empty string if no match is found. +func groupedDeviceKey(groupedDevices map[string][]models.Device, target string) string { + for key := range groupedDevices { + if strings.EqualFold(key, target) { + return key + } + } + return "" +} diff --git a/collector/pkg/detect/detect_test.go b/collector/pkg/detect/detect_test.go index d684c1d3..aed9cb7b 100644 --- a/collector/pkg/detect/detect_test.go +++ b/collector/pkg/detect/detect_test.go @@ -549,6 +549,52 @@ func TestDetect_DeviceFullPath_StandardDeviceGetsPrefixed(t *testing.T) { require.NotEqual(t, "sda", result, "standard device should have a prefix added") } +func TestDetect_TransformDetectedDevices_UppercaseByIDPath(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeConfig.EXPECT().GetString("host.id").AnyTimes().Return("") + fakeConfig.EXPECT().GetDeviceOverrides().AnyTimes().Return([]models.ScanOverride{ + {Device: "/dev/disk/by-id/ata-HGST_HUH721212ALE600_ABC123"}, + }) + fakeConfig.EXPECT().IsAllowlistedDevice(gomock.Any()).AnyTimes().Return(true) + + // Simulate smartctl --scan returning no results; the device is config-only. + detectedDevices := models.Scan{} + + d := detect.Detect{Config: fakeConfig} + transformedDevices := d.TransformDetectedDevices(detectedDevices) + + require.Equal(t, 1, len(transformedDevices)) + // The uppercase by-id path must be preserved verbatim so smartctl can resolve it. + require.Equal(t, "disk/by-id/ata-HGST_HUH721212ALE600_ABC123", transformedDevices[0].DeviceName) + require.Equal(t, "ata", transformedDevices[0].DeviceType) +} + +func TestDetect_TransformDetectedDevices_UppercaseByIDPathScanned(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeConfig.EXPECT().GetString("host.id").AnyTimes().Return("") + fakeConfig.EXPECT().GetDeviceOverrides().AnyTimes().Return([]models.ScanOverride{}) + fakeConfig.EXPECT().IsAllowlistedDevice(gomock.Any()).AnyTimes().Return(true) + + // Simulate smartctl --scan returning a by-id path with uppercase characters. + const byIDPath = "/dev/disk/by-id/ata-HGST_HUH721212ALE600_ABC123" + detectedDevices := models.Scan{ + Devices: []models.ScanDevice{ + {Name: byIDPath, InfoName: byIDPath, Protocol: "ata", Type: "ata"}, + }, + } + + d := detect.Detect{Config: fakeConfig} + transformedDevices := d.TransformDetectedDevices(detectedDevices) + + require.Equal(t, 1, len(transformedDevices)) + // Case must be preserved so smartctl receives the correct path. + require.Equal(t, "disk/by-id/ata-HGST_HUH721212ALE600_ABC123", transformedDevices[0].DeviceName) +} + func TestDetect_TransformDetectedDevices_RaidWithLabel(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() From 37d9f6a61749f746014b1da676f94fd966afa4bd Mon Sep 17 00:00:00 2001 From: Eder Date: Tue, 10 Mar 2026 18:45:00 -0400 Subject: [PATCH 2/7] feat(notify): add notifications for collector-side smartctl errors (#334) - Add notify_on_collector_error bool setting (default true) with DB migration m20260410000000 - Add NotifyFailureTypeCollectorError constant and NewCollectorError() constructor to notify package - Add POST /api/device/:id/collector-error and POST /api/collector/scan-error endpoints - Collector reports smartctl --scan failures via scan-error endpoint and --xall fatal errors via device collector-error endpoint - Missing WWN after info phase also reported as scan-level error - Notifications respect mute, quiet hours, and rate limiting via existing NotificationGate - Frontend: add notify_on_collector_error to AppConfig interface, defaults, settings component, and settings dialog toggle --- collector/pkg/collector/metrics.go | 39 +++++ .../scrutiny_repository_migrations.go | 14 ++ webapp/backend/pkg/models/settings.go | 3 + webapp/backend/pkg/notify/notify.go | 54 +++++++ .../pkg/web/handler/upload_collector_error.go | 133 ++++++++++++++++++ webapp/backend/pkg/web/server.go | 2 + .../src/app/core/config/app.config.ts | 3 + .../dashboard-settings.component.html | 12 ++ .../dashboard-settings.component.ts | 7 + 9 files changed, 267 insertions(+) create mode 100644 webapp/backend/pkg/web/handler/upload_collector_error.go diff --git a/collector/pkg/collector/metrics.go b/collector/pkg/collector/metrics.go index ef4824c7..6486c064 100644 --- a/collector/pkg/collector/metrics.go +++ b/collector/pkg/collector/metrics.go @@ -64,6 +64,7 @@ func (mc *MetricsCollector) Run() error { } rawDetectedStorageDevices, err := deviceDetector.Start() if err != nil { + mc.ReportScanError("scan", err.Error()) return err } @@ -120,6 +121,7 @@ func (mc *MetricsCollector) Collect(deviceWWN string, deviceName string, deviceT //defer wg.Done() if len(deviceWWN) == 0 { mc.logger.Errorf("no device WWN detected for %s. Skipping collection for this device (no data association possible).\n", deviceName) + mc.ReportScanError("info", fmt.Sprintf("no WWN detected for device %s; smartctl --info may have failed", deviceName)) return } mc.logger.Infof("Collecting smartctl results for %s\n", deviceName) @@ -146,6 +148,7 @@ func (mc *MetricsCollector) Collect(deviceWWN string, deviceName string, deviceT mc.logger.Errorf("error while attempting to execute smartctl: %s\n", deviceName) mc.logger.Errorf("ERROR MESSAGE: %v", err) mc.logger.Errorf("IGNORING RESULT: %v", result) + mc.ReportDeviceError(deviceWWN, "xall", err.Error()) return } } @@ -237,3 +240,39 @@ func (mc *MetricsCollector) Publish(deviceWWN string, payload []byte) error { return nil } + +// collectorErrorPayload is the JSON body sent to the backend collector-error endpoints. +type collectorErrorPayload struct { + ErrorType string `json:"error_type"` + ErrorMessage string `json:"error_message"` +} + +// ReportDeviceError posts a collector error to /api/device/:wwn/collector-error. +// Errors from this call are logged but do not abort the collection run. +func (mc *MetricsCollector) ReportDeviceError(deviceWWN string, errorType string, errorMessage string) { + if deviceWWN == "" { + mc.logger.Debugf("Cannot report device error without WWN; skipping") + return + } + apiEndpoint, _ := url.Parse(mc.apiEndpoint.String()) + apiEndpoint, _ = apiEndpoint.Parse(fmt.Sprintf("api/device/%s/collector-error", strings.ToLower(deviceWWN))) + + body := collectorErrorPayload{ErrorType: errorType, ErrorMessage: errorMessage} + var result map[string]interface{} + if err := mc.postJson(apiEndpoint.String(), body, &result); err != nil { + mc.logger.Warnf("Failed to report collector device error for %s: %v", deviceWWN, err) + } +} + +// ReportScanError posts a collector scan-level error to /api/collector/scan-error. +// Errors from this call are logged but do not abort the collection run. +func (mc *MetricsCollector) ReportScanError(errorType string, errorMessage string) { + apiEndpoint, _ := url.Parse(mc.apiEndpoint.String()) + apiEndpoint, _ = apiEndpoint.Parse("api/collector/scan-error") + + body := collectorErrorPayload{ErrorType: errorType, ErrorMessage: errorMessage} + var result map[string]interface{} + if err := mc.postJson(apiEndpoint.String(), body, &result); err != nil { + mc.logger.Warnf("Failed to report collector scan error: %v", err) + } +} diff --git a/webapp/backend/pkg/database/scrutiny_repository_migrations.go b/webapp/backend/pkg/database/scrutiny_repository_migrations.go index ad47c7d7..0e0f0ea8 100644 --- a/webapp/backend/pkg/database/scrutiny_repository_migrations.go +++ b/webapp/backend/pkg/database/scrutiny_repository_migrations.go @@ -822,6 +822,20 @@ func (sr *scrutinyRepository) Migrate(ctx context.Context) error { return nil }, }, + { + ID: "m20260410000000", // add notify_on_collector_error setting + Migrate: func(tx *gorm.DB) error { + var defaultSettings = []m20220716214900.Setting{ + { + SettingKeyName: "metrics.notify_on_collector_error", + SettingKeyDescription: "Enable notifications when the collector encounters smartctl errors (true | false)", + SettingDataType: "bool", + SettingValueBool: true, + }, + } + return tx.Create(&defaultSettings).Error + }, + }, }) if err := m.Migrate(); err != nil { diff --git a/webapp/backend/pkg/models/settings.go b/webapp/backend/pkg/models/settings.go index a72752a4..8f7ca5a2 100644 --- a/webapp/backend/pkg/models/settings.go +++ b/webapp/backend/pkg/models/settings.go @@ -40,6 +40,9 @@ type Settings struct { NotificationQuietStart string `json:"notification_quiet_start" mapstructure:"notification_quiet_start"` NotificationQuietEnd string `json:"notification_quiet_end" mapstructure:"notification_quiet_end"` + // Collector error notification settings + NotifyOnCollectorError bool `json:"notify_on_collector_error" mapstructure:"notify_on_collector_error"` + // Heartbeat notification settings HeartbeatEnabled bool `json:"heartbeat_enabled" mapstructure:"heartbeat_enabled"` HeartbeatIntervalHours int `json:"heartbeat_interval_hours" mapstructure:"heartbeat_interval_hours"` diff --git a/webapp/backend/pkg/notify/notify.go b/webapp/backend/pkg/notify/notify.go index 210c77f7..d05e0ed7 100644 --- a/webapp/backend/pkg/notify/notify.go +++ b/webapp/backend/pkg/notify/notify.go @@ -36,6 +36,7 @@ const NotifyFailureTypeMissedPing = "MissedPing" const NotifyFailureTypeHeartbeat = "Heartbeat" const NotifyFailureTypePerformanceDegradation = "PerformanceDegradation" const NotifyFailureTypeReport = "Report" +const NotifyFailureTypeCollectorError = "CollectorError" // ShouldNotify check if the error Message should be filtered (level mismatch or filtered_attributes) func ShouldNotify(logger logrus.FieldLogger, device *models.Device, smartAttrs *measurements.Smart, notifyLevel pkg.MetricsNotifyLevel, statusThreshold pkg.MetricsStatusThreshold, statusFilterAttributes pkg.MetricsStatusFilterAttributes, repeatNotifications bool, wwn string, c *gin.Context, deviceRepo database.DeviceRepo, cfg config.Interface) bool { @@ -980,3 +981,56 @@ func NewReport(logger logrus.FieldLogger, appconfig config.Interface, subject, m Payload: payload, } } + +// NewCollectorError creates a Notify instance for collector-side smartctl error notifications. +// device may be a zero-value Device when the error occurs before a device is fully identified +// (e.g. during --scan). errorType is a short tag (e.g. "scan", "info", "xall") and +// errorMessage is the human-readable description of the failure. +func NewCollectorError(logger logrus.FieldLogger, appconfig config.Interface, device models.Device, errorType, errorMessage string) Notify { + deviceIdentifier := device.DeviceName + if deviceIdentifier == "" { + deviceIdentifier = "(unknown device)" + } + hostPrefix := "" + if device.HostId != "" { + hostPrefix = fmt.Sprintf("[%s] ", strings.TrimSpace(device.HostId)) + } + + subject := fmt.Sprintf("Scrutiny collector error (%s) on %sdevice: %s", errorType, hostPrefix, deviceIdentifier) + + var messageParts []string + messageParts = append(messageParts, fmt.Sprintf("Scrutiny collector error notification for device: %s", deviceIdentifier)) + if device.HostId != "" { + messageParts = append(messageParts, fmt.Sprintf("Host Id: %s", strings.TrimSpace(device.HostId))) + } + messageParts = append(messageParts, + fmt.Sprintf("Error Type: %s", errorType), + fmt.Sprintf("Error: %s", errorMessage), + "", + fmt.Sprintf("Date: %s", time.Now().Format(time.RFC3339)), + ) + if device.SerialNumber != "" { + messageParts = append([]string{fmt.Sprintf("Device Serial: %s", device.SerialNumber)}, messageParts...) + } + + message := strings.Join(messageParts, "\n") + + payload := Payload{ + HostId: strings.TrimSpace(device.HostId), + DeviceType: device.DeviceType, + DeviceName: device.DeviceName, + DeviceSerial: device.SerialNumber, + DeviceLabel: strings.TrimSpace(device.Label), + Test: false, + Date: time.Now().Format(time.RFC3339), + FailureType: NotifyFailureTypeCollectorError, + Subject: subject, + Message: message, + } + + return Notify{ + Logger: logger, + Config: appconfig, + Payload: payload, + } +} diff --git a/webapp/backend/pkg/web/handler/upload_collector_error.go b/webapp/backend/pkg/web/handler/upload_collector_error.go new file mode 100644 index 00000000..bf1b3a17 --- /dev/null +++ b/webapp/backend/pkg/web/handler/upload_collector_error.go @@ -0,0 +1,133 @@ +package handler + +import ( + "fmt" + "net/http" + + "github.com/analogj/scrutiny/webapp/backend/pkg/config" + "github.com/analogj/scrutiny/webapp/backend/pkg/database" + "github.com/analogj/scrutiny/webapp/backend/pkg/models" + "github.com/analogj/scrutiny/webapp/backend/pkg/notify" + "github.com/gin-gonic/gin" + "github.com/sirupsen/logrus" +) + +// CollectorErrorRequest is the JSON payload sent by the collector when smartctl +// returns an unrecoverable error during --scan, --info, or --xall. +type CollectorErrorRequest struct { + // ErrorType is a short tag identifying which operation failed: "scan", "info", or "xall". + ErrorType string `json:"error_type" binding:"required"` + // ErrorMessage is the human-readable description of the failure. + ErrorMessage string `json:"error_message" binding:"required"` +} + +// UploadCollectorError handles POST /api/device/:id/collector-error. +// The collector calls this endpoint when smartctl returns an error during +// device detection or SMART data collection. When the notify_on_collector_error +// setting is enabled the backend forwards the error through the notification pipeline. +func UploadCollectorError(c *gin.Context) { + logger := c.MustGet("LOGGER").(*logrus.Entry) + appConfig := c.MustGet("CONFIG").(config.Interface) + deviceRepo := c.MustGet("DEVICE_REPOSITORY").(database.DeviceRepo) + + device, resolveErr := ResolveDevice(c, logger, deviceRepo) + if resolveErr != nil { + return + } + + var req CollectorErrorRequest + if err := c.BindJSON(&req); err != nil { + logger.Errorln("Cannot parse collector error payload", err) + c.JSON(http.StatusBadRequest, gin.H{"success": false}) + return + } + + notifyEnabled := appConfig.GetBool(fmt.Sprintf("%s.metrics.notify_on_collector_error", config.DB_USER_SETTINGS_SUBKEY)) + if !notifyEnabled { + logger.Debugf("notify_on_collector_error is disabled; skipping notification for device %s", device.DeviceID) + c.JSON(http.StatusOK, gin.H{"success": true}) + return + } + + if device.Muted { + logger.Debugf("Device %s is muted; skipping collector error notification", device.DeviceID) + c.JSON(http.StatusOK, gin.H{"success": true}) + return + } + + errorNotify := notify.NewCollectorError(logger, appConfig, device, req.ErrorType, req.ErrorMessage) + errorNotify.LoadDatabaseUrls(c, deviceRepo) + + if gateVal, exists := c.Get("NOTIFICATION_GATE"); exists { + if gate, ok := gateVal.(*notify.NotificationGate); ok { + settings, settingsErr := deviceRepo.LoadSettings(c) + if settingsErr != nil { + logger.Warnf("Failed to load settings for notification gate: %v", settingsErr) + } + if settings != nil { + gate.TrySend(&errorNotify, settings, false) + } else { + if sendErr := errorNotify.Send(); sendErr != nil { + logger.Warnf("Failed to send collector error notification for device %s: %v", device.DeviceID, sendErr) + } + } + } + } else { + if sendErr := errorNotify.Send(); sendErr != nil { + logger.Warnf("Failed to send collector error notification for device %s: %v", device.DeviceID, sendErr) + } + } + + c.JSON(http.StatusOK, gin.H{"success": true}) +} + +// UploadCollectorScanError handles POST /api/collector/scan-error. +// The collector calls this endpoint when smartctl --scan itself fails (no devices +// available to attach the error to). The notification is sent host-scoped rather +// than device-scoped. +func UploadCollectorScanError(c *gin.Context) { + logger := c.MustGet("LOGGER").(*logrus.Entry) + appConfig := c.MustGet("CONFIG").(config.Interface) + deviceRepo := c.MustGet("DEVICE_REPOSITORY").(database.DeviceRepo) + + var req CollectorErrorRequest + if err := c.BindJSON(&req); err != nil { + logger.Errorln("Cannot parse collector scan error payload", err) + c.JSON(http.StatusBadRequest, gin.H{"success": false}) + return + } + + notifyEnabled := appConfig.GetBool(fmt.Sprintf("%s.metrics.notify_on_collector_error", config.DB_USER_SETTINGS_SUBKEY)) + if !notifyEnabled { + logger.Debugf("notify_on_collector_error is disabled; skipping scan error notification") + c.JSON(http.StatusOK, gin.H{"success": true}) + return + } + + // For scan errors we have no specific device, so we use a minimal Device struct. + device := models.Device{} + errorNotify := notify.NewCollectorError(logger, appConfig, device, req.ErrorType, req.ErrorMessage) + errorNotify.LoadDatabaseUrls(c, deviceRepo) + + if gateVal, exists := c.Get("NOTIFICATION_GATE"); exists { + if gate, ok := gateVal.(*notify.NotificationGate); ok { + settings, settingsErr := deviceRepo.LoadSettings(c) + if settingsErr != nil { + logger.Warnf("Failed to load settings for notification gate: %v", settingsErr) + } + if settings != nil { + gate.TrySend(&errorNotify, settings, false) + } else { + if sendErr := errorNotify.Send(); sendErr != nil { + logger.Warnf("Failed to send collector scan error notification: %v", sendErr) + } + } + } + } else { + if sendErr := errorNotify.Send(); sendErr != nil { + logger.Warnf("Failed to send collector scan error notification: %v", sendErr) + } + } + + c.JSON(http.StatusOK, gin.H{"success": true}) +} diff --git a/webapp/backend/pkg/web/server.go b/webapp/backend/pkg/web/server.go index 662ba62c..297a26e5 100644 --- a/webapp/backend/pkg/web/server.go +++ b/webapp/backend/pkg/web/server.go @@ -141,6 +141,8 @@ func (ae *AppEngine) Setup(logger *logrus.Entry) *gin.Engine { api.DELETE("/device/:id", handler.DeleteDevice) // used by UI to delete device api.POST("/device/:id/performance", handler.UploadDevicePerformance) // used by Collector to upload performance benchmarks api.GET("/device/:id/performance", handler.GetDevicePerformance) // used by UI to view performance history + api.POST("/device/:id/collector-error", handler.UploadCollectorError) // used by Collector to report smartctl errors + api.POST("/collector/scan-error", handler.UploadCollectorScanError) // used by Collector to report scan-level errors (no device context) api.GET("/settings", handler.GetSettings) //used to get settings api.POST("/settings", handler.SaveSettings) //used to save settings diff --git a/webapp/frontend/src/app/core/config/app.config.ts b/webapp/frontend/src/app/core/config/app.config.ts index a3cce5ca..457381c5 100644 --- a/webapp/frontend/src/app/core/config/app.config.ts +++ b/webapp/frontend/src/app/core/config/app.config.ts @@ -106,6 +106,8 @@ export interface AppConfig { status_filter_attributes?: MetricsStatusFilterAttributes status_threshold?: MetricsStatusThreshold repeat_notifications?: boolean + // Collector error notifications + notify_on_collector_error?: boolean // Missed collector ping notifications notify_on_missed_ping?: boolean missed_ping_timeout_minutes?: number @@ -171,6 +173,7 @@ export const appConfig: AppConfig = { status_filter_attributes: MetricsStatusFilterAttributes.All, status_threshold: MetricsStatusThreshold.Both, repeat_notifications: true, + notify_on_collector_error: true, notify_on_missed_ping: false, missed_ping_timeout_minutes: 60, missed_ping_check_interval_mins: 5, diff --git a/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.html b/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.html index 45176f8c..6e4ed830 100644 --- a/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.html +++ b/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.html @@ -132,6 +132,18 @@

Scrutiny Settings

+ +
+ + Notify on Collector smartctl Errors + + Enabled + Disabled + + Alert when smartctl encounters errors during device scan or data collection + +
+
diff --git a/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts b/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts index 185bebdc..3d617630 100644 --- a/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts +++ b/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts @@ -45,6 +45,9 @@ export class DashboardSettingsComponent implements OnInit { statusFilterAttributes: number; repeatNotifications: boolean; + // Collector error settings + notifyOnCollectorError: boolean; + // Missed ping settings notifyOnMissedPing: boolean; missedPingTimeoutMinutes: number; @@ -159,6 +162,9 @@ export class DashboardSettingsComponent implements OnInit { this.statusThreshold = config.metrics.status_threshold; this.repeatNotifications = config.metrics.repeat_notifications; + // Collector error settings + this.notifyOnCollectorError = config.metrics.notify_on_collector_error ?? true; + // Missed ping settings this.notifyOnMissedPing = config.metrics.notify_on_missed_ping ?? false; this.missedPingTimeoutMinutes = config.metrics.missed_ping_timeout_minutes ?? 60; @@ -369,6 +375,7 @@ export class DashboardSettingsComponent implements OnInit { status_filter_attributes: this.statusFilterAttributes as MetricsStatusFilterAttributes, status_threshold: this.statusThreshold as MetricsStatusThreshold, repeat_notifications: this.repeatNotifications, + notify_on_collector_error: this.notifyOnCollectorError, notify_on_missed_ping: this.notifyOnMissedPing, missed_ping_timeout_minutes: this.missedPingTimeoutMinutes, missed_ping_check_interval_mins: this.missedPingCheckIntervalMins, From d1c0a98702b59460493a6e3086f803f14d6f2844 Mon Sep 17 00:00:00 2001 From: Eder Date: Tue, 10 Mar 2026 19:05:23 -0400 Subject: [PATCH 3/7] fix(notify): address code review issues for collector error notifications - fix(collector): report fatal xall exit codes (0x01, 0x02) to backend instead of silently continuing with unusable smartctl output - fix(handler): extract sendNotificationViaGate helper to eliminate duplicate notification dispatch blocks in both error handlers - fix(handler): accept optional device_name in scan-error payload so the notification subject shows the device name instead of "(unknown device)" - fix(collector): pass device name through ReportScanError for info errors - fix(models): document why bool-default-true fields cannot be set in ApplyDefaults - test(handler): add handler tests covering bad JSON, notify disabled, device not found, and muted device paths --- collector/pkg/collector/metrics.go | 20 ++- webapp/backend/pkg/models/settings.go | 8 + .../pkg/web/handler/upload_collector_error.go | 73 ++++---- .../handler/upload_collector_error_test.go | 156 ++++++++++++++++++ 4 files changed, 210 insertions(+), 47 deletions(-) create mode 100644 webapp/backend/pkg/web/handler/upload_collector_error_test.go diff --git a/collector/pkg/collector/metrics.go b/collector/pkg/collector/metrics.go index 6486c064..496fead7 100644 --- a/collector/pkg/collector/metrics.go +++ b/collector/pkg/collector/metrics.go @@ -64,7 +64,7 @@ func (mc *MetricsCollector) Run() error { } rawDetectedStorageDevices, err := deviceDetector.Start() if err != nil { - mc.ReportScanError("scan", err.Error()) + mc.ReportScanError("scan", err.Error(), "") return err } @@ -121,7 +121,7 @@ func (mc *MetricsCollector) Collect(deviceWWN string, deviceName string, deviceT //defer wg.Done() if len(deviceWWN) == 0 { mc.logger.Errorf("no device WWN detected for %s. Skipping collection for this device (no data association possible).\n", deviceName) - mc.ReportScanError("info", fmt.Sprintf("no WWN detected for device %s; smartctl --info may have failed", deviceName)) + mc.ReportScanError("info", fmt.Sprintf("no WWN detected for device %s; smartctl --info may have failed", deviceName), deviceName) return } mc.logger.Infof("Collecting smartctl results for %s\n", deviceName) @@ -144,6 +144,14 @@ func (mc *MetricsCollector) Collect(deviceWWN string, deviceName string, deviceT // smartctl command exited with an error, we should still push the data to the API server mc.logger.Errorf("smartctl returned an error code (%d) while processing %s\n", exitError.ExitCode(), deviceName) mc.LogSmartctlExitCode(exitError.ExitCode()) + // Bits 0x01 and 0x02 mean smartctl could not parse the command line or open the + // device respectively. In these cases the JSON output is unusable, so report the + // failure to the backend and skip publishing. + exitCode := exitError.ExitCode() + if exitCode&0x01 != 0 || exitCode&0x02 != 0 { + mc.ReportDeviceError(deviceWWN, "xall", fmt.Sprintf("smartctl exited with fatal code %d while reading %s", exitCode, deviceName)) + return + } } else { mc.logger.Errorf("error while attempting to execute smartctl: %s\n", deviceName) mc.logger.Errorf("ERROR MESSAGE: %v", err) @@ -245,6 +253,8 @@ func (mc *MetricsCollector) Publish(deviceWWN string, payload []byte) error { type collectorErrorPayload struct { ErrorType string `json:"error_type"` ErrorMessage string `json:"error_message"` + // DeviceName is an optional hint used when no WWN is available (scan/info errors). + DeviceName string `json:"device_name,omitempty"` } // ReportDeviceError posts a collector error to /api/device/:wwn/collector-error. @@ -265,12 +275,14 @@ func (mc *MetricsCollector) ReportDeviceError(deviceWWN string, errorType string } // ReportScanError posts a collector scan-level error to /api/collector/scan-error. +// deviceName is an optional hint included in the payload so the backend can produce +// a more informative notification subject when no WWN is available. // Errors from this call are logged but do not abort the collection run. -func (mc *MetricsCollector) ReportScanError(errorType string, errorMessage string) { +func (mc *MetricsCollector) ReportScanError(errorType string, errorMessage string, deviceName string) { apiEndpoint, _ := url.Parse(mc.apiEndpoint.String()) apiEndpoint, _ = apiEndpoint.Parse("api/collector/scan-error") - body := collectorErrorPayload{ErrorType: errorType, ErrorMessage: errorMessage} + body := collectorErrorPayload{ErrorType: errorType, ErrorMessage: errorMessage, DeviceName: deviceName} var result map[string]interface{} if err := mc.postJson(apiEndpoint.String(), body, &result); err != nil { mc.logger.Warnf("Failed to report collector scan error: %v", err) diff --git a/webapp/backend/pkg/models/settings.go b/webapp/backend/pkg/models/settings.go index 8f7ca5a2..4b6f80ef 100644 --- a/webapp/backend/pkg/models/settings.go +++ b/webapp/backend/pkg/models/settings.go @@ -85,6 +85,14 @@ func defaultInt(p *int, def int) { // This prevents the API from returning empty strings that break the frontend // (e.g. theme="" produces invalid CSS class "treo-theme-", layout="" matches // no template case). Called after loading settings from the database. +// +// Note: bool fields whose intended default is true (e.g. NotifyOnCollectorError, +// RepeatNotifications, NotifyOnMissedPing) cannot be safely defaulted here because +// Go's zero value for bool is false, making it impossible to distinguish between +// "user explicitly set to false" and "setting was never written to the database". +// These fields rely on the database migration to seed the correct default value on +// fresh installations. ApplyDefaults only covers string and int fields where 0/"" is +// an unambiguous sentinel for "not configured". func (s *Settings) ApplyDefaults() { // Top-level string settings defaultStr(&s.Theme, "system") diff --git a/webapp/backend/pkg/web/handler/upload_collector_error.go b/webapp/backend/pkg/web/handler/upload_collector_error.go index bf1b3a17..021889a2 100644 --- a/webapp/backend/pkg/web/handler/upload_collector_error.go +++ b/webapp/backend/pkg/web/handler/upload_collector_error.go @@ -19,6 +19,29 @@ type CollectorErrorRequest struct { ErrorType string `json:"error_type" binding:"required"` // ErrorMessage is the human-readable description of the failure. ErrorMessage string `json:"error_message" binding:"required"` + // DeviceName is an optional hint used when no WWN is available (e.g. during --info). + // It is included in the notification subject so operators can identify the device. + DeviceName string `json:"device_name"` +} + +// sendNotificationViaGate dispatches n through the NOTIFICATION_GATE middleware if present, +// falling back to a direct send. Errors are logged but do not affect the HTTP response. +func sendNotificationViaGate(c *gin.Context, logger *logrus.Entry, n *notify.Notify, deviceRepo database.DeviceRepo) { + if gateVal, exists := c.Get("NOTIFICATION_GATE"); exists { + if gate, ok := gateVal.(*notify.NotificationGate); ok { + settings, settingsErr := deviceRepo.LoadSettings(c) + if settingsErr != nil { + logger.Warnf("Failed to load settings for notification gate: %v", settingsErr) + } + if settings != nil { + gate.TrySend(n, settings, false) + return + } + } + } + if sendErr := n.Send(); sendErr != nil { + logger.Warnf("Failed to send notification: %v", sendErr) + } } // UploadCollectorError handles POST /api/device/:id/collector-error. @@ -57,26 +80,7 @@ func UploadCollectorError(c *gin.Context) { errorNotify := notify.NewCollectorError(logger, appConfig, device, req.ErrorType, req.ErrorMessage) errorNotify.LoadDatabaseUrls(c, deviceRepo) - - if gateVal, exists := c.Get("NOTIFICATION_GATE"); exists { - if gate, ok := gateVal.(*notify.NotificationGate); ok { - settings, settingsErr := deviceRepo.LoadSettings(c) - if settingsErr != nil { - logger.Warnf("Failed to load settings for notification gate: %v", settingsErr) - } - if settings != nil { - gate.TrySend(&errorNotify, settings, false) - } else { - if sendErr := errorNotify.Send(); sendErr != nil { - logger.Warnf("Failed to send collector error notification for device %s: %v", device.DeviceID, sendErr) - } - } - } - } else { - if sendErr := errorNotify.Send(); sendErr != nil { - logger.Warnf("Failed to send collector error notification for device %s: %v", device.DeviceID, sendErr) - } - } + sendNotificationViaGate(c, logger, &errorNotify, deviceRepo) c.JSON(http.StatusOK, gin.H{"success": true}) } @@ -84,7 +88,8 @@ func UploadCollectorError(c *gin.Context) { // UploadCollectorScanError handles POST /api/collector/scan-error. // The collector calls this endpoint when smartctl --scan itself fails (no devices // available to attach the error to). The notification is sent host-scoped rather -// than device-scoped. +// than device-scoped. An optional device_name hint in the payload is used to +// produce a more informative notification subject when no WWN is available. func UploadCollectorScanError(c *gin.Context) { logger := c.MustGet("LOGGER").(*logrus.Entry) appConfig := c.MustGet("CONFIG").(config.Interface) @@ -104,30 +109,12 @@ func UploadCollectorScanError(c *gin.Context) { return } - // For scan errors we have no specific device, so we use a minimal Device struct. - device := models.Device{} + // For scan errors we have no specific device. Populate DeviceName from the request + // hint (if provided) so the notification subject is more informative than "(unknown device)". + device := models.Device{DeviceName: req.DeviceName} errorNotify := notify.NewCollectorError(logger, appConfig, device, req.ErrorType, req.ErrorMessage) errorNotify.LoadDatabaseUrls(c, deviceRepo) - - if gateVal, exists := c.Get("NOTIFICATION_GATE"); exists { - if gate, ok := gateVal.(*notify.NotificationGate); ok { - settings, settingsErr := deviceRepo.LoadSettings(c) - if settingsErr != nil { - logger.Warnf("Failed to load settings for notification gate: %v", settingsErr) - } - if settings != nil { - gate.TrySend(&errorNotify, settings, false) - } else { - if sendErr := errorNotify.Send(); sendErr != nil { - logger.Warnf("Failed to send collector scan error notification: %v", sendErr) - } - } - } - } else { - if sendErr := errorNotify.Send(); sendErr != nil { - logger.Warnf("Failed to send collector scan error notification: %v", sendErr) - } - } + sendNotificationViaGate(c, logger, &errorNotify, deviceRepo) c.JSON(http.StatusOK, gin.H{"success": true}) } diff --git a/webapp/backend/pkg/web/handler/upload_collector_error_test.go b/webapp/backend/pkg/web/handler/upload_collector_error_test.go new file mode 100644 index 00000000..0d17e719 --- /dev/null +++ b/webapp/backend/pkg/web/handler/upload_collector_error_test.go @@ -0,0 +1,156 @@ +package handler_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + mock_config "github.com/analogj/scrutiny/webapp/backend/pkg/config/mock" + mock_database "github.com/analogj/scrutiny/webapp/backend/pkg/database/mock" + "github.com/analogj/scrutiny/webapp/backend/pkg/models" + "github.com/analogj/scrutiny/webapp/backend/pkg/web/handler" + "github.com/gin-gonic/gin" + "github.com/golang/mock/gomock" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" +) + +const testDeviceWWN = "0x5000cca264eb01d7" +const testNotifySettingKey = "user.metrics.notify_on_collector_error" + +// setupCollectorErrorRouter creates a minimal router for UploadCollectorError tests. +// device is the Device returned by the mock repo; if nil the mock returns an error (device not found). +func setupCollectorErrorRouter(t *testing.T, device *models.Device, notifyEnabled bool) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + + mockCtrl := gomock.NewController(t) + t.Cleanup(func() { mockCtrl.Finish() }) + + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + if device != nil { + fakeRepo.EXPECT().GetDeviceDetails(gomock.Any(), testDeviceWWN).Return(*device, nil).AnyTimes() + fakeConfig.EXPECT().GetBool(testNotifySettingKey).Return(notifyEnabled).AnyTimes() + } else { + fakeRepo.EXPECT().GetDeviceDetails(gomock.Any(), gomock.Any()).Return(models.Device{}, fmt.Errorf("not found")).AnyTimes() + fakeRepo.EXPECT().GetDeviceByWWN(gomock.Any(), gomock.Any()).Return(models.Device{}, fmt.Errorf("not found")).AnyTimes() + } + + logger := logrus.WithField("test", t.Name()) + r := gin.New() + r.Use(func(c *gin.Context) { + c.Set("CONFIG", fakeConfig) + c.Set("DEVICE_REPOSITORY", fakeRepo) + c.Set("LOGGER", logger) + c.Next() + }) + r.POST("/api/device/:id/collector-error", handler.UploadCollectorError) + return r +} + +// setupScanErrorRouter creates a minimal router for UploadCollectorScanError tests. +func setupScanErrorRouter(t *testing.T, notifyEnabled bool) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + + mockCtrl := gomock.NewController(t) + t.Cleanup(func() { mockCtrl.Finish() }) + + fakeConfig := mock_config.NewMockInterface(mockCtrl) + fakeRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + fakeConfig.EXPECT().GetBool(testNotifySettingKey).Return(notifyEnabled).AnyTimes() + + logger := logrus.WithField("test", t.Name()) + r := gin.New() + r.Use(func(c *gin.Context) { + c.Set("CONFIG", fakeConfig) + c.Set("DEVICE_REPOSITORY", fakeRepo) + c.Set("LOGGER", logger) + c.Next() + }) + r.POST("/api/collector/scan-error", handler.UploadCollectorScanError) + return r +} + +// --- UploadCollectorError tests --- + +func TestUploadCollectorError_BadJSON(t *testing.T) { + device := &models.Device{DeviceID: testDeviceWWN, DeviceName: "/dev/sda"} + router := setupCollectorErrorRouter(t, device, true) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/device/"+testDeviceWWN+"/collector-error", strings.NewReader("not json")) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestUploadCollectorError_NotifyDisabled(t *testing.T) { + device := &models.Device{DeviceID: testDeviceWWN, DeviceName: "/dev/sda"} + router := setupCollectorErrorRouter(t, device, false) + + w := httptest.NewRecorder() + body := `{"error_type":"xall","error_message":"smartctl exited with fatal code 2"}` + req, _ := http.NewRequest("POST", "/api/device/"+testDeviceWWN+"/collector-error", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) +} + +func TestUploadCollectorError_DeviceNotFound(t *testing.T) { + router := setupCollectorErrorRouter(t, nil, true) + + w := httptest.NewRecorder() + body := `{"error_type":"xall","error_message":"some error"}` + req, _ := http.NewRequest("POST", "/api/device/"+testDeviceWWN+"/collector-error", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusNotFound, w.Code) +} + +func TestUploadCollectorError_MutedDevice(t *testing.T) { + device := &models.Device{DeviceID: testDeviceWWN, DeviceName: "/dev/sda", Muted: true} + router := setupCollectorErrorRouter(t, device, true) + + w := httptest.NewRecorder() + body := `{"error_type":"xall","error_message":"smartctl exited with fatal code 2"}` + req, _ := http.NewRequest("POST", "/api/device/"+testDeviceWWN+"/collector-error", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + // Muted device: handler returns 200 but skips notification + require.Equal(t, http.StatusOK, w.Code) +} + +// --- UploadCollectorScanError tests --- + +func TestUploadCollectorScanError_BadJSON(t *testing.T) { + router := setupScanErrorRouter(t, true) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/collector/scan-error", strings.NewReader("not json")) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestUploadCollectorScanError_NotifyDisabled(t *testing.T) { + router := setupScanErrorRouter(t, false) + + w := httptest.NewRecorder() + body := `{"error_type":"scan","error_message":"permission denied"}` + req, _ := http.NewRequest("POST", "/api/collector/scan-error", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) +} From 8d243473a5ac336409737ec8d93f4033d0802793 Mon Sep 17 00:00:00 2001 From: Eder Date: Tue, 10 Mar 2026 20:26:05 -0400 Subject: [PATCH 4/7] fix(notify): pass device by pointer in NewCollectorError to satisfy gocritic lint --- webapp/backend/pkg/notify/notify.go | 2 +- webapp/backend/pkg/web/handler/upload_collector_error.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/webapp/backend/pkg/notify/notify.go b/webapp/backend/pkg/notify/notify.go index d05e0ed7..141118f7 100644 --- a/webapp/backend/pkg/notify/notify.go +++ b/webapp/backend/pkg/notify/notify.go @@ -986,7 +986,7 @@ func NewReport(logger logrus.FieldLogger, appconfig config.Interface, subject, m // device may be a zero-value Device when the error occurs before a device is fully identified // (e.g. during --scan). errorType is a short tag (e.g. "scan", "info", "xall") and // errorMessage is the human-readable description of the failure. -func NewCollectorError(logger logrus.FieldLogger, appconfig config.Interface, device models.Device, errorType, errorMessage string) Notify { +func NewCollectorError(logger logrus.FieldLogger, appconfig config.Interface, device *models.Device, errorType, errorMessage string) Notify { deviceIdentifier := device.DeviceName if deviceIdentifier == "" { deviceIdentifier = "(unknown device)" diff --git a/webapp/backend/pkg/web/handler/upload_collector_error.go b/webapp/backend/pkg/web/handler/upload_collector_error.go index 021889a2..80e1d620 100644 --- a/webapp/backend/pkg/web/handler/upload_collector_error.go +++ b/webapp/backend/pkg/web/handler/upload_collector_error.go @@ -78,7 +78,7 @@ func UploadCollectorError(c *gin.Context) { return } - errorNotify := notify.NewCollectorError(logger, appConfig, device, req.ErrorType, req.ErrorMessage) + errorNotify := notify.NewCollectorError(logger, appConfig, &device, req.ErrorType, req.ErrorMessage) errorNotify.LoadDatabaseUrls(c, deviceRepo) sendNotificationViaGate(c, logger, &errorNotify, deviceRepo) @@ -112,7 +112,7 @@ func UploadCollectorScanError(c *gin.Context) { // For scan errors we have no specific device. Populate DeviceName from the request // hint (if provided) so the notification subject is more informative than "(unknown device)". device := models.Device{DeviceName: req.DeviceName} - errorNotify := notify.NewCollectorError(logger, appConfig, device, req.ErrorType, req.ErrorMessage) + errorNotify := notify.NewCollectorError(logger, appConfig, &device, req.ErrorType, req.ErrorMessage) errorNotify.LoadDatabaseUrls(c, deviceRepo) sendNotificationViaGate(c, logger, &errorNotify, deviceRepo) From 199ff4d056941ce8a789918a89642923fcaed5b5 Mon Sep 17 00:00:00 2001 From: Eder Date: Tue, 10 Mar 2026 21:55:29 -0400 Subject: [PATCH 5/7] docs: update documentation to match current project state - CLAUDE.md (gitignored, local only): added 5 missing API endpoints (mqtt-sync, reset-status, missed-ping-timeout, collector-error, scan-error) - README.md: add Collector-Side Error Notifications to Extended Features - docs/DEPENDENCIES.md: update date, correct 6 Go package versions, add 6 new packages (paho.mqtt, fpdf, mapstructure/v2, jwt/v5, uuid, automaxprocs), mark Phase 2 updates complete - docs/TROUBLESHOOTING_NOTIFICATIONS.md: add collector error notifications section documenting per-device and scan-level error reporting (#334) - Deleted OVERRIDE_FIX_PROGRESS.md (was untracked, contained credentials) --- README.md | 1 + docs/DEPENDENCIES.md | 72 ++++++++++++++++++++------- docs/TROUBLESHOOTING_NOTIFICATIONS.md | 16 ++++++ 3 files changed, 71 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 4468ad23..7b1606e4 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,7 @@ These S.M.A.R.T hard drive self-tests can help you detect and replace failing ha - **Uptime Kuma Push Monitor** - Dedicated push-based health status updates to Uptime Kuma endpoints - **Seagate FARM Log Support** - Collect Field Accessible Reliability Metrics from Seagate Exos, IronWolf, and BarraCuda drives - **UI-Configurable Notification URLs** - Manage notification endpoints directly in the web UI (add, edit, test, delete) +- **Collector-Side Error Notifications** - Receive alerts when smartctl fails to read a drive during collection, not just when SMART attribute thresholds are exceeded # Migration from AnalogJ/scrutiny diff --git a/docs/DEPENDENCIES.md b/docs/DEPENDENCIES.md index 84944636..5a5ceef3 100644 --- a/docs/DEPENDENCIES.md +++ b/docs/DEPENDENCIES.md @@ -2,7 +2,7 @@ This document provides a comprehensive inventory of all project dependencies, their current versions, health status, and update recommendations. -Last updated: 2026-01-08 +Last updated: 2026-03-10 ## Table of Contents @@ -19,7 +19,7 @@ Last updated: 2026-01-08 | Category | Total | Current | Outdated | Vulnerable | |----------|-------|---------|----------|------------| -| Go Direct | 18 | 3 | 15 | TBD | +| Go Direct | 24 | 13 | 11 | TBD | | Go Indirect | ~60 | - | - | TBD | | NPM Production | 21 | 15 | 6 | 1 | | NPM Development | 25 | 22 | 3 | 0 | @@ -34,7 +34,9 @@ Last updated: 2026-01-08 |---------|------|----------|--------------|--------| | quill | npm | Moderate | GHSA-4943-9vgg-gr5r (XSS) | Deferred - requires breaking upgrade | -### Recently Fixed (2026-01-08) +### History + +#### Fixed/Updated (2026-01-08) | Package | Severity | Fix Applied | |---------|----------|-------------| @@ -43,6 +45,30 @@ Last updated: 2026-01-08 | semver | High | @angular-eslint update | | js-yaml | Moderate | @angular-eslint update | +#### Updated (2026-03-10) + +Go direct dependencies updated as part of feature development: + +| Package | Old | New | Reason | +|---------|-----|-----|--------| +| fatih/color | v1.15.0 | v1.18.0 | Routine update | +| nicholas-fedor/shoutrrr | v0.8.17 | v0.13.2 | Major update, new features | +| sirupsen/logrus | v1.6.0 | v1.8.3 | Routine update | +| spf13/viper | v1.15.0 | v1.21.0 | Routine update | +| stretchr/testify | v1.8.1 | v1.11.1 | Routine update | +| golang.org/x/sync | v0.3.0 | v0.19.0 | Routine update | + +New Go direct dependencies added: + +| Package | Version | Added For | +|---------|---------|-----------| +| github.com/eclipse/paho.mqtt.golang | v1.5.0 | Home Assistant MQTT Discovery | +| github.com/go-pdf/fpdf | v0.9.0 | Scheduled reports PDF export | +| github.com/go-viper/mapstructure/v2 | v2.5.0 | Successor to mitchellh/mapstructure | +| github.com/golang-jwt/jwt/v5 | v5.3.1 | API authentication | +| github.com/google/uuid | v1.3.0 | UUID generation | +| go.uber.org/automaxprocs | v1.6.0 | Auto GOMAXPROCS tuning | + --- ## Go Dependencies @@ -57,22 +83,28 @@ Last updated: 2026-01-08 | Package | Version | Latest | Gap | Priority | |---------|---------|--------|-----|----------| | github.com/analogj/go-util | v0.0.0-20190301 | v0.0.0-20210417 | 2 years | Low | -| github.com/nicholas-fedor/shoutrrr | v0.8.17 | v0.12.0 | Go 1.24 compatible | - | -| github.com/fatih/color | v1.15.0 | v1.18.0 | 3 minor | Low | -| github.com/gin-gonic/gin | v1.6.3 | v1.11.0 | 4 minor | High | +| github.com/eclipse/paho.mqtt.golang | v1.5.0 | v1.5.0 | Current | - | +| github.com/fatih/color | v1.18.0 | v1.18.0 | Current | - | +| github.com/gin-gonic/gin | v1.9.1 | v1.11.0 | 2 minor | High | | github.com/glebarez/sqlite | v1.4.5 | v1.11.0 | 6 minor | Medium | | github.com/go-gormigrate/gormigrate/v2 | v2.0.0 | v2.1.5 | 1 minor | Low | +| github.com/go-pdf/fpdf | v0.9.0 | v0.9.0 | Current | - | +| github.com/go-viper/mapstructure/v2 | v2.5.0 | v2.5.0 | Current | - | +| github.com/golang-jwt/jwt/v5 | v5.3.1 | v5.3.1 | Current | - | | github.com/golang/mock | v1.6.0 | v1.6.0 | Current | - | +| github.com/google/uuid | v1.3.0 | v1.6.0 | 3 minor | Low | | github.com/influxdata/influxdb-client-go/v2 | v2.9.0 | v2.14.0 | 5 minor | Medium | | github.com/jaypipes/ghw | v0.6.1 | v0.21.2 | 15 minor | High | | github.com/mitchellh/mapstructure | v1.5.0 | v1.5.0 | Current | - | +| github.com/nicholas-fedor/shoutrrr | v0.13.2 | v0.13.2 | Current | - | | github.com/prometheus/client_golang | v1.17.0 | v1.23.2 | 6 minor | Medium | | github.com/samber/lo | v1.25.0 | v1.52.0 | 27 minor | Medium | -| github.com/sirupsen/logrus | v1.6.0 | v1.9.3 | 3 minor | Low | -| github.com/spf13/viper | v1.15.0 | v1.21.0 | 6 minor | Medium | -| github.com/stretchr/testify | v1.8.1 | v1.11.1 | 3 minor | Low | +| github.com/sirupsen/logrus | v1.8.3 | v1.9.3 | 1 minor | Low | +| github.com/spf13/viper | v1.21.0 | v1.21.0 | Current | - | +| github.com/stretchr/testify | v1.11.1 | v1.11.1 | Current | - | | github.com/urfave/cli/v2 | v2.2.0 | v2.27.7 | 25 minor | High | -| golang.org/x/sync | v0.3.0 | v0.19.0 | 16 minor | Low | +| go.uber.org/automaxprocs | v1.6.0 | v1.6.0 | Current | - | +| golang.org/x/sync | v0.19.0 | v0.19.0 | Current | - | | gorm.io/gorm | v1.23.5 | v1.31.1 | 8 minor | High | ### Deprecated Indirect Dependencies @@ -168,14 +200,15 @@ Last updated: 2026-01-08 - [x] npm audit fix (non-breaking) - [x] Update @angular-eslint to v21.x -### Phase 2: Low-Risk Go Updates +### Phase 2: Low-Risk Go Updates (Complete) -```bash -go get -u github.com/sirupsen/logrus@latest -go get -u github.com/fatih/color@latest -go get -u github.com/stretchr/testify@latest -go get -u golang.org/x/sync@latest -``` +The following were updated as part of feature development (2026-03-10): +- fatih/color v1.15.0 → v1.18.0 +- sirupsen/logrus v1.6.0 → v1.8.3 +- stretchr/testify v1.8.1 → v1.11.1 +- golang.org/x/sync v0.3.0 → v0.19.0 +- spf13/viper v1.15.0 → v1.21.0 +- nicholas-fedor/shoutrrr v0.8.17 → v0.13.2 ### Phase 3: Angular Material Alignment @@ -209,7 +242,10 @@ go get -u gorm.io/gorm@latest - GitHub #69: Quill v2.0 upgrade (XSS vulnerability fix) - GitHub #70: moment.js migration to date-fns - TBD: Angular Material v21 upgrade -- TBD: Go dependency updates (multiple phases) +- Phase 2 Go updates: Complete (2026-03-10) +- Phase 3 Angular Material: Pending +- Phase 4 medium-risk Go updates: Pending +- Phase 5 high-risk Go updates: Pending --- diff --git a/docs/TROUBLESHOOTING_NOTIFICATIONS.md b/docs/TROUBLESHOOTING_NOTIFICATIONS.md index c08d70ca..45b97904 100644 --- a/docs/TROUBLESHOOTING_NOTIFICATIONS.md +++ b/docs/TROUBLESHOOTING_NOTIFICATIONS.md @@ -116,3 +116,19 @@ For setup instructions, see the [Home Assistant Integration](../README.md#home-a - **Check `client_id`**: If multiple Scrutiny instances connect to the same broker with the same `client_id`, they will kick each other off. Use unique client IDs (e.g., `scrutiny-server1`, `scrutiny-server2`). - **Check broker logs**: Most brokers log connection/disconnection events. Look for authentication failures or client ID conflicts. + +# Collector-Side Error Notifications + +Scrutiny notifies you when the collector fails to read SMART data from a drive via `smartctl`, or when a device scan fails entirely. This is distinct from SMART attribute threshold failures — it alerts you when the collection process itself errors. + +**Per-device errors** occur when `smartctl` successfully scans and finds a device but fails to retrieve its SMART data. These are reported to `POST /api/device/:wwn/collector-error` and include device context in the notification. + +**Scan-level errors** occur when `smartctl --scan` itself fails (no device context available). These are reported to `POST /api/collector/scan-error`. + +Common causes: + +- Collector not running as root or without `SYS_RAWIO`/`SYS_ADMIN` capabilities +- Drive physically failing to respond to commands +- Unsupported drive type or interface + +No additional configuration is required. If notification URLs are configured, collector errors are sent through the same channels as SMART failures. From 9cecd4865270259db75654697db62b2d63a12603 Mon Sep 17 00:00:00 2001 From: Eder <34795193+Starosdev@users.noreply.github.com> Date: Tue, 10 Mar 2026 22:29:36 -0400 Subject: [PATCH 6/7] feat(collector): add built-in cron scheduling (#333) (#343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Embeds `robfig/cron/v3` scheduler directly in the collector binary, removing the dependency on an external `crond` daemon - Adds `--cron-schedule`, `--run-startup`, and `--run-startup-sleep` CLI flags - Adds `cron.schedule`, `cron.run_on_startup`, and `cron.startup_sleep_secs` YAML config keys - Supports `COLLECTOR_CRON_SCHEDULE`, `COLLECTOR_RUN_STARTUP`, and `COLLECTOR_RUN_STARTUP_SLEEP` env vars - Without a schedule configured, single-run behavior is completely unchanged - Blocks on SIGINT/SIGTERM and drains the scheduler gracefully on shutdown - Documents the new config block in `example.collector.yaml` ## Linked Issues Closes #333 ## Test plan - [x] `go build ./collector/cmd/collector-metrics/` passes - [x] `go vet ./collector/...` passes - [x] `go mod tidy` run — `robfig/cron/v3` correctly classified as direct dependency - [ ] Manual: run collector with `--cron-schedule "* * * * *"` and verify recurring collection - [ ] Manual: run collector without `--cron-schedule` and verify single-run exit behavior is unchanged - [ ] Manual: verify `COLLECTOR_CRON_SCHEDULE` env var is picked up correctly --- .../collector-metrics/collector-metrics.go | 73 ++++++++++++++++++- collector/pkg/config/config.go | 4 + example.collector.yaml | 20 +++++ go.mod | 1 + go.sum | 2 + 5 files changed, 99 insertions(+), 1 deletion(-) diff --git a/collector/cmd/collector-metrics/collector-metrics.go b/collector/cmd/collector-metrics/collector-metrics.go index 446d15d8..436a04dd 100644 --- a/collector/cmd/collector-metrics/collector-metrics.go +++ b/collector/cmd/collector-metrics/collector-metrics.go @@ -6,7 +6,9 @@ import ( "io" "log" "os" + "os/signal" "strings" + "syscall" "time" _ "go.uber.org/automaxprocs" @@ -17,6 +19,7 @@ import ( "github.com/analogj/scrutiny/collector/pkg/errors" "github.com/analogj/scrutiny/webapp/backend/pkg/version" "github.com/fatih/color" + "github.com/robfig/cron/v3" "github.com/sirupsen/logrus" "github.com/urfave/cli/v2" ) @@ -138,6 +141,18 @@ OPTIONS: config.Set("api.token", c.String("api-token")) } + if c.IsSet("cron-schedule") { + config.Set("cron.schedule", c.String("cron-schedule")) + } + + if c.IsSet("run-startup") { + config.Set("cron.run_on_startup", c.Bool("run-startup")) + } + + if c.IsSet("run-startup-sleep") { + config.Set("cron.startup_sleep_secs", c.Int("run-startup-sleep")) + } + collectorLogger, logFile, err := CreateLogger(config) if logFile != nil { defer logFile.Close() @@ -168,7 +183,44 @@ OPTIONS: return err } - return metricCollector.Run() + cronSchedule := config.GetString("cron.schedule") + if cronSchedule == "" { + // No schedule configured: run once and exit (original behavior). + return metricCollector.Run() + } + + // Schedule configured: run on a recurring cron schedule. + runFunc := func() { + if runErr := metricCollector.Run(); runErr != nil { + collectorLogger.Errorf("collector run failed: %v", runErr) + } + } + + if config.GetBool("cron.run_on_startup") { + sleepSecs := config.GetInt("cron.startup_sleep_secs") + if sleepSecs > 0 { + collectorLogger.Infof("Waiting %d seconds before startup run", sleepSecs) + time.Sleep(time.Duration(sleepSecs) * time.Second) + } + collectorLogger.Info("Running startup collection before first scheduled tick") + runFunc() + } + + c2 := cron.New() + _, err = c2.AddFunc(cronSchedule, runFunc) + if err != nil { + return fmt.Errorf("invalid cron schedule %q: %w", cronSchedule, err) + } + c2.Start() + collectorLogger.Infof("Collector scheduled with cron expression: %s", cronSchedule) + + quit := make(chan os.Signal, 1) + signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM) + <-quit + collectorLogger.Info("Shutting down collector scheduler") + ctx := c2.Stop() + <-ctx.Done() + return nil }, Flags: []cli.Flag{ @@ -207,6 +259,25 @@ OPTIONS: Usage: "API token for authenticating with the Scrutiny server", EnvVars: []string{"COLLECTOR_METRICS_API_TOKEN", "COLLECTOR_API_TOKEN"}, }, + + &cli.StringFlag{ + Name: "cron-schedule", + Usage: "Cron expression for scheduled collection (e.g. \"0 * * * *\"). If not set, the collector runs once and exits.", + EnvVars: []string{"COLLECTOR_CRON_SCHEDULE"}, + }, + + &cli.BoolFlag{ + Name: "run-startup", + Usage: "Run an immediate collection on startup before the first scheduled tick", + EnvVars: []string{"COLLECTOR_RUN_STARTUP"}, + }, + + &cli.IntFlag{ + Name: "run-startup-sleep", + Usage: "Seconds to sleep before the startup run (requires --run-startup)", + Value: 0, + EnvVars: []string{"COLLECTOR_RUN_STARTUP_SLEEP"}, + }, }, }, }, diff --git a/collector/pkg/config/config.go b/collector/pkg/config/config.go index ad6eab45..09d47ff6 100644 --- a/collector/pkg/config/config.go +++ b/collector/pkg/config/config.go @@ -72,6 +72,10 @@ func (c *configuration) Init() error { c.SetDefault("allow_listed_devices", []string{}) + c.SetDefault("cron.schedule", "") + c.SetDefault("cron.run_on_startup", false) + c.SetDefault("cron.startup_sleep_secs", 0) + //if you want to load a non-standard location system config file (~/drawbridge.yml), use ReadConfig c.SetConfigType("yaml") //c.SetConfigName("drawbridge") diff --git a/example.collector.yaml b/example.collector.yaml index dfd5ea56..21f91359 100644 --- a/example.collector.yaml +++ b/example.collector.yaml @@ -159,6 +159,26 @@ devices: #commands: # performance_fio_bin: 'fio' # Path to fio binary +######################################################################################################################## +# Built-in Cron Scheduling +# +# The collector can run on a recurring schedule without an external cron daemon. +# When a schedule is configured, the process stays alive and collects on each tick. +# Without a schedule, the collector runs once and exits (default behavior). +# +# Environment variable overrides: +# cron.schedule -> COLLECTOR_CRON_SCHEDULE +# cron.run_on_startup -> COLLECTOR_RUN_STARTUP +# cron.startup_sleep_secs -> COLLECTOR_RUN_STARTUP_SLEEP +# +######################################################################################################################## + +#cron: +# schedule: "0 * * * *" # Standard 5-field cron expression. Leave empty to run once and exit. +# run_on_startup: false # Run an immediate collection on startup before the first scheduled tick. +# startup_sleep_secs: 0 # Seconds to sleep before the startup run (useful for letting the system settle). + + ######################################################################################################################## # FEATURES COMING SOON # diff --git a/go.mod b/go.mod index 3f1ef204..b912f1be 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/mitchellh/mapstructure v1.5.0 github.com/nicholas-fedor/shoutrrr v0.13.2 github.com/prometheus/client_golang v1.17.0 + github.com/robfig/cron/v3 v3.0.1 github.com/samber/lo v1.25.0 github.com/sirupsen/logrus v1.8.3 github.com/spf13/viper v1.21.0 diff --git a/go.sum b/go.sum index 7361e23a..cbbe7619 100644 --- a/go.sum +++ b/go.sum @@ -263,6 +263,8 @@ github.com/prometheus/procfs v0.11.1 h1:xRC8Iq1yyca5ypa9n1EZnWZkt7dwcoRPQwX/5gwa github.com/prometheus/procfs v0.11.1/go.mod h1:eesXgaPo1q7lBpVMoMy0ZOFTth9hBn4W/y0/p/ScXhY= github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0 h1:OdAsTTz6OkFY5QxjkYwrChwuRruF69c169dPK26NUlk= github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= +github.com/robfig/cron/v3 v3.0.1 h1:WdRxkvbJztn8LMz/QEvLN5sBU+xKpSqwwUO1Pjr4qDs= +github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro= github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= From 6e3e48981c98859d3bbf7eedb47cec14955dddef Mon Sep 17 00:00:00 2001 From: Eder <34795193+Starosdev@users.noreply.github.com> Date: Wed, 11 Mar 2026 00:16:51 -0400 Subject: [PATCH 7/7] fix(overrides): address gaps in attribute override system (#345) (#346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Add DB migration (`m20260411000000`) to enforce a unique constraint on `(protocol, attribute_id, wwn)` in `attribute_overrides`, deduplicating any existing rows first - Update `AttributeOverride` model GORM tags to use `uniqueIndex` to match the new constraint - Add `GetAllOverridesForDisplay` repository method that merges UI overrides (DB) with config-file overrides, so the settings panel shows everything currently active — config overrides appear as read-only (source: `config`, ID: 0) - Add corresponding mock method for `GetAllOverridesForDisplay` - Refactor `SaveAttributeOverride` to extract validation helpers (`validateAttributeOverride`, `validateForceStatus`, `validateThresholds`) to keep cognitive complexity in bounds - Add input validation: at least one threshold required in custom mode, `warn_above < fail_above`, non-negative values, and WWN hex-format check for all action types - Fix misleading comment in `applier.go` that implied thresholds can be combined with `force_status` — they are mutually exclusive at the call site - Default new override form action to custom threshold (empty string) so threshold fields are visible on first open instead of `ignore` - Add 18 handler unit tests for `GetAttributeOverrides` and `SaveAttributeOverride` (mock-based, no InfluxDB required) ## Linked Issues Closes #345 ## Test plan - [x] `go build ./...` passes cleanly - [x] All 18 new handler tests pass (`go test -v -run TestAttributeOverride ./webapp/backend/pkg/web/handler/...`) - [x] All overrides package unit tests pass (`go test -v ./webapp/backend/pkg/overrides/...`) - [x] Migration ordering verified (m20260410 before m20260411 in slice) - [x] WWN validation tested for all three action types - [x] Threshold edge cases verified (`warn >= fail` correctly rejected) --- webapp/backend/pkg/database/interface.go | 5 + .../m20260411000000/attribute_override.go | 8 + .../pkg/database/mock/mock_database.go | 15 + .../scrutiny_repository_migrations.go | 26 ++ .../database/scrutiny_repository_overrides.go | 54 +++ .../backend/pkg/models/attribute_override.go | 6 +- webapp/backend/pkg/overrides/applier.go | 8 +- .../pkg/web/handler/attribute_overrides.go | 98 +++-- .../web/handler/attribute_overrides_test.go | 379 ++++++++++++++++++ .../dashboard-settings.component.ts | 4 +- 10 files changed, 562 insertions(+), 41 deletions(-) create mode 100644 webapp/backend/pkg/database/migrations/m20260411000000/attribute_override.go create mode 100644 webapp/backend/pkg/web/handler/attribute_overrides_test.go diff --git a/webapp/backend/pkg/database/interface.go b/webapp/backend/pkg/database/interface.go index 15752f92..8dc19f5b 100644 --- a/webapp/backend/pkg/database/interface.go +++ b/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 diff --git a/webapp/backend/pkg/database/migrations/m20260411000000/attribute_override.go b/webapp/backend/pkg/database/migrations/m20260411000000/attribute_override.go new file mode 100644 index 00000000..38f33eea --- /dev/null +++ b/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. diff --git a/webapp/backend/pkg/database/mock/mock_database.go b/webapp/backend/pkg/database/mock/mock_database.go index 5de32b07..ddd285e4 100644 --- a/webapp/backend/pkg/database/mock/mock_database.go +++ b/webapp/backend/pkg/database/mock/mock_database.go @@ -127,6 +127,21 @@ func (mr *MockDeviceRepoMockRecorder) GetAttributeOverrides(ctx interface{}) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAttributeOverrides", reflect.TypeOf((*MockDeviceRepo)(nil).GetAttributeOverrides), ctx) } +// GetAllOverridesForDisplay mocks base method. +func (m *MockDeviceRepo) GetAllOverridesForDisplay(ctx context.Context) ([]models.AttributeOverride, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetAllOverridesForDisplay", ctx) + ret0, _ := ret[0].([]models.AttributeOverride) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetAllOverridesForDisplay indicates an expected call of GetAllOverridesForDisplay. +func (mr *MockDeviceRepoMockRecorder) GetAllOverridesForDisplay(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllOverridesForDisplay", reflect.TypeOf((*MockDeviceRepo)(nil).GetAllOverridesForDisplay), ctx) +} + // GetAvailableInfluxDBBuckets mocks base method. func (m *MockDeviceRepo) GetAvailableInfluxDBBuckets(ctx context.Context) ([]string, error) { m.ctrl.T.Helper() diff --git a/webapp/backend/pkg/database/scrutiny_repository_migrations.go b/webapp/backend/pkg/database/scrutiny_repository_migrations.go index 0e0f0ea8..651cb1b3 100644 --- a/webapp/backend/pkg/database/scrutiny_repository_migrations.go +++ b/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 { diff --git a/webapp/backend/pkg/database/scrutiny_repository_overrides.go b/webapp/backend/pkg/database/scrutiny_repository_overrides.go index 19b0ebc9..44a30dc3 100644 --- a/webapp/backend/pkg/database/scrutiny_repository_overrides.go +++ b/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 { diff --git a/webapp/backend/pkg/models/attribute_override.go b/webapp/backend/pkg/models/attribute_override.go index 14b458a6..0e0d75bd 100644 --- a/webapp/backend/pkg/models/attribute_override.go +++ b/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 diff --git a/webapp/backend/pkg/overrides/applier.go b/webapp/backend/pkg/overrides/applier.go index 19d2fd2b..6b9b7220 100644 --- a/webapp/backend/pkg/overrides/applier.go +++ b/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 diff --git a/webapp/backend/pkg/web/handler/attribute_overrides.go b/webapp/backend/pkg/web/handler/attribute_overrides.go index e2c2da29..24e4f464 100644 --- a/webapp/backend/pkg/web/handler/attribute_overrides.go +++ b/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" diff --git a/webapp/backend/pkg/web/handler/attribute_overrides_test.go b/webapp/backend/pkg/web/handler/attribute_overrides_test.go new file mode 100644 index 00000000..b816dd68 --- /dev/null +++ b/webapp/backend/pkg/web/handler/attribute_overrides_test.go @@ -0,0 +1,379 @@ +package handler_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + mock_database "github.com/analogj/scrutiny/webapp/backend/pkg/database/mock" + "github.com/analogj/scrutiny/webapp/backend/pkg/models" + "github.com/analogj/scrutiny/webapp/backend/pkg/web/handler" + "github.com/gin-gonic/gin" + "github.com/golang/mock/gomock" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" +) + +// setupOverridesRouter creates a minimal Gin router wired to the attribute override handlers. +func setupOverridesRouter(t *testing.T, mockRepo *mock_database.MockDeviceRepo) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + logger := logrus.WithField("test", t.Name()) + + r := gin.New() + r.Use(func(c *gin.Context) { + c.Set("LOGGER", logger) + c.Set("DEVICE_REPOSITORY", mockRepo) + c.Next() + }) + r.GET("/api/settings/overrides", handler.GetAttributeOverrides) + r.POST("/api/settings/overrides", handler.SaveAttributeOverride) + r.DELETE("/api/settings/overrides/:id", handler.DeleteAttributeOverride) + return r +} + +// --- GetAttributeOverrides --- + +func TestGetAttributeOverrides_ReturnsEmptyList(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + mockRepo.EXPECT().GetAllOverridesForDisplay(gomock.Any()).Return([]models.AttributeOverride{}, nil) + + router := setupOverridesRouter(t, mockRepo) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/api/settings/overrides", nil) + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Equal(t, true, response["success"]) + require.NotNil(t, response["data"]) +} + +func TestGetAttributeOverrides_ReturnsBothUIAndConfigOverrides(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + warnVal := int64(10) + mockRepo.EXPECT().GetAllOverridesForDisplay(gomock.Any()).Return([]models.AttributeOverride{ + {Protocol: "ATA", AttributeId: "5", Action: "ignore", Source: "ui"}, + {Protocol: "NVMe", AttributeId: "media_errors", WarnAbove: &warnVal, Source: "config"}, + }, nil) + + router := setupOverridesRouter(t, mockRepo) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/api/settings/overrides", nil) + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Equal(t, true, response["success"]) + data, ok := response["data"].([]interface{}) + require.True(t, ok) + require.Len(t, data, 2) +} + +// --- SaveAttributeOverride validation --- + +func TestSaveAttributeOverride_MissingProtocol(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"attribute_id": "5", "action": "ignore"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Equal(t, false, response["success"]) + require.Contains(t, response["error"], "Protocol") +} + +func TestSaveAttributeOverride_MissingAttributeId(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "action": "ignore"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestSaveAttributeOverride_InvalidProtocol(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "SATA", "attribute_id": "5", "action": "ignore"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Contains(t, response["error"], "protocol") +} + +func TestSaveAttributeOverride_InvalidAction(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "unknown"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestSaveAttributeOverride_ForceStatusMissingStatus(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "force_status"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Contains(t, response["error"], "Status") +} + +func TestSaveAttributeOverride_ForceStatusInvalidStatus(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "force_status", "status": "unknown"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestSaveAttributeOverride_CustomThreshold_NoThresholds(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": ""}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Contains(t, response["error"], "warn_above or fail_above") +} + +func TestSaveAttributeOverride_CustomThreshold_NegativeWarn(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "", "warn_above": -1}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Contains(t, response["error"], "non-negative") +} + +func TestSaveAttributeOverride_CustomThreshold_WarnNotLessThanFail(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "", "warn_above": 10, "fail_above": 5}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Contains(t, response["error"], "warn_above must be less than fail_above") +} + +func TestSaveAttributeOverride_InvalidWWN(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "ignore", "wwn": "not-a-wwn"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Contains(t, response["error"], "WWN") +} + +func TestSaveAttributeOverride_ValidIgnore(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + mockRepo.EXPECT().SaveAttributeOverride(gomock.Any(), gomock.Any()).Return(nil) + mockRepo.EXPECT().GetDevices(gomock.Any()).Return([]models.Device{}, nil) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "ignore"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Equal(t, true, response["success"]) +} + +func TestSaveAttributeOverride_ValidForceStatus(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + mockRepo.EXPECT().SaveAttributeOverride(gomock.Any(), gomock.Any()).Return(nil) + mockRepo.EXPECT().GetDevices(gomock.Any()).Return([]models.Device{}, nil) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "NVMe", "attribute_id": "media_errors", "action": "force_status", "status": "passed"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) +} + +func TestSaveAttributeOverride_ValidCustomThreshold(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + mockRepo.EXPECT().SaveAttributeOverride(gomock.Any(), gomock.Any()).Return(nil) + mockRepo.EXPECT().GetDevices(gomock.Any()).Return([]models.Device{}, nil) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "187", "action": "", "warn_above": 5, "fail_above": 10}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) +} + +func TestSaveAttributeOverride_ValidWWN(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + mockRepo.EXPECT().SaveAttributeOverride(gomock.Any(), gomock.Any()).Return(nil) + mockRepo.EXPECT().GetDevices(gomock.Any()).Return([]models.Device{}, nil) + + router := setupOverridesRouter(t, mockRepo) + + body := strings.NewReader(`{"protocol": "ATA", "attribute_id": "5", "action": "ignore", "wwn": "0x5000cca264eb01d7"}`) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/api/settings/overrides", body) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) +} + +// --- DeleteAttributeOverride --- + +func TestDeleteAttributeOverride_InvalidID(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + + router := setupOverridesRouter(t, mockRepo) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("DELETE", "/api/settings/overrides/not-a-number", nil) + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestDeleteAttributeOverride_Success(t *testing.T) { + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + mockRepo := mock_database.NewMockDeviceRepo(mockCtrl) + mockRepo.EXPECT().GetAttributeOverrideByID(gomock.Any(), uint(1)).Return(&models.AttributeOverride{ + Protocol: "ATA", AttributeId: "5", Action: "ignore", Source: "ui", + }, nil) + mockRepo.EXPECT().DeleteAttributeOverride(gomock.Any(), uint(1)).Return(nil) + mockRepo.EXPECT().GetDevices(gomock.Any()).Return([]models.Device{}, nil) + + router := setupOverridesRouter(t, mockRepo) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("DELETE", "/api/settings/overrides/1", nil) + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + var response map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response)) + require.Equal(t, true, response["success"]) +} diff --git a/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts b/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts index 3d617630..a642941a 100644 --- a/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts +++ b/webapp/frontend/src/app/layout/common/dashboard-settings/dashboard-settings.component.ts @@ -100,7 +100,7 @@ export class DashboardSettingsComponent implements OnInit { newOverride: Partial = { protocol: 'ATA', attribute_id: '', - action: 'ignore' + action: '' }; // Notification URL management @@ -240,7 +240,7 @@ export class DashboardSettingsComponent implements OnInit { this.newOverride = { protocol: 'ATA', attribute_id: '', - action: 'ignore' + action: '' }; }); }