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/CHANGELOG.md b/CHANGELOG.md index 15b9ada6..d26dd39c 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. @@ -28,6 +30,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 - **Policy `_in` is now enforced on both the row-`filter` and insert-`check` paths, closing a fail-open row-security gap** (`internal/policy/policy.go`, `internal/api/ingest.go`, `docs/src/content/docs/access-control.mdx`, plus tests in `internal/policy/policy_test.go`, `internal/api/ingest_test.go`): closes #224. The `Filter` schema accepted `_in` but the engine never read it: on the row-`filter`/SELECT path `resolveFilters` had no `_in` branch, so a row-security filter like `tenant_id: { _in: … }` produced **no `WHERE` predicate** and the role saw every row instead of its tenant subset (a fail-open, same family as #223); on the `check`/INSERT path only `_eq` was honored, silently dropping any other operator. `_in` now takes a single claim that resolves to a JSON **array** (the multi-tenant case — a token's `tenant_ids` list) and emits `col IN (?, …)` with one bound param per element; a scalar claim is a one-element set, and an empty/absent claim matches **no rows** (fail-closed) rather than widening to all of them. On the insert path an `_in` check requires the column be present and one of the set — there is no single value to auto-inject as `_eq` does, so an omitted column is rejected (`403 check failed`). The comparison operators are enforced on `filter` (`_eq`/`_neq`/`_gt`/`_lt`/`_in` all produce predicates now, so nothing is rejected there) and, on `check`, `_neq`/`_gt`/`_lt` become a loud config-load rejection (no insert-time semantics; `check` honors `_eq` + `_in`). The `_in` value stays a single templated string in the wire schema (Go `Filter.In`, SDK `PolicyFilter._in`), matching the established "set = array" shape of the caller-query `in` operator. diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index 698de535..fb6ad8d9 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -331,6 +331,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..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`. 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/docs/src/content/docs/architecture.md b/docs/src/content/docs/architecture.md index 3865d6d3..575bd9e9 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. Each connection registers one `Subscriber` (the `stream/` package) with both the event `Hub` (under its `(topic, role)`) and the shared keepalive wheel, then drains both from a single byte-pump — so idle streams keep emitting `:` keepalive comments (surviving reverse-proxy idle timeouts) while live events arrive already projected and serialized. Per-event projection/serialization happens **once per role** in the `Hub`, not once per subscriber ([#294](https://github.com/Wave-RF/WaveHouse/issues/294)). Gap-fill replay from NATS JetStream (`DeliverByStartTime`) stays per-connection (low-volume, one-time on connect). - **schema.go** — Schema discovery API: list all schemas, get one table, trigger refresh. @@ -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 @@ -168,7 +168,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 f283c59a..64920719 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)"), @@ -401,29 +424,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 129aa965..0ffae029 100644 --- a/internal/api/ingest_test.go +++ b/internal/api/ingest_test.go @@ -530,6 +530,125 @@ 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") + testutil.AssertJSONErrorResponse(t, w) + 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..7a3dd22d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -133,9 +133,18 @@ 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"` + 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 } 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.