chore: migrate detailed lines to new schema pt1 #3798
Conversation
📝 WalkthroughWalkthroughAdds invoice schema versioning and write-time schema management, migrates customer invoices from schema level 1→2, propagates schema level through invoice creation/upserts, introduces schema-aware line mapping and V2 detailed-line paths, renames customer override to lock, and adds DB migration functions and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BillingService
participant Adapter
participant DB
rect rgba(0,128,0,0.5)
Client->>BillingService: CreateInvoice(...)
BillingService->>Adapter: CreateInvoice(ctx, input)
Adapter->>Adapter: GetInvoiceDefaultSchemaLevel(ctx)
Adapter->>DB: SELECT write_schema_level
DB-->>Adapter: defaultSchemaLevel
Adapter->>DB: INSERT invoice (schema_level = defaultSchemaLevel)
DB-->>Adapter: invoiceID
Adapter->>Adapter: UpsertInvoiceLines(SchemaLevel, InvoiceID, ...)
Adapter->>DB: upsert lines (schema-aware)
DB-->>Adapter: OK
Adapter-->>BillingService: InvoiceCreated
BillingService-->>Client: 201 Created
end
sequenceDiagram
participant Caller
participant Service
participant Adapter
participant DB
Caller->>Service: LockCustomerForUpdate(customerID)
Service->>Adapter: LockCustomerForUpdate(ctx, customerID)
Adapter->>DB: SELECT lock row FOR UPDATE
DB-->>Adapter: lockRow
Adapter->>Adapter: shouldInvoicesBeMigrated(customerID)
Adapter->>DB: SELECT min(schema_level) FROM billing_invoices WHERE customer=...
DB-->>Adapter: minSchemaLevel
alt minSchemaLevel < defaultSchemaLevel
Adapter->>Adapter: migrateCustomerInvoices(customerID, minSchemaLevel)
Adapter->>DB: SELECT om_func_migrate_customer_invoices_to_schema_level_2(customerID)
DB-->>Adapter: updatedCount
end
Adapter->>DB: UpsertCustomerLock(...)
DB-->>Adapter: OK
Adapter-->>Service: lock acquired
Service-->>Caller: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/adapter/invoicelinemapper.go (1)
17-42: GetLinesForSubscription is missing detailed-line edge preloading.At line 794,
GetLinesForSubscriptionusesexpandLineItemsinstead ofexpandLineItemsWithDetailedLines. This meansDetailedLinesandDetailedLinesV2edges aren't preloaded. WhenmapInvoiceLineFromDBruns,dbLine.Edges.DetailedLinesV2(schema level 2+) anddbLine.Edges.DetailedLines(schema level 1) will be nil, causingMapWithErrto silently return empty lists instead of the actual detailed lines—a data loss bug.Change line 794 to use
tx.expandLineItemsWithDetailedLines(query)instead oftx.expandLineItems(query). The other three call sites (lines 644, 767, 749) already do this correctly.
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/invoicelines.go`:
- Around line 286-294: The refetch call can mis-map lines when input.Lines
contains lines from multiple invoices; before calling tx.refetchInvoiceLines
construct a guard that verifies every billing.Line in input.Lines has the same
InvoiceID as input.InvoiceID (or return an error), e.g. iterate input.Lines and
compare each line.ID's associated InvoiceID or the line.InvoiceID field and fail
early if any mismatch is found; update the caller to return a clear validation
error when lines span invoices so refetchInvoiceLinesInput.InvoiceID remains
authoritative.
In `@openmeter/billing/adapter/schemamigration.go`:
- Around line 69-83: The call in migrateSchemaLevel1 to the DB function
om_func_migrate_customer_invoices_to_schema_level_2 must include the tenant
namespace to avoid cross-namespace migrations: modify the tx.db.QueryContext
invocation in migrateSchemaLevel1 to pass the CustomerID's namespace value (from
the NamespacedID) as the first/appropriate argument and the customer ID as the
other argument, and update the DB function signature and migration SQL to accept
the namespace parameter and add a WHERE predicate that filters invoices by
namespace as well as customer ID.
In `@tools/migrate/migrations/20260121143838_detailed-lines-migration.up.sql`:
- Around line 16-147: The migration function
om_func_migrate_customer_invoices_to_schema_level_2 currently only accepts
p_customer_id and can cross-tenant migrate; change its signature to accept
p_namespace (e.g., p_namespace TEXT, p_customer_id TEXT) and add AND i.namespace
= p_namespace (or WHERE namespace = p_namespace in the UPDATE) to all three SQL
statements to scope by namespace; then update the Go caller in
openmeter/billing/adapter/schemamigration.go to call the function with
customerID.Namespace and customerID.ID (adjusting the QueryContext parameter
order and SQL argument count to match the new two-arg signature).
🧹 Nitpick comments (4)
openmeter/billing/adapter/schemamigration.go (1)
56-66: Fail fast on unsupported schema levels.
Right now a non‑1minLevelsilently no‑ops even if migration is required. Consider returning an explicit error so unsupported upgrades don’t slip by.💡 Suggested guard
@@ func (a *adapter) migrateCustomerInvoices(ctx context.Context, customerID customer.CustomerID, minLevel int) error { return entutils.TransactingRepoWithNoValue(ctx, a, func(ctx context.Context, tx *adapter) error { - if minLevel == 1 { - err := tx.migrateSchemaLevel1(ctx, customerID) - if err != nil { - return err - } - } - - return nil + switch minLevel { + case 1: + return tx.migrateSchemaLevel1(ctx, customerID) + default: + return fmt.Errorf("unsupported invoice schema level: %d", minLevel) + } }) }@@ -import ( - "context" +import ( + "context" + "fmt"openmeter/billing/adapter/schemalevel.go (1)
36-44: Optional: validate schema level inputs.
If only specific schema levels are supported, consider guarding here to prevent persisting invalid values.openmeter/billing/adapter/invoicelines.go (2)
689-698: Potential perf tweak: avoid loading both detailed-line variants.
expandLineItemsWithDetailedLinesnow always fetches both v1 and v2 detailed lines, which adds extra queries. If the schema level is known at the call site, consider branching to only load the relevant edge to trim DB work. As per coding guidelines, focusing on DB query efficiency is important.
801-810: Consider scoping schema lookup to just needed invoices.
getSchemaLevelPerInvoiceuses the full customer set; for large customers this could be heavier than necessary. You already have the invoice IDs indbLines, so narrowing the lookup to those IDs would reduce DB work. As per coding guidelines, DB-path performance is worth tightening.
|
Reviewed comments and nitpick comments and they are safe to ignore. |
gergely-kurucz-konghq
left a comment
gergely-kurucz-konghq
left a comment
There was a problem hiding this comment.
Minor/nit comments, otherwise LGTM!
|
|
||
| // Step 3: Let's create the detailed lines | ||
| if !lineDiffs.DetailedLine.IsEmpty() { | ||
| if input.SchemaLevel == 1 { |
There was a problem hiding this comment.
The IsEmpty check is not needed needed anymore?
There was a problem hiding this comment.
It doesn't matter as the upsert is noop if the diff is empty.
| @@ -52,7 +53,7 @@ type CustomerOverrideAdapter interface { | |||
| type CustomerSynchronizationAdapter interface { | |||
| // UpsertCustomerOverride upserts a customer override ignoring the transactional context, the override | |||
There was a problem hiding this comment.
Update comment after method rename.
2 participants
Overview
This patch introduces schema level 2 for billing. The difference is that detailed lines are stored in the
billing_standard_invoice_detailed_linestable.The attached SQL migration can convert a customer to the new schema level.
The migration flow is the following:
Step 1) This gets merged and deployed to all production environments
With this step we ensure that:
Note: the QueryContext-based invoication is temporary for the time of the migration.
Step 2) #3799 gets merged
This PR sets the default schema level to 2, resulting all writes resulting the customers to be migrated to the new version.
The SQL Statement included will lock all the existing customers and perform an automated upgrade of the lines. Given we acquire the lock no parallelism can take place.
Any customers created during the upgrade will be using the schema level 2 already.
Step 3) Remove old tables and schema level field
Will be performed later.
Notes for reviewer
The tests are passing, so schemaLevel 1 works fine.
Given this #3799 passes too schemaLevel 2 is also operational.
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.