From 4d661f06f2913f269dc93385841211cf672a9254 Mon Sep 17 00:00:00 2001 From: taitelee Date: Mon, 6 Jul 2026 15:01:37 -0400 Subject: [PATCH 1/4] feat(dedupe): per-table id_field/require_id overrides, table-namespaced keys, null-id fix --- CHANGELOG.md | 7 ++ cmd/wavehouse/main.go | 7 ++ config.yaml | 10 ++ docs/src/content/docs/api.md | 2 +- docs/src/content/docs/architecture.md | 5 +- docs/src/content/docs/configuration.mdx | 19 +++- internal/api/ingest.go | 94 +++++++++++++------ internal/api/ingest_test.go | 118 ++++++++++++++++++++++++ internal/config/config.go | 9 ++ internal/dedupe/dedupe.go | 5 +- internal/dedupe/embedded.go | 7 +- internal/dedupe/embedded_test.go | 25 ++++- internal/observability/metrics_test.go | 8 +- internal/testutil/mocks.go | 7 +- 14 files changed, 276 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07825cf9..77195db5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Per-table dedupe `id_field` and `require_id` overrides** (`internal/config/config.go`, `internal/api/ingest.go`, `cmd/wavehouse/main.go`, `config.yaml`, `docs/src/content/docs/configuration.mdx`, plus tests in `internal/api/ingest_test.go`): closes #222. The dedupe `id_field` and `require_id` were single deployment-wide settings, forcing every table onto one id-field name and one strictness level. A new optional `dedupe.tables` map (YAML-only; the flat `dedupe.*` settings stay the defaults) overrides `id_field` and/or `require_id` per table — so one table can require a strict id while another stays lenient, or dedupe on a different field; setting a table's `id_field` to `""` skips deduplication for it. A table absent from the map inherits the defaults, so existing `dedupe.*` config is unchanged. Pairs with the table-namespaced dedupe keyspace fix (also #222, see Fixed). + - **Missing-dedupe-id observability + optional strict mode** (`internal/api/ingest.go`, `internal/api/ingest_test.go`, `internal/config/config.go`, `cmd/wavehouse/main.go`, `config.yaml`, `deployments/compose/standalone.yaml`, `docs/src/content/docs/configuration.mdx`, `docs/src/content/docs/api.md`, `docs/src/content/docs/architecture.md`): closes #219. With dedupe enabled, a row missing the configured `id_field` can't be deduped — previously it was published with idempotency silently disabled and *no* log or metric, so a producer bug that dropped the id turned off the guarantee for those rows unnoticed. Now every such row is logged at `WARN` and counted by a new `wavehouse_ingest_dedupe_missing_id_total` counter (labeled by `table`), making the loss observable server-side. A new opt-in `dedupe.require_id` (`WH_DEDUPE_REQUIRE_ID`, default `false`) turns that signal into enforcement: a row missing the id is rejected (`400` for a single insert; a per-record failure in a batch) instead of published — a tripwire for producers that must guarantee the id (complements the client-side [#202](https://github.com/Wave-RF/WaveHouse/issues/202)). Default behavior is unchanged. - **"Durability & Storage" operations guide** (`docs/src/content/docs/durability.md` (new), `docs/src/config/sidebar.ts`, `docs/src/content/docs/reverse-proxy.mdx`, `docs/src/content/docs/configuration.mdx`, `docs/src/content/docs/deployment.md`): documents #84. A new Operations page making the embedded-JetStream durability contract explicit before the docs site publishes: a `200` from `POST /v1/ingest` means the event has been `fsync`'d to disk on the node (the server runs with `SyncAlways: true` in `internal/mq/embedded.go`), which makes the storage substrate's `fsync` tail the ingest latency floor. Covers the contract (and how it differs from JetStream's default page-cache-then-periodic-sync mode), why a slow `fsync` tail manifests as `create stream: ... context deadline exceeded` and `503` backpressure, a where-it's-cheap-vs-expensive substrate table (managed cloud block storage and PLP NVMe vs. ZFS-without-SLOG / qcow2-on-`ext4` / spinning disks), an `fio` recipe + verdict bands to measure your own storage (with the macOS `F_FULLFSYNC` honesty caveat), and the symptom checklist. Forward-references the configurable group-commit interval (`mq.sync_interval`, [#139](https://github.com/Wave-RF/WaveHouse/issues/139)) and the planned `wavehouse storage-check` preflight ([#84](https://github.com/Wave-RF/WaveHouse/issues/84)) without claiming either exists yet. Cross-linked from Configuration (Message Queue), Deployment (Persistent Storage), and the Ingest Pipeline's worker-side ack section; no code changes. - **"Behind a reverse proxy" deployment guide** (`docs/src/content/docs/reverse-proxy.mdx` (new), `docs/src/config/sidebar.ts`, `docs/src/content/docs/deployment.md`, `docs/src/content/docs/configuration.mdx`, `docs/src/content/docs/api.md`, `internal/api/stream.go`, `internal/api/stream_test.go`): closes #241. A new Operations page for the common "WaveHouse behind nginx / Caddy / Cloudflare Tunnel" setup, since several behaviors only matter behind a proxy and weren't documented together. Covers: TLS termination (WaveHouse serves plain HTTP and manages no certs); the request-body size limits and the division of responsibility (WaveHouse ships fixed in-code memory-safety backstops — 1 MiB control / 16 MiB ingest — while the proxy is the tunable *outer* limit, so a missing/loose proxy limit can't OOM the server); Server-Sent Events buffering + idle-timeout tuning (WaveHouse sends a `: connected` comment on open plus a periodic `:` keepalive comment so quiet streams survive proxy idle timeouts, [#226](https://github.com/Wave-RF/WaveHouse/issues/226)); the `?token=` / `since` / `Last-Event-ID` forwarding streams need; `X-Forwarded-For` trust (don't expose `:8080` directly — it's honored, so a direct client could spoof it); and which health paths to expose (`/livez`/`/readyz` internal-optional, `/v1/health` must stay public). Ships full example nginx, Caddy, and Cloudflare-Tunnel configs, and is cross-linked from Deployment, Configuration, and the API reference. One small code change lands with it: the SSE endpoint (`GET /v1/stream`) now sets `X-Accel-Buffering: no` so nginx-class proxies stream events without buffering out of the box (nginx strips the header before the client sees it; Caddy/Cloudflare ignore it). The health-probe guidance is also upgraded from "optional" to a recommendation — keep the bare `/livez`/`/readyz`/`/healthz` paths internal (a public `/readyz` turns each hit into a ClickHouse `Ping`) and expose only `/v1/health` publicly. @@ -26,6 +28,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Dependabot npm updates moved to the pnpm workspace root, fixing recurring `ERR_PNPM_OUTDATED_LOCKFILE` CI failures on dependency PRs** (`.github/dependabot.yml`, `docs/src/content/docs/development.md`, `SECURITY.md`): the three per-member npm update configs (`directory: /docs`, `/clients/ts`, `/tests/e2e/sdk`) are replaced by a single config at the workspace root (`directory: /`, group `npm-deps`, prefix `deps`). The repo has one root `pnpm-lock.yaml` (the #190 consolidation), and Dependabot only updates a lockfile co-located with the manifest it targets — so a per-member config bumped a member's `package.json` without regenerating the root lockfile, and every such PR (e.g. #211, #337) then failed CI's `pnpm install --frozen-lockfile` with `ERR_PNPM_OUTDATED_LOCKFILE`; no rebase could fix it, because a rebase never regenerates the lockfile. Pointing at the root lets Dependabot read `pnpm-workspace.yaml`, walk every member, and update the one lockfile within the PR, and as a bonus brings the root `package.json`'s own devDeps (biome, markdownlint, nyc) under Dependabot. Trade-off: one combined weekly npm PR with a single `deps:` prefix instead of three per-area PRs (`docs:` / `deps(sdk):` / `deps(tests):`). +### Fixed + +- **Dedupe no longer collapses explicit-null `id_field` values onto one key, and `require_id` now rejects them** (`internal/api/ingest.go`, `internal/api/ingest_test.go`, `docs/src/content/docs/configuration.mdx`, `docs/src/content/docs/api.md`, `docs/src/content/docs/architecture.md`): closes #370. The dedupe check keyed on field *presence*, so an explicit JSON `null` id (`{"event_id": null}`) — a present key with no value — was treated as a real id and deduped on `fmt.Sprint(nil)`, the literal string `""`. Every null-id row across every table collided onto that one key (all but the first silently dropped as duplicates), and the row was invisible to both `require_id` strict mode and the `wavehouse_ingest_dedupe_missing_id_total` counter. A `null` `id_field` is now treated exactly like a missing one — logged, counted, and rejected under `require_id` (or published un-deduped otherwise). Scoped to `null`; an empty string stays a real value that dedupes. Reachable whenever the `id_field` column is `Nullable(...)` or has a `DEFAULT`, which `discovery.Validate` lets a null pass through. +- **Dedupe keys are now namespaced by table, closing a silent cross-table collision** (`internal/dedupe/dedupe.go`, `internal/dedupe/embedded.go`, `internal/api/ingest.go`, plus tests in `internal/dedupe/embedded_test.go`, `internal/testutil/mocks.go`): part of #222. `CheckAndMark` keyed Pebble on the raw id value with no table prefix, so the same id *value* ingested into two different tables collided — the second row was silently dropped as a "duplicate." The key is now `table\x00id` (a ClickHouse identifier can't contain a NUL byte, so the table prefix can't be forged by an id value). **Breaking:** the on-disk key format changes, so existing dedupe state no longer matches after upgrade — every event is treated as new for one round. Best-effort idempotency means no stored data is lost, but a duplicate submitted across the upgrade boundary won't be caught. + ### Security - **`denied_aggregations` is now enforced case-insensitively against the caller-supplied function name, closing a policy bypass** (`internal/policy/policy.go`, plus tests in `internal/policy/policy_test.go`): closes #318. `IsAggregationAllowed` lower-cased the aggregation name only *after* the deny-list loop and the empty-allow-list early return, so the deny check compared a lower-cased deny entry against the raw caller input — a denied aggregation slipped past simply by changing case (`SUM` bypassed a `sum` deny entry, and with an empty allow list the call was then permitted). `isValidAggFn` already accepts any casing and the SQL builder emits the function name verbatim, so the denied aggregation actually executed. The case fold now happens once, before the deny check, so deny wins regardless of caller casing — matching the case-insensitive contract the access-control docs already specified. diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index d56f560b..ef3e3676 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -334,6 +334,13 @@ func run() int { ingestHandler.Dedup = dedup ingestHandler.IDField = cfg.Dedupe.IDField ingestHandler.RequireID = cfg.Dedupe.RequireID + if len(cfg.Dedupe.Tables) > 0 { + overrides := make(map[string]api.DedupeOverride, len(cfg.Dedupe.Tables)) + for t, o := range cfg.Dedupe.Tables { + overrides[t] = api.DedupeOverride{IDField: o.IDField, RequireID: o.RequireID} + } + ingestHandler.DedupeTables = overrides + } } var dlqHandler *api.DLQHandler diff --git a/config.yaml b/config.yaml index 1f4cb47e..e371da3d 100644 --- a/config.yaml +++ b/config.yaml @@ -73,6 +73,16 @@ dedupe: # Reject rows missing id_field instead of publishing them un-deduped. Off by # default (they are logged + counted via wavehouse_ingest_dedupe_missing_id_total). require_id: false + # Per-table overrides of id_field / require_id (YAML-only; the settings above + # are the defaults). A table not listed inherits the defaults; set a table's + # id_field to "" to skip dedupe for it. (#222) + # tables: + # payments: + # require_id: true # payments must carry an id or be rejected + # page_views: + # id_field: view_id # dedupe this table on view_id instead of event_id + # audit_log: + # id_field: "" # skip dedupe for this table entirely cache: l1_max_cost: 67108864 diff --git a/docs/src/content/docs/api.md b/docs/src/content/docs/api.md index 13b1608f..e0e58114 100644 --- a/docs/src/content/docs/api.md +++ b/docs/src/content/docs/api.md @@ -225,7 +225,7 @@ The body is a **flat JSON object** whose keys must match column names in the tar | ------ | ---- | ----- | | 400 | `{"error":"invalid json"}` | Malformed request body | | 400 | `{"error":"unknown column ... for table ..."}` (also: `missing required column ...`, `type mismatch for column ...`, `null value for non-nullable column ...`) | Schema validation failure (unknown fields, type mismatches, missing required columns, null in a non-nullable column). The body is the validator's message verbatim — there is no `validation failed:` prefix. | -| 400 | `{"error":"missing dedupe id field \"event_id\""}` | Only when dedupe is enabled with `dedupe.require_id: true` and the row lacks the configured `id_field`. With `require_id: false` (the default) the row is instead published un-deduped. Either way — reject or publish — the row is logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`. In a batch this is a per-record failure, not a whole-request error. | +| 400 | `{"error":"missing dedupe id field \"event_id\""}` | Only when dedupe is enabled with `dedupe.require_id: true` and the row lacks the configured `id_field` — whether the field is missing or sent as an explicit JSON `null`. With `require_id: false` (the default) the row is instead published un-deduped. Either way — reject or publish — the row is logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`. In a batch this is a per-record failure, not a whole-request error. | | 401 | `{"error":"invalid token"}` / `{"error":"token expired"}` | A present-but-invalid/expired token was supplied and denied (the gate surfaces the token reason rather than silently falling back to `default_role`) | | 403 | `{"error":"forbidden"}` (empty-role variant: `forbidden: request has no role and no public default_role is configured`) | The resolved role lacks `insert` on the table | | 404 | `{"error":"unknown table: ..."}` | Table not found in ClickHouse schema | diff --git a/docs/src/content/docs/architecture.md b/docs/src/content/docs/architecture.md index 1cb700bb..e12eea3a 100644 --- a/docs/src/content/docs/architecture.md +++ b/docs/src/content/docs/architecture.md @@ -74,7 +74,7 @@ The API layer uses [Chi](https://github.com/go-chi/chi) for routing with Request - **policy.go** — CRUD handler for access control policies (`/v1/admin/policy`). - **pipes.go** — Named query pipe handlers: admin CRUD and execution with parameter binding. - **structured_query.go** — Handler for `POST /v1/query?table={table}`: validates query AST, enforces permissions, builds and executes SQL. -- **ingest.go** — Accepts flat JSON body for `POST /v1/ingest?table={table}`, validates against discovered schema, optional dedup, publishes to NATS subject `ingest.{table}`. When dedup is on, a row missing the configured `id_field` can't be deduped: it is logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total` (labeled by `table`), then published un-deduped — or rejected when `dedupe.require_id` is set (#219). +- **ingest.go** — Accepts flat JSON body for `POST /v1/ingest?table={table}`, validates against discovered schema, optional dedup, publishes to NATS subject `ingest.{table}`. When dedup is on, a row missing the configured `id_field` (or sending it as an explicit `null`) can't be deduped: it is logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total` (labeled by `table`), then published un-deduped — or rejected when `dedupe.require_id` is set (#219). The `id_field` and `require_id` are resolved per table (global defaults plus `dedupe.tables` overrides), and the dedup key is namespaced by table so equal id values in different tables never collide (#222). - **query.go** — Proxies raw SQL for `POST /v1/admin/query` straight to ClickHouse's HTTP interface. **Not cached** — sets `Cache-Control: no-store` so every request hits ClickHouse; DateTime is rendered ISO-8601 via `date_time_output_format=iso` (the Go-side type conversion lives in the structured-query / pipes path, not here). - **stream.go** — Real-time streaming via SSE. Callers select a table with the `?table=` query parameter. Supports gap-fill from NATS JetStream using `DeliverByStartTime`. Each connection registers with the shared keepalive wheel (the `stream/` package) so idle streams keep emitting `:` keepalive comments and survive reverse-proxy idle timeouts. - **transform.go** — Shared `transformForClient` function: passes through `table_name`, `received_timestamp`, and `data` from the wire format. @@ -169,7 +169,8 @@ Client POST /v1/ingest?table={table} → Policy column rules + check clauses (disallowed columns rejected; claim-derived values enforced or injected) → Optional deduplication check (configurable ID field; a row missing that - field is published un-deduped + logged/counted, or rejected under require_id) + field — or carrying it as an explicit null — is published un-deduped + + logged/counted, or rejected under require_id) → Publish to NATS JetStream (ingest.{table}) → 200 OK returned immediately → (If NATS stream is full: 503 + Retry-After header) diff --git a/docs/src/content/docs/configuration.mdx b/docs/src/content/docs/configuration.mdx index a46f06f9..ef04ad2a 100644 --- a/docs/src/content/docs/configuration.mdx +++ b/docs/src/content/docs/configuration.mdx @@ -129,7 +129,24 @@ WaveHouse's per-role caps are sent as per-query `SETTINGS` on its connection, so | --- | --- | ------- | ----------- | | `dedupe.enabled` | `WH_DEDUPE_ENABLED` | `false` | Enable event deduplication. When enabled, the ingest handler checks for duplicates using the configured ID field. | | `dedupe.id_field` | `WH_DEDUPE_ID_FIELD` | `event_id` | JSON field name in the ingest body used as the dedup key. | -| `dedupe.require_id` | `WH_DEDUPE_REQUIRE_ID` | `false` | With dedupe enabled, controls what happens to a row missing `id_field` (which can't be deduped, so idempotency wouldn't apply to it). Such a row is always logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`, in both modes. Default (`false`): it is then published un-deduped. Set `true` to reject it instead (`400` for a single insert; a per-record failure in a batch) — a server-side tripwire for producers that must guarantee the id. | +| `dedupe.require_id` | `WH_DEDUPE_REQUIRE_ID` | `false` | With dedupe enabled, controls what happens to a row whose `id_field` is missing or an explicit JSON `null` (which can't be deduped, so idempotency wouldn't apply to it). Such a row is always logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`, in both modes. Default (`false`): it is then published un-deduped. Set `true` to reject it instead (`400` for a single insert; a per-record failure in a batch) — a server-side tripwire for producers that must guarantee the id. | +| `dedupe.tables` | *(YAML only)* | *(none)* | Per-table overrides of `id_field` / `require_id`, keyed by table name. A table absent here inherits the global defaults above; set a table's `id_field` to `""` to skip deduplication for it. | + +Each table's dedup keyspace is independent — the same id value ingested into two different tables is **not** treated as a collision. The flat `dedupe.*` settings are the defaults; `dedupe.tables` overrides them per table (YAML-only — there is no env var for per-table config): + +```yaml +dedupe: + enabled: true + id_field: event_id # default for tables not listed below + require_id: false + tables: + payments: + require_id: true # payments must carry an id or be rejected + page_views: + id_field: view_id # dedupe this table on view_id instead + audit_log: + id_field: "" # skip dedupe for this table entirely +``` ### Cache diff --git a/internal/api/ingest.go b/internal/api/ingest.go index 46c3f54b..6091eadf 100644 --- a/internal/api/ingest.go +++ b/internal/api/ingest.go @@ -36,13 +36,14 @@ const maxReportedResults = 10000 // IngestHandler handles POST /v1/ingest?table={table} type IngestHandler struct { - Registry *discovery.SchemaRegistry - Dedup dedupe.Deduplicator // nil if dedup disabled - IDField string // dedup key field name (e.g. "event_id") - RequireID bool // reject rows missing IDField instead of publishing un-deduped (dedupe.require_id) - Publisher mq.Publisher - PolicyStore *policy.Store - logger *slog.Logger + Registry *discovery.SchemaRegistry + Dedup dedupe.Deduplicator // nil if dedup disabled + IDField string // default dedup key field name (e.g. "event_id"); DedupeTables overrides it per table + RequireID bool // default strict mode: reject rows missing the id (dedupe.require_id); DedupeTables overrides it per table + DedupeTables map[string]DedupeOverride + Publisher mq.Publisher + PolicyStore *policy.Store + logger *slog.Logger // maxRequestBytes optionally overrides the default inbound request body cap // (maxRequestBodyBytes). When 0, the default applies. Exists so same-package @@ -55,6 +56,28 @@ func NewIngestHandler(registry *discovery.SchemaRegistry, pub mq.Publisher, logg return &IngestHandler{Registry: registry, Publisher: pub, logger: logger} } +type DedupeOverride struct { + IDField *string + RequireID *bool +} + +// dedupeFieldsFor resolves the effective dedupe id field and strict-mode flag +// for a table: the IDField/RequireID defaults, with any per-table override from +// DedupeTables applied. An empty id field means dedupe is skipped for the +// table. +func (h *IngestHandler) dedupeFieldsFor(table string) (idField string, requireID bool) { + idField, requireID = h.IDField, h.RequireID + if o, ok := h.DedupeTables[table]; ok { + if o.IDField != nil { + idField = *o.IDField + } + if o.RequireID != nil { + requireID = *o.RequireID + } + } + return idField, requireID +} + var dedupeMissingIDCounter, _ = otel.Meter("wavehouse-ingest").Int64Counter( "wavehouse_ingest_dedupe_missing_id_total", metric.WithDescription("Ingested records missing the configured dedupe id_field (idempotency skipped)"), @@ -387,29 +410,40 @@ func (h *IngestHandler) processRecord( } } - // Optional deduplication. - if h.Dedup != nil && h.IDField != "" { - idVal, ok := data[h.IDField] - if !ok { - dedupeMissingIDCounter.Add(ctx, 1, metric.WithAttributes(attribute.String("table", table))) - if h.RequireID { - h.logger.WarnContext(ctx, "dedupe id_field missing; rejecting", "id_field", h.IDField, "table", table) - return false, &recordReject{ - Status: http.StatusBadRequest, - Message: fmt.Sprintf("missing dedupe id field %q", h.IDField), - }, nil - } - h.logger.WarnContext(ctx, "dedupe id_field missing; publishing without idempotency", "id_field", h.IDField, "table", table) - } else { - eventID := fmt.Sprint(idVal) - dup, err := h.Dedup.CheckAndMark(ctx, eventID) - if err != nil { - h.logger.ErrorContext(ctx, "dedupe check failed", "error", err, "event_id", eventID) - return false, nil, &requestAbort{Status: http.StatusInternalServerError, Message: "dedupe failed"} - } - if dup { - h.logger.InfoContext(ctx, "duplicate event skipped", "event_id", eventID) - return true, nil, nil + // Optional deduplication. The id field and strict-mode flag are resolved per + // table — global defaults with per-table overrides — and an empty id field + // skips dedupe for this table. + if h.Dedup != nil { + idField, requireID := h.dedupeFieldsFor(table) + if idField != "" { + // A present key with an explicit JSON null (ok == true, idVal == nil) + // carries no usable id — semantically the same as the field being + // absent. Treat it as missing so it isn't deduped on the + // fmt.Sprint(nil) sentinel "" (which collides every null-id row + // onto one key, dropping all but the first as duplicates) and so + // require_id still enforces. + idVal, ok := data[idField] + if !ok || idVal == nil { + dedupeMissingIDCounter.Add(ctx, 1, metric.WithAttributes(attribute.String("table", table))) + if requireID { + h.logger.WarnContext(ctx, "dedupe id_field missing or null; rejecting", "id_field", idField, "table", table) + return false, &recordReject{ + Status: http.StatusBadRequest, + Message: fmt.Sprintf("missing dedupe id field %q", idField), + }, nil + } + h.logger.WarnContext(ctx, "dedupe id_field missing or null; publishing without idempotency", "id_field", idField, "table", table) + } else { + eventID := fmt.Sprint(idVal) + dup, err := h.Dedup.CheckAndMark(ctx, table, eventID) + if err != nil { + h.logger.ErrorContext(ctx, "dedupe check failed", "error", err, "event_id", eventID) + return false, nil, &requestAbort{Status: http.StatusInternalServerError, Message: "dedupe failed"} + } + if dup { + h.logger.InfoContext(ctx, "duplicate event skipped", "event_id", eventID) + return true, nil, nil + } } } } diff --git a/internal/api/ingest_test.go b/internal/api/ingest_test.go index 35ec0888..3ef4953e 100644 --- a/internal/api/ingest_test.go +++ b/internal/api/ingest_test.go @@ -425,6 +425,124 @@ func TestIngest_Dedup_RequireID_Rejects(t *testing.T) { assert.NotNil(t, pub.LastMessage(), "a record carrying the id is still accepted") } +// TestIngest_Dedup_NullIDField_RequireID_Rejects covers the null-id hole in +// strict mode: an explicit JSON null is a present key but carries no usable id, +// so require_id must reject it exactly like a missing field rather than let it +// slip through and dedupe on the fmt.Sprint(nil) "" sentinel. +func TestIngest_Dedup_NullIDField_RequireID_Rejects(t *testing.T) { + t.Parallel() + pub := &testutil.MockPublisher{} + h := NewIngestHandler(testRegistry(), pub, testutil.NopLogger()) + h.Dedup = testutil.NewMockDeduplicator() + h.IDField = "event_id" + h.RequireID = true + + w := httptest.NewRecorder() + h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": "/home", "event_id": nil})) + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains(t, w.Body.String(), "missing dedupe id field") + testutil.AssertJSONErrorResponse(t, w) + assert.Nil(t, pub.LastMessage(), "must not publish a row whose dedupe id is an explicit null under require_id") +} + +// TestIngest_Dedup_NullIDField_ObserveMode_PublishesEach is the regression for +// the null-id collision: with require_id off, a null id is treated as missing +// and published un-deduped, so two distinct null-id rows both publish. Before +// the fix they collapsed onto the fmt.Sprint(nil) "" key and the second was +// silently dropped as a duplicate. +func TestIngest_Dedup_NullIDField_ObserveMode_PublishesEach(t *testing.T) { + t.Parallel() + pub := &testutil.MockPublisher{} + h := NewIngestHandler(testRegistry(), pub, testutil.NopLogger()) + h.Dedup = testutil.NewMockDeduplicator() + h.IDField = "event_id" + + for _, page := range []string{"/a", "/b"} { + w := httptest.NewRecorder() + h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": page, "event_id": nil})) + assert.Equal(t, http.StatusOK, w.Code) + var resp map[string]bool + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.False(t, resp["duplicate"], "a null-id row must not be treated as a duplicate") + } + assert.Len(t, pub.Messages, 2, "both null-id rows publish un-deduped; neither collapses onto the key") +} + +func ptr[T any](v T) *T { return &v } + +// TestIngest_Dedup_PerTable_RequireIDOverride verifies a per-table override of +// require_id: strict mode is off by default, but the "clicks" table turns it on, +// so a clicks row missing the id is rejected (#222). +func TestIngest_Dedup_PerTable_RequireIDOverride(t *testing.T) { + t.Parallel() + pub := &testutil.MockPublisher{} + h := NewIngestHandler(testRegistry(), pub, testutil.NopLogger()) + h.Dedup = testutil.NewMockDeduplicator() + h.IDField = "event_id" + h.RequireID = false // global default: lenient + h.DedupeTables = map[string]DedupeOverride{ + "clicks": {RequireID: ptr(true)}, // this table is strict + } + + w := httptest.NewRecorder() + h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": "/home"})) // no event_id + assert.Equal(t, http.StatusBadRequest, w.Code, "clicks overrides require_id=true, so a missing id is rejected") + assert.Contains(t, w.Body.String(), "missing dedupe id field") + assert.Nil(t, pub.LastMessage()) +} + +// TestIngest_Dedup_PerTable_IDFieldOverride verifies a per-table override of the +// dedupe id field: "clicks" dedupes on org_id instead of the global event_id, so +// two rows sharing an org_id collapse to one (#222). +func TestIngest_Dedup_PerTable_IDFieldOverride(t *testing.T) { + t.Parallel() + pub := &testutil.MockPublisher{} + h := NewIngestHandler(testRegistry(), pub, testutil.NopLogger()) + h.Dedup = testutil.NewMockDeduplicator() + h.IDField = "event_id" + h.DedupeTables = map[string]DedupeOverride{ + "clicks": {IDField: ptr("org_id")}, + } + + // Two rows share org_id (the overridden key) but carry different event_ids → + // the second is a duplicate because clicks keys on org_id, not event_id. + w := httptest.NewRecorder() + h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": "/a", "org_id": "org-1", "event_id": "e1"})) + assert.Equal(t, http.StatusOK, w.Code) + + w = httptest.NewRecorder() + h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": "/b", "org_id": "org-1", "event_id": "e2"})) + assert.Equal(t, http.StatusOK, w.Code) + var resp map[string]bool + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.True(t, resp["duplicate"], "second row with the same org_id is a duplicate under the id_field override") + assert.Len(t, pub.Messages, 1) +} + +// TestIngest_Dedup_PerTable_OptOut verifies that a per-table id_field of "" +// disables dedupe for that table: two rows with the same event_id both publish +// even though dedupe is globally enabled (#222). +func TestIngest_Dedup_PerTable_OptOut(t *testing.T) { + t.Parallel() + pub := &testutil.MockPublisher{} + h := NewIngestHandler(testRegistry(), pub, testutil.NopLogger()) + h.Dedup = testutil.NewMockDeduplicator() + h.IDField = "event_id" + h.DedupeTables = map[string]DedupeOverride{ + "clicks": {IDField: ptr("")}, // opt clicks out of dedupe + } + + for _, n := range []string{"first", "second"} { + w := httptest.NewRecorder() + h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": "/" + n, "event_id": "same-id"})) + assert.Equal(t, http.StatusOK, w.Code) + var resp map[string]bool + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.False(t, resp["duplicate"], "dedupe is off for clicks, so no row is a duplicate") + } + assert.Len(t, pub.Messages, 2, "both rows publish; clicks is opted out of dedupe") +} + // TestIngest_NDJSON_RequireID_Rejects confirms strict mode is per-record: the // missing-id line fails while the rest of the batch is published. func TestIngest_NDJSON_RequireID_Rejects(t *testing.T) { diff --git a/internal/config/config.go b/internal/config/config.go index b14d8ff4..15bf7bad 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -136,6 +136,15 @@ type Dedupe struct { Enabled bool `yaml:"enabled" env:"WH_DEDUPE_ENABLED" env-default:"false"` IDField string `yaml:"id_field" env:"WH_DEDUPE_ID_FIELD" env-default:"event_id"` RequireID bool `yaml:"require_id" env:"WH_DEDUPE_REQUIRE_ID" env-default:"false"` + Tables map[string]DedupeTable `yaml:"tables"` +} + +// DedupeTable overrides the global dedupe defaults for one table. A nil +// field inherits the corresponding global default; an id_field of "" skips +// deduplication for the table. +type DedupeTable struct { + IDField *string `yaml:"id_field"` + RequireID *bool `yaml:"require_id"` } type Cache struct { diff --git a/internal/dedupe/dedupe.go b/internal/dedupe/dedupe.go index 01594220..20da5b68 100644 --- a/internal/dedupe/dedupe.go +++ b/internal/dedupe/dedupe.go @@ -5,8 +5,9 @@ import "context" // Deduplicator checks whether an event has been seen before and marks it. type Deduplicator interface { // CheckAndMark returns true if the event was already seen (duplicate). - // If not seen, it atomically marks the event as seen. - CheckAndMark(ctx context.Context, eventID string) (isDuplicate bool, err error) + // If not seen, it atomically marks the event as seen. The key is namespaced + // by table so equal id values in different tables never collide (#222). + CheckAndMark(ctx context.Context, table, eventID string) (isDuplicate bool, err error) Stats() map[string]int64 diff --git a/internal/dedupe/embedded.go b/internal/dedupe/embedded.go index 6ba2a13a..da1dadb4 100644 --- a/internal/dedupe/embedded.go +++ b/internal/dedupe/embedded.go @@ -25,8 +25,11 @@ func NewEmbedded(dir string) (*EmbeddedDeduplicator, error) { } // CheckAndMark returns true if the event was already seen. -func (d *EmbeddedDeduplicator) CheckAndMark(_ context.Context, eventID string) (bool, error) { - key := []byte(eventID) +func (d *EmbeddedDeduplicator) CheckAndMark(_ context.Context, table, eventID string) (bool, error) { + // Namespace the key by table so equal id values in different tables don't + // collide. A ClickHouse identifier can't contain a NUL byte, so the + // first NUL unambiguously separates the table prefix from the id. + key := []byte(table + "\x00" + eventID) _, closer, err := d.db.Get(key) if err == nil { diff --git a/internal/dedupe/embedded_test.go b/internal/dedupe/embedded_test.go index 9851164b..5f612237 100644 --- a/internal/dedupe/embedded_test.go +++ b/internal/dedupe/embedded_test.go @@ -24,19 +24,38 @@ func TestEmbeddedDeduplicator_FirstSeenThenDuplicate(t *testing.T) { d := newEmbedded(t) ctx := context.Background() - dup, err := d.CheckAndMark(ctx, "event-1") + dup, err := d.CheckAndMark(ctx, "clicks", "event-1") require.NoError(t, err) assert.False(t, dup, "first occurrence must not be a duplicate") - dup, err = d.CheckAndMark(ctx, "event-1") + dup, err = d.CheckAndMark(ctx, "clicks", "event-1") require.NoError(t, err) assert.True(t, dup, "second occurrence of the same id must be a duplicate") - dup, err = d.CheckAndMark(ctx, "event-2") + dup, err = d.CheckAndMark(ctx, "clicks", "event-2") require.NoError(t, err) assert.False(t, dup, "distinct ids are independent") } +func TestEmbeddedDeduplicator_TableNamespacing(t *testing.T) { + t.Parallel() + + d := newEmbedded(t) + ctx := context.Background() + + dup, err := d.CheckAndMark(ctx, "clicks", "shared-id") + require.NoError(t, err) + assert.False(t, dup, "first table's id is new") + + dup, err = d.CheckAndMark(ctx, "purchases", "shared-id") + require.NoError(t, err) + assert.False(t, dup, "same id value in a different table must not be a duplicate") + + dup, err = d.CheckAndMark(ctx, "clicks", "shared-id") + require.NoError(t, err) + assert.True(t, dup, "same id in the same table is still a duplicate") +} + func TestEmbeddedDeduplicator_Stats(t *testing.T) { t.Parallel() diff --git a/internal/observability/metrics_test.go b/internal/observability/metrics_test.go index 15d5368b..959014f8 100644 --- a/internal/observability/metrics_test.go +++ b/internal/observability/metrics_test.go @@ -20,9 +20,11 @@ type stubDeduplicator struct { stats map[string]int64 } -func (s *stubDeduplicator) CheckAndMark(context.Context, string) (bool, error) { return false, nil } -func (s *stubDeduplicator) Stats() map[string]int64 { return s.stats } -func (s *stubDeduplicator) Close() error { return nil } +func (s *stubDeduplicator) CheckAndMark(context.Context, string, string) (bool, error) { + return false, nil +} +func (s *stubDeduplicator) Stats() map[string]int64 { return s.stats } +func (s *stubDeduplicator) Close() error { return nil } func TestRegisterSystemMetrics_NilInputs(t *testing.T) { // No t.Parallel(): all three TestRegisterSystemMetrics_* tests mutate the diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index eb7ab654..2cc0cfbd 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -97,16 +97,17 @@ func (m *MockDeduplicator) Stats() map[string]int64 { } } -func (m *MockDeduplicator) CheckAndMark(_ context.Context, eventID string) (bool, error) { +func (m *MockDeduplicator) CheckAndMark(_ context.Context, table, eventID string) (bool, error) { if m.Err != nil { return false, m.Err } m.mu.Lock() defer m.mu.Unlock() - if m.seen[eventID] { + key := table + "\x00" + eventID + if m.seen[key] { return true, nil } - m.seen[eventID] = true + m.seen[key] = true return false, nil } From 5b477a18ac010a42f42531fcd86ad17ac7d027e8 Mon Sep 17 00:00:00 2001 From: taitelee Date: Mon, 6 Jul 2026 15:01:58 -0400 Subject: [PATCH 2/4] make fix --- internal/api/ingest.go | 8 ++++---- internal/config/config.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/api/ingest.go b/internal/api/ingest.go index 6091eadf..748ac1a6 100644 --- a/internal/api/ingest.go +++ b/internal/api/ingest.go @@ -36,10 +36,10 @@ const maxReportedResults = 10000 // IngestHandler handles POST /v1/ingest?table={table} type IngestHandler struct { - Registry *discovery.SchemaRegistry - Dedup dedupe.Deduplicator // nil if dedup disabled - IDField string // default dedup key field name (e.g. "event_id"); DedupeTables overrides it per table - RequireID bool // default strict mode: reject rows missing the id (dedupe.require_id); DedupeTables overrides it per table + Registry *discovery.SchemaRegistry + Dedup dedupe.Deduplicator // nil if dedup disabled + IDField string // default dedup key field name (e.g. "event_id"); DedupeTables overrides it per table + RequireID bool // default strict mode: reject rows missing the id (dedupe.require_id); DedupeTables overrides it per table DedupeTables map[string]DedupeOverride Publisher mq.Publisher PolicyStore *policy.Store diff --git a/internal/config/config.go b/internal/config/config.go index 15bf7bad..7a3dd22d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -133,10 +133,10 @@ type MQ struct { } type Dedupe struct { - Enabled bool `yaml:"enabled" env:"WH_DEDUPE_ENABLED" env-default:"false"` - IDField string `yaml:"id_field" env:"WH_DEDUPE_ID_FIELD" env-default:"event_id"` - RequireID bool `yaml:"require_id" env:"WH_DEDUPE_REQUIRE_ID" env-default:"false"` - Tables map[string]DedupeTable `yaml:"tables"` + Enabled bool `yaml:"enabled" env:"WH_DEDUPE_ENABLED" env-default:"false"` + IDField string `yaml:"id_field" env:"WH_DEDUPE_ID_FIELD" env-default:"event_id"` + RequireID bool `yaml:"require_id" env:"WH_DEDUPE_REQUIRE_ID" env-default:"false"` + Tables map[string]DedupeTable `yaml:"tables"` } // DedupeTable overrides the global dedupe defaults for one table. A nil From 0996ffea9c89d3d56265ad8554d3b0f026776d2e Mon Sep 17 00:00:00 2001 From: taitelee Date: Mon, 6 Jul 2026 15:35:08 -0400 Subject: [PATCH 3/4] test(dedupe): e2e-cover per-table wiring, assert JSON error shape, generalize id_field doc --- docs/src/content/docs/api.md | 2 +- internal/api/ingest_test.go | 1 + tests/e2e/fixtures/config.yaml | 3 +++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/src/content/docs/api.md b/docs/src/content/docs/api.md index e0e58114..38db7970 100644 --- a/docs/src/content/docs/api.md +++ b/docs/src/content/docs/api.md @@ -225,7 +225,7 @@ The body is a **flat JSON object** whose keys must match column names in the tar | ------ | ---- | ----- | | 400 | `{"error":"invalid json"}` | Malformed request body | | 400 | `{"error":"unknown column ... for table ..."}` (also: `missing required column ...`, `type mismatch for column ...`, `null value for non-nullable column ...`) | Schema validation failure (unknown fields, type mismatches, missing required columns, null in a non-nullable column). The body is the validator's message verbatim — there is no `validation failed:` prefix. | -| 400 | `{"error":"missing dedupe id field \"event_id\""}` | Only when dedupe is enabled with `dedupe.require_id: true` and the row lacks the configured `id_field` — whether the field is missing or sent as an explicit JSON `null`. With `require_id: false` (the default) the row is instead published un-deduped. Either way — reject or publish — the row is logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`. In a batch this is a per-record failure, not a whole-request error. | +| 400 | `{"error":"missing dedupe id field \"\""}` (`` is the table's effective dedupe field — the `dedupe.id_field` default, e.g. `event_id`, or a `dedupe.tables` per-table override) | Only when dedupe is enabled and `require_id` is in effect for the table (the global `dedupe.require_id`, or a per-table `dedupe.tables` override) and the row lacks that `id_field` — whether the field is missing or sent as an explicit JSON `null`. With `require_id` off (the default) the row is instead published un-deduped. Either way — reject or publish — the row is logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`. In a batch this is a per-record failure, not a whole-request error. | | 401 | `{"error":"invalid token"}` / `{"error":"token expired"}` | A present-but-invalid/expired token was supplied and denied (the gate surfaces the token reason rather than silently falling back to `default_role`) | | 403 | `{"error":"forbidden"}` (empty-role variant: `forbidden: request has no role and no public default_role is configured`) | The resolved role lacks `insert` on the table | | 404 | `{"error":"unknown table: ..."}` | Table not found in ClickHouse schema | diff --git a/internal/api/ingest_test.go b/internal/api/ingest_test.go index 40ddf20c..0ffae029 100644 --- a/internal/api/ingest_test.go +++ b/internal/api/ingest_test.go @@ -593,6 +593,7 @@ func TestIngest_Dedup_PerTable_RequireIDOverride(t *testing.T) { h.Handle(w, ingestRequest(t, "clicks", map[string]any{"page": "/home"})) // no event_id assert.Equal(t, http.StatusBadRequest, w.Code, "clicks overrides require_id=true, so a missing id is rejected") assert.Contains(t, w.Body.String(), "missing dedupe id field") + testutil.AssertJSONErrorResponse(t, w) assert.Nil(t, pub.LastMessage()) } diff --git a/tests/e2e/fixtures/config.yaml b/tests/e2e/fixtures/config.yaml index da387be5..f2b54eaf 100644 --- a/tests/e2e/fixtures/config.yaml +++ b/tests/e2e/fixtures/config.yaml @@ -17,6 +17,9 @@ mq: dedupe: enabled: true id_field: event_id + tables: + events_ingest: + require_id: true # JWT validation. The suite signs test tokens with this fixed dev secret; # don't reuse outside the e2e rig. From b94b175c10e79a94b300949265c9cb6a3150a2c0 Mon Sep 17 00:00:00 2001 From: taitelee Date: Mon, 6 Jul 2026 15:50:15 -0400 Subject: [PATCH 4/4] doc fixes --- AGENTS.md | 2 +- docs/src/content/docs/architecture.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8154b0cf..21f463c2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,7 +54,7 @@ The invariant index — what must stay true. Full narrative and rationale live i 5. **Per-table batching** — the worker groups events by table and bulk-INSERTs in schema column order; each table's batch is independent. 6. **Dead Letter Queue** — failed batch inserts publish to `WAVEHOUSE_DLQ` (`dlq.`), gated by `dlq.enabled`. No silent data loss. 7. **Auth: always on, fail-loud, decoupled from authz (security)** — the JWT middleware always runs (no `auth.enabled`/`dev_mode` flag); it verifies with HMAC **or** JWKS (not both), with accepted `alg` pinned to the active verifier and checked before any key is used (rejects `alg:none` and cross-family confusion). No/invalid/expired token → empty role → policy `default_role`, with the bad-token reason stashed so a denying gate returns a loud `401`, not a bare `403`. Elevated access needs a valid granted role. Detail: architecture.md § `api/` + `internal/auth`; see also #11, §Security Considerations. -8. **Optional dedup** — opt-in via `dedupe.enabled`; `dedupe.id_field` selects the JSON key. +8. **Optional dedup** — opt-in via `dedupe.enabled`; `dedupe.id_field`/`require_id` are global defaults, overridable per table via `dedupe.tables` (an `id_field` of `""` opts a table out). Dedup keys are namespaced by table (`table` + NUL + id) so equal ids in different tables never collide — preserve when touching `internal/dedupe` (#222). 9. **Singleflight** — `TieredCache` coalesces concurrent misses (`x/sync/singleflight`) to prevent cache stampede. 10. **Active Sweeper** — purges NATS messages that are both ACKed (written to CH) and older than the gap window; SSE gap-fill uses `DeliverByStartTime`, no in-process ring buffer. 11. **Hasura-style access control: fail-closed (security)** — `policy.IsAdmin` (role == `admin_role`, **exact case-sensitive**, default `"admin"`) is the single admin check, shared by `Evaluate`/`ResolveRole`/`Validate`/the `/v1/admin` gate/`RoleAllowed`. Empty/absent role matches nothing (no `"*"` wildcard); `Validate` rejects empty role keys; a `nil` policy (deleted) denies **everyone incl. admin** — a total lockout, so bootstrap from the policy file, never an implicit admin grant. `default_role` is the one sanctioned roleless exception (`ResolveRole` maps empty → it pre-eval); `default_role == admin_role` is permitted but dev-only and loudly warned (`policy.DefaultRoleGrantsAdmin`). Preserve when touching `internal/policy` (policy twin of #13; see #159). Detail: architecture.md § `policy/`. diff --git a/docs/src/content/docs/architecture.md b/docs/src/content/docs/architecture.md index 5c7f1cf9..575bd9e9 100644 --- a/docs/src/content/docs/architecture.md +++ b/docs/src/content/docs/architecture.md @@ -108,8 +108,8 @@ The SSE fan-out, factored out of `api/` so the delivery hot path ([#294](https:/ ### `dedupe/` — Deduplication (Optional) -- **dedupe.go** — `Deduplicator` interface: `CheckAndMark(ctx, eventID) (bool, error)`. -- **embedded.go** — Uses [Pebble](https://github.com/cockroachdb/pebble) (embedded key-value store). Key = event ID. +- **dedupe.go** — `Deduplicator` interface: `CheckAndMark(ctx, table, eventID) (bool, error)`. +- **embedded.go** — Uses [Pebble](https://github.com/cockroachdb/pebble) (embedded key-value store). Key is namespaced by table (`table` + NUL + event ID) so equal id values in different tables never collide (#222). ### `discovery/` — Schema Discovery & Validation