Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| CREATE INDEX "billinggatheringinvoiceline_invoice_id" ON "billing_gathering_invoice_lines" ("invoice_id"); | ||
| -- create "billing_invoice_search_v1s" view | ||
| CREATE VIEW "billing_invoice_search_v1s" AS | ||
| SELECT "billing_invoices"."id", "billing_invoices"."namespace", "billing_invoices"."customer_id", "billing_invoices"."customer_name", "billing_invoices"."currency", 'billing_invoice' AS "storage_table", "billing_invoices"."type" AS "invoice_type", "billing_invoices"."status", "billing_invoices"."issued_at", "billing_invoices"."period_start" AS "service_period_start", "billing_invoices"."period_end" AS "service_period_end", "billing_invoices"."created_at", "billing_invoices"."updated_at", "billing_invoices"."deleted_at", "billing_invoices"."draft_until", "billing_invoices"."collection_at", "billing_invoices"."status_details_cache", "billing_invoices"."invoicing_app_external_id", "billing_invoices"."payment_app_external_id", "billing_invoices"."tax_app_external_id", "billing_invoices"."schema_level" FROM "billing_invoices" WHERE "billing_invoices"."status" <> 'gathering' UNION ALL SELECT "billing_invoices"."id", "billing_invoices"."namespace", "billing_invoices"."customer_id", "t1"."name" AS "customer_name", "billing_invoices"."currency", 'billing_invoice' AS "storage_table", 'gathering' AS "invoice_type", 'gathering' AS "status", NULL::timestamptz AS "issued_at", "billing_invoices"."period_start" AS "service_period_start", "billing_invoices"."period_end" AS "service_period_end", "billing_invoices"."created_at", "billing_invoices"."updated_at", "billing_invoices"."deleted_at", NULL::timestamptz AS "draft_until", "billing_invoices"."collection_at", NULL::jsonb AS "status_details_cache", NULL::text AS "invoicing_app_external_id", NULL::text AS "payment_app_external_id", NULL::text AS "tax_app_external_id", "billing_invoices"."schema_level" FROM "billing_invoices" JOIN "customers" AS "t1" ON "t1"."namespace" = "billing_invoices"."namespace" AND "t1"."id" = "billing_invoices"."customer_id" LEFT JOIN "billing_gathering_invoices" AS "t2" ON "t2"."namespace" = "billing_invoices"."namespace" AND "t2"."id" = "billing_invoices"."id" WHERE "billing_invoices"."status" = 'gathering' AND "t2"."id" IS NULL UNION ALL SELECT "billing_gathering_invoices"."id", "billing_gathering_invoices"."namespace", "billing_gathering_invoices"."customer_id", "t1"."name" AS "customer_name", "billing_gathering_invoices"."currency", 'billing_gathering_invoice' AS "storage_table", 'gathering' AS "invoice_type", 'gathering' AS "status", NULL::timestamptz AS "issued_at", "billing_gathering_invoices"."service_period_start", "billing_gathering_invoices"."service_period_end", "billing_gathering_invoices"."created_at", "billing_gathering_invoices"."updated_at", "billing_gathering_invoices"."deleted_at", NULL::timestamptz AS "draft_until", "billing_gathering_invoices"."next_collection_at" AS "collection_at", NULL::jsonb AS "status_details_cache", NULL::text AS "invoicing_app_external_id", NULL::text AS "payment_app_external_id", NULL::text AS "tax_app_external_id", "billing_gathering_invoices"."schema_level" FROM "billing_gathering_invoices" JOIN "customers" AS "t1" ON "t1"."namespace" = "billing_gathering_invoices"."namespace" AND "t1"."id" = "billing_gathering_invoices"."customer_id"; |
Contributor
There was a problem hiding this comment.
The legacy gathering branch excludes a row whenever the dedicated table contains the same invoice ID. During a partial or invalid migration, the search view therefore exposes only the dedicated row, so GetGatheringInvoiceById, GetInvoiceType, and list hydration cannot report the conflicting state and instead select one source implicitly.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/migrate/migrations/20260717060930_create_billing_gathering_invoices.up.sql
Line: 47
Comment:
**View Hides Conflicting Invoices**
The legacy gathering branch excludes a row whenever the dedicated table contains the same invoice ID. During a partial or invalid migration, the search view therefore exposes only the dedicated row, so `GetGatheringInvoiceById`, `GetInvoiceType`, and list hydration cannot report the conflicting state and instead select one source implicitly.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/openmeter/github/openmeterio/openmeter/-/custom-context?memory=eb020e3b-8e3b-45ea-a8b7-4006b2b2065b))
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
30
to
+50
| var _ billing.GatheringInvoiceAdapter = (*adapter)(nil) | ||
|
|
||
| func (a *adapter) DeleteGatheringInvoices(ctx context.Context, input billing.DeleteGatheringInvoicesInput) error { | ||
| if err := input.Validate(); err != nil { | ||
| return billing.ValidationError{ | ||
| Err: err, | ||
| } | ||
| } | ||
|
|
||
| return entutils.TransactingRepoWithNoValue(ctx, a, func(ctx context.Context, tx *adapter) error { | ||
| nAffected, err := tx.db.BillingInvoice.Update(). | ||
| Where(billinginvoice.IDIn(input.InvoiceIDs...)). | ||
| Where(billinginvoice.Namespace(input.Namespace)). | ||
| Where(billinginvoice.StatusEQ(billing.StandardInvoiceStatusGathering)). | ||
| ClearPeriodStart(). | ||
| ClearPeriodEnd(). | ||
| SetDeletedAt(clock.Now()). | ||
| Save(ctx) | ||
| if err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
This adapter now serves gathering invoices from both storage tables, but bulk deletion still updates only BillingInvoice. Passing an invoice stored in billing_gathering_invoices updates zero rows, returns invoices failed to delete, and leaves the dedicated invoice active.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/adapter/gatheringinvoice.go
Line: 30-50
Comment:
**Dedicated Invoices Cannot Be Deleted**
This adapter now serves gathering invoices from both storage tables, but bulk deletion still updates only `BillingInvoice`. Passing an invoice stored in `billing_gathering_invoices` updates zero rows, returns `invoices failed to delete`, and leaves the dedicated invoice active.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/openmeter/github/openmeterio/openmeter/-/custom-context?memory=eb020e3b-8e3b-45ea-a8b7-4006b2b2065b))
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+67
to
+88
| dedicatedQuery := tx.db.BillingGatheringInvoiceLine.Query(). | ||
| Where(billinggatheringinvoiceline.Namespace(in.Namespace)). | ||
| Where(billinggatheringinvoiceline.SubscriptionID(in.SubscriptionID)). | ||
| Where( | ||
| billinggatheringinvoiceline.Or( | ||
| billinggatheringinvoiceline.DeletedAtIsNil(), | ||
| billinggatheringinvoiceline.And( | ||
| billinggatheringinvoiceline.DeletedAtNotNil(), | ||
| billinggatheringinvoiceline.ManagedByEQ(billing.ManuallyManagedLine), | ||
| ), | ||
| ), | ||
| ). | ||
| WithTaxCode() | ||
|
|
||
| if !in.IncludeChargeManaged { | ||
| dedicatedQuery = dedicatedQuery.Where(billinggatheringinvoiceline.ChargeIDIsNil()) | ||
| } | ||
|
|
||
| dbDedicatedLines, err := dedicatedQuery.All(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fetching dedicated gathering lines: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
The dedicated query returns lines with a SplitLineGroupID, while GetStandardLinesForSubscription also loads those rows inside their split-line hierarchy. Service.GetLinesForSubscription appends both results, so subscription synchronization can receive the same dedicated line once as a hierarchy child and again as an independent gathering line, leading to duplicate or conflicting reconciliation.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/adapter/gatheringlines.go
Line: 67-88
Comment:
**Split Children Become Top-Level Lines**
The dedicated query returns lines with a `SplitLineGroupID`, while `GetStandardLinesForSubscription` also loads those rows inside their split-line hierarchy. `Service.GetLinesForSubscription` appends both results, so subscription synchronization can receive the same dedicated line once as a hierarchy child and again as an independent gathering line, leading to duplicate or conflicting reconciliation.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/openmeter/github/openmeterio/openmeter/-/custom-context?memory=eb020e3b-8e3b-45ea-a8b7-4006b2b2065b))
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+42
to
+44
| ALTER TABLE "billing_gathering_invoice_lines" DROP CONSTRAINT "billing_gathering_line_invoice_fk", ADD CONSTRAINT "service_period_not_inverted" CHECK (service_period_start <= service_period_end), ADD CONSTRAINT "billing_gathering_line_invoice_fk" FOREIGN KEY ("invoice_id") REFERENCES "billing_gathering_invoices" ("id") ON UPDATE NO ACTION ON DELETE CASCADE; | ||
| -- create index "billinggatheringinvoiceline_invoice_id" to table: "billing_gathering_invoice_lines" | ||
| CREATE INDEX "billinggatheringinvoiceline_invoice_id" ON "billing_gathering_invoice_lines" ("invoice_id"); |
Contributor
There was a problem hiding this comment.
The migration repoints every existing billing_gathering_invoice_lines.invoice_id from billing_invoices to the newly created billing_gathering_invoices table without first copying its parent invoice. If this line table already contains legacy rows, PostgreSQL rejects the new constraint because their parent IDs are absent from the new table, preventing the migration from applying.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/migrate/migrations/20260717060930_create_billing_gathering_invoices.up.sql
Line: 42-44
Comment:
**Foreign Key Repoint Can Block Migration**
The migration repoints every existing `billing_gathering_invoice_lines.invoice_id` from `billing_invoices` to the newly created `billing_gathering_invoices` table without first copying its parent invoice. If this line table already contains legacy rows, PostgreSQL rejects the new constraint because their parent IDs are absent from the new table, preventing the migration from applying.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/openmeter/github/openmeterio/openmeter/-/custom-context?memory=eb020e3b-8e3b-45ea-a8b7-4006b2b2065b))
How can I resolve this? If you propose a fix, please make it concise.
turip
added
the
keep-open
This branch has not been deployed
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
1 participant
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.
Summary
Migration behavior
This is transitional scaffolding for both storage sources. Reads support legacy and dedicated gathering invoice data while migration is in progress.
Conflicting rows are surfaced as errors instead of selecting one implicitly. We expect migrations between the legacy and dedicated tables to be atomic, so the same namespaced invoice or line existing in both sources indicates an invalid migration state.
Validation
POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./test/billing -run '^TestBillingAdapter$' -count=1go vet -tags=dynamic ./openmeter/billing/adapter ./openmeter/billing/service ./test/billingmake migrate-check-validateGreptile Summary
This PR adds dual-storage reads for gathering invoices during migration. The main changes are:
Confidence Score: 4/5
The dual-storage migration path can hide conflicting invoices, reject existing line data, and return split children twice.
openmeter/billing/adapter/gatheringinvoice.go, openmeter/billing/adapter/gatheringlines.go, openmeter/ent/schema/billing_invoice_search.go, and the gathering-invoice migration
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Invoice read] --> B[billing_invoice_search_v1s] B --> C[Legacy billing_invoices] B --> D[Dedicated billing_gathering_invoices] C --> E[Hydrate by namespace and ID] D --> E E --> F[Invoice response] G[Subscription line read] --> H[Legacy gathering lines] G --> I[Dedicated gathering lines] H --> J[Combined persisted state] I --> J%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Invoice read] --> B[billing_invoice_search_v1s] B --> C[Legacy billing_invoices] B --> D[Dedicated billing_gathering_invoices] C --> E[Hydrate by namespace and ID] D --> E E --> F[Invoice response] G[Subscription line read] --> H[Legacy gathering lines] G --> I[Dedicated gathering lines] H --> J[Combined persisted state] I --> JPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: support dedicated gathering invoic..." | Re-trigger Greptile
Context used (3)