refactor(notification): persist invoice api response#3985
Merged
gergely-kurucz-konghq merged 3 commits intomainfrom Mar 23, 2026
Merged
refactor(notification): persist invoice api response#3985gergely-kurucz-konghq merged 3 commits intomainfrom
gergely-kurucz-konghq merged 3 commits intomainfrom
Conversation
gergely-kurucz-konghq
temporarily deployed
to
dev
GitHub Actions
Inactive
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds versioned notification event payloads and a version-aware JSON parser that decodes legacy v0 billing invoice shapes by mapping them to the API invoice model, preserves current v1 payloads, updates consumers/constructors to use API-mapped invoices, and adds unit tests for multiple shapes and error cases. Changes
Sequence DiagramsequenceDiagram
participant Consumer as Event Consumer
participant Entity as Entity Mapping
participant Parser as eventPayloadFromJSON
participant Legacy as Legacy Mapper (billinghttp.MapEventInvoiceToAPI)
participant Current as Current Parser (v1)
participant Validator as Validator
Consumer->>Entity: EventFromDBEntity(raw_json)
Entity->>Parser: eventPayloadFromJSON(raw_json)
Parser->>Parser: unmarshal meta (type, version)
alt type == invoice and version == 0
Parser->>Legacy: unmarshal v0 envelope -> billing.EventStandardInvoice
Legacy->>Legacy: MapEventInvoiceToAPI -> api.Invoice
Legacy-->>Parser: EventPayload{Invoice: InvoicePayload(api.Invoice), Version:1}
else type == invoice and version == 1
Parser->>Current: unmarshal directly into EventPayload (InvoicePayload)
Current-->>Parser: EventPayload{Invoice: InvoicePayload(...), Version:1}
else non-invoice
Parser->>Parser: single-pass unmarshal to EventPayload
end
Parser-->>Entity: EventPayload
Entity->>Validator: payload.Validate()
Validator-->>Entity: ok / error
Entity-->>Consumer: Event (or error)
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 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 |
gergely-kurucz-konghq
added
release-note/ignore
gergely-kurucz-konghq
changed the title
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/notification/adapter/entitymapping.go`:
- Around line 132-158: The legacy v0 branch populates only payload.Invoice and
thus drops the metadata (Type/Version); after mapping the legacy invoice to
apiInvoice, copy the incoming meta into the returned payload (e.g., set
payload.EventPayloadMeta = meta or assign payload.Type = meta.Type and
payload.Version = meta.Version) so the resulting notification.EventPayload
preserves Type and Version alongside payload.Invoice.
In `@openmeter/notification/invoice.go`:
- Around line 26-27: InvoicePayload.Validate currently returns nil, removing
critical validation that billing.EventStandardInvoice.Validate used to perform;
restore validation by implementing InvoicePayload.Validate to validate the
invoice shape and associated app payloads (reusing or delegating to the previous
billing.EventStandardInvoice.Validate logic where possible), ensure it checks
required invoice fields and app payload integrity, and return any validation
error instead of nil so invoice events routed via eventpayload.go are properly
validated before persistence/readback.
🪄 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: a90f7466-350d-456a-8650-f38bcd94a404
📒 Files selected for processing (8)
openmeter/notification/adapter/entitymapping.goopenmeter/notification/adapter/entitymapping_test.goopenmeter/notification/consumer/invoice.goopenmeter/notification/eventpayload.goopenmeter/notification/httpdriver/mapping.goopenmeter/notification/httpdriver/mapping_test.goopenmeter/notification/internal/rule.goopenmeter/notification/invoice.go
gergely-kurucz-konghq
temporarily deployed
to
dev
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/notification/adapter/entitymapping_test.go (3)
99-103: Consider assertingTypeandVersionare preserved too.The test verifies the Invoice content round-trips correctly, but doesn't check that
payload.Typeandpayload.Versionmatchoriginal. Adding those assertions would catch regressions in meta preservation.💡 Suggested addition
payload, err := eventPayloadFromJSON(data) require.NoError(t, err) + assert.Equal(t, original.Type, payload.Type) + assert.Equal(t, original.Version, payload.Version) require.NotNil(t, payload.Invoice) assert.Equal(t, original.Invoice.Invoice, payload.Invoice.Invoice)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/notification/adapter/entitymapping_test.go` around lines 99 - 103, The test currently checks invoice round-tripping but misses metadata assertions; update the test that calls eventPayloadFromJSON (where variables original and payload are used) to also assert that payload.Type == original.Type and payload.Version == original.Version (e.g., add require.Equal/ assert.Equal checks for Type and Version after deserialization) so meta fields are preserved.
106-119: Good error case coverage!These tests nail down the expected error messages for edge cases. Quick thought: you could also add a test for completely malformed JSON (e.g.,
[]or{invalid}) to verify the first-pass meta unmarshal fails gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/notification/adapter/entitymapping_test.go` around lines 106 - 119, Add a new test that checks eventPayloadFromJSON returns an error for completely malformed JSON (e.g., `[]` and `{invalid}`) to validate the initial meta unmarshal failure path; create a test function alongside TestEventPayloadFromJSON_V0MissingInvoice and TestEventPayloadFromJSON_UnknownVersion (e.g., TestEventPayloadFromJSON_MalformedJSON) that calls eventPayloadFromJSON with malformed payloads and asserts an error is returned and contains an appropriate message indicating JSON/unmarshal failure.
121-137: Non-invoice path coverage looks good.One tiny addition that'd be nice: asserting
payload.Type == notification.EventTypeBalanceThresholdto verify the type doesn't get lost in the single-pass unmarshal path.💡 Suggested addition
payload, err := eventPayloadFromJSON(data) require.NoError(t, err) + assert.Equal(t, notification.EventTypeBalanceThreshold, payload.Type) assert.NotNil(t, payload.BalanceThreshold) assert.Nil(t, payload.Invoice)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/notification/adapter/entitymapping_test.go` around lines 121 - 137, Add an assertion in TestEventPayloadFromJSON_NonInvoiceType to verify the unmarshalled payload preserves the event type: after calling eventPayloadFromJSON and before other assertions, assert that payload.Type == notification.EventTypeBalanceThreshold (use assert.Equal or require.Equal consistent with the file) so the single-pass unmarshal path doesn't drop the Type field.
🤖 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/notification/adapter/entitymapping_test.go`:
- Around line 99-103: The test currently checks invoice round-tripping but
misses metadata assertions; update the test that calls eventPayloadFromJSON
(where variables original and payload are used) to also assert that payload.Type
== original.Type and payload.Version == original.Version (e.g., add
require.Equal/ assert.Equal checks for Type and Version after deserialization)
so meta fields are preserved.
- Around line 106-119: Add a new test that checks eventPayloadFromJSON returns
an error for completely malformed JSON (e.g., `[]` and `{invalid}`) to validate
the initial meta unmarshal failure path; create a test function alongside
TestEventPayloadFromJSON_V0MissingInvoice and
TestEventPayloadFromJSON_UnknownVersion (e.g.,
TestEventPayloadFromJSON_MalformedJSON) that calls eventPayloadFromJSON with
malformed payloads and asserts an error is returned and contains an appropriate
message indicating JSON/unmarshal failure.
- Around line 121-137: Add an assertion in
TestEventPayloadFromJSON_NonInvoiceType to verify the unmarshalled payload
preserves the event type: after calling eventPayloadFromJSON and before other
assertions, assert that payload.Type == notification.EventTypeBalanceThreshold
(use assert.Equal or require.Equal consistent with the file) so the single-pass
unmarshal path doesn't drop the Type field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b1980bd-cd47-49e1-b158-782583287650
📒 Files selected for processing (2)
openmeter/notification/adapter/entitymapping.goopenmeter/notification/adapter/entitymapping_test.go
gergely-kurucz-konghq
force-pushed
the
refactor/OM-203-persist-invoice-api-response
branch
from
eb1491e to
7ba6cdb
Compare
gergely-kurucz-konghq
temporarily deployed
to
dev
GitHub Actions
Inactive
gergely-kurucz-konghq
deleted the
refactor/OM-203-persist-invoice-api-response
branch
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.
What
Decouple invoice notification events from the internal
billing.EventStandardInvoicetype by persisting new events asapi.Invoiceinstead.Why
The internal billing type was leaking into the notification persistence layer, requiring lazy transformation at every read site (Svix dispatch, HTTP API). This is the first step in a broader invoice type refactoring — decoupling the notification layer first limits the blast radius of that upcoming change.
How
Version inttoEventPayloadMeta(omitempty: absent = legacy v0,1= new v1)InvoicePayloadnow embedsapi.Invoicedirectly;EventPayload.Invoiceis*InvoicePayloadbilling.EventStandardInvoice → api.Invoiceeagerly in the consumer before persisting, mirroring how entitlement events already workeventPayloadFromJSON: v0 events are transformed on read (no data loss), v1 events pass through directlyFromEventAsInvoice*PayloadinhttpdriverandeventAsPayloadin the reconciler become trivial pass-throughs — no functional change to Svix or API outputDB contains a mix of v0 and v1 events during the rollout window; both are handled transparently.
Step 2 (migration of legacy v0 events) and Step 3 (removal of the v0 read path) follow separately.
Summary by CodeRabbit
New Features
Bug Fixes
Tests