fix: allow marshaling event invoice into api types #2749
Conversation
📝 WalkthroughWalkthroughThis change introduces a new event data abstraction for app entities, extending the Changes
Possibly related PRs
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (13)
✨ Finishing Touches
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. 🪧 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 (
|
turip
force-pushed
the
fix/allow-marshaling-event-invoice-into-api-types
branch
from
dd61bc0 to
9fb7916
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
openmeter/app/httpdriver/mapper.go (1)
16-35:⚠️ Potential issueType assertion may panic when the concrete app is a pointer
MapAppToAPIasserts the incomingapp.Appto value types.
If any caller happens to store a*appsandbox.App(pointer) in the interface the assertion will fail at runtime.A safer pattern is a type-switch that handles both forms:
switch item.GetType() { case app.AppTypeStripe: - stripeApp := item.(appstripeentityapp.App) + switch v := item.(type) { + case appstripeentityapp.App: + stripeApp = v + case *appstripeentityapp.App: + stripeApp = *v + default: + return api.App{}, fmt.Errorf("unexpected concrete type for Stripe app: %T", item) + }Apply the same approach to the Sandbox and Custom-Invoicing branches.
openmeter/app/event.go (1)
192-200:⚠️ Potential issueNil-pointer dereference when
DeletedAtis unset
AppDeleteEvent.EventMetadata()unconditionally dereferencese.AppBase.DeletedAt, which will panic if the field isnil(e.g. if someone manually crafts or migrates an event without setting the timestamp).- Time: *e.AppBase.DeletedAt, + Time: func() time.Time { + if e.AppBase.DeletedAt != nil { + return *e.AppBase.DeletedAt + } + // Fallback – choose whichever strategy fits your domain better. + // Returning `time.Now()` keeps the event valid; returning `time.Time{}` surfaces the error to consumers. + return time.Now() + }(),At minimum, guard the dereference or extend
Validate()to assertDeletedAt != nilbefore this method is called.
🧹 Nitpick comments (7)
openmeter/app/custominvoicing/factory.go (1)
120-120: Variable renamed for clarity.Changed variable name from
appBasetonewAppto better reflect that it's a complete app instance rather than just the base fields.openmeter/billing/service/invoice.go (1)
473-481: Improved error handling for invoice event creationThe code now properly separates event creation from publishing with explicit error handling. This is a good practice that ensures events are only published if they can be successfully created.
Consider adding a debug log statement before publishing the event to help with troubleshooting. This would provide visibility into the event creation process.
openmeter/billing/service/invoicestate.go (1)
249-249: Publisher properly initializedThe
Publisherfield is correctly assigned from the service's publisher in theWithInvoiceStateMachinemethod.Consider also clearing this field in the defer block at line 254 to avoid potential resource leaks:
defer func() { sm.Invoice = billing.Invoice{} sm.Calculator = nil + sm.Publisher = nil invoiceStateMachineCache.Put(sm) }()test/billing/suite.go (2)
292-296:Namespacefield is never used — remove or honour it
CreateDraftInvoiceignoresin.Namespaceand always derives the namespace fromin.Customer.Namespace.
Either drop the redundant field or decide which one is the authoritative source and validate that they match to avoid silent inconsistencies.
313-316: Use the project clock helper for determinism
time.Now()makes the helper non-deterministic and harder to debug.
Elsewhere the codebase relies onclock.Now(); re-using it keeps tests consistent and simplifies time-based assertions.openmeter/app/sandbox/app.go (1)
154-156: Returnnilinstead of an empty map to minimise event payload size
GetEventAppDatacurrently returnsapp.EventAppData{}which serialises to{}and will always be emitted, even though Sandbox has no app-specific data.
Returningnilallows the field to be omitted (provided the struct tag isomitemptyin the parent), trimming event size.-func (a App) GetEventAppData() (app.EventAppData, error) { - return app.EventAppData{}, nil +func (a App) GetEventAppData() (app.EventAppData, error) { + return nil, nil }openmeter/app/event.go (1)
24-36: Avoid double JSON round-trips when input is already a map
NewEventAppDatamarshals, then immediately unmarshals, even ifvis already amap[string]anyorjson.RawMessage. That costs allocations and can coerce numbers tofloat64.Consider a fast-path:
func NewEventAppData(v any) (EventAppData, error) { - jsonBytes, err := json.Marshal(v) - if err != nil { - return nil, err - } - - var data EventAppData - if err := json.Unmarshal(jsonBytes, &data); err != nil { - return nil, err - } - - return data, nil + switch x := v.(type) { + case map[string]any: + return EventAppData(x), nil + case EventAppData: + return x, nil + default: + var data EventAppData + jsonBytes, err := json.Marshal(v) + if err != nil { + return nil, err + } + if err := json.Unmarshal(jsonBytes, &data); err != nil { + return nil, err + } + return data, nil + } }This keeps the flexible API while eliminating unnecessary work in the common case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
openmeter/app/app.go(1 hunks)openmeter/app/custominvoicing/app.go(2 hunks)openmeter/app/custominvoicing/factory.go(2 hunks)openmeter/app/event.go(6 hunks)openmeter/app/httpdriver/customer.go(3 hunks)openmeter/app/httpdriver/mapper.go(7 hunks)openmeter/app/sandbox/app.go(4 hunks)openmeter/app/sandbox/mock.go(1 hunks)openmeter/app/service/app.go(3 hunks)openmeter/app/stripe/entity/app/app.go(2 hunks)openmeter/app/stripe/entity/input.go(1 hunks)openmeter/app/stripe/service/factory.go(1 hunks)openmeter/billing/events.go(5 hunks)openmeter/billing/httpdriver/invoice.go(2 hunks)openmeter/billing/service/invoice.go(1 hunks)openmeter/billing/service/invoiceline.go(1 hunks)openmeter/billing/service/invoicestate.go(4 hunks)test/app/custominvoicing/event_test.go(1 hunks)test/billing/event_test.go(2 hunks)test/billing/invoice_test.go(3 hunks)test/billing/suite.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (14)
openmeter/app/sandbox/mock.go (1)
openmeter/app/event.go (1)
EventAppData(19-19)
openmeter/billing/service/invoiceline.go (1)
openmeter/billing/events.go (1)
NewInvoiceCreatedEvent(97-104)
openmeter/app/app.go (1)
openmeter/app/event.go (1)
EventAppData(19-19)
openmeter/app/service/app.go (1)
openmeter/app/event.go (2)
NewAppUpdateEvent(121-131)NewAppDeleteEvent(168-176)
openmeter/app/custominvoicing/factory.go (3)
openmeter/app/custominvoicing/app.go (2)
Meta(39-42)Configuration(25-28)openmeter/app/appbase.go (1)
AppBase(38-46)openmeter/app/app.go (1)
CreateAppInput(145-152)
test/billing/event_test.go (3)
test/billing/suite.go (1)
DraftInvoiceInput(293-296)openmeter/billing/events.go (1)
NewInvoiceCreatedEvent(97-104)openmeter/app/stripe/entity/input.go (1)
AppBase(409-412)
openmeter/billing/httpdriver/invoice.go (3)
openmeter/billing/events.go (1)
EventInvoice(39-42)api/api.gen.go (3)
Invoice(3240-3348)BillingProfileApps(1276-1285)BillingProfileAppsOrReference(1301-1303)openmeter/app/httpdriver/mapper.go (1)
MapEventAppToAPI(110-151)
openmeter/billing/service/invoice.go (1)
openmeter/billing/events.go (1)
NewInvoiceCreatedEvent(97-104)
openmeter/app/sandbox/app.go (4)
openmeter/app/appbase.go (1)
AppBase(38-46)openmeter/app/event.go (3)
EventAppParser(15-17)EventApp(52-55)EventAppData(19-19)openmeter/app/custominvoicing/app.go (2)
Meta(39-42)App(56-61)openmeter/app/app.go (1)
App(14-35)
openmeter/app/stripe/entity/input.go (2)
openmeter/ent/db/appstripe/where.go (5)
StripeAccountID(89-91)APIKey(99-101)MaskedAPIKey(104-106)StripeWebhookID(109-111)WebhookSecret(114-116)openmeter/secret/entity/secret.go (1)
SecretID(12-16)
openmeter/app/custominvoicing/app.go (4)
openmeter/app/appbase.go (1)
AppBase(38-46)openmeter/app/event.go (4)
EventAppParser(15-17)EventApp(52-55)EventAppData(19-19)NewEventAppData(24-36)openmeter/app/sandbox/app.go (2)
Meta(36-38)App(48-52)openmeter/app/app.go (1)
App(14-35)
openmeter/billing/service/invoicestate.go (2)
openmeter/watermill/eventbus/eventbus.go (1)
Publisher(61-73)openmeter/billing/events.go (1)
NewInvoiceUpdatedEvent(129-136)
test/app/custominvoicing/event_test.go (4)
test/app/custominvoicing/invocing_test.go (1)
CustomInvoicingTestSuite(30-34)openmeter/app/custominvoicing/app.go (2)
Configuration(25-28)Meta(39-42)test/billing/suite.go (1)
DraftInvoiceInput(293-296)openmeter/billing/events.go (2)
NewInvoiceCreatedEvent(97-104)InvoiceCreatedEvent(93-95)
test/billing/invoice_test.go (1)
test/billing/suite.go (1)
DraftInvoiceInput(293-296)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (35)
openmeter/app/app.go (1)
24-24: Interface extension adds marshaling capability for event data.Adding the
GetEventAppData()method to theAppinterface enables consistent serialization of app-specific data within event payloads. This change supports the new event abstraction system for app entities.openmeter/app/sandbox/mock.go (1)
210-212: Implementation satisfies the extended App interface.The mock implementation correctly returns an empty
app.EventAppDatamap and nil error, which is appropriate for a mock object without actual data requirements.openmeter/billing/service/invoiceline.go (1)
204-211: Improved error handling for invoice event creation.The code now properly separates event creation errors from publishing errors, adding explicit error handling for the event creation step. This is more robust and provides better error context for debugging.
This change correctly handles the new error return value from
billing.NewInvoiceCreatedEvent(), which now returns an error when it fails to construct the event payload with app data.openmeter/app/custominvoicing/factory.go (2)
105-110: Refactored App structure to use Meta embedding.The
Appstruct now embeds app data in aMetastruct containing bothAppBaseandConfiguration, which supports the new event app data architecture and standardizes the app implementation pattern across the codebase.
128-128: Updated to use proper accessor method.Now correctly calls
GetAppBase()on the app instance rather than using it directly, which is consistent with the new app structure whereAppBaseis embedded within theMetastruct.openmeter/billing/httpdriver/invoice.go (1)
566-597: LGTM: Well-structured marshaling of event invoice to API typesThis new function correctly maps
billing.EventInvoicetoapi.Invoiceby:
- First mapping the base invoice
- Mapping each app type (Invoicing, Payment, Tax) using
apphttpdriver.MapEventAppToAPI- Properly handling errors at each step
The implementation is thorough and follows a consistent pattern of error handling.
openmeter/app/stripe/service/factory.go (1)
216-219: Refactored app structure with Meta field implementationThis change aligns with the broader refactoring pattern across app packages to unify metadata handling via the
Metastruct. TheMetastruct likely implements theapp.EventAppParserinterface to support event data serialization.openmeter/app/httpdriver/customer.go (3)
348-349: Updated to use Meta field for consistencyThe code now correctly uses the Stripe app's Meta field when mapping to API types, ensuring consistency with the overall design pattern.
364-365: Updated to use Meta field for consistencyThe code now correctly uses the Sandbox app's Meta field when mapping to API types, maintaining consistency with the refactoring pattern.
377-395: Added support for Custom Invoicing app typeThe implementation for Custom Invoicing app type follows the same pattern as other app types, using the Meta field for mapping and providing proper error handling. This is a good addition that completes the app type support.
openmeter/app/service/app.go (4)
77-82: Updated error handling for event creation in UpdateAppThe code now correctly handles errors returned from
app.NewAppUpdateEvent(), which is necessary based on the recent changes to this function's signature to return an error. This ensures that event creation failures are properly propagated to the caller.
105-110: Added necessary retrieval of existing app before uninstallGood addition to retrieve the existing app before uninstalling it. This is required to extract event-related app data that will be needed when creating the delete event.
118-123: Enhanced event data extraction with proper error handlingThe code now correctly extracts event app data using
existingApp.GetEventAppData()and passes it toapp.NewAppDeleteEvent(), with appropriate error handling. This is necessary to support the richer event payloads in the new event system.
149-153: Added error handling for UpdateAppStatus event creationSimilar to the change in
UpdateApp, this adds proper error handling for theapp.NewAppUpdateEvent()call, maintaining consistency throughout the codebase.openmeter/app/stripe/entity/input.go (1)
416-421: Added JSON struct tags to control serialization of AppDataGood addition of JSON struct tags to the
AppDatastruct fields. Sensitive fields likeAPIKeyandWebhookSecretare correctly excluded from JSON serialization withjson:"-", while other fields have explicit JSON field names which helps maintain a stable API.test/billing/event_test.go (3)
33-35: Updated to use new draft invoice creation helperChanged from using the private
s.createDraftInvoiceto the exporteds.CreateDraftInvoicewhich uses the newDraftInvoiceInputstruct. This aligns with the test helper refactoring that centralizes draft invoice creation.
38-39: Added error handling for invoice event creationThe code now correctly checks for errors returned from
billing.NewInvoiceCreatedEvent(), which is necessary based on the recent changes to the event creation function signature.
52-54: Updated field access to match new event structureUpdated how app base fields are accessed in the event, changing from
event.AppBases.Taxtoevent.Apps.Tax.AppBase(and similarly for Payment and Invoicing). This matches the structural changes in the billing event model whereInvoiceAppBaseswas renamed toInvoiceApps.test/billing/invoice_test.go (3)
906-909: Migrated to new draft invoice creation methodChanged to use the exported
s.CreateDraftInvoicemethod with the newDraftInvoiceInputstruct. This is part of refactoring that centralizes draft invoice creation intest/billing/suite.go.
968-971: Migrated to new draft invoice creation methodUpdated this test case to use the standardized
s.CreateDraftInvoicemethod with the newDraftInvoiceInputstruct, consistent with the refactoring pattern applied throughout the tests.
1198-1201: Migrated to new draft invoice creation methodChanged to use the exported
s.CreateDraftInvoicemethod with the newDraftInvoiceInputstruct, maintaining consistency with other test cases after the draft invoice creation refactoring.openmeter/app/custominvoicing/app.go (4)
39-42: Looks good: Introduced new Meta struct for event data handlingThe
Metastruct effectively encapsulates the app base and configuration data, following a good design pattern for embedding related fields.
56-58: App structure update looks goodThe refactored
Appstruct properly embeds the newMetastruct, aligning with the broader changes to enhance app data handling.
83-85: Implementation of GetEventAppData is correctThe
GetEventAppDatamethod correctly serializes the app configuration into event app data, fulfilling the interface requirement.
44-54:⚠️ Potential issueFix the implementation of
FromEventAppDatamethodThere's a bug in the implementation of the
FromEventAppDatamethod. It's attempting to parse app data into itself rather than into theMetastruct'sConfigurationfield.func (m *Meta) FromEventAppData(event app.EventApp) error { m.AppBase = event.AppBase - if err := event.AppData.ParseInto(&m.Configuration); err != nil { + if err := event.AppData.ParseInto(&m.Configuration); err != nil { return fmt.Errorf("error parsing app data: %w", err) } return nil }Likely an incorrect or invalid review comment.
openmeter/billing/service/invoicestate.go (3)
17-17: Import looks goodThe addition of the eventbus import correctly supports the new event publishing functionality.
26-26: New Publisher field added to InvoiceStateMachineThe addition of a
Publisherfield toInvoiceStateMachineis appropriate for the event publishing capability.
381-389:Details
✅ Verification successful
Event publishing for invoice state transitions
This implementation properly publishes an event after each successful state transition, enhancing the system's observability.
Verify that it's intentional for the method to return early if event creation or publishing fails, potentially leaving the state transition incomplete:
🏁 Script executed:
#!/bin/bash # Find event error handling patterns in similar contexts rg -A 3 'error creating .* event' --glob '*.go' rg -A 3 'error .* event' --glob '*.go' | rg 'return'Length of output: 982
I didn’t find any other patterns of swallowing or deferring state persistence around the event emit. It looks like the invoice is saved (and its transaction committed) before we build and publish the event, so a failure during event creation/publish will return early after the DB change has been applied.
If you intend for the entire transition—including state persistence and the outbound event—to be atomic (i.e. rolled back on publish failures), you’ll need to wrap persistence and publish in the same transactional boundary or add compensation/cleanup logic here.
– If this behavior is intentional (persist then best-effort publish), no changes are needed.
– Otherwise, consider moving the publish into the DB transaction callback or adding a rollback on publish errors.openmeter/app/stripe/entity/app/app.go (2)
16-19: Meta struct looks goodThe
Metastruct effectively encapsulates the app base and app data, following the same pattern used in other app implementations.
90-92: Implementation of GetEventAppData is correctThe
GetEventAppDatamethod correctly serializes the app data into event app data, fulfilling the interface requirement.test/app/custominvoicing/event_test.go (4)
15-21: Test suite structure looks goodThe test suite structure is well organized, properly embedding the existing
CustomInvoicingTestSuiteand registering with the testing framework.
23-40: Test setup and event creation looks goodThe test properly sets up a billing profile with specific configuration flags and creates an invoice event, checking for errors during creation.
41-50: JSON serialization testing looks goodThe test properly verifies that the event can be marshaled to and unmarshaled from JSON, which is an important aspect for events that will be transmitted.
51-62:Details
✅ Verification successful
Event data validation is comprehensive
The test effectively validates that the event contains the correct app bases and that the app data can be unmarshaled into the
Metastruct, verifying that configuration flags are preserved.Given the identified bugs in the
FromEventAppDatamethods, verify that this test is actually testing the correct behavior:
🏁 Script executed:
#!/bin/bash # Check the implementation of the FromEventAppData method in both files rg -A 10 'func \(.+\) FromEventAppData' --glob 'openmeter/app/*/app.go'Length of output: 1211
In order to confirm that the test is exercising the
custominvoicing.Meta.FromEventAppDatalogic correctly (and that there are no typos in the app field names), let’s dump the contents of the test file:
🏁 Script executed:
#!/bin/bash # Show the first 200 lines of the custominvoicing event test sed -n '1,200p' test/app/custominvoicing/event_test.goLength of output: 2059
To confirm how the
InvoiceCreatedEventstruct defines itsAppsfields (and whetherInvocingis a typo), let’s dump its definition:
🏁 Script executed:
#!/bin/bash # Show InvoiceCreatedEvent and its Apps sub-struct rg -A20 'type InvoiceCreatedEvent' openmeter/billingLength of output: 1141
To confirm whether
Apps.Invocingis a typo in the test or also present in the event struct, let’s dump theEventInvoicedefinition and search for “Invocing”:
🏁 Script executed:
#!/bin/bash # Dump the EventInvoice struct (which is inlined into InvoiceCreatedEvent) rg -A50 'type EventInvoice' openmeter/billing/events.go # Search the repo for any occurrence of the misspelled 'Invocing' rg -n 'Invocing' -n .Length of output: 2136
Event data validation is correct and comprehensive
TheTestCreateInvoiceEventsuite accurately exercises the custom-invoicing path:
- It uses the code’s (consistently spelled)
Apps.Invocingfield as defined onInvoiceCreatedEvent.- It verifies both the
AppBaseand thatMeta.FromEventAppDatacorrectly parses and preservesEnableDraftSyncHookandEnableIssuingSyncHook.No changes are needed.
openmeter/billing/events.go (1)
58-90:InvoiceApps.Validatecan emit false positives when any app is optional
NewEventInvoiceleavesInvoiceAppsfields zero-valued if the corresponding workflow app isnil, butValidateblindly calls.Validate()on them, which will fail for a zeroEventApp.If the business rule requires all three apps, enforce it up-front (e.g., return an error from
NewEventInvoice).
Otherwise, guard each call:if a.Tax.IsZero() { /* skip */ } else if err := a.Tax.Validate(); err != nil { … }Consider adding an explicit
IsZero()helper toEventApp.
turip
force-pushed
the
fix/allow-marshaling-event-invoice-into-api-types
branch
from
fb374bd to
ed3228b
Compare
tothandras
deleted the
fix/allow-marshaling-event-invoice-into-api-types
branch
3 participants
Overview
This patch adds invoice update events.
This patch provides a friendlier representation of the App type in terms of Events:
The app is not json unmarshalable, this change makes sure we have an interim representation that can travel on the event bus and can be deserialized.
Given the current structure of the code, we are using an map[string]any encoding for the app data, and the apps are expected to provide this data. We are relying on the json library for the conversion for now, so that we don't have to create custom mappers.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests