feat(dedupe): per-table id_field/require_id overrides#377
Conversation
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
|
| Layer / File(s) | Summary |
|---|---|
Config schema and guidance internal/config/config.go, config.yaml, AGENTS.md, CHANGELOG.md, docs/src/content/docs/configuration.mdx, docs/src/content/docs/api.md, docs/src/content/docs/architecture.md |
Adds dedupe.tables config types and YAML guidance, updates operational notes, and documents per-table overrides, null-id handling, and dedupe behavior changes. |
Deduplicator contract and namespaced storage internal/dedupe/dedupe.go, internal/dedupe/embedded.go, internal/dedupe/embedded_test.go, internal/testutil/mocks.go, internal/observability/metrics_test.go |
Changes CheckAndMark to accept table, namespaces dedupe keys by table, and updates tests and mocks to match the new keying behavior. |
Ingest handler resolution and wiring internal/api/ingest.go, cmd/wavehouse/main.go |
Adds per-table override resolution in the ingest path, treats explicit JSON null ids as missing, and wires config overrides into the handler. |
Ingest behavior tests internal/api/ingest_test.go |
Adds coverage for explicit-null ids and per-table overrides for RequireID, IDField, and dedupe opt-out. |
Estimated code review effort: 3 (Moderate) | ~30 minutes
Sequence Diagram(s)
sequenceDiagram
participant Client
participant IngestHandler
participant Deduplicator
participant Pebble
Client->>IngestHandler: POST /v1/ingest?table={table}
IngestHandler->>IngestHandler: resolve dedupeFieldsFor(table)
alt id field empty
IngestHandler-->>Client: publish without dedupe
else id missing or explicit null
alt requireID true
IngestHandler-->>Client: 400 reject
else
IngestHandler-->>Client: publish un-deduped
end
else id present
IngestHandler->>Deduplicator: CheckAndMark(table, eventID)
Deduplicator->>Pebble: lookup table + "\x00" + eventID
Pebble-->>Deduplicator: found / not found
Deduplicator-->>IngestHandler: isDuplicate
IngestHandler-->>Client: publish or mark duplicate
end
Possibly related issues
- Per-table dedupe id_field (+ fix cross-table dedupe keyspace collision) #222: Matches the per-table dedupe override and table-namespaced key changes in this PR.
- Wave-RF/WaveHouse#369: Matches the explicit
nulldedupe id handling change in this PR.
Possibly related PRs
- Wave-RF/WaveHouse#364: Also changes ingest dedupe enforcement around missing ids and
RequireID, with this PR extending that flow to per-table overrides and JSONnullhandling.
Suggested labels: go, area/api, area/infra
Suggested reviewers: EricAndrechek
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main change: per-table dedupe overrides for id_field and require_id. |
| Description check | ✅ Passed | The description matches the changeset and accurately describes the per-table overrides, key namespacing, and null-id fix. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
dedupe-per-table
✨ Simplify code
- Create PR with simplified code
- Commit simplified code in branch
dedupe-per-table
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Code Coverage OverviewLanguages: Go GoThe overall coverage in the branch remains at 90%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
|
📚 Docs preview is live → https://7c5a105f-wavehouse-docs.wave-rf.workers.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f8723d4-7881-4381-981c-e130bfb559fb
📒 Files selected for processing (14)
CHANGELOG.mdcmd/wavehouse/main.goconfig.yamldocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxinternal/api/ingest.gointernal/api/ingest_test.gointernal/config/config.gointernal/dedupe/dedupe.gointernal/dedupe/embedded.gointernal/dedupe/embedded_test.gointernal/observability/metrics_test.gointernal/testutil/mocks.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: Lint
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / PR housekeeping: feat(dedupe): per-table id_field/require_id overrides
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
GitHub Actions: PR housekeeping / 0_PR housekeeping.txt: feat(dedupe): per-table id_field/require_id overrides
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
🧰 Additional context used
📓 Path-based instructions (5)
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Core packages should be interface-first: behaviors such as cache, dedupe, publisher, and subscriber should be defined behind interfaces to keep implementations swappable.
Files:
internal/dedupe/embedded.gointernal/observability/metrics_test.gointernal/dedupe/embedded_test.gointernal/dedupe/dedupe.gointernal/testutil/mocks.gointernal/api/ingest_test.gointernal/config/config.gointernal/api/ingest.go
cmd/wavehouse/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Wire new dependencies in
cmd/wavehouse/main.gorather than relying on global state or hidden initialization.
Files:
cmd/wavehouse/main.go
docs/src/content/docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Update the relevant docs pages when APIs, configuration, architecture, ingest format, deployment, or development workflows change; keep examples and prose accurate.
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.md
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
Author Mermaid diagrams vertically by default, keep labels short, and avoid placing two large diagrams side by side.
Files:
docs/src/content/docs/configuration.mdx
internal/config/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add configuration fields with matching
yaml,env, andenv-defaultstruct tags, and keep the config struct in sync with documented settings.
Files:
internal/config/config.go
🧠 Learnings (5)
📚 Learning: 2026-05-25T11:25:08.794Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/observability/instruments.go:40-117
Timestamp: 2026-05-25T11:25:08.794Z
Learning: In WaveHouse (Go), it is idiomatic to declare OpenTelemetry (OTel) metric instruments as package-level `var` values created via the OTel Go SDK global proxy pattern (e.g., `var h metric.Float64Histogram = Meter().Float64Histogram(...)`, `var c metric.Int64Counter = Meter().Int64Counter(...)`). When reviewing, do NOT flag these as “global state” violations under the AGENTS.md constructor-injection rule; that rule is intended for swappable application-level dependencies (Cache/Publisher/Subscriber/Deduplicator), not OTel proxy instrument declarations. Do not recommend refactoring these instruments behind an `Instruments` struct for dependency injection.
Applied to files:
internal/observability/metrics_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/observability/metrics_test.gointernal/dedupe/embedded_test.gointernal/api/ingest_test.go
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mdCHANGELOG.md
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/ingest_test.go
🔇 Additional comments (15)
CHANGELOG.md (1)
14-15: LGTM!Also applies to: 33-37
docs/src/content/docs/architecture.md (1)
77-77: LGTM!Also applies to: 171-172
docs/src/content/docs/configuration.mdx (1)
132-149: LGTM!internal/config/config.go (1)
135-148: LGTM!config.yaml (1)
76-85: LGTM!internal/dedupe/dedupe.go (1)
6-16: LGTM!internal/dedupe/embedded.go (1)
28-51: LGTM!internal/dedupe/embedded_test.go (1)
21-57: LGTM! Sequential test structure here correctly follows the prior preference for not forcing single-scenario tests into a table-driven form.internal/testutil/mocks.go (1)
100-112: LGTM!internal/observability/metrics_test.go (1)
23-27: LGTM!internal/api/ingest.go (1)
39-46: LGTM!Also applies to: 59-79, 427-463
cmd/wavehouse/main.go (1)
334-340: LGTM!internal/api/ingest_test.go (3)
533-551: LGTM!
553-576: LGTM!
599-650: LGTM!
…neralize id_field doc
|
So just to be sure I'm understanding this correctly:
|
|
To answer Eric's questions:
GOing to fix now. Leaving We realized that having dedupe settings requiring a config file could get messy for the user, and solely using env variables for dedupe settings can be a complex task, so we are deferring this until we have solved issue #48. |
2 participants
Summary
Dedupe's
id_fieldandrequire_idwere single, deployment-wide settings. This PR makes them per-table and fixes two dedupe keying bugs found along the way:dedupe.tablesmap overridesid_field/require_idper table; the flatdedupe.*settings remain the defaults for any table not listed. Setting a table'sid_fieldto""disables dedupe for that table.table\x00id, so the same id value in two different tables no longer collides. Breaking: the on-disk key format changes, so existing dedupe state is treated as new for one round after upgrade (best-effort idempotency; no stored data lost).id_field({"event_id": null}) was deduping every null row onto the literal"<nil>"key. A null id is now treated as missing — logged, counted, and rejected underrequire_id(or published un-deduped otherwise).Related Issues
Closes #222
Closes #370