fix: invoiceline splitgroup is not marshalable#3875
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds JSON marshal/unmarshal for LineWithInvoiceHeader via a generic serde that discriminates between Standard and Gathering invoice/line variants, with explicit (de)serialization logic and error handling; includes a unit test that verifies JSON round-trip fidelity for SplitLineHierarchy. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/invoicelinesplitgroup.go (1)
241-282: Consider adding nil guards forl.Invoiceandl.Linebefore dereferencing.If
MarshalJSONis ever called on a zero-value or partially-initializedLineWithInvoiceHeader, lines 242 and 253/269 will panic with a nil interface method call. A quick guard at the top would make this more resilient.Also a small style nit:
UnmarshalJSONuses adefault:case in its switch, whileMarshalJSONfalls through to the error return after the switch. Using the same pattern in both would be a bit more readable.Suggested nil guard + consistent switch
func (l *LineWithInvoiceHeader) MarshalJSON() ([]byte, error) { + if l.Invoice == nil || l.Line == nil { + return nil, fmt.Errorf("cannot marshal LineWithInvoiceHeader: Invoice and Line must not be nil") + } + invoice := l.Invoice.AsInvoice() invoiceType := invoice.Type() switch invoiceType { case InvoiceTypeStandard: // ... unchanged ... case InvoiceTypeGathering: // ... unchanged ... + default: + return nil, fmt.Errorf("unknown invoice type: %s", invoiceType) } - - return nil, fmt.Errorf("unknown invoice type: %s", invoiceType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/invoicelinesplitgroup.go` around lines 241 - 282, Add nil guards at the start of LineWithInvoiceHeader.MarshalJSON to check that l, l.Invoice and l.Line are non-nil and return a clear error if any are nil to avoid panics when calling l.Invoice.AsInvoice() or l.Line.AsInvoiceLine(); then refactor the switch on invoiceType to include a default: case that returns the unknown-invoice-type error (matching UnmarshalJSON style) rather than falling through to a post-switch return. Ensure you reference the existing methods Invoice.AsInvoice, Line.AsInvoiceLine, AsStandardInvoice/AsGatheringInvoice and AsStandardLine/AsGatheringLine when adding the guards and error paths.openmeter/billing/invoicelinesplitgroup_test.go (1)
72-87: Nice round-trip test — covers the core fix well.Two small suggestions:
- Style: You're mixing
t.Fatalf(lines 74, 82) withrequire(line 87). Sincerequireis already imported, using it consistently keeps things tidy:- jsonBytes, err := json.Marshal(testGroup) - if err != nil { - t.Fatalf("failed to marshal test group: %v", err) - } + jsonBytes, err := json.Marshal(testGroup) + require.NoError(t, err, "failed to marshal test group") t.Logf("test group: %s", string(jsonBytes)) - var unmarshalled SplitLineHierarchy - err = json.Unmarshal(jsonBytes, &unmarshalled) - if err != nil { - t.Fatalf("failed to unmarshal test group: %v", err) - } + var unmarshalled SplitLineHierarchy + err = json.Unmarshal(jsonBytes, &unmarshalled) + require.NoError(t, err, "failed to unmarshal test group")
- Coverage: Consider adding a sub-test for unmarshaling with an unknown
"type"value (e.g.,"type":"bogus") to exercise the error path inUnmarshalJSON. A quick negative test goes a long way for confidence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/invoicelinesplitgroup_test.go` around lines 72 - 87, Replace the manual t.Fatalf checks around json.Marshal/json.Unmarshal with require assertions (e.g., require.NoError(t, err) or require.NoErrorf) to be consistent with the existing require usage, and add a small sub-test that attempts to json.Unmarshal a payload into SplitLineHierarchy with an unknown "type" value (e.g., "type":"bogus") and asserts that UnmarshalJSON returns an error (using require.Error) to cover the negative path in UnmarshalJSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/invoicelinesplitgroup_test.go`:
- Around line 72-87: Replace the manual t.Fatalf checks around
json.Marshal/json.Unmarshal with require assertions (e.g., require.NoError(t,
err) or require.NoErrorf) to be consistent with the existing require usage, and
add a small sub-test that attempts to json.Unmarshal a payload into
SplitLineHierarchy with an unknown "type" value (e.g., "type":"bogus") and
asserts that UnmarshalJSON returns an error (using require.Error) to cover the
negative path in UnmarshalJSON.
In `@openmeter/billing/invoicelinesplitgroup.go`:
- Around line 241-282: Add nil guards at the start of
LineWithInvoiceHeader.MarshalJSON to check that l, l.Invoice and l.Line are
non-nil and return a clear error if any are nil to avoid panics when calling
l.Invoice.AsInvoice() or l.Line.AsInvoiceLine(); then refactor the switch on
invoiceType to include a default: case that returns the unknown-invoice-type
error (matching UnmarshalJSON style) rather than falling through to a
post-switch return. Ensure you reference the existing methods Invoice.AsInvoice,
Line.AsInvoiceLine, AsStandardInvoice/AsGatheringInvoice and
AsStandardLine/AsGatheringLine when adding the guards and error paths.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/invoicelinesplitgroup.go (2)
241-282: Optional: nil guards +defaultcase tidinessTwo small things worth considering:
- Nil check: if someone accidentally marshals a zero-value
LineWithInvoiceHeader{},l.Invoice.AsInvoice()(and similarlyl.Line.AsInvoiceLine()) will panic. A quick guard at the top keeps things tidy:🛡️ Suggested nil guard
func (l LineWithInvoiceHeader) MarshalJSON() ([]byte, error) { + if l.Invoice == nil || l.Line == nil { + return nil, fmt.Errorf("LineWithInvoiceHeader has nil Invoice or Line") + } invoice := l.Invoice.AsInvoice()
default:case: the switch inMarshalJSONfalls through to the error after the closing brace, which is correct — but having an explicitdefault:mirrors the style inUnmarshalJSONand makes exhaustiveness more obvious:♻️ Suggested default case
case InvoiceTypeGathering: ... return json.Marshal(...) + default: + return nil, fmt.Errorf("unknown invoice type: %s", invoiceType) } - - return nil, fmt.Errorf("unknown invoice type: %s", invoiceType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/invoicelinesplitgroup.go` around lines 241 - 282, Add a nil-guard at the start of LineWithInvoiceHeader.MarshalJSON to avoid panics when called on a zero-value (check that l.Invoice and l.Line are non-nil/valid before calling l.Invoice.AsInvoice() and l.Line.AsInvoiceLine() and return a clear error if missing), and make the switch on invoiceType explicitly exhaustive by adding a default: branch that returns the same fmt.Errorf("unknown invoice type: %s", invoiceType) (this mirrors UnmarshalJSON style and prevents fall-through ambiguity); update MarshalJSON accordingly around the existing calls to AsInvoice, AsStandardInvoice/AsGatheringInvoice and AsInvoiceLine/AsStandardLine/AsGatheringLine.
206-239: Quick heads-up:Invoicehas different dynamic types depending on construction pathJust noticed that
NewLineWithInvoiceHeaderandUnmarshalJSONstore the invoice differently:
NewLineWithInvoiceHeaderstores the value directly (e.g.,StandardInvoice)UnmarshalJSONstores a pointer (e.g.,*StandardInvoice)Both satisfy the
GenericInvoiceReaderinterface, so it works fine, and I didn't find any type assertions that would break. But if someone adds a type assertion down the road, it could silently fail after a JSON round-trip. Worth keeping an eye on if you're refactoring this area.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/invoicelinesplitgroup.go` around lines 206 - 239, UnmarshalJSON currently sets l.Invoice to a pointer (e.g., &unmarshalled.Invoice) while NewLineWithInvoiceHeader stores invoice values directly, causing inconsistent dynamic types; make UnmarshalJSON assign the invoice value the same way as NewLineWithInvoiceHeader by setting l.Invoice = unmarshalled.Invoice (for both StandardInvoice and GatheringInvoice branches) so l.Invoice consistently holds the same concrete type shape; update both branches in UnmarshalJSON (references: UnmarshalJSON, lineWithInvoiceHeaderSerde, StandardInvoice, GatheringInvoice, l.Invoice) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/invoicelinesplitgroup.go`:
- Around line 241-282: Add a nil-guard at the start of
LineWithInvoiceHeader.MarshalJSON to avoid panics when called on a zero-value
(check that l.Invoice and l.Line are non-nil/valid before calling
l.Invoice.AsInvoice() and l.Line.AsInvoiceLine() and return a clear error if
missing), and make the switch on invoiceType explicitly exhaustive by adding a
default: branch that returns the same fmt.Errorf("unknown invoice type: %s",
invoiceType) (this mirrors UnmarshalJSON style and prevents fall-through
ambiguity); update MarshalJSON accordingly around the existing calls to
AsInvoice, AsStandardInvoice/AsGatheringInvoice and
AsInvoiceLine/AsStandardLine/AsGatheringLine.
- Around line 206-239: UnmarshalJSON currently sets l.Invoice to a pointer
(e.g., &unmarshalled.Invoice) while NewLineWithInvoiceHeader stores invoice
values directly, causing inconsistent dynamic types; make UnmarshalJSON assign
the invoice value the same way as NewLineWithInvoiceHeader by setting l.Invoice
= unmarshalled.Invoice (for both StandardInvoice and GatheringInvoice branches)
so l.Invoice consistently holds the same concrete type shape; update both
branches in UnmarshalJSON (references: UnmarshalJSON,
lineWithInvoiceHeaderSerde, StandardInvoice, GatheringInvoice, l.Invoice)
accordingly.
Contributor
|
approved ✅ |
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.
Overview
Fixes event marshaling error:
json: cannot unmarshal object into Go struct field StandardInvoice.old.invoice.lines.progressiveLineHierarchy.Lines.Line of type billing.GenericInvoiceLine
Notes for reviewer
Summary by CodeRabbit
Refactor
Tests