feat(dedupe): per-table id_field/require_id overrides#377
Conversation
github-actions
Bot
added
documentation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds per-table dedupe overrides ( ChangesPer-table dedupe overrides and table namespacing
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant IngestHandler
participant Deduplicator
Client->>IngestHandler: POST /v1/ingest?table={table}
IngestHandler->>IngestHandler: dedupeFieldsFor(table)
alt idField empty for table
IngestHandler-->>Client: publish without dedupe
else id missing or explicit null
alt requireID true
IngestHandler-->>Client: 400 reject (missing dedupe id)
else
IngestHandler-->>Client: publish un-deduped
end
else id present
IngestHandler->>Deduplicator: CheckAndMark(table, eventID)
Deduplicator-->>IngestHandler: isDuplicate
IngestHandler-->>Client: publish or mark duplicate
end
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
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://304a76cb-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
1 participant
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