refactor: split get*subscription calls - #4748
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe billing API now retrieves standard lines, gathering lines, and split-line groups through separate typed methods. Adapters implement type-specific queries. Subscription synchronization loads, normalizes, combines, and validates these collections independently. ChangesSubscription line retrieval
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ 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}
turip
force-pushed
the
feat/split-get-subscription-calls-reconstructed
branch
from
August 11, 2026 12:18
97279e3 to
4ef880a
Compare
turip
marked this pull request as ready for review
August 11, 2026 12:19
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/adapter/gatheringlines.go (1)
38-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the query predicates with
GetStandardLinesForSubscription.This predicate chain is identical to
openmeter/billing/adapter/stdinvoicelines.golines 805-827. Only the invoice-status predicate differs:StatusEQhere,StatusNEQthere. The two filters must stay exactly complementary, otherwise subscription sync silently loses or duplicates lines.A small shared builder that takes the status predicate would keep that invariant in one place. Fully optional for this PR.
♻️ Sketch of a shared builder
func (a *adapter) subscriptionLinesQuery( q *db.BillingInvoiceLineQuery, in billing.GetLinesForSubscriptionInput, invoiceStatus predicate.BillingInvoice, ) *db.BillingInvoiceLineQuery { q = q. Where(billinginvoiceline.Namespace(in.Namespace)). Where(billinginvoiceline.SubscriptionID(in.SubscriptionID)). // Split-line children are loaded through their hierarchy instead of as independent subscription items. Where(billinginvoiceline.ParentLineIDIsNil()). Where(billinginvoiceline.HasBillingInvoiceWith(invoiceStatus)). Where(billinginvoiceline.Or( billinginvoiceline.DeletedAtIsNil(), billinginvoiceline.And( billinginvoiceline.DeletedAtNotNil(), billinginvoiceline.ManagedByEQ(billing.ManuallyManagedLine), ), )). WithBillingInvoice(func(q *db.BillingInvoiceQuery) { q.Where(billinginvoice.Namespace(in.Namespace)) }) if !in.IncludeChargeManaged { q = q.Where(billinginvoiceline.ChargeIDIsNil()) } return a.expandLineItems(q, in.Namespace) }🤖 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/adapter/gatheringlines.go` around lines 38 - 60, Extract the shared subscription line predicates from the current query and GetStandardLinesForSubscription into a common builder, such as subscriptionLinesQuery, accepting the invoice-status predicate as its variable input. Update both callers to use it with StatusEQ and StatusNEQ respectively, while preserving the existing charge-managed filtering and line-item expansion behavior.openmeter/billing/adapter/invoicelinesplitgroup.go (1)
52-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing
mapSplitLineHierarchyFromDBhelper.This closure repeats
mapSplitLineHierarchyFromDB(lines 252-269) exactly: same group mapping, same line mapping, same struct assembly. Calling the helper keeps one mapping path, so future changes to hierarchy mapping land in both readers.♻️ Proposed refactor
- 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 - }) + groups, err := slicesx.MapWithErr(dbGroups, func(dbGroup *db.BillingInvoiceSplitLineGroup) (billing.SplitLineHierarchy, error) { + return tx.mapSplitLineHierarchyFromDB(ctx, dbGroup) + }) if err != nil { return nil, fmt.Errorf("mapping split line groups: %w", err) }🤖 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/adapter/invoicelinesplitgroup.go` around lines 52 - 70, Replace the duplicated mapping closure passed to slicesx.MapWithErr with the existing mapSplitLineHierarchyFromDB helper. Preserve the current error propagation and “mapping split line groups” wrapping while routing each dbGroup through that helper.
🤖 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.
Nitpick comments:
In `@openmeter/billing/adapter/gatheringlines.go`:
- Around line 38-60: Extract the shared subscription line predicates from the
current query and GetStandardLinesForSubscription into a common builder, such as
subscriptionLinesQuery, accepting the invoice-status predicate as its variable
input. Update both callers to use it with StatusEQ and StatusNEQ respectively,
while preserving the existing charge-managed filtering and line-item expansion
behavior.
In `@openmeter/billing/adapter/invoicelinesplitgroup.go`:
- Around line 52-70: Replace the duplicated mapping closure passed to
slicesx.MapWithErr with the existing mapSplitLineHierarchyFromDB helper.
Preserve the current error propagation and “mapping split line groups” wrapping
while routing each dbGroup through that helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a6bb210e-4754-4368-a952-3d8621a9109f
📒 Files selected for processing (13)
openmeter/billing/adapter.goopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/invoicelinesplitgroup.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/invoicelinesplitgroup.goopenmeter/billing/service.goopenmeter/billing/service/gatheringinvoiceline.goopenmeter/billing/service/invoicelinesplitgroup.goopenmeter/billing/service/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/item.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/loader.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.goopenmeter/server/server_test.go
💤 Files with no reviewable changes (3)
- openmeter/billing/worker/subscriptionsync/service/persistedstate/item.go
- openmeter/billing/invoicelinesplitgroup.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go
Member
Author
|
Regarding CodeRabbit’s suggestion to consolidate the standard and gathering subscription-line predicates: I’m intentionally keeping them separate. These loaders are expected to move into separate modules, so extracting a shared query builder here would reintroduce coupling and create temporary churn. The current predicates are deliberately complementary. |
turip
enabled auto-merge (squash)
August 11, 2026 12:40
borosr
approved these changes
Aug 11, 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.
Summary
LineOrHierarchyownership into subscription sync and assemble typed results thereMotivation
This separates the subscription read paths as a prerequisite for moving split-line-group handling into the legacy billing line engine.
Reconstruction note
This draft reconstructs the original commit from #4745. The existing #4745 branch and PR were intentionally left unchanged.
Validation
make test-nocache: 6,156 passed, 9 skipped on the original commitSummary by CodeRabbit
Greptile Summary
This PR refactors the subscription billing read path by splitting the single combined
GetLinesForSubscriptionmethod (returning theLineOrHierarchyunion type) into three typed methods —GetStandardLinesForSubscription,GetGatheringLinesForSubscription, andGetSplitLineGroupsForSubscription— each expressed through their respective service and adapter interfaces. TheLineOrHierarchyunion type, its constructors, and the now-unusedGetDeletePatchesForLinehelper are removed.HasBillingInvoiceWith) rather than done in-memory, keeping the query results inherently typed.Items and duplicateChildUniqueReferenceIDdetection are moved from the adapter layer to the loader, usinglo.GroupBy+MapValuesErrwith richer error messages than before.normalizePersistedLine,normalizePersistedSplitLineHierarchy) is cleanly separated into two focused functions.Confidence Score: 5/5
LineOrHierarchy,NewItemFromLineOrHierarchy,GetDeletePatchesForLine) is confirmed unused in the rest of the codebase. No logic is altered, only decomposed.Important Files Changed
Sequence Diagram
sequenceDiagram participant Loader participant BillingService participant Adapter participant DB Note over Loader: LoadForSubscription() Loader->>BillingService: GetStandardLinesForSubscription(input) BillingService->>Adapter: GetStandardLinesForSubscription(input) Adapter->>DB: "BillingInvoiceLine WHERE status != Gathering" DB-->>Adapter: []StandardLine rows Adapter-->>BillingService: StandardLines BillingService-->>Loader: StandardLines Loader->>BillingService: GetGatheringLinesForSubscription(input) BillingService->>Adapter: GetGatheringLinesForSubscription(input) Adapter->>DB: "BillingInvoiceLine WHERE status = Gathering" DB-->>Adapter: []GatheringLine rows Adapter-->>BillingService: GatheringLines BillingService-->>Loader: GatheringLines Loader->>BillingService: GetSplitLineGroupsForSubscription(input) BillingService->>Adapter: GetSplitLineGroupsForSubscription(input) Adapter->>DB: BillingInvoiceSplitLineGroup WITH lines+invoices DB-->>Adapter: []SplitLineHierarchy rows Adapter-->>BillingService: []SplitLineHierarchy BillingService-->>Loader: []SplitLineHierarchy Note over Loader: Assemble typed Items Note over Loader: Normalize timestamps (per type) Note over Loader: Duplicate uniqueID detection (lo.GroupBy) Note over Loader: Load invoices for itemsReviews (3): Last reviewed commit: "refactor: reuse split line hierarchy map..." | Re-trigger Greptile