fix(ingestor): observer metadata nested stats + SNR/RSSI case fallback (#336)

## Problem Two data integrity bugs in the Go ingestor cause observer metadata and signal quality data to be missing for all Go-backend users. ### #320 — Observer metadata never populated `extractObserverMeta()` reads `battery_mv`, `uptime_secs`, and `noise_floor` from the **top level** of the MQTT status message. However, the actual MQTT payload nests these under a `stats` object: ```json { "status": "online", "origin": "ObserverName", "model": "Heltec V3", "firmware_version": "v1.14.0-9f1a3ea", "stats": { "battery_mv": 4174, "uptime_secs": 80277, "noise_floor": -110 } } ``` Result: battery, uptime, and noise floor are always NULL in the database. ### #321 — SNR and RSSI always missing on raw packets The raw packet handler reads `msg["SNR"]` and `msg["RSSI"]` (uppercase only). Some MQTT bridges send these as lowercase `snr`/`rssi`. The companion BLE handler already has a case-insensitive fallback — the raw packet path did not. Result: SNR/RSSI are NULL for all raw packet observations from bridges that use lowercase keys. ## Fix ### #320 — Nested stats with top-level fallback - Added `nestedOrTopLevel()` helper that checks `msg["stats"][key]` first, then `msg[key]` - `extractObserverMeta` now uses this helper for `battery_mv`, `uptime_secs`, `noise_floor` - Top-level fallback preserved for backward compatibility with bridges that flatten the structure - Safe type assertion: `stats, _ := msg["stats"].(map[string]interface{})` — no crash if stats is missing or wrong type ### #321 — Lowercase SNR/RSSI fallback - Raw packet handler now uses `else if` to check lowercase `snr`/`rssi` when uppercase keys are absent - Matches the pattern already used in the companion channel and direct message handlers ## Tests 10 new test cases added: | Test | What it verifies | |------|-----------------| | `TestExtractObserverMetaNestedStats` | All 5 fields populated from nested stats object | | `TestExtractObserverMetaNestedStatsPrecedence` | Nested stats wins over top-level when both present | | `TestExtractObserverMetaFlatFallback` | Flat structure still works (backward compat) | | `TestExtractObserverMetaEmptyStats` | Empty stats object — no crash, model still works | | `TestExtractObserverMetaStatsNotAMap` | stats is a string — no crash, falls back to top-level | | `TestExtractObserverMetaNoiseFloorFloat` | Float precision preserved (noise_floor REAL migration) | | `TestHandleMessageWithLowercaseSNRRSSI` | Lowercase snr/rssi both stored correctly | | `TestHandleMessageSNRRSSIUppercaseWins` | When both cases present, uppercase takes precedence | | `TestHandleMessageNoSNRRSSI` | Neither key present — nil, no crash | | Existing `TestExtractObserverMeta` | Still passes (flat structure backward compat) | All tests pass: `go test ./... -count=1` and `go vet ./...` clean. Closes #320 Closes #321 --------- Co-authored-by: Kpa-clawbot <259247574+Kpa-clawbot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-05-25 18:44:04 +00:00 · 2026-03-31 17:53:04 -07:00
parent 38e5f02a00
commit 4898541bce
3 changed files with 235 additions and 3 deletions
@@ -1313,3 +1313,147 @@ func TestTelemetryMigrationAddsColumns(t *testing.T) {
 		t.Errorf("migration node_telemetry_v1 should be recorded, count=%d", count)
 	}
 }
+
+// --- Bug #320: Observer metadata nested stats ---
+
+func TestExtractObserverMetaNestedStats(t *testing.T) {
+	// Real-world MQTT status payload: stats fields nested under "stats"
+	msg := map[string]interface{}{
+		"status":           "online",
+		"origin":           "ObserverName",
+		"model":            "Heltec V3",
+		"firmware_version": "v1.14.0-9f1a3ea",
+		"stats": map[string]interface{}{
+			"battery_mv":  4174.0,
+			"uptime_secs": 80277.0,
+			"noise_floor": -110.0,
+		},
+	}
+	meta := extractObserverMeta(msg)
+	if meta == nil {
+		t.Fatal("expected non-nil meta")
+	}
+	if meta.Model == nil || *meta.Model != "Heltec V3" {
+		t.Errorf("Model=%v, want Heltec V3", meta.Model)
+	}
+	if meta.Firmware == nil || *meta.Firmware != "v1.14.0-9f1a3ea" {
+		t.Errorf("Firmware=%v, want v1.14.0-9f1a3ea", meta.Firmware)
+	}
+	if meta.BatteryMv == nil || *meta.BatteryMv != 4174 {
+		t.Errorf("BatteryMv=%v, want 4174", meta.BatteryMv)
+	}
+	if meta.UptimeSecs == nil || *meta.UptimeSecs != 80277 {
+		t.Errorf("UptimeSecs=%v, want 80277", meta.UptimeSecs)
+	}
+	if meta.NoiseFloor == nil || *meta.NoiseFloor != -110.0 {
+		t.Errorf("NoiseFloor=%v, want -110", meta.NoiseFloor)
+	}
+}
+
+func TestExtractObserverMetaNestedStatsPrecedence(t *testing.T) {
+	// If stats has a value AND top-level has a value, nested wins
+	msg := map[string]interface{}{
+		"battery_mv":  9999.0, // top-level (stale/wrong)
+		"noise_floor": -120.0, // top-level (stale/wrong)
+		"stats": map[string]interface{}{
+			"battery_mv":  4174.0, // nested (correct)
+			"noise_floor": -110.5, // nested (correct)
+		},
+	}
+	meta := extractObserverMeta(msg)
+	if meta == nil {
+		t.Fatal("expected non-nil meta")
+	}
+	if meta.BatteryMv == nil || *meta.BatteryMv != 4174 {
+		t.Errorf("BatteryMv=%v, want 4174 (nested should win over top-level)", meta.BatteryMv)
+	}
+	if meta.NoiseFloor == nil || *meta.NoiseFloor != -110.5 {
+		t.Errorf("NoiseFloor=%v, want -110.5 (nested should win over top-level)", meta.NoiseFloor)
+	}
+}
+
+func TestExtractObserverMetaFlatFallback(t *testing.T) {
+	// Backward compatibility: flat structure (no stats object) still works
+	msg := map[string]interface{}{
+		"battery_mv":  3500.0,
+		"uptime_secs": 86400.0,
+		"noise_floor": -115.5,
+	}
+	meta := extractObserverMeta(msg)
+	if meta == nil {
+		t.Fatal("expected non-nil meta for flat structure")
+	}
+	if meta.BatteryMv == nil || *meta.BatteryMv != 3500 {
+		t.Errorf("BatteryMv=%v, want 3500", meta.BatteryMv)
+	}
+	if meta.UptimeSecs == nil || *meta.UptimeSecs != 86400 {
+		t.Errorf("UptimeSecs=%v, want 86400", meta.UptimeSecs)
+	}
+	if meta.NoiseFloor == nil || *meta.NoiseFloor != -115.5 {
+		t.Errorf("NoiseFloor=%v, want -115.5", meta.NoiseFloor)
+	}
+}
+
+func TestExtractObserverMetaEmptyStats(t *testing.T) {
+	// Empty stats object should not crash, top-level fallback still applies
+	msg := map[string]interface{}{
+		"model": "T-Beam",
+		"stats": map[string]interface{}{},
+	}
+	meta := extractObserverMeta(msg)
+	if meta == nil {
+		t.Fatal("expected non-nil meta (model is present)")
+	}
+	if meta.Model == nil || *meta.Model != "T-Beam" {
+		t.Errorf("Model=%v, want T-Beam", meta.Model)
+	}
+	if meta.BatteryMv != nil {
+		t.Errorf("BatteryMv should be nil, got %v", *meta.BatteryMv)
+	}
+}
+
+func TestExtractObserverMetaStatsNotAMap(t *testing.T) {
+	// stats field is not a map (e.g., string) — should not crash, fall back to top-level
+	msg := map[string]interface{}{
+		"stats":      "invalid",
+		"battery_mv": 3700.0,
+	}
+	meta := extractObserverMeta(msg)
+	if meta == nil {
+		t.Fatal("expected non-nil meta")
+	}
+	if meta.BatteryMv == nil || *meta.BatteryMv != 3700 {
+		t.Errorf("BatteryMv=%v, want 3700 (top-level fallback when stats is not a map)", meta.BatteryMv)
+	}
+}
+
+func TestExtractObserverMetaNoiseFloorFloat(t *testing.T) {
+	// noise_floor migrated to REAL — verify float precision preserved
+	msg := map[string]interface{}{
+		"stats": map[string]interface{}{
+			"noise_floor": -108.75,
+		},
+	}
+	meta := extractObserverMeta(msg)
+	if meta == nil {
+		t.Fatal("expected non-nil meta")
+	}
+	if meta.NoiseFloor == nil || *meta.NoiseFloor != -108.75 {
+		t.Errorf("NoiseFloor=%v, want -108.75", meta.NoiseFloor)
+	}
+}
+
+func TestExtractObserverMetaNestedNilSkipsTopLevel(t *testing.T) {
+	// JSON {"stats": {"battery_mv": null}} decodes to nil value in the map.
+	// Nested nil should suppress top-level fallback (nested wins semantics).
+	msg := map[string]interface{}{
+		"battery_mv": 3700.0,
+		"stats": map[string]interface{}{
+			"battery_mv": nil,
+		},
+	}
+	meta := extractObserverMeta(msg)
+	if meta != nil && meta.BatteryMv != nil {
+		t.Error("nested nil should suppress top-level fallback")
+	}
+}
@@ -241,11 +241,19 @@ func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message,
 			if f, ok := toFloat64(v); ok {
 				mqttMsg.SNR = &f
 			}
+		} else if v, ok := msg["snr"]; ok {
+			if f, ok := toFloat64(v); ok {
+				mqttMsg.SNR = &f
+			}
 		}
 		if v, ok := msg["RSSI"]; ok {
 			if f, ok := toFloat64(v); ok {
 				mqttMsg.RSSI = &f
 			}
+		} else if v, ok := msg["rssi"]; ok {
+			if f, ok := toFloat64(v); ok {
+				mqttMsg.RSSI = &f
+			}
 		}
 		if v, ok := msg["origin"].(string); ok {
 			mqttMsg.Origin = v
@@ -511,21 +519,25 @@ func extractObserverMeta(msg map[string]interface{}) *ObserverMeta {
 		hasData = true
 	}

-	if v, ok := msg["battery_mv"]; ok {
+	// Stats fields may be nested under a "stats" object or at top level.
+	// Try nested first, fall back to top-level for backward compatibility.
+	stats, _ := msg["stats"].(map[string]interface{})
+
+	if v := nestedOrTopLevel(stats, msg, "battery_mv"); v != nil {
 		if f, ok := toFloat64(v); ok {
 			iv := int(math.Round(f))
 			meta.BatteryMv = &iv
 			hasData = true
 		}
 	}
-	if v, ok := msg["uptime_secs"]; ok {
+	if v := nestedOrTopLevel(stats, msg, "uptime_secs"); v != nil {
 		if f, ok := toFloat64(v); ok {
 			iv := int64(math.Round(f))
 			meta.UptimeSecs = &iv
 			hasData = true
 		}
 	}
-	if v, ok := msg["noise_floor"]; ok {
+	if v := nestedOrTopLevel(stats, msg, "noise_floor"); v != nil {
 		if f, ok := toFloat64(v); ok {
 			meta.NoiseFloor = &f
 			hasData = true
@@ -538,6 +550,19 @@ func extractObserverMeta(msg map[string]interface{}) *ObserverMeta {
 	return meta
 }

+// nestedOrTopLevel looks up a key in the nested map first, then the top-level map.
+func nestedOrTopLevel(nested, toplevel map[string]interface{}, key string) interface{} {
+	if nested != nil {
+		if v, ok := nested[key]; ok {
+			return v
+		}
+	}
+	if v, ok := toplevel[key]; ok {
+		return v
+	}
+	return nil
+}
+
 func firstNonEmpty(vals ...string) string {
 	for _, v := range vals {
 		if v != "" {
@@ -623,3 +623,66 @@ func TestLoadChannelKeysSkipExplicit(t *testing.T) {
 		t.Errorf("#General = %q, want my_explicit_key", keys["#General"])
 	}
 }
+
+// --- Bug #321: SNR/RSSI case-insensitive fallback ---
+
+func TestHandleMessageWithLowercaseSNRRSSI(t *testing.T) {
+	store := newTestStore(t)
+	source := MQTTSource{Name: "test"}
+
+	rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976"
+	payload := []byte(`{"raw":"` + rawHex + `","snr":5.5,"rssi":-102}`)
+	msg := &mockMessage{topic: "meshcore/SJC/obs1/packets", payload: payload}
+
+	handleMessage(store, "test", source, msg, nil, nil)
+
+	var snr, rssi *float64
+	store.db.QueryRow("SELECT snr, rssi FROM observations LIMIT 1").Scan(&snr, &rssi)
+	if snr == nil || *snr != 5.5 {
+		t.Errorf("snr=%v, want 5.5 (lowercase key)", snr)
+	}
+	if rssi == nil || *rssi != -102 {
+		t.Errorf("rssi=%v, want -102 (lowercase key)", rssi)
+	}
+}
+
+func TestHandleMessageSNRRSSIUppercaseWins(t *testing.T) {
+	store := newTestStore(t)
+	source := MQTTSource{Name: "test"}
+
+	// Both uppercase and lowercase present — uppercase should take precedence
+	rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976"
+	payload := []byte(`{"raw":"` + rawHex + `","SNR":7.2,"snr":1.0,"RSSI":-95,"rssi":-50}`)
+	msg := &mockMessage{topic: "meshcore/SJC/obs1/packets", payload: payload}
+
+	handleMessage(store, "test", source, msg, nil, nil)
+
+	var snr, rssi *float64
+	store.db.QueryRow("SELECT snr, rssi FROM observations LIMIT 1").Scan(&snr, &rssi)
+	if snr == nil || *snr != 7.2 {
+		t.Errorf("snr=%v, want 7.2 (uppercase should take precedence)", snr)
+	}
+	if rssi == nil || *rssi != -95 {
+		t.Errorf("rssi=%v, want -95 (uppercase should take precedence)", rssi)
+	}
+}
+
+func TestHandleMessageNoSNRRSSI(t *testing.T) {
+	store := newTestStore(t)
+	source := MQTTSource{Name: "test"}
+
+	rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976"
+	payload := []byte(`{"raw":"` + rawHex + `"}`)
+	msg := &mockMessage{topic: "meshcore/SJC/obs1/packets", payload: payload}
+
+	handleMessage(store, "test", source, msg, nil, nil)
+
+	var snr, rssi *float64
+	store.db.QueryRow("SELECT snr, rssi FROM observations LIMIT 1").Scan(&snr, &rssi)
+	if snr != nil {
+		t.Errorf("snr should be nil when not present, got %v", *snr)
+	}
+	if rssi != nil {
+		t.Errorf("rssi should be nil when not present, got %v", *rssi)
+	}
+}