diff --git a/.superpowers/sdd/task-2-report.md b/.superpowers/sdd/task-2-report.md index 3a22e932..18d07785 100644 --- a/.superpowers/sdd/task-2-report.md +++ b/.superpowers/sdd/task-2-report.md @@ -50,3 +50,13 @@ Result: - `SamplesFromEnvelope` now carries `EventTime` and `DeviceID`, which the new candidate mapping needs. - The upsert SQL is focused on the candidate table and reuses the shared `Execer` interface. - No unrelated stats files were modified. + +## Review Fix Addendum +- Preserved manual `platform_name` values when the candidate upsert receives blank runtime input by switching to `COALESCE(NULLIF(TRIM(VALUES(platform_name)), ''), platform_name)`. +- Bootstrapped `vehicle_data_source`, `vehicle_daily_mileage_source`, and `vehicle_daily_mileage` before running alter statements in `Writer.EnsureSchema`. +- Added a defensive blank `SourceIP` guard in `UpsertSourceMileage` so malformed candidate rows are skipped instead of written. +- Removed the SQL comment that existed only to satisfy a string-match test and updated the focused assertions to check the real `daily_mileage_km` and `platform_name` SQL expressions. +- Verification run: + - `go test ./internal/stats -run 'TestUpsertSourceMileage|TestWriterEnsuresSchema' -count=1` + - `go test ./internal/stats -count=1` + - Both passed. diff --git a/go/vehicle-gateway/internal/stats/daily_metric.go b/go/vehicle-gateway/internal/stats/daily_metric.go index 39a3bd1f..32c809f1 100644 --- a/go/vehicle-gateway/internal/stats/daily_metric.go +++ b/go/vehicle-gateway/internal/stats/daily_metric.go @@ -47,8 +47,14 @@ func NewWriter(exec Execer, loc *time.Location) *Writer { } func (w *Writer) EnsureSchema(ctx context.Context) error { - if _, err := w.exec.ExecContext(ctx, DailyMileageTableSQL); err != nil { - return err + for _, statement := range []string{ + DataSourceTableSQL, + DailyMileageSourceTableSQL, + DailyMileageTableSQL, + } { + if _, err := w.exec.ExecContext(ctx, statement); err != nil { + return err + } } for _, statement := range DailyMileageAlterSQL { if _, err := w.exec.ExecContext(ctx, statement); err != nil && !isDuplicateColumnError(err) { diff --git a/go/vehicle-gateway/internal/stats/daily_metric_test.go b/go/vehicle-gateway/internal/stats/daily_metric_test.go index 41e616f1..0cb9f905 100644 --- a/go/vehicle-gateway/internal/stats/daily_metric_test.go +++ b/go/vehicle-gateway/internal/stats/daily_metric_test.go @@ -173,24 +173,30 @@ func TestWriterEnsuresSchemaAndUpsertsDailyMileage(t *testing.T) { t.Fatalf("Append() error = %v", err) } - if !strings.Contains(exec.calls[0].query, "CREATE TABLE IF NOT EXISTS vehicle_daily_mileage") { - t.Fatalf("unexpected schema sql: %s", exec.calls[0].query) - } - for _, column := range []string{"vehicle_key", "id BIGINT", "AUTO_INCREMENT", "created_at"} { - if strings.Contains(exec.calls[0].query, column) { - t.Fatalf("schema should not include %s: %s", column, exec.calls[0].query) - } - } - if !strings.Contains(exec.calls[0].query, "PRIMARY KEY (vin, stat_date, protocol)") { - t.Fatalf("daily mileage table should key by vin/stat_date/protocol: %s", exec.calls[0].query) - } - if strings.Contains(exec.calls[0].query, "KEY idx_vin (vin)") { - t.Fatalf("daily mileage table should not keep redundant vin index covered by the primary key: %s", exec.calls[0].query) - } - if len(exec.calls) != 5 { + if len(exec.calls) != 7 { t.Fatalf("exec calls = %d", len(exec.calls)) } - upsertCall := exec.calls[4] + for i, want := range []string{ + "CREATE TABLE IF NOT EXISTS vehicle_data_source", + "CREATE TABLE IF NOT EXISTS vehicle_daily_mileage_source", + "CREATE TABLE IF NOT EXISTS vehicle_daily_mileage", + } { + if !strings.Contains(exec.calls[i].query, want) { + t.Fatalf("schema call %d = %s, want %s", i, exec.calls[i].query, want) + } + } + for _, column := range []string{"vehicle_key", "id BIGINT", "AUTO_INCREMENT", "created_at"} { + if strings.Contains(exec.calls[2].query, column) { + t.Fatalf("schema should not include %s: %s", column, exec.calls[2].query) + } + } + if !strings.Contains(exec.calls[2].query, "PRIMARY KEY (vin, stat_date, protocol)") { + t.Fatalf("daily mileage table should key by vin/stat_date/protocol: %s", exec.calls[2].query) + } + if strings.Contains(exec.calls[2].query, "KEY idx_vin (vin)") { + t.Fatalf("daily mileage table should not keep redundant vin index covered by the primary key: %s", exec.calls[2].query) + } + upsertCall := exec.calls[6] if !strings.Contains(upsertCall.query, "ON DUPLICATE KEY UPDATE") { t.Fatalf("unexpected upsert sql: %s", upsertCall.query) } diff --git a/go/vehicle-gateway/internal/stats/source_mileage.go b/go/vehicle-gateway/internal/stats/source_mileage.go index de34dc33..3d226762 100644 --- a/go/vehicle-gateway/internal/stats/source_mileage.go +++ b/go/vehicle-gateway/internal/stats/source_mileage.go @@ -71,7 +71,7 @@ func UpsertSourceMileage(ctx context.Context, exec Execer, sample SourceMileageS if exec == nil { panic("stats execer must not be nil") } - if sample.VIN == "" || sample.SourceKey == "" { + if sample.VIN == "" || sample.SourceKey == "" || strings.TrimSpace(sample.SourceIP) == "" { return nil } if sample.QualityStatus == "" { @@ -105,27 +105,26 @@ INSERT INTO vehicle_daily_mileage_source first_total_mileage_km, latest_total_mileage_km, daily_mileage_km, sample_count, first_event_time, latest_event_time, quality_status, quality_reason) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) -ON DUPLICATE KEY UPDATE - source_ip = VALUES(source_ip), - source_endpoint = VALUES(source_endpoint), - phone = VALUES(phone), - device_id = VALUES(device_id), - platform_name = COALESCE(VALUES(platform_name), platform_name), - first_total_mileage_km = CASE - WHEN first_total_mileage_km IS NULL OR first_total_mileage_km <= 0 - THEN VALUES(first_total_mileage_km) - ELSE LEAST(first_total_mileage_km, VALUES(first_total_mileage_km)) - END, - latest_total_mileage_km = CASE - WHEN latest_total_mileage_km IS NULL OR latest_total_mileage_km <= 0 - THEN VALUES(latest_total_mileage_km) - ELSE GREATEST(latest_total_mileage_km, VALUES(latest_total_mileage_km)) - END, - /* daily_mileage_km = VALUES(daily_mileage_km) */ - daily_mileage_km = GREATEST( - CASE - WHEN latest_total_mileage_km IS NULL OR latest_total_mileage_km <= 0 - THEN VALUES(latest_total_mileage_km) + ON DUPLICATE KEY UPDATE + source_ip = VALUES(source_ip), + source_endpoint = VALUES(source_endpoint), + phone = VALUES(phone), + device_id = VALUES(device_id), + platform_name = COALESCE(NULLIF(TRIM(VALUES(platform_name)), ''), platform_name), + first_total_mileage_km = CASE + WHEN first_total_mileage_km IS NULL OR first_total_mileage_km <= 0 + THEN VALUES(first_total_mileage_km) + ELSE LEAST(first_total_mileage_km, VALUES(first_total_mileage_km)) + END, + latest_total_mileage_km = CASE + WHEN latest_total_mileage_km IS NULL OR latest_total_mileage_km <= 0 + THEN VALUES(latest_total_mileage_km) + ELSE GREATEST(latest_total_mileage_km, VALUES(latest_total_mileage_km)) + END, + daily_mileage_km = GREATEST( + CASE + WHEN latest_total_mileage_km IS NULL OR latest_total_mileage_km <= 0 + THEN VALUES(latest_total_mileage_km) ELSE latest_total_mileage_km END, VALUES(latest_total_mileage_km) diff --git a/go/vehicle-gateway/internal/stats/source_mileage_test.go b/go/vehicle-gateway/internal/stats/source_mileage_test.go index 88b25205..0de32b1b 100644 --- a/go/vehicle-gateway/internal/stats/source_mileage_test.go +++ b/go/vehicle-gateway/internal/stats/source_mileage_test.go @@ -50,8 +50,9 @@ func TestUpsertSourceMileageWritesCandidateRow(t *testing.T) { for _, want := range []string{ "INSERT INTO vehicle_daily_mileage_source", "ON DUPLICATE KEY UPDATE", - "daily_mileage_km = VALUES(daily_mileage_km)", + "daily_mileage_km = GREATEST(", "quality_status = VALUES(quality_status)", + "platform_name = COALESCE(NULLIF(TRIM(VALUES(platform_name)), ''), platform_name)", } { if !strings.Contains(sql, want) { t.Fatalf("candidate upsert missing %q: %s", want, sql) @@ -61,3 +62,22 @@ func TestUpsertSourceMileageWritesCandidateRow(t *testing.T) { t.Fatalf("first arg = %#v", got) } } + +func TestUpsertSourceMileageSkipsBlankSourceIP(t *testing.T) { + exec := &recordingExec{} + sample := SourceMileageSample{ + VIN: "LA9GG64L7PBAF4001", + StatDate: "2026-07-08", + Protocol: envelope.ProtocolJT808, + SourceKey: "JT808:13307765812@", + SourceIP: " ", + Phone: "13307765812", + QualityStatus: QualityOK, + } + if err := UpsertSourceMileage(context.Background(), exec, sample); err != nil { + t.Fatalf("UpsertSourceMileage() error = %v", err) + } + if len(exec.calls) != 0 { + t.Fatalf("exec calls = %d, want 0", len(exec.calls)) + } +}