refactor: simplify discount handling #2599
Conversation
📝 WalkthroughWalkthroughThis update streamlines discount handling throughout the codebase. Intermediary helper methods and wrapper types for discount slices (such as Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
openmeter/billing/adapter/invoicelinediff.go (4)
354-359:⚠️ Potential issueEnsure nil safety in discount deletion.
Similar to the previous issue, direct iteration over
line.Discountswithout an.OrEmpty()safety check could cause a nil pointer dereference.Add a nil check to prevent potential panics:
-for _, discount := range line.Discounts { +if line.Discounts != nil { + for _, discount := range line.Discounts { out.Discounts.NeedsDelete(discountWithLine{ Discount: discount, Line: line, }) + } }
371-378:⚠️ Potential issueEnsure nil safety when filtering discounts.
The code directly filters
line.Discountswithout checking if it's nil. Ifline.Discountscan be nil, this would cause a panic.Modify the code to handle nil values:
currentDiscountIDs := lo.GroupBy( lo.Filter( - line.Discounts, + lo.EmptyIfNil(line.Discounts), func(d billing.LineDiscount, _ int) bool { return d.ID != "" }, ), func(d billing.LineDiscount) string { return d.ID }, )
385-395:⚠️ Potential issueEnsure nil safety when iterating DB discounts.
Direct iteration over DB discounts without a nil check could cause a panic.
Add a nil check:
-for _, dbDiscount := range line.DBState.Discounts { +for _, dbDiscount := range lo.EmptyIfNil(line.DBState.Discounts) { if _, ok := currentDiscountIDs[dbDiscount.ID]; !ok { // We need to delete this discount out.Discounts.NeedsDelete(discountWithLine{ Discount: dbDiscount, Line: line, }) out.AffectedLineIDs.Add(lineParentIDs(line, lineIDIncludeSelf)...) } }
397-428:⚠️ Potential issueAdd nil check for current discounts iteration.
Direct iteration over
line.Discountswithout a nil check could cause a panic if discounts are nil.Add a nil check:
-for _, currentDiscount := range line.Discounts { +if line.Discounts != nil { + for _, currentDiscount := range line.Discounts { if currentDiscount.ID == "" { // We need to create this discount out.Discounts.NeedsCreate(discountWithLine{ Discount: currentDiscount, Line: line, }) out.AffectedLineIDs.Add(lineParentIDs(line, lineIDIncludeSelf)...) continue } dbDiscount, ok := dbDiscountIDs[currentDiscount.ID] if !ok { return fmt.Errorf("discount[%s]: not found in DB", currentDiscount.ID) } dbItem := dbDiscount[0] currentDiscount.ID = dbItem.ID currentDiscount.CreatedAt = dbItem.CreatedAt currentDiscount.UpdatedAt = dbItem.UpdatedAt if !dbItem.Equal(currentDiscount) { out.Discounts.NeedsUpdate(discountWithLine{ Discount: currentDiscount, Line: line, }) out.AffectedLineIDs.Add(lineParentIDs(line, lineIDIncludeSelf)...) } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
openmeter/billing/adapter/invoicelinediff.go(4 hunks)openmeter/billing/adapter/invoicelinediff_test.go(5 hunks)openmeter/billing/adapter/invoicelinemapper.go(1 hunks)openmeter/billing/app.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(1 hunks)openmeter/billing/invoiceline.go(4 hunks)openmeter/billing/service/lineservice/feeline.go(1 hunks)openmeter/billing/service/lineservice/usagebasedline.go(1 hunks)test/app/stripe/invoice_test.go(1 hunks)test/billing/adapter_test.go(7 hunks)test/billing/invoice_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
openmeter/billing/adapter/invoicelinemapper.go (1)
openmeter/productcatalog/discount.go (1)
Discounts(330-330)
openmeter/billing/service/lineservice/feeline.go (1)
openmeter/billing/invoiceline.go (1)
LineDiscount(733-743)
openmeter/billing/adapter/invoicelinediff_test.go (1)
openmeter/billing/invoiceline.go (1)
LineDiscount(733-743)
openmeter/billing/httpdriver/invoiceline.go (4)
openmeter/billing/invoiceline.go (1)
LineDiscounts(750-750)api/api.gen.go (1)
InvoiceLineDiscount(3282-3284)api/client/go/client.gen.go (1)
InvoiceLineDiscount(3022-3024)api/client/javascript/src/client/schemas.ts (1)
InvoiceLineDiscount(9051-9051)
openmeter/billing/adapter/invoicelinediff.go (1)
openmeter/billing/invoiceline.go (2)
Line(274-287)LineDiscount(733-743)
test/billing/adapter_test.go (1)
openmeter/billing/invoiceline.go (2)
NewLineChildren(557-564)LineDiscount(733-743)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
openmeter/billing/adapter/invoicelinemapper.go (1)
225-225: LGTM: Direct assignment simplifies discount handlingThe change aligns with the PR objective by directly assigning the discount slice instead of wrapping it with
billing.NewLineDiscounts.openmeter/billing/service/lineservice/feeline.go (1)
51-51:Details
❓ Verification inconclusive
Potential nil slice issue after removing .OrEmpty()
The removal of
.OrEmpty()could cause a nil panic ifl.line.Discountsis nil. The.OrEmpty()method likely provided a safety mechanism that returned an empty slice when the discounts were nil.Verify that
l.line.Discountsis always initialized before reaching this point, or add a nil check:
🏁 Script executed:
#!/bin/bash # Check if there are any places where Discounts are accessed without initialization rg -p "line\.Discounts\s+=" openmeter/billing echo "Looking for potential initialization patterns..." rg -p "Discounts\s*:" openmeter/billingLength of output: 1943
Action Required: Verify nil-safety for Discounts in feeline.go
The removal of
.OrEmpty()raises a potential risk: ifl.line.Discountsis ever nil, passing it directly tolo.Mapmight be unsafe (even though nil slices are often safely handled by range loops in Go, confirm thatlo.Mapdoes not assume the slice is non-nil). Our grep results show that, in several parts of the code (especially test files), discounts are explicitly initialized (e.g. using literal empty slices), but there’s no conclusive evidence that production code guarantees a non-nil slice onl.line.Discounts.Recommendations:
- Review Initialization: Make sure that every code path setting up
l.line.Discounts(or populating its parent struct) ensures it is non-nil when reaching feeline.go at line 51.- Add Defensive Check: If there’s any uncertainty, consider either reapplying a nil-coalescing mechanism (such as reintroducing
.OrEmpty()) or performing an explicit nil check before callinglo.Map.openmeter/billing/service/lineservice/usagebasedline.go (1)
1030-1030: LGTM: Direct assignment of discountsThis change simplifies the code by directly assigning
in.Discountsto theDiscountsfield, removing unnecessary abstraction.openmeter/billing/httpdriver/invoiceline.go (1)
427-439: LGTM: Simplified discount handling.The function has been streamlined by removing the unnecessary
IsAbsent()check andOrEmpty()call, which aligns with the PR objective to simplify discount handling. The parameter name change fromoptDiscountstodiscountsalso better reflects that discounts are always provided as a valid slice.test/billing/invoice_test.go (1)
3212-3212: Simplification of discount access: direct property access replaces MustGet()This change aligns with the PR objective of simplifying discount handling by directly accessing the
Discountsslice rather than usingmo.Option. The modification maintains the same functionality while reducing unnecessary wrapper usage.test/app/stripe/invoice_test.go (1)
561-565: Simplified discount iteration logic.The code now directly iterates over
line.Discountsslice rather than using a wrapper method like.OrEmpty(), which aligns with the PR's objective to simplify discount handling.openmeter/billing/adapter/invoicelinediff_test.go (5)
49-54: Direct discount array initialization.The code now directly initializes
Discountsas a slice ofbilling.LineDiscountinstead of using a wrapper constructor likebilling.NewLineDiscounts(), which aligns with the PR's goal of simplifying discount handling.
181-181: Direct access to discount properties.The code now directly modifies the discount description through array indexing rather than using wrapper methods.
210-210: Direct nil assignment for discounts.Now directly setting
Discounts = nilinstead of using a wrapper method to indicate the absence of discounts.
229-229: Direct discount property modification.Changed from potentially using a wrapper method to directly modifying the discount amount through array indexing.
248-249: Simplified discount access pattern.Directly accessing the discounts slice and modifying its properties is more straightforward than using wrapper methods.
test/billing/adapter_test.go (9)
213-213: Direct nil assignment for discounts.Setting
Discounts = nildirectly instead of using a wrapper constructor with an empty array.
219-219: Consistent discount handling.Consistent with the previous change, directly setting
Discounts = nil.
270-270: Simplified discount initialization.Direct nil assignment for discounts maintains consistency with the approach used throughout the codebase.
642-657: Direct discount array initialization.Initializing discounts as a slice of
billing.LineDiscountinstead of using a wrapper constructor likebilling.NewLineDiscounts().
669-669: Direct access to discounts.Directly accessing the discounts slice from the line object rather than using a getter method.
674-674: Simplified discount referencing.Direct reference to the line's discounts without intermediate variables or wrapper methods.
678-701: Direct discount array modification.Creating a new discount array directly instead of using wrapper methods, which simplifies the code.
718-718: Consistent discount access pattern.Direct access to the discounts property is consistent with the changes throughout the file.
721-721: Simplified variable reference.Direct reference to the child line's discounts without using wrapper methods.
openmeter/billing/adapter/invoicelinediff.go (2)
347-352:⚠️ Potential issueEnsure nil safety when iterating over discounts.
The code now directly iterates over
line.Discountsinstead of using.OrEmpty(). While this simplifies the code, it introduces a potential nil pointer dereference ifline.Discountscan be nil.Ensure that
line.Discountsis initialized to an empty slice when creating a newLineobject, or add a nil check before the loop:-for _, discount := range line.Discounts { +if line.Discounts != nil { + for _, discount := range line.Discounts { out.Discounts.NeedsCreate(discountWithLine{ Discount: discount, Line: line, }) + } }Likely an incorrect or invalid review comment.
347-428:Details
❓ Verification inconclusive
Verify discount handling logic across codebase.
While simplifying the discount handling by removing
mo.Optionand.OrEmpty()calls is a good refactoring goal, ensure that all parts of the codebase consistently handle nilDiscountsarrays. Consider creating a helper function likesafeDiscountsto handle nil values consistently.Run this script to check how discounts are initialized throughout the codebase:
🏁 Script executed:
#!/bin/bash echo "Checking Line struct initialization patterns..." rg -A 3 "Line\s*{" --type go echo "Checking how Discounts are handled..." rg "\.Discounts\s*=" --type go echo "Checking for nil checks before accessing Discounts..." rg "if.*\.Discounts\s*!=\s*nil" --type goLength of output: 2871
Discount Handling Consistency Check
The updated discount logic in this adapter safely iterates over the Discounts slice without explicit nil checks (which is acceptable in Go). However, our repository search shows that other parts of the codebase (for example, in
openmeter/productcatalog/http/mapping.goandopenmeter/subscription/repo/subscriptionitemrepo.go) are explicitly checking for nil Discounts before use. To ensure consistency across the system—and to avoid any subtle discrepancies between nil and empty slices—it would be beneficial to verify that all modules treat a nil Discounts slice the same way. Consider refactoring common discount operations into a helper (e.g.,safeDiscounts) that defaults nil values to an empty slice.Please review and confirm that this change aligns with the overall discount handling strategy.
openmeter/billing/invoiceline.go (4)
300-302: Simplified discount iteration approachThe code now uses direct iteration over the
i.Discountsslice instead of a helper method, which aligns with the PR objective of simplifying discount handling.
408-409: Improved discount cloning logicThe condition for cloning discounts now directly checks the length of the slice instead of using an option wrapper's presence check, which is more straightforward and efficient.
750-750: Core type simplificationThe fundamental change of converting
LineDiscountsfrom a struct with anmo.Optionwrapper to a direct slice type aligns perfectly with the PR objective to simplify discount handling.
753-783: Updated reuse logic for new typeThe
ChildrenWithIDReusemethod has been correctly updated to work with the new direct slice type instead of the option wrapper, maintaining the same functionality while simplifying the implementation.
turip
force-pushed
the
refactor/simplify-discount-handling
branch
from
4262e83 to
87b3b85
Compare
turip
added
release-note/ignore
turip
force-pushed
the
refactor/simplify-discount-handling
branch
from
87b3b85 to
0acd3b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openmeter/billing/invoiceline.go (2)
297-303: Consider checking for duplicate discount IDs.
The newDiscountsByIDfunction may overwrite entries if multiple discounts share the same ID. If such collisions are unintended, consider adding checks or error handling.Would you like me to open a new issue or create a helper function to detect potential discount ID collisions?
752-783: Potential issue with multiple matches.
When reusing discounts byChildUniqueReferenceID, only the first match fromGroupByis used. If you anticipate multiple existing items with the same reference ID, consider clarifying or adding a safeguard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
openmeter/app/stripe/entity/app/invoice.go(2 hunks)openmeter/billing/adapter/invoicelinediff.go(4 hunks)openmeter/billing/adapter/invoicelinediff_test.go(5 hunks)openmeter/billing/adapter/invoicelinemapper.go(1 hunks)openmeter/billing/app.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(1 hunks)openmeter/billing/invoiceline.go(5 hunks)openmeter/billing/service/lineservice/feeline.go(1 hunks)openmeter/billing/service/lineservice/usagebasedline.go(1 hunks)pkg/slicesx/empty.go(1 hunks)test/app/stripe/invoice_test.go(1 hunks)test/billing/adapter_test.go(7 hunks)test/billing/invoice_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- openmeter/billing/app.go
- openmeter/billing/service/lineservice/usagebasedline.go
- openmeter/billing/adapter/invoicelinemapper.go
- openmeter/billing/service/lineservice/feeline.go
- openmeter/billing/httpdriver/invoiceline.go
- openmeter/billing/adapter/invoicelinediff_test.go
- openmeter/billing/adapter/invoicelinediff.go
- test/app/stripe/invoice_test.go
- test/billing/invoice_test.go
- test/billing/adapter_test.go
🧰 Additional context used
🧬 Code Definitions (1)
openmeter/billing/invoiceline.go (2)
openmeter/productcatalog/discount.go (1)
Discounts(330-330)pkg/slicesx/empty.go (1)
EmptyAsNil(6-11)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
pkg/slicesx/empty.go (1)
1-11: Well-designed utility function to support simplified slice handling.The new
EmptyAsNilgeneric function is a clean, focused utility that normalizes the representation of empty slices. This supports the PR's goal of simplifying discount handling by providing a consistent way to treat empty slices as nil values, which is particularly useful in testing contexts where struct equality comparisons are performed.The implementation is correct and the documentation clearly explains the purpose and use case. The function signature appropriately uses Go's generics feature to work with slices of any type.
openmeter/billing/invoiceline.go (3)
17-17: No issues with the new import statement.
Theslicesxpackage seems to be used correctly for empty-slice handling.
409-410: LGTM: Conditional discount cloning is clear.
This neatly avoids copying discounts unless required.
750-750: Direct slice definition is understandable.
Switching to a simple slice type is straightforward and maintains clarity.openmeter/app/stripe/entity/app/invoice.go (2)
187-189: New loop aligns with simplified discount handling.
Usingline.DiscountsByID()is consistent with the updated approach.
297-297: Consistent usage ofDiscountsByID.
Fetching discounts via the new helper maintains uniformity and readability.
2 participants
Overview
Discounts are always expanded so let's remove the unnecessary mo.Option usage here.
This will simplify how the usage discount is to be implemented.
Summary by CodeRabbit