fix: collect leftover invoices too#4216
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/v3/filters/convert_test.go (1)
327-332: Tiny coverage gap: includeExists: falsetoo.
$exists=falseis the path that drivesIS NULL, so it’s worth asserting the converter preserves a non-nilfalsepointer.🧪 Suggested table-driven version
- t.Run("exists maps to filter.FilterTime Exists", func(t *testing.T) { - out, err := FromAPIFilterDateTime(&FilterDateTime{Exists: lo.ToPtr(true)}) - require.NoError(t, err) - require.NotNil(t, out) - assert.Equal(t, lo.ToPtr(true), out.Exists) - }) + t.Run("exists maps to filter.FilterTime Exists", func(t *testing.T) { + for _, exists := range []bool{true, false} { + t.Run(fmt.Sprintf("%v", exists), func(t *testing.T) { + out, err := FromAPIFilterDateTime(&FilterDateTime{Exists: lo.ToPtr(exists)}) + require.NoError(t, err) + require.NotNil(t, out) + assert.Equal(t, lo.ToPtr(exists), out.Exists) + }) + } + })This also needs
fmtin the imports, or use explicitt.Run("true"/"false", ...)cases to avoid the new import.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/filters/convert_test.go` around lines 327 - 332, Add a test that asserts FromAPIFilterDateTime preserves a non-nil false pointer for FilterDateTime.Exists (since $exists=false drives IS NULL); update the existing t.Run block that tests Exists=true to also cover Exists=false either by turning it into a small table-driven loop over true/false or by adding a separate t.Run for the false case; if you choose the table-driven approach add the necessary import (fmt) for subtest names or avoid fmt by using explicit names, and ensure the calls to FromAPIFilterDateTime and assertions check that out.Exists equals lo.ToPtr(false) when input is FilterDateTime{Exists: lo.ToPtr(false)}.pkg/filter/filter_test.go (1)
2412-2418: Please cover the generatedIS NULL/IS NOT NULLpredicates too.This new case only exercises
IsEmpty(). Since the production change also added SQL builder and Ent predicate generation forFilterTimeandFilterTimeUnix, it’d be great to addExists: trueandExists: falsecases there as well.🧪 Suggested test additions
func TestFilterTime_SelectAndSelectWhereExpr(t *testing.T) { @@ { name: "nil filter", filter: filter.FilterTime{}, field: "created_at", wantEmpty: true, }, + { + name: "exists filter", + filter: filter.FilterTime{Exists: lo.ToPtr(true)}, + field: "created_at", + wantExprSQL: "SELECT * FROM table WHERE created_at IS NOT NULL", + wantExprArgs: nil, + wantEntSQL: `SELECT * FROM "test_table" WHERE "test_table"."created_at" IS NOT NULL`, + wantEntArgs: nil, + }, + { + name: "not exists filter", + filter: filter.FilterTime{Exists: lo.ToPtr(false)}, + field: "created_at", + wantExprSQL: "SELECT * FROM table WHERE created_at IS NULL", + wantExprArgs: nil, + wantEntSQL: `SELECT * FROM "test_table" WHERE "test_table"."created_at" IS NULL`, + wantEntArgs: nil, + },func TestFilterTimeUnix_SelectAndSelectWhereExpr(t *testing.T) { @@ { name: "nil filter", filter: filter.FilterTimeUnix{}, field: "created_at", wantEmpty: true, }, + { + name: "exists filter", + filter: filter.FilterTimeUnix{FilterTime: filter.FilterTime{Exists: lo.ToPtr(true)}}, + field: "created_at", + wantExprSQL: "SELECT * FROM table WHERE created_at IS NOT NULL", + wantExprArgs: nil, + wantEntSQL: `SELECT * FROM "test_table" WHERE "test_table"."created_at" IS NOT NULL`, + wantEntArgs: nil, + }, + { + name: "not exists filter", + filter: filter.FilterTimeUnix{FilterTime: filter.FilterTime{Exists: lo.ToPtr(false)}}, + field: "created_at", + wantExprSQL: "SELECT * FROM table WHERE created_at IS NULL", + wantExprArgs: nil, + wantEntSQL: `SELECT * FROM "test_table" WHERE "test_table"."created_at" IS NULL`, + wantEntArgs: nil, + },As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/filter/filter_test.go` around lines 2412 - 2418, Add test cases for FilterTime and FilterTimeUnix that cover Exists: true and Exists: false in addition to the existing IsEmpty() case; for each new case assert the SQL builder output contains "IS NOT NULL" when Exists=true and "IS NULL" when Exists=false, and verify corresponding Ent predicate generation produces the expected nil/non-nil-null checks (use the same helpers you use elsewhere to render Ent predicates). Locate the tests around the existing FilterTime block (look for the FilterTime struct in the tests and the IsEmpty() assertions) and add parallel table-driven entries for FilterTimeUnix, calling the SQL build function (e.g., the package's SQL builder helper) and the Ent predicate generator to validate both SQL and Ent predicate behavior for Exists=true/false.test/billing/invoice_test.go (1)
689-697: Tiny nit: reusenowfor the filter bound.You've already captured
now := time.Now().UTC().Truncate(time.Second)earlier — using a freshtime.Now()forLtehere makes the bound drift slightly and reads a bit inconsistently. Not a correctness issue (any time after the invoice creation works), just a readability papercut.♻️ Optional tweak
- CollectionAt: filter.FilterTime{ - Lte: lo.ToPtr(time.Now()), - }, + CollectionAt: filter.FilterTime{ + Lte: lo.ToPtr(now), + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/invoice_test.go` around lines 689 - 697, Replace the fresh time.Now() passed to billing.ListGatheringInvoices's CollectionAt.Lte with the previously captured now variable so the filter uses the same truncated UTC timestamp; locate the ListGatheringInvoices call in the test (invoices, err := s.BillingService.ListGatheringInvoices(...)) and set CollectionAt: filter.FilterTime{ Lte: lo.ToPtr(now) } to avoid the minor drift and keep the test consistent with the earlier now value.openmeter/billing/worker/collect/collect.go (1)
68-74: Warn log could get noisy on large backlogs.If a legacy-state customer has many gathering invoices with
NextCollectionAt == nil, this will emit a single log line carrying every ID — which can balloon log sizes and make downstream log ingestion unhappy on first rollout. Logging the count (and maybe a capped sample) is usually friendlier for ops, and you still get enough signal to go investigate.🛠️ Suggested tweak
- nilCollectionAtInvoiceIDs := lo.FilterMap(resp.Items, func(invoice billing.GatheringInvoice, _ int) (string, bool) { - return invoice.ID, invoice.NextCollectionAt == nil - }) - - if len(nilCollectionAtInvoiceIDs) > 0 { - a.logger.WarnContext(ctx, "gathering invoices have nil next collection at; this may indicate legacy or inconsistent state", "invoice_ids", nilCollectionAtInvoiceIDs) - } + nilCollectionAtInvoiceIDs := lo.FilterMap(resp.Items, func(invoice billing.GatheringInvoice, _ int) (string, bool) { + return invoice.ID, invoice.NextCollectionAt == nil + }) + + if n := len(nilCollectionAtInvoiceIDs); n > 0 { + const sampleSize = 10 + sample := nilCollectionAtInvoiceIDs + if n > sampleSize { + sample = sample[:sampleSize] + } + a.logger.WarnContext(ctx, "gathering invoices have nil next collection at; this may indicate legacy or inconsistent state", + "count", n, "invoice_ids_sample", sample) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/collect/collect.go` around lines 68 - 74, The warn log in collect.go currently emits all IDs from nilCollectionAtInvoiceIDs which can be huge; update the logic around nilCollectionAtInvoiceIDs and the call to a.logger.WarnContext to log only the total count and a capped sample (e.g., first 10 IDs) instead of the full slice: compute count := len(nilCollectionAtInvoiceIDs), build a small sample (use lo.Take or slice) when count > sampleSize, and pass "count" and "sample_ids" (not the entire invoice_ids) to WarnContext so large backlogs don't blow up logs while keeping enough info to investigate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/filters/filter.go`:
- Around line 40-41: The comment for the Exists field is inaccurate; update the
comment for the Exists *bool field (tagged json:"$exists,omitempty") to reflect
that it can be set via the public API JSON key "$exists" as well as by bare-key
query forms and is accepted by the converter; locate the Exists declaration in
filter.go and replace the comment to state it represents the $exists operator
accepted directly in the API payload and by bare-key shorthand.
---
Nitpick comments:
In `@api/v3/filters/convert_test.go`:
- Around line 327-332: Add a test that asserts FromAPIFilterDateTime preserves a
non-nil false pointer for FilterDateTime.Exists (since $exists=false drives IS
NULL); update the existing t.Run block that tests Exists=true to also cover
Exists=false either by turning it into a small table-driven loop over true/false
or by adding a separate t.Run for the false case; if you choose the table-driven
approach add the necessary import (fmt) for subtest names or avoid fmt by using
explicit names, and ensure the calls to FromAPIFilterDateTime and assertions
check that out.Exists equals lo.ToPtr(false) when input is
FilterDateTime{Exists: lo.ToPtr(false)}.
In `@openmeter/billing/worker/collect/collect.go`:
- Around line 68-74: The warn log in collect.go currently emits all IDs from
nilCollectionAtInvoiceIDs which can be huge; update the logic around
nilCollectionAtInvoiceIDs and the call to a.logger.WarnContext to log only the
total count and a capped sample (e.g., first 10 IDs) instead of the full slice:
compute count := len(nilCollectionAtInvoiceIDs), build a small sample (use
lo.Take or slice) when count > sampleSize, and pass "count" and "sample_ids"
(not the entire invoice_ids) to WarnContext so large backlogs don't blow up logs
while keeping enough info to investigate.
In `@pkg/filter/filter_test.go`:
- Around line 2412-2418: Add test cases for FilterTime and FilterTimeUnix that
cover Exists: true and Exists: false in addition to the existing IsEmpty() case;
for each new case assert the SQL builder output contains "IS NOT NULL" when
Exists=true and "IS NULL" when Exists=false, and verify corresponding Ent
predicate generation produces the expected nil/non-nil-null checks (use the same
helpers you use elsewhere to render Ent predicates). Locate the tests around the
existing FilterTime block (look for the FilterTime struct in the tests and the
IsEmpty() assertions) and add parallel table-driven entries for FilterTimeUnix,
calling the SQL build function (e.g., the package's SQL builder helper) and the
Ent predicate generator to validate both SQL and Ent predicate behavior for
Exists=true/false.
In `@test/billing/invoice_test.go`:
- Around line 689-697: Replace the fresh time.Now() passed to
billing.ListGatheringInvoices's CollectionAt.Lte with the previously captured
now variable so the filter uses the same truncated UTC timestamp; locate the
ListGatheringInvoices call in the test (invoices, err :=
s.BillingService.ListGatheringInvoices(...)) and set CollectionAt:
filter.FilterTime{ Lte: lo.ToPtr(now) } to avoid the minor drift and keep the
test consistent with the earlier now value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ca24faa-8d0b-4589-aa05-877d1722aec0
📒 Files selected for processing (11)
api/v3/filters/convert.goapi/v3/filters/convert_test.goapi/v3/filters/filter.goapi/v3/filters/parse.goapi/v3/filters/parse_test.goopenmeter/billing/adapter/gatheringinvoice.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/worker/collect/collect.gopkg/filter/filter.gopkg/filter/filter_test.gotest/billing/invoice_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/spec/packages/legacy/src/filter.tsp (1)
286-290: Nice addition — consider a slightly richer doc string.The new
$existsoperator is a clean, consistent extension ofFilterTime. 🎉 One tiny nitpick: the doc "The field must exist." doesn't convey the boolean semantics (i.e.true→ field is not null,false→ field is null). Since this gets surfaced to SDK users, a hint like the following would save a trip to the source:📝 Suggested doc tweak
- /** - * The field must exist. - */ + /** + * When true, the field must be set (not null); when false, the field must be null. + */ `@extension`("x-omitempty", true) $exists?: boolean | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/legacy/src/filter.tsp` around lines 286 - 290, Update the docstring for the $exists property to explicitly document its boolean semantics for SDK users: state that $exists true means the field must exist / be non-null and $exists false means the field must be null or absent, and include that the property is optional and can be null (as declared on $exists) so callers know how to pass it when using FilterTime/$exists.test/billing/invoice_test.go (1)
690-698: Consider locking down the filter timestamp and adding a positive-case assertion.Two small things worth considering here:
Lte: lo.ToPtr(time.Now())uses a fresh wall-clock value rather than thenowalready established on line 660. Reusingnow(orclock.Now()) makes the test deterministic and matches the convention used elsewhere in this file.- The test only proves the "nil
NextCollectionAtis excluded" half of the fix. A sibling assertion that a gathering invoice with a non-nilNextCollectionAt ≤ Lteis returned would nail down that the filter still works positively — guarding against a future regression that over-filters everything.🧪 Sketch of the tweak
- invoices, err := s.BillingService.ListGatheringInvoices(ctx, billing.ListGatheringInvoicesInput{ - Namespaces: []string{namespace}, - Customers: []string{customerEntity.ID}, - CollectionAt: filter.FilterTime{ - Lte: lo.ToPtr(time.Now()), - }, - }) - require.NoError(s.T(), err) - require.Empty(s.T(), invoices.Items) + farFuture := now.Add(365 * 24 * time.Hour) + invoices, err := s.BillingService.ListGatheringInvoices(ctx, billing.ListGatheringInvoicesInput{ + Namespaces: []string{namespace}, + Customers: []string{customerEntity.ID}, + CollectionAt: filter.FilterTime{ + Lte: lo.ToPtr(farFuture), + }, + }) + require.NoError(s.T(), err) + require.Empty(s.T(), invoices.Items, "gathering invoices with nil NextCollectionAt must be filtered out")…plus an additional positive-case call (without forcing
NextCollectionAt = nil) to assert the invoice is returned.As per coding guidelines ("Make sure the tests are comprehensive and cover the changes").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/invoice_test.go` around lines 690 - 698, The test uses a fresh time.Now() for the CollectionAt filter and only asserts the negative case; update the call to use the existing now (or clock.Now()) variable instead of time.Now() when building billing.ListGatheringInvoicesInput.CollectionAt to make the test deterministic, and add a positive-case: create a gathering invoice with a non-nil NextCollectionAt <= now, call s.BillingService.ListGatheringInvoices (same ListGatheringInvoicesInput) and assert that invoices.Items contains that invoice (i.e., it is returned), referencing the ListGatheringInvoices/ListGatheringInvoicesInput, CollectionAt and NextCollectionAt symbols to locate the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/spec/packages/legacy/src/filter.tsp`:
- Around line 286-290: Update the docstring for the $exists property to
explicitly document its boolean semantics for SDK users: state that $exists true
means the field must exist / be non-null and $exists false means the field must
be null or absent, and include that the property is optional and can be null (as
declared on $exists) so callers know how to pass it when using
FilterTime/$exists.
In `@test/billing/invoice_test.go`:
- Around line 690-698: The test uses a fresh time.Now() for the CollectionAt
filter and only asserts the negative case; update the call to use the existing
now (or clock.Now()) variable instead of time.Now() when building
billing.ListGatheringInvoicesInput.CollectionAt to make the test deterministic,
and add a positive-case: create a gathering invoice with a non-nil
NextCollectionAt <= now, call s.BillingService.ListGatheringInvoices (same
ListGatheringInvoicesInput) and assert that invoices.Items contains that invoice
(i.e., it is returned), referencing the
ListGatheringInvoices/ListGatheringInvoicesInput, CollectionAt and
NextCollectionAt symbols to locate the relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa9fbd94-4744-4320-8990-204d8baef463
⛔ Files ignored due to path filters (5)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (4)
api/api.gen.goapi/spec/packages/legacy/src/filter.tspopenmeter/apiconverter/filter.gen.gotest/billing/invoice_test.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/apiconverter/filter.gen.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/filters/parse_test.go`:
- Around line 627-629: Add a focused test (near TestParse_FilterDateTime or
alongside TestParse_EmptyTypedValueRejected) that exercises
FilterDateTime/nullable timestamp behavior by asserting the parser's handling of
filter[created_at], filter[created_at][exists], and filter[created_at][nexists];
specifically, enumerate and assert the expected outcomes for an empty datetime
value, an explicit exists=true, and exists=false inputs using the same helper
parsing functions as other tests so the new cases cover the removed
datetime-rejection branch and document the intended nullable timestamp
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5828e062-df5b-45b3-9278-ff5aad4214c8
📒 Files selected for processing (3)
api/v3/filters/parse.goapi/v3/filters/parse_test.goopenmeter/apiconverter/filter.go
💤 Files with no reviewable changes (1)
- api/v3/filters/parse.go
tothandras
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This patch adds support for collecting any gathering invoice that is in an old leftover state.
To achieve that we are using the filter package for collected at filtering.
The patch introduces the exists operator and exposes that in an AIP compliant way.
Notes for reviewer
Summary by CodeRabbit
New Features
$existsoperator to time-based filters, enabling null value checks on datetime fieldsBug Fixes
Tests
$existsoperator