refactor: simplify line service #3807
Merged
+138
−366
Conversation
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
Contributor
📝 WalkthroughWalkthroughRefactors lineservice from stateful Service methods to package-level functions, removes context-based validation and meters.go feature-usage logic, updates invoice/calculator dependencies to drop LineService, and adds cross-line currency consistency validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BillingSvc as Billing Service
participant LinePkg as lineservice (package)
participant Calculator
participant DB
Client->>BillingSvc: request (recalculate/simulate/create invoice)
BillingSvc->>BillingSvc: resolve FeatureMeters
BillingSvc->>LinePkg: FromEntities(inScopeLines, featureMeters)
LinePkg-->>BillingSvc: Lines (no ctx)
BillingSvc->>LinePkg: ResolveBillablePeriod(lines, input)
LinePkg-->>BillingSvc: LinesWithBillablePeriod
BillingSvc->>Calculator: Calculate(featureMeters, linesWithPeriods)
Calculator-->>BillingSvc: Calculated invoice/lines
BillingSvc->>DB: persist invoice and lines
DB-->>BillingSvc: ack
BillingSvc-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
turip
force-pushed
the
refactor/simplify-lineservice
branch
from
f3a9c0c to
78d5c7a
Compare
turip
force-pushed
the
refactor/simplify-lineservice
branch
3 times, most recently
from
5113e3e to
5706397
Compare
turip
force-pushed
the
refactor/simplify-lineservice
branch
from
5706397 to
af00cc9
Compare
Member
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
openmeter/billing/service/gatheringinvoicependinglines.go (3)
200-210: Duplicate nil check forFeatureMeters.There are two nil checks for
FeatureMetersin the sameValidate()method - one at lines 200-202 and another at lines 208-210. The error messages are also inconsistent ("cannot be nil" vs "are required").🧹 Proposed fix to remove duplicate
if in.FeatureMeters == nil { return fmt.Errorf("feature meters cannot be nil") } if len(in.InScopeLines) == 0 { return fmt.Errorf("in scope lines must contain at least one line") } - if in.FeatureMeters == nil { - return fmt.Errorf("feature meters are required") - } - return nil
378-388: Another duplicate nil check forFeatureMeters.Same issue as in
handleInvoicePendingLinesForCurrencyInput.Validate()- duplicate checks with inconsistent messages.🧹 Proposed fix to remove duplicate
if i.FeatureMeters == nil { errs = append(errs, fmt.Errorf("feature meters cannot be nil")) } if i.GatheringInvoice.Lines.IsAbsent() { errs = append(errs, fmt.Errorf("gathering invoice must have lines expanded")) } - if i.FeatureMeters == nil { - errs = append(errs, fmt.Errorf("feature meters are required")) - } - for _, line := range i.InScopeLines {
633-649: Third duplicate nil check forFeatureMeters.Same pattern repeating in
createStandardInvoiceFromGatheringLinesInput.Validate().🧹 Proposed fix to remove duplicate
if in.FeatureMeters == nil { errs = append(errs, fmt.Errorf("feature meters cannot be nil")) } if len(in.Lines) == 0 { errs = append(errs, fmt.Errorf("lines must contain at least one line")) } for _, line := range in.Lines { if err := line.Validate(); err != nil { errs = append(errs, fmt.Errorf("line[%s]: %w", line.ID, err)) } } - if in.FeatureMeters == nil { - errs = append(errs, fmt.Errorf("feature meters are required")) - } - return errors.Join(errs...)openmeter/billing/service/invoice.go (1)
153-164: Duplicate nil check forcustomerProfile.Customer.There are two identical nil checks at lines 153-155 and 162-164, both returning the same error message.
🧹 Proposed fix to remove duplicate
if customerProfile.Customer == nil { return invoice, fmt.Errorf("customer profile is nil") } featureMeters, err := s.resolveFeatureMeters(ctx, invoice.Lines.OrEmpty()) if err != nil { return invoice, fmt.Errorf("resolving feature meters: %w", err) } - if customerProfile.Customer == nil { - return invoice, fmt.Errorf("customer profile is nil") - } - inScopeLines := lo.Filter(invoice.Lines.OrEmpty(), func(line *billing.StandardLine, _ int) bool {
🤖 Fix all issues with AI agents
In `@openmeter/billing/stdinvoiceline.go`:
- Around line 318-342: The code assumes i.UsageBased is non-nil but it’s a
pointer; add a nil-check before calling i.UsageBased.Validate() and before
accessing i.UsageBased.Price.Type(): if i.UsageBased == nil append a validation
error (e.g., a ValidationError indicating a missing UsageBased) and skip the
usage-based-specific checks, otherwise call i.UsageBased.Validate(), perform the
Price.Type() branching and period/invoice checks, and only call
RateCardDiscounts.ValidateForPrice with i.UsageBased.Price when UsageBased is
non-nil; update references around Validate, Price.Type(), and
RateCardDiscounts.ValidateForPrice accordingly.
🧹 Nitpick comments (2)
openmeter/billing/service/lineservice/service.go (1)
59-60: Minor typo in comment.Small nit: "syncorinzed" → "synchronized".
📝 Suggested fix
- // The usageBasedLine will never be syncorinzed directly to stripe or other apps, only the detailed lines. + // The usageBasedLine will never be synchronized directly to stripe or other apps, only the detailed lines.openmeter/billing/stdinvoice.go (1)
294-300: Currency validation is a nice addition, but consider consistent error wrapping.The cross-line currency consistency check is great for catching mismatches early. However, the error format differs from other validations in this method. Other errors use
ValidationWithFieldPrefix, but this one uses a directfmt.Errorf.For consistency, consider:
♻️ Suggested refactor for consistent error formatting
if i.Lines.IsPresent() { for _, line := range i.Lines.OrEmpty() { if line.Currency != i.Currency { - outErr = errors.Join(outErr, fmt.Errorf("line[%s]: currency[%s] is not equal to invoice currency[%s]", line.ID, line.Currency, i.Currency)) + outErr = errors.Join(outErr, ValidationWithFieldPrefix(fmt.Sprintf("line[%s]", line.ID), + fmt.Errorf("currency[%s] is not equal to invoice currency[%s]", line.Currency, i.Currency))) } } }
tothandras
approved these changes
Jan 29, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Validations are moved to the standard line itself, as they are not strictly part of the calculation flow.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.