Eric Rakestraw
90abc536dd
All checks were successful
ci/woodpecker/push/build-image Pipeline was successful
231 lines
10 KiB
Markdown
231 lines
10 KiB
Markdown
# Active Alert Filtering Cleanup
|
|
|
|
## Summary
|
|
|
|
Fix `/alerts/active` so it returns alerts that are active at request time, not merely every alert from the latest stored alert snapshot.
|
|
|
|
The current implementation reads the latest `alert_runs` parent row, loads all child `alerts`, and presents the run unchanged. This can expose expired alerts when the latest persisted weatherfeeder snapshot is stale or when a snapshot contains alerts that later expire before the next successful ingestion cycle.
|
|
|
|
This cleanup preserves the existing route, response envelope, repository contract, and weatherfeeder table ownership. The change should be implemented as app-layer filtering over the latest stored snapshot, following the same architectural pattern already used by convective outlook active filtering.
|
|
|
|
## Current Behavior To Correct
|
|
|
|
- `GET /alerts/active` calls `LatestAlertRun(ctx)`.
|
|
- `LatestAlertRun(ctx)` returns the latest stored alert snapshot from Postgres.
|
|
- All child alerts for that run are returned unchanged.
|
|
- The endpoint does not compare `effective`, `onset`, `expires`, `status`, or `messageType` to the request time.
|
|
- Expired alerts can therefore appear under an endpoint named `/alerts/active`.
|
|
|
|
## Target Behavior
|
|
|
|
- `GET /alerts/active` returns the latest stored alert run with `alerts` filtered to items active at request time.
|
|
- Missing latest alert run still returns `{ "data": null }`.
|
|
- A latest run with no currently active alerts returns a non-null run object with `alerts: []`.
|
|
- The repository remains a latest-snapshot reader and does not own active-time policy.
|
|
- The app service owns alert active filtering policy.
|
|
- The HTTP adapter supplies the request-time instant using an injectable clock for tests.
|
|
- Presentation remains responsible only for response shape, units no-op behavior, and rendering.
|
|
|
|
## Active Alert Policy
|
|
|
|
Use a single app-layer predicate for determining whether an alert is active at an instant `activeAt`.
|
|
|
|
An alert is active when all of the following are true:
|
|
|
|
- `messageType` is not `Cancel`, case-insensitive after trimming.
|
|
- `effective` is absent or `effective <= activeAt`.
|
|
- `expires` is absent or `activeAt < expires`.
|
|
|
|
Additional policy notes:
|
|
|
|
- Do not use `onset` as a required active boundary. `onset` can describe hazard onset and may be later than alert effective time; using it as a hard lower bound could hide valid watches, warnings, or advisories that are already in effect from an alerting perspective.
|
|
- Do not filter by `status` in the first cleanup unless existing model/test data proves a specific non-active status must be excluded. NWS active feeds commonly use `Actual`; persisted historical snapshots may include other values, but time and cancellation policy are the high-confidence active criteria.
|
|
- Treat nil `expires` as active if the other criteria pass. This preserves data when an upstream alert omits an expiration, while still allowing future tightening if real data shows nil expiration should be suppressed.
|
|
- Preserve input alert order after filtering.
|
|
- Preserve run metadata such as `asOf`, location fields, latitude, and longitude even when all alerts are filtered out.
|
|
|
|
## Public API Impact
|
|
|
|
- Route remains `GET /alerts/active`.
|
|
- Supported query parameters remain `format` and `units`.
|
|
- `precision`, `tz` / `TZ`, and unknown query parameters remain rejected unless a separate roadmap explicitly changes alert query support.
|
|
- JSON/XML/text format support remains unchanged.
|
|
- Response envelope remains `{ "data": ... }`.
|
|
- No Postgres schema or weatherfeeder migration is required.
|
|
|
|
## Stage 1: App Service Filtering
|
|
|
|
### Changes
|
|
|
|
- Keep `app.Repository.LatestAlertRun(ctx)` unchanged.
|
|
- Add an app-level alert read use case, either:
|
|
- `LatestActiveAlertRun(ctx context.Context, activeAt time.Time) (*model.WeatherAlertRun, error)`, or
|
|
- `LatestAlerts(ctx context.Context, filter AlertFilter) (*model.WeatherAlertRun, error)` with `AlertFilter.ActiveAt`.
|
|
- Prefer the first option unless another alert route is being added at the same time; it is narrower and clearer for the existing API.
|
|
- Implement the new service method by:
|
|
- reading `s.repo.LatestAlertRun(ctx)`;
|
|
- returning `nil, nil` when no run exists;
|
|
- cloning the run before mutation;
|
|
- filtering cloned `Alerts` with the active predicate;
|
|
- returning the cloned run.
|
|
- Add package-local helpers for:
|
|
- cloning `WeatherAlertRun`;
|
|
- cloning `WeatherAlert` values deeply enough to avoid mutating repository-owned slices;
|
|
- copying `References` slices;
|
|
- evaluating `isActiveAlert(alert, activeAt)`.
|
|
|
|
### Clone Requirements
|
|
|
|
The clone must protect repository-returned data from service mutation:
|
|
|
|
- allocate a new `Alerts` slice;
|
|
- allocate new `References` slices for each alert;
|
|
- copy pointer time fields only if the model uses pointer fields for alert timestamps;
|
|
- preserve string, boolean, enum, and scalar values exactly.
|
|
|
|
### Tests
|
|
|
|
Add app tests covering:
|
|
|
|
- repository delegation and error propagation;
|
|
- `nil, nil` when the repository has no latest alert run;
|
|
- expired alerts are removed when `activeAt >= expires`;
|
|
- alerts expiring exactly at `activeAt` are inactive;
|
|
- alerts effective exactly at `activeAt` are active;
|
|
- future-effective alerts are inactive;
|
|
- missing `effective` does not make an otherwise valid alert inactive;
|
|
- missing `expires` does not make an otherwise valid alert inactive;
|
|
- `messageType=Cancel` is excluded case-insensitively;
|
|
- alert order is preserved;
|
|
- run metadata is preserved when all alerts are filtered out;
|
|
- filtering does not mutate the repository-owned run, alerts, references, or timestamp pointers.
|
|
|
|
### Verification
|
|
|
|
```sh
|
|
go test ./internal/app
|
|
```
|
|
|
|
## Stage 2: HTTP Adapter Wiring
|
|
|
|
### Changes
|
|
|
|
- Extend `internal/adapters/inbound/httpapi.Service` with the app service method selected in Stage 1.
|
|
- Add adapter-local clock injection near `alerts_endpoint.go`:
|
|
|
|
```go
|
|
var alertNow = time.Now
|
|
```
|
|
|
|
- Update `/alerts/active` handler to call the new active-alert service method with `alertNow().UTC()`.
|
|
- Keep `bindQuery` as the binder so public query support remains `format` and `units` only.
|
|
- Keep the text template name `alerts_active.txt.tmpl`.
|
|
- Do not add timezone support in this cleanup. Alert timestamp presentation currently returns canonical model timestamps; timezone support would be a separate public API expansion.
|
|
|
|
### Tests
|
|
|
|
Update HTTP tests covering:
|
|
|
|
- route registration remains unchanged;
|
|
- handler passes `alertNow().UTC()` to the service;
|
|
- JSON success response renders filtered service output;
|
|
- text output renders zero active alerts as `Alerts: 0` when data is non-null;
|
|
- `data: null` still renders no-data behavior;
|
|
- `units=us` remains accepted and has no schema effect;
|
|
- `precision`, `tz`, `TZ`, and unknown query params still return `400`.
|
|
|
|
### Verification
|
|
|
|
```sh
|
|
go test ./internal/adapters/inbound/httpapi
|
|
```
|
|
|
|
## Stage 3: Presenter And Template Check
|
|
|
|
### Changes
|
|
|
|
- Keep `presenter.AlertsPayload` as a pass-through unless filtering requires defensive copy behavior at presentation time.
|
|
- Do not move active filtering into the presenter.
|
|
- Review `templates/alerts_active.txt.tmpl` and ensure it behaves correctly when `Data` is non-null and `Alerts` is empty.
|
|
- If needed, add an explicit text fixture/assertion rather than changing template wording broadly.
|
|
|
|
### Tests
|
|
|
|
Add or update presenter/template-sensitive tests only if current endpoint tests do not prove:
|
|
|
|
- nil data renders no-data text;
|
|
- non-null data with empty alerts renders an active-alert run with `Alerts: 0`;
|
|
- expired/canceled alerts do not appear in text output after service filtering.
|
|
|
|
### Verification
|
|
|
|
```sh
|
|
go test ./internal/adapters/inbound/httpapi ./internal/adapters/inbound/httpapi/presenter
|
|
```
|
|
|
|
## Stage 4: Documentation Updates
|
|
|
|
### Changes
|
|
|
|
Update current-behavior docs after implementation is complete:
|
|
|
|
- `docs/api.md`:
|
|
- clarify that `/alerts/active` returns the latest alert run filtered to alerts active at request time;
|
|
- document that no current active alerts returns a run with `alerts: []` when a latest run exists;
|
|
- keep `data: null` limited to no stored alert run;
|
|
- keep supported query params as `format` and `units`.
|
|
- `README.md` if endpoint summaries mention active alerts.
|
|
- `docs/internal/postgres-repository.md`:
|
|
- clarify that `LatestAlertRun` reads the latest stored snapshot and active filtering is performed in the app service.
|
|
- `docs/policy/architecture.md` only if its latest-resource wording needs to distinguish latest snapshots from request-time derived active views.
|
|
|
|
Do not document this unimplemented behavior outside `docs/roadmap/` before the code change lands.
|
|
|
|
### Tests
|
|
|
|
No doc-specific tests are required unless existing docs consistency tests cover endpoint summaries.
|
|
|
|
## Stage 5: Final Verification
|
|
|
|
Run focused tests:
|
|
|
|
```sh
|
|
go test ./internal/app ./internal/adapters/inbound/httpapi ./internal/adapters/inbound/httpapi/presenter
|
|
```
|
|
|
|
Run the full suite:
|
|
|
|
```sh
|
|
go test ./...
|
|
```
|
|
|
|
Manual smoke checks after deployment:
|
|
|
|
```sh
|
|
curl 'https://weather.api.rakestrawhome.com/alerts/active?format=json'
|
|
curl 'https://weather.api.rakestrawhome.com/alerts/active?format=text'
|
|
```
|
|
|
|
Expected behavior when all alerts in the latest stored run are expired:
|
|
|
|
- JSON returns a non-null `data` object with `alerts: []`.
|
|
- Text output shows the run metadata and `Alerts: 0`.
|
|
|
|
## Guardrails
|
|
|
|
- Do not change weatherfeeder ingestion behavior in this cleanup.
|
|
- Do not alter Postgres tables or SQL filtering unless a later performance issue justifies it.
|
|
- Do not make `weatherapi` poll NWS or any upstream provider.
|
|
- Do not rename `/alerts/active`.
|
|
- Do not add alert history endpoints in this cleanup.
|
|
- Do not introduce generic filtering frameworks; use small app-local helpers.
|
|
- Do not move route query validation into the app layer.
|
|
- Do not move active filtering into presenters or text templates.
|
|
|
|
## Assumptions
|
|
|
|
- `weatherfeeder` persists alert snapshots that were active at ingestion time, but `weatherapi` must still enforce request-time active semantics.
|
|
- Cancellation messages are operational metadata, not active hazards, and should not be returned by `/alerts/active`.
|
|
- Missing `expires` should not suppress an alert unless future real-world data shows this creates stale records.
|
|
- Time comparisons should use UTC instants; timezone presentation is not part of this cleanup.
|