refactor: split get*subscription calls#4745
Open
turip wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughBilling now separates standard, gathering, and split-line-group subscription retrieval. Progressive billing and quantity snapshotting move into a dedicated legacy line engine, while subscription sync and service wiring consume the new typed APIs and hierarchy models. ChangesBilling line contracts and models
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
19
to
+66
| var _ billing.InvoiceSplitLineGroupAdapter = (*adapter)(nil) | ||
|
|
||
| func (a *adapter) GetSplitLineGroupsForSubscription(ctx context.Context, in billing.GetLinesForSubscriptionInput) ([]billing.SplitLineHierarchy, error) { | ||
| if err := in.Validate(); err != nil { | ||
| return nil, billing.ValidationError{ | ||
| Err: err, | ||
| } | ||
| } | ||
|
|
||
| return entutils.TransactingRepo(ctx, a, func(ctx context.Context, tx *adapter) ([]billing.SplitLineHierarchy, error) { | ||
| dbGroups, err := tx.db.BillingInvoiceSplitLineGroup.Query(). | ||
| Where(billinginvoicesplitlinegroup.Namespace(in.Namespace)). | ||
| Where(billinginvoicesplitlinegroup.SubscriptionID(in.SubscriptionID)). | ||
| WithBillingInvoiceLines(func(q *db.BillingInvoiceLineQuery) { | ||
| tx.expandLineItems(q) | ||
| q.WithBillingInvoice(func(q *db.BillingInvoiceQuery) { | ||
| q.WithBillingWorkflowConfig(workflowConfigWithTaxCode) | ||
| }) | ||
| }). | ||
| Where(billinginvoicesplitlinegroup.DeletedAtIsNil()). | ||
| All(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fetching split line groups: %w", err) | ||
| } | ||
|
|
||
| groups, err := slicesx.MapWithErr(dbGroups, func(dbGroup *db.BillingInvoiceSplitLineGroup) (billing.SplitLineHierarchy, error) { | ||
| group, err := tx.mapSplitLineGroupFromDB(dbGroup) | ||
| if err != nil { | ||
| return billing.SplitLineHierarchy{}, err | ||
| } | ||
|
|
||
| lines, err := tx.mapSplitLineHierarchyLinesFromDB(ctx, dbGroup.Edges.BillingInvoiceLines) | ||
| if err != nil { | ||
| return billing.SplitLineHierarchy{}, err | ||
| } | ||
|
|
||
| return billing.SplitLineHierarchy{ | ||
| Group: group, | ||
| Lines: lines, | ||
| }, nil | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("mapping split line groups: %w", err) | ||
| } | ||
|
|
||
| return groups, nil | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
This file was added in the PR but references three types that were simultaneously deleted from the billing package by removing billing/invoicelinesplitgroup.go:
- Line 19:
billing.InvoiceSplitLineGroupAdapter— interface deleted frombilling/adapter.go - Line 21+:
billing.SplitLineHierarchy— type deleted from billing package - Line 268+:
billing.LineWithInvoiceHeader— type deleted from billing package
The compile-time interface check var _ billing.InvoiceSplitLineGroupAdapter = (*adapter)(nil) and all subsequent method signatures and return-type literals are compilation errors. The adapter appears to need updating to use the newly defined types in the splitlinegroup package instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/invoicing/legacy/splitlinegroup/adapter/invoicelinesplitgroup.go
Line: 19-66
Comment:
**New adapter file references deleted billing types**
This file was added in the PR but references three types that were simultaneously deleted from the `billing` package by removing `billing/invoicelinesplitgroup.go`:
- Line 19: `billing.InvoiceSplitLineGroupAdapter` — interface deleted from `billing/adapter.go`
- Line 21+: `billing.SplitLineHierarchy` — type deleted from billing package
- Line 268+: `billing.LineWithInvoiceHeader` — type deleted from billing package
The compile-time interface check `var _ billing.InvoiceSplitLineGroupAdapter = (*adapter)(nil)` and all subsequent method signatures and return-type literals are compilation errors. The adapter appears to need updating to use the newly defined types in the `splitlinegroup` package instead.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+79
to
+86
| invoiceUpdater, err := invoiceupdater.New(invoiceupdater.Config{ | ||
| BillingService: config.BillingService, | ||
| QuantitySnapshotter: config.LegacyBillingLineEngine, | ||
| Logger: config.Logger, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating invoice updater: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
SplitLineGroupService never provided to invoiceupdater.New, so initialization always fails
invoiceupdater.Config.Validate() requires SplitLineGroupService to be non-nil and returns an error when it is absent. The call here omits it — the reconciler.Config struct also has no SplitLineGroupService field — so invoiceupdater.New will always return "split line group service is required", meaning the reconciler itself can never be constructed successfully.
Previously billingService.DeleteSplitLineGroup / billingService.UpdateSplitLineGroup were called directly; those methods were removed from billing.Service in this PR and transferred to splitlinegroup.Service. The service needs to be wired through reconciler.Config and service.Config down to the invoiceupdater.New call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go
Line: 79-86
Comment:
**`SplitLineGroupService` never provided to `invoiceupdater.New`, so initialization always fails**
`invoiceupdater.Config.Validate()` requires `SplitLineGroupService` to be non-nil and returns an error when it is absent. The call here omits it — the `reconciler.Config` struct also has no `SplitLineGroupService` field — so `invoiceupdater.New` will always return `"split line group service is required"`, meaning the reconciler itself can never be constructed successfully.
Previously `billingService.DeleteSplitLineGroup` / `billingService.UpdateSplitLineGroup` were called directly; those methods were removed from `billing.Service` in this PR and transferred to `splitlinegroup.Service`. The service needs to be wired through `reconciler.Config` and `service.Config` down to the `invoiceupdater.New` call.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go (1)
90-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAddShrink still uses the old nested child shape
Inpatchinvoicelinehierarchy.go:90-133,existingHierarchy.Lines()now returns[]LineHeaderAccessor, sochild.Line/child.Invoiceno longer match this loop. The shrink path needs the flat accessors here too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go` around lines 90 - 133, The AddShrink loop over existingHierarchy.Lines() still uses the old nested child shape. Update the loop in the shrink path to use the returned LineHeaderAccessor directly, replacing child.Line and child.Invoice references with the corresponding flat accessor methods while preserving the existing delete, clone, update, and invoice-type behavior.
🧹 Nitpick comments (3)
openmeter/billing/invoicing/legacy/splitlinegroup/headers.go (1)
66-74: 🗄️ Data Integrity & Integration | 🔵 TrivialClone()/CloneOrNil() leave
Subscriptionshared, not copied.Both clones deep-copy
Annotationsbut skip theSubscription *billing.SubscriptionReferencepointer, so the "cloned" line still points at the sameSubscriptionReferenceas the original.persistedstate/loader.go'snormalizePersistedSplitLineHierarchythen mutates that shared pointer in place (ref.BillingPeriod = ...), which defeats the point of cloning before mutating. Low risk today since the pre-clone value isn't reused afterward in that call site, but it's a footgun for future refactors. See consolidated comment for the paired fix.♻️ Deep-clone Subscription too
func (l StandardLine) Clone() (StandardLine, error) { var err error l.Annotations, err = l.Annotations.Clone() if err != nil { return StandardLine{}, fmt.Errorf("cloning annotations: %w", err) } + + if l.Subscription != nil { + l.Subscription = l.Subscription.Clone() + } return l, nil }Also applies to: 118-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/invoicing/legacy/splitlinegroup/headers.go` around lines 66 - 74, Update StandardLine.Clone and the corresponding CloneOrNil path to deep-copy the Subscription *billing.SubscriptionReference instead of retaining the original pointer. Preserve nil subscriptions as nil, and return the cloned line with independent Subscription and Annotations values.openmeter/billing/lineengine/engine.go (1)
29-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffValidate() methods keep returning on the first failure instead of aggregating. Each of these files adds new nil-checks to a
Config.Validate()that still returns on the first error found, rather than collecting all failures and returningmodels.NewNillableGenericValidationError(errors.Join(errs...))as required by the guideline. Same root cause across the cohort — the pre-existing early-return style in these functions, now extended with new checks.
openmeter/billing/lineengine/engine.go#L29-L51: rewriteConfig.Validate()to accumulate errs for SplitLineGroupAdapter/RatingService/FeatureService/StreamingConnector/MaxParallelQuantitySnapshots and returnmodels.NewNillableGenericValidationError(errors.Join(errs...)).openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go#L45-L48: fold the newLegacyBillingLineEnginenil-check into an aggregated-errorsValidate().openmeter/billing/worker/subscriptionsync/service/service.go#L48-L51: fold the newLegacyBillingLineEnginenil-check into an aggregated-errorsValidate().openmeter/billing/service/service.go#L88-L91: fold the newLegacyBillingLineEnginenil-check into an aggregated-errorsValidate().Based on learnings from the coding guidelines: "For Validate() error methods, collect all validation failures and return models.NewNillableGenericValidationError(errors.Join(errs...)), preserving field context with wrapped errors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/lineengine/engine.go` around lines 29 - 51, Update each Config.Validate method to collect all validation failures in errs instead of returning early, preserving field context with wrapped errors, then return models.NewNillableGenericValidationError(errors.Join(errs...)). Apply this to openmeter/billing/lineengine/engine.go lines 29-51 for all five checks, and fold the LegacyBillingLineEngine nil-check into the aggregated validation in openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go lines 45-48, openmeter/billing/worker/subscriptionsync/service/service.go lines 48-51, and openmeter/billing/service/service.go lines 88-91.Source: Coding guidelines
openmeter/billing/lineengine/quantitysnapshot.go (1)
234-240: 🩺 Stability & Availability | 🔵 TrivialHeads up: there's a
TODO[BeforeMerge]sitting right on the new validation branch.The comment on line 236 flags that this new "split line group id set but hierarchy not expanded" check needs a test before merging. Since this is exactly that moment, want me to sketch a test for
getFeatureUsageInput.Validate()covering this branch, or would you rather track it as a follow-up issue?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/lineengine/quantitysnapshot.go` around lines 234 - 240, Add a test for getFeatureUsageInput.Validate() that supplies a line with SplitLineGroupID set and SplitLineHierarchy unset, and asserts the validation returns the expected error. Remove the TODO[BeforeMerge] comment after the branch is covered, preserving existing validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go`:
- Around line 349-352: Update stubRatingService.GenerateDetailedLines to return
the configured s.result after recording the options, instead of constructing an
empty billingrating.GenerateDetailedLinesResult, so tests initializing
stubRatingService with a result receive their configured detailed lines.
In `@openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go`:
- Line 43: Update every affected Validate method in
openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go at
lines 43, 98, 118, 150, and 293 to return
models.NewNillableGenericValidationError(errors.Join(errs...)). In the methods
at lines 43, 98, 118, and 150, wrap nested validation errors with descriptive
field context using wrapped errors before appending them; the line 293 site
requires only the final error wrapper.
In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go`:
- Around line 333-352: Update StandardLine.Clone() and
GatheringLine.CloneOrNil() to deep-copy the Subscription field, not only
Annotations, before returning their clones. This ensures
normalizePersistedSplitLineHierarchy can call normalizeSubscriptionReference
without mutating the original SubscriptionReference through shared pointers.
- Around line 26-28: Update the NewLoader call in the reconciliation setup to
pass the available splitLineGroupService implementation as the third dependency,
alongside billingService and chargesService. Ensure the constructor invocation
matches the expanded splitLineGroupService interface and preserves the existing
dependency wiring.
---
Outside diff comments:
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go`:
- Around line 90-133: The AddShrink loop over existingHierarchy.Lines() still
uses the old nested child shape. Update the loop in the shrink path to use the
returned LineHeaderAccessor directly, replacing child.Line and child.Invoice
references with the corresponding flat accessor methods while preserving the
existing delete, clone, update, and invoice-type behavior.
---
Nitpick comments:
In `@openmeter/billing/invoicing/legacy/splitlinegroup/headers.go`:
- Around line 66-74: Update StandardLine.Clone and the corresponding CloneOrNil
path to deep-copy the Subscription *billing.SubscriptionReference instead of
retaining the original pointer. Preserve nil subscriptions as nil, and return
the cloned line with independent Subscription and Annotations values.
In `@openmeter/billing/lineengine/engine.go`:
- Around line 29-51: Update each Config.Validate method to collect all
validation failures in errs instead of returning early, preserving field context
with wrapped errors, then return
models.NewNillableGenericValidationError(errors.Join(errs...)). Apply this to
openmeter/billing/lineengine/engine.go lines 29-51 for all five checks, and fold
the LegacyBillingLineEngine nil-check into the aggregated validation in
openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go lines
45-48, openmeter/billing/worker/subscriptionsync/service/service.go lines 48-51,
and openmeter/billing/service/service.go lines 88-91.
In `@openmeter/billing/lineengine/quantitysnapshot.go`:
- Around line 234-240: Add a test for getFeatureUsageInput.Validate() that
supplies a line with SplitLineGroupID set and SplitLineHierarchy unset, and
asserts the validation returns the expected error. Remove the TODO[BeforeMerge]
comment after the branch is covered, preserving existing validation behavior.
🪄 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: da4ceb26-1071-4b04-9367-e3d5629da650
📒 Files selected for processing (48)
app/common/billing.goopenmeter/billing/adapter.goopenmeter/billing/charges/flatfee/service/realizations/credittheninvoice.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/service/rating/delta/engine_test.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/invoicelinesplitgroup.goopenmeter/billing/invoicing/legacy/splitlinegroup/adapter.goopenmeter/billing/invoicing/legacy/splitlinegroup/adapter/invoicelinesplitgroup.goopenmeter/billing/invoicing/legacy/splitlinegroup/headers.goopenmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.goopenmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup_test.goopenmeter/billing/invoicing/legacy/splitlinegroup/service.goopenmeter/billing/invoicing/legacy/splitlinegroup/service/invoicelinesplitgroup.goopenmeter/billing/lineengine/engine.goopenmeter/billing/lineengine/quantitysnapshot.goopenmeter/billing/lineengine/splitlinegroup.goopenmeter/billing/lineengine/stdinvoice.goopenmeter/billing/lineengine/stdline.goopenmeter/billing/rating/line.goopenmeter/billing/rating/service.goopenmeter/billing/rating/service/detailedline.goopenmeter/billing/rating/service/mutator/credits_test.goopenmeter/billing/rating/service/mutator/forbidunitconfig_test.goopenmeter/billing/rating/service/mutator/unitconfig_test.goopenmeter/billing/rating/service/rate/types.goopenmeter/billing/rating/service/testutil/ubptest.goopenmeter/billing/service.goopenmeter/billing/service/service.goopenmeter/billing/service/stdinvoiceline.goopenmeter/billing/stdinvoiceedit.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/item.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/loader.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchhelpers.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/server/server_test.gotest/app/testenv.gotest/billing/subscription_test.gotest/billing/suite.gotest/customer/testenv.gotest/subscription/framework_test.go
💤 Files with no reviewable changes (8)
- openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go
- openmeter/billing/service/stdinvoiceline.go
- openmeter/billing/charges/usagebased/rating.go
- openmeter/billing/invoicelinesplitgroup.go
- openmeter/server/server_test.go
- openmeter/billing/stdinvoiceedit.go
- openmeter/billing/adapter.go
- openmeter/billing/service.go
Comment on lines
349
to
352
| func (s *stubRatingService) GenerateDetailedLines(in billingrating.StandardLineAccessor, opts ...billingrating.GenerateDetailedLinesOption) (billingrating.GenerateDetailedLinesResult, error) { | ||
| s.lastOpts = billingrating.NewGenerateDetailedLinesOptions(opts...) | ||
| return s.result, nil | ||
| return billingrating.GenerateDetailedLinesResult{}, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'stubRatingService\{' -A5 openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go
rg -n '\.result\b' openmeter/billing/charges/usagebased/service/rating/delta/engine_test.goRepository: openmeterio/openmeter
Length of output: 1430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go
printf '\n----\n'
sed -n '220,420p' openmeter/billing/charges/usagebased/service/rating/delta/engine_test.goRepository: openmeterio/openmeter
Length of output: 12681
Return the configured stub result — the tests in this file still initialize stubRatingService{result: ...}, so GenerateDetailedLines should return s.result instead of an empty value; otherwise those cases will always get zeroed detailed lines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go`
around lines 349 - 352, Update stubRatingService.GenerateDetailedLines to return
the configured s.result after recording the options, instead of constructing an
empty billingrating.GenerateDetailedLinesResult, so tests initializing
stubRatingService with a result receive their configured detailed lines.
| errs = append(errs, err) | ||
| } | ||
|
|
||
| return errors.Join(errs...) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap validation errors and preserve field context.
The Validate methods currently collect errors but fail to wrap the final joined error in models.NewNillableGenericValidationError and omit field context for nested validations. As per coding guidelines for **/*.go, validation methods must return models.NewNillableGenericValidationError(errors.Join(errs...)) and preserve field context with wrapped errors to ensure errors are mapped correctly in the API layer.
openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L43-L43: returnmodels.NewNillableGenericValidationError(errors.Join(errs...))and wrap nested validation calls above with field context (e.g.,fmt.Errorf("service period: %w", err)).openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L98-L98: returnmodels.NewNillableGenericValidationError(errors.Join(errs...))and wrap nested errors.openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L118-L118: returnmodels.NewNillableGenericValidationError(errors.Join(errs...))and wrap nested errors.openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L150-L150: returnmodels.NewNillableGenericValidationError(errors.Join(errs...))and wrap nested errors.openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L293-L293: returnmodels.NewNillableGenericValidationError(errors.Join(errs...)).
📍 Affects 1 file
openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L43-L43(this comment)openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L98-L98openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L118-L118openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L150-L150openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go#L293-L293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go`
at line 43, Update every affected Validate method in
openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go at
lines 43, 98, 118, 150, and 293 to return
models.NewNillableGenericValidationError(errors.Join(errs...)). In the methods
at lines 43, 98, 118, and 150, wrap nested validation errors with descriptive
field context using wrapped errors before appending them; the line 293 site
requires only the final error wrapper.
Source: Coding guidelines
Comment on lines
+26
to
+28
| type splitLineGroupService interface { | ||
| GetSplitLineGroupsForSubscription(ctx context.Context, input billing.GetLinesForSubscriptionInput) ([]splitlinegroup.SplitLineHierarchy, error) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find what implements splitLineGroupService and how it's wired into NewLoader.
rg -n 'func.*GetSplitLineGroupsForSubscription' openmeter/billing -g '*.go'
rg -n 'NewLoader\(' openmeter/billing -g '*.go' -A3Repository: openmeterio/openmeter
Length of output: 1647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go
printf '\n---\n'
sed -n '1,120p' openmeter/billing/invoicing/legacy/splitlinegroup/service/invoicelinesplitgroup.go
printf '\n---\n'
sed -n '1,120p' openmeter/billing/invoicing/legacy/splitlinegroup/adapter/invoicelinesplitgroup.go
printf '\n---\n'
sed -n '1,120p' openmeter/billing/worker/subscriptionsync/service/reconcile.goRepository: openmeterio/openmeter
Length of output: 14054
Wire splitLineGroupService into NewLoader
In openmeter/billing/worker/subscriptionsync/service/reconcile.go:31, persistedstate.NewLoader is still called with only billingService and chargesService, but the constructor now requires splitLineGroupService too. That leaves the new dependency unwired and breaks the build.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go`
around lines 26 - 28, Update the NewLoader call in the reconciliation setup to
pass the available splitLineGroupService implementation as the third dependency,
alongside billingService and chargesService. Ensure the constructor invocation
matches the expanded splitLineGroupService interface and preserves the existing
dependency wiring.
Comment on lines
+333
to
352
| func normalizePersistedSplitLineHierarchy(hierarchy splitlinegroup.SplitLineHierarchy) (*splitlinegroup.SplitLineHierarchy, error) { | ||
| cloned, err := hierarchy.Clone() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cloning hierarchy: %w", err) | ||
| } | ||
|
|
||
| for i := range cloned.Lines { | ||
| cloned.Lines[i].Line.UpdateServicePeriod(func(period *timeutil.ClosedPeriod) { | ||
| *period = period.Truncate(streaming.MinimumWindowSizeDuration) | ||
| }) | ||
| cloned.Group.ServicePeriod = cloned.Group.ServicePeriod.Truncate(streaming.MinimumWindowSizeDuration) | ||
|
|
||
| if invoiceAtAccessor, ok := cloned.Lines[i].Line.(billing.InvoiceAtAccessor); ok { | ||
| invoiceAtAccessor.SetInvoiceAt(invoiceAtAccessor.GetInvoiceAt().Truncate(streaming.MinimumWindowSizeDuration)) | ||
| } | ||
| for i, line := range cloned.StandardLines { | ||
| line.ServicePeriod = line.ServicePeriod.Truncate(streaming.MinimumWindowSizeDuration) | ||
| normalizeSubscriptionReference(line.Subscription) | ||
|
|
||
| normalizeSubscriptionReference(cloned.Lines[i].Line.GetSubscriptionReference()) | ||
| } | ||
| cloned.StandardLines[i] = line | ||
| } | ||
|
|
||
| return billing.NewLineOrHierarchy(&cloned), nil | ||
| default: | ||
| return lineOrHierarchy, nil | ||
| if cloned.GatheringLine != nil { | ||
| cloned.GatheringLine.ServicePeriod = cloned.GatheringLine.ServicePeriod.Truncate(streaming.MinimumWindowSizeDuration) | ||
| cloned.GatheringLine.InvoiceAt = cloned.GatheringLine.InvoiceAt.Truncate(streaming.MinimumWindowSizeDuration) | ||
| normalizeSubscriptionReference(cloned.GatheringLine.Subscription) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files | rg 'openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go|splitlinegroup|headers.go|normalizeSubscriptionReference|StandardLine'Repository: openmeterio/openmeter
Length of output: 1190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline: loader.go ---'
ast-grep outline openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go --view expanded
echo
echo '--- outline: splitlinegroup headers/service files ---'
ast-grep outline openmeter/billing/invoicing/legacy/splitlinegroup/headers.go --view expanded
echo
ast-grep outline openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go --view expanded
echo
echo '--- relevant slices: loader.go ---'
sed -n '300,380p' openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go
echo
echo '--- relevant slices: splitlinegroup/headers.go ---'
sed -n '1,260p' openmeter/billing/invoicing/legacy/splitlinegroup/headers.go
echo
echo '--- relevant slices: splitlinegroup/invoicelinesplitgroup.go ---'
sed -n '1,260p' openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.goRepository: openmeterio/openmeter
Length of output: 20033
Deep-copy Subscription in the line clones
In openmeter/billing/invoicing/legacy/splitlinegroup/headers.go, StandardLine.Clone() and GatheringLine.CloneOrNil() only copy Annotations, so normalizeSubscriptionReference(...) still mutates the original SubscriptionReference through a shared pointer. Copy Subscription in the clone helpers before normalizing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go`
around lines 333 - 352, Update StandardLine.Clone() and
GatheringLine.CloneOrNil() to deep-copy the Subscription field, not only
Annotations, before returning their clones. This ensures
normalizePersistedSplitLineHierarchy can call normalizeSubscriptionReference
without mutating the original SubscriptionReference through shared pointers.
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
GetLinesForSubscriptionAPI into typed standard-line, gathering-line, and split-line-group service/adapter methodsLineOrHierarchyunion and the obsolete downstream conversion helpersMotivation
This establishes the ownership boundary needed to move split-line-group handling into the legacy billing line engine. Standard lines, gathering lines, and split-line hierarchies can now be loaded independently, so the main billing path no longer needs a combined line-or-hierarchy abstraction and split-line-group behavior can move behind the legacy engine incrementally.
Validation
nix develop --impure .#ci -c make test-nocacheSummary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR refactors the combined
GetLinesForSubscriptionAPI into three typed adapter/service methods (GetStandardLinesForSubscription,GetGatheringLinesForSubscription,GetSplitLineGroupsForSubscription), moves split-line-group domain types into a newbilling/invoicing/legacy/splitlinegrouppackage, and removes thebilling.LineOrHierarchyunion type along with the downstreamGetLinesForSubscriptionconversion helpers. The architectural direction is sound — cleanly typed retrieval methods and isolated domain ownership — but the migration is incomplete in several files, resulting in compile errors and a broken initialization path.splitlinegroup/adapter/invoicelinesplitgroup.goand thepatchinvoicelinehierarchy.goAddShrink/AddExtendmethods still referencebilling.SplitLineHierarchy,billing.LineWithInvoiceHeader, andbilling.InvoiceSplitLineGroupAdapter, all of which were deleted from thebillingpackage in this same PR.billing/rating/service/testutil/ubptest.gowas updated to usebilling.StandardLineWithSplitLineHierarchy(defined inlineengine, notbilling) and several other deletedbilling.*hierarchy types.invoiceupdater.Confignow requiresSplitLineGroupServicebut the field is never threaded throughreconciler.Config→invoiceupdater.New, so service initialization always returns an error at startup.Confidence Score: 1/5
Not safe to merge: the PR introduces compile errors in at least three packages and a runtime initialization failure in the subscription-sync reconciler.
Three packages fail to compile because deleted billing.* types are still referenced after the migration. Separately, the reconciler can never be constructed at runtime because SplitLineGroupService is a required field in invoiceupdater.Config but is not wired through any of the calling configs. The overall refactoring direction is well-designed, but these gaps need to be resolved before the code can run at all.
patchinvoicelinehierarchy.go (AddShrink loop body + AddExtend entirely unupdated), splitlinegroup/adapter/invoicelinesplitgroup.go (all billing.* hierarchy type references), ubptest.go (billing.StandardLineWithSplitLineHierarchy and other deleted types), reconciler.go (missing SplitLineGroupService in invoiceupdater.New call).
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant SS as SubscriptionSync participant L as persistedstate.Loader participant BS as billing.Service participant SLG as splitlinegroup.Service participant R as reconciler participant IU as invoiceupdater.Updater SS->>L: LoadForSubscription(sub) L->>BS: GetStandardLinesForSubscription() L->>BS: GetGatheringLinesForSubscription() L->>SLG: GetSplitLineGroupsForSubscription() L-->>SS: "State{lineItems, hierarchyItems, byUniqueID}" SS->>R: Apply(plan) R->>IU: ApplyPatches(patches) IU->>SLG: DeleteSplitLineGroup() / UpdateSplitLineGroup() IU->>BS: (standard invoice line ops)%%{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"}}}%% sequenceDiagram participant SS as SubscriptionSync participant L as persistedstate.Loader participant BS as billing.Service participant SLG as splitlinegroup.Service participant R as reconciler participant IU as invoiceupdater.Updater SS->>L: LoadForSubscription(sub) L->>BS: GetStandardLinesForSubscription() L->>BS: GetGatheringLinesForSubscription() L->>SLG: GetSplitLineGroupsForSubscription() L-->>SS: "State{lineItems, hierarchyItems, byUniqueID}" SS->>R: Apply(plan) R->>IU: ApplyPatches(patches) IU->>SLG: DeleteSplitLineGroup() / UpdateSplitLineGroup() IU->>BS: (standard invoice line ops)Comments Outside Diff (2)
openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go, line 90-133 (link)AddShrinkandAddExtendexistingHierarchy.Lines()now returns[]splitlinegroup.LineHeaderAccessor, but the loop body throughoutAddShrink(line 95 onwards) still accesseschild.Line.*andchild.Invoice.*— fields that do not exist on theLineHeaderAccessorinterface. This is a compilation error on every affected statement (child.Line.GetServicePeriod(),child.Line.CloneWithoutChildren(),child.Invoice.AsInvoice().Type(), etc.).AddExtendwas not updated at all: line 166 doeslen(existingHierarchy.Lines)whereLinesis now a method value (a function), which cannot be passed tolen, and line 168 referencesbilling.LineWithInvoiceHeaderwhich was deleted from the billing package. Both are compile errors that block any build of this package.Prompt To Fix With AI
openmeter/billing/rating/service/testutil/ubptest.go, line 62-103 (link)This file was updated to use
billing.StandardLineWithSplitLineHierarchy(line 62) and a set of hierarchy types (lines 79–103), but none of these exist in thebillingpackage after this PR:billing.StandardLineWithSplitLineHierarchy— defined inlineengine.StandardLineWithSplitLineHierarchy, not exported throughbillingbilling.SplitLineHierarchy— deleted from billing package (moved tosplitlinegroup)billing.SplitLineGroup/billing.SplitLineGroupMutableFields— deletedbilling.LineWithInvoiceHeader/billing.NewLineWithInvoiceHeader— deletedEvery reference to these types is a compilation error. The import would need to be updated to
billinglineengineorsplitlinegroupdepending on which concrete type the test should use.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "feat: move legacy splitline handling to ..." | Re-trigger Greptile