feat: skaffold custom invoicing #2678
Conversation
📝 WalkthroughWalkthroughThis change introduces a new "Custom Invoicing" app type throughout the platform. It adds new schemas, API endpoints, and service implementations to support this app type alongside existing Stripe and Sandbox integrations. The change includes new TypeScript and OpenAPI schema definitions, updates to the main API specification, and introduces Go interfaces, adapters, services, and HTTP handler logic for managing custom invoicing app instances and their customer data. The app registry and dependency injection setup are refactored to support the new app type. Database schema migrations add a unique index for custom invoicing app records. The codebase is updated to use a unified app registry and to handle the new app type in all relevant API and service flows. 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 🔧 SQLFluff (3.3.0)tools/migrate/migrations/20250417145207_app-custominvoicing.up.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: tools/migrate/migrations/20250417145207_app-custominvoicing.down.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: ✨ 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
feat/skaffold-custom-invoicing
branch
3 times, most recently
from
ebdc6dd to
76b08a8
Compare
turip
force-pushed
the
feat/skaffold-custom-invoicing
branch
2 times, most recently
from
4b135b4 to
09c6292
Compare
turip
force-pushed
the
feat/custominvoicing-db-struct
branch
from
b16020e to
df46851
Compare
turip
force-pushed
the
feat/skaffold-custom-invoicing
branch
from
09c6292 to
7c704ce
Compare
turip
force-pushed
the
feat/skaffold-custom-invoicing
branch
5 times, most recently
from
0effb8f to
f1ef948
Compare
turip
added
release-note/ignore
turip
force-pushed
the
feat/skaffold-custom-invoicing
branch
2 times, most recently
from
5f69561 to
fe0da5e
Compare
turip
force-pushed
the
feat/skaffold-custom-invoicing
branch
from
fe0da5e to
88ea39f
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: 17
🔭 Outside diff range comments (3)
openmeter/app/httpdriver/customer.go (1)
377-378:⚠️ Potential issueMissing case for custom invoicing app in customerAppToAPI function.
While the
getCustomerDatafunction has been updated to handle the custom invoicing app type, thecustomerAppToAPIfunction is missing a corresponding case to convert custom invoicing customer data back to the API format. This could lead to errors when trying to serialize custom invoicing app data.Add a case for custom invoicing app data in the switch statement:
default: + // Handle custom invoicing app + if customInvoicingData, ok := a.CustomerData.(appcustominvoicing.CustomerData); ok { + customInvoicingApp, ok := a.App.(appcustominvoicing.App) + if !ok { + return apiCustomerAppData, fmt.Errorf("error casting app to custom invoicing app") + } + + apiApp := mapCustomInvoicingAppToAPI(customInvoicingApp) + + apiCustomInvoicingCustomerAppData := api.CustomInvoicingCustomerAppData{ + Id: &appId, + Type: api.CustomInvoicingCustomerAppDataTypeCustomInvoicing, + App: &apiApp, + Metadata: lo.ToPtr(customInvoicingData.Metadata), + } + + err := apiCustomerAppData.FromCustomInvoicingCustomerAppData(apiCustomInvoicingCustomerAppData) + if err != nil { + return apiCustomerAppData, fmt.Errorf("error converting to custom invoicing customer app: %w", err) + } + + return apiCustomerAppData, nil + } return apiCustomerAppData, fmt.Errorf("unsupported customer data for app: %s", appId)api/spec/src/app/app.tsp (1)
6-6:⚠️ Potential issuePotential self‑import causes a compilation loop
import "./app.tsp";references the very file we are in.
Typespec will treat this as a circular import and fail to compile.
Unless the intention is to import a sibling file with a different name, please remove or rename this import.openmeter/app/httpdriver/mapper.go (1)
16-27:⚠️ Potential issueUnsafe type assertions can crash the server
In every
caseblock (Stripe,Sandbox,CustomInvoicing) you assert:stripeApp := item.(appstripeentityapp.App)If the concrete value is
*appstripeentityapp.App(pointer) – which is likely when the service layer returns pointers – this will cause a panic, even thoughitem.GetType()matched the case.
Use the comma‑ok form and handle pointer variants:- stripeApp := item.(appstripeentityapp.App) + stripeAppPtr, ok := item.(*appstripeentityapp.App) + if !ok { + return api.App{}, fmt.Errorf("unexpected Stripe app concrete type %T", item) + } + stripeApp := *stripeAppPtrRepeat for Sandbox & CustomInvoicing or switch on
typeinstead ofGetType().
🧹 Nitpick comments (27)
tools/migrate/migrations/20250417145207_app-custominvoicing.down.sql (1)
1-2: Make DROP INDEX idempotent
To avoid errors when rolling back if the index is already removed, consider usingIF EXISTS.Apply this diff:
-DROP INDEX "appcustominvoicing_namespace_id"; +DROP INDEX IF EXISTS "appcustominvoicing_namespace_id";tools/migrate/migrations/20250417145207_app-custominvoicing.up.sql (1)
1-3: Create unique index on namespace and ID
The migration correctly createsappcustominvoicing_namespace_idand suppresses lint MF101.Consider making the statement safe to re-run by adding
IF NOT EXISTS:-CREATE UNIQUE INDEX "appcustominvoicing_namespace_id" ON "app_custom_invoicings" ("namespace", "id"); +CREATE UNIQUE INDEX IF NOT EXISTS "appcustominvoicing_namespace_id" ON "app_custom_invoicings" ("namespace", "id");api/spec/src/productcatalog/tax.tsp (2)
23-27: Provide example & pattern guidance forcustomInvoicingtax codeFor parity with
StripeTaxConfig, consider documenting the expected format ofcodeso client SDKs and UI forms can validate early.@summary("Custom invoicing tax config") customInvoicing?: CustomInvoicingTaxConfig;model CustomInvoicingTaxConfig { ... @summary("Tax code") + @example("my_provider_tax_code_001") + // @pattern("^[A-Za-z0-9_\\-]{1,64}$") // adjust to real constraints code: string; }If the provider imposes no format, at least clarify that with a comment to avoid mistaken assumptions.
64-76: Alphabetical order & separation of provider modelsMinor style suggestion: keep provider‑specific models grouped (Stripe, then CustomInvoicing) or alphabetical to make the file easier to scan as more providers are added.
No functional change required.
api/spec/src/app/customer.tsp (1)
93-93: Incorrect documentation for app property.The comment for the
appproperty incorrectly refers to a "sandbox app" instead of a "custom invoicing app"./** - * The installed sandbox app this data belongs to. + * The installed custom invoicing app this data belongs to. */api/spec/src/app/stripe.tsp (1)
712-726: Optional: Add format / pattern validation for Stripe keys
stripeAccountId,livemode, andmaskedAPIKeyare now read‑only, but you may still want explicit validation:
stripeAccountId→@pattern("acct_[a-zA-Z0-9]{16}")maskedAPIKey→@pattern("^sk_(live|test)_[\\*]{10,}[a-zA-Z0-9]{3}$")This guards accidental bad data in migrations or manual DB edits.
openmeter/app/stripe/entity/app/config.go (1)
23-37: Consider narrowing the type assertion to interface to avoid tight coupling
input.(Configuration)requires callers to pass the concrete struct and forbids future wrappers.
If you only needSecretAPIKey, define a small interface:type secretAPIKeyCarrier interface { GetSecretAPIKey() *string }and assert against that.
This loosens coupling between the HTTP layer DTO and the domain entity.openmeter/app/httpdriver/app.go (2)
134-136: Fix misleading error message when decoding request bodyThe message references “upsert customer data” although this handler updates an app.
Helps troubleshooting if the correct context is logged.- return UpdateAppRequest{}, fmt.Errorf("field to decode upsert customer data request: %w", err) + return UpdateAppRequest{}, fmt.Errorf("failed to decode update app request body: %w", err)
138-203: Reduce duplicated switch–case mapping logicEach case manually maps shared fields (
Name,Default,Description,Metadata,AppID).
Extract the common part and plug in theAppConfigUpdateto keep this handler short and future‑proof:switch updateType { case string(app.AppTypeStripe): - payload, err := body.AsStripeAppReplaceUpdate() - ... - return UpdateAppRequest{ ...commonFields..., AppConfigUpdate: appstripeentityapp.Configuration{ ... } }, nil + payload, err := body.AsStripeAppReplaceUpdate() + if err != nil { ... } + return makeUpdateRequest(appId, namespace, payload, appstripeentityapp.Configuration{ + SecretAPIKey: payload.SecretAPIKey, + }), nil ... } +func makeUpdateRequest(id, ns string, p api.ReplaceApp, cfg app.AppConfigUpdate) UpdateAppRequest { + return UpdateAppRequest{ + AppID: app.AppID{ID: id, Namespace: ns}, + Name: p.Name, Default: p.Default, Description: p.Description, + Metadata: p.Metadata, AppConfigUpdate: cfg, + } +}This trims ~60 lines, lowers cognitive load, and makes adding new app types trivial.
openmeter/app/custominvoicing/service/service.go (2)
28-41: Consider aggregating validation errors for better caller feedback
Config.Validate()short‑circuits on the first missing dependency. In practice the caller may have to iterate through several run–fix cycles to learn about each missing field. Returning a combined error (e.g. viaerrors.Join, a custom multi‑error, orgithub.com/hashicorp/go-multierror) allows the caller to see all configuration problems at once.Optional diff illustrating the idea:
-func (c Config) Validate() error { - if c.Adapter == nil { - return errors.New("adapter cannot be nil") - } - ... - return nil -} +func (c Config) Validate() error { + var errs []error + if c.Adapter == nil { + errs = append(errs, errors.New("adapter cannot be nil")) + } + if c.Logger == nil { + errs = append(errs, errors.New("logger cannot be nil")) + } + if c.AppService == nil { + errs = append(errs, errors.New("app service cannot be nil")) + } + return errors.Join(errs...) +}
44-54: Enrich logger context during constructionRight now the logger injected via
Configis stored unchanged. Adding contextual fields (With("component", "custominvoicing_service"), or similar) at construction time makes downstream logs easier to filter.-logger: config.Logger, +logger: config.Logger.With("component", "custominvoicing_service"),api/spec/src/app/app.tsp (1)
85-89: Out‑of‑date JSDoc – mention all app kindsThe description still states “One of: stripe”, but the union now includes
sandboxandcustom_invoicing. Update the comment so that generated SDK docs remain accurate.openmeter/app/custominvoicing/service/customerdata.go (1)
10-26: Add minimal tracing / logging around transactional boundariesEach method merely proxies the call to the adapter inside a transaction. When diagnosing production issues it will be valuable to know:
- which customer/app IDs were involved,
- whether the transaction was read‑only or read‑write,
- how long it took.
Consider wrapping the adapter call with structured logging or OpenTelemetry spans, e.g.:
start := time.Now() defer s.logger.Debug("upsert_customer_data", slog.Any("input", input), slog.Duration("elapsed", time.Since(start)), )or, if OTEL is enabled,
tracer.Start(ctx, "Service.UpsertCustomerData").No functional change, but it greatly improves observability.
openmeter/app/custominvoicing/app.go (2)
24-26: Consider strengthening the Configuration validation.The
Validate()method currently has an empty implementation that always returns nil. While this may be intentional for now, consider adding actual validation logic to ensure configuration values are valid, especially if there are any business rules or constraints that should be enforced.func (c Configuration) Validate() error { + // Add validation logic here if needed in the future return nil }
36-38: Consider adding validation logic for customer capabilities.The
ValidateCustomermethod currently returns nil without any validation checks. If there are specific capabilities that should be validated for custom invoicing customers, they should be implemented here. Otherwise, consider adding a comment explaining why no validation is needed.func (a App) ValidateCustomer(ctx context.Context, customer *customer.Customer, capabilities []app.CapabilityType) error { + // No validation needed for custom invoicing customers at this time return nil }app/common/app.go (1)
124-131: Consider documenting the marketplace registration side-effect more explicitly.The comment about registering the app with the marketplace as a side-effect is helpful, but this kind of side effect can be easy to miss. Consider making the registration more explicit or documenting it more thoroughly.
openmeter/app/custominvoicing/adapter/adapter.go (2)
61-68: Consider documenting the WithTx method purpose.While the implementation is correct, adding a comment explaining that this method creates a transaction-aware adapter instance would improve code readability and maintainability.
+// WithTx returns a new adapter instance that uses the provided transaction. +// This allows operations to be performed within an existing transaction context. func (a *adapter) WithTx(ctx context.Context, tx *entutils.TxDriver) *adapter {
70-72: Consider documenting or reviewing necessity of Self method.The Self method appears to simply return the adapter instance itself. If this is required for transaction management or other framework functionality, consider adding a documentation comment explaining its purpose.
+// Self returns the adapter itself, which is useful for transaction operations +// that require the concrete adapter type. func (a *adapter) Self() *adapter {openmeter/app/custominvoicing/adapter/appconfig.go (2)
37-51: Add input validation to UpsertAppConfiguration.For consistency with other methods in the codebase, consider adding input validation to verify the AppID and Configuration values before performing the database operation.
func (a *adapter) UpsertAppConfiguration(ctx context.Context, input custominvoicing.UpsertAppConfigurationInput) error { + if input.AppID.ID == "" || input.AppID.Namespace == "" { + return errors.New("app ID and namespace are required") + } + return entutils.TransactingRepoWithNoValue(ctx, a, func(ctx context.Context, tx *adapter) error {
53-64: Add input validation to DeleteAppConfiguration.Similar to the UpsertAppConfiguration method, consider adding input validation here to ensure the AppID is valid before attempting to delete the configuration.
func (a *adapter) DeleteAppConfiguration(ctx context.Context, input app.AppID) error { + if input.ID == "" || input.Namespace == "" { + return errors.New("app ID and namespace are required") + } + return entutils.TransactingRepoWithNoValue(ctx, a, func(ctx context.Context, tx *adapter) error {openmeter/app/custominvoicing/customerdata.go (1)
18-20:CustomerData.Validateis a no‑op – consider enforcing basic invariantsRight now the method always returns nil, so any malformed metadata (e.g. excessively large payloads, illegal characters, reserved keys) passes through unchecked. Even a lightweight validation (length cap, key format regexp, etc.) would prevent unexpected DB errors later and align with how other app data structs are validated in the codebase.
openmeter/app/httpdriver/mapper.go (1)
91-108: Metadata pointer helper deviates from Stripe/Sandbox mappingHere you use
lo.EmptyableToPtr, whereas the Stripe mapper useslo.ToPtr. This means:
- For an empty map
{}, Custom Invoicing metadata becomesnil(field omitted),- For the same input, Stripe metadata becomes
&map[string]string{}(field present but empty).Unless this difference is intentional, consider aligning helpers for consistent API behaviour.
openmeter/app/custominvoicing/factory.go (1)
136-146: Consider validating the Configuration fieldThe
CreateAppInputstruct includes aConfigfield of typeConfiguration, but the validation method doesn't validate this field. IfConfigurationrequires validation, consider adding it.If Configuration requires validation, add it to the validation method:
func (i CreateAppInput) Validate() error { if i.Namespace == "" { return fmt.Errorf("namespace is required") } if i.Name == "" { return fmt.Errorf("name is required") } + // Add any validation for the Config field if necessary + // if err := i.Config.Validate(); err != nil { + // return fmt.Errorf("invalid configuration: %w", err) + // } return nil }api/openapi.cloud.yaml (2)
19117-19123: AddSandboxAppReplaceUpdateschema
Introducing theSandboxAppReplaceUpdatemodel standardizes updates for sandbox apps. Confirm that this definition aligns with the coreapi/openapi.yamlto prevent spec drift.
19361-19364: Secure the Stripe API key in ReplaceUpdate schema
Consider marking thesecretAPIKeyfield aswriteOnly: trueso that it never appears in responses or generated client SDKs.api/openapi.yaml (2)
941-945: Add deprecation rationale for Stripe API key endpointWhile marking the Stripe API key update endpoint as
deprecated: trueis correct, consider enhancing thesummaryor adding adescriptionnote explaining the alternative new endpoint and planned removal timeline.
12300-12395: DefineCustomInvoicingAppschemaThe new
CustomInvoicingAppschema is well-structured and aligns with existing app models (required fields, patterns, metadata, hooks). For improved user guidance, consider adding an example block under this schema to illustrate a typical payload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
api/client/javascript/src/client/schemas.ts(6 hunks)api/openapi.cloud.yaml(13 hunks)api/openapi.yaml(15 hunks)api/spec/src/app/app.tsp(3 hunks)api/spec/src/app/customer.tsp(2 hunks)api/spec/src/app/custominvoicing.tsp(1 hunks)api/spec/src/app/main.tsp(1 hunks)api/spec/src/app/stripe.tsp(2 hunks)api/spec/src/productcatalog/tax.tsp(2 hunks)app/common/app.go(3 hunks)cmd/billing-worker/main.go(1 hunks)cmd/billing-worker/wire.go(1 hunks)cmd/billing-worker/wire_gen.go(8 hunks)cmd/server/main.go(2 hunks)cmd/server/wire.go(1 hunks)cmd/server/wire_gen.go(11 hunks)docker-compose.yaml(1 hunks)openmeter/app/adapter/marketplace.go(2 hunks)openmeter/app/app.go(4 hunks)openmeter/app/appbase.go(1 hunks)openmeter/app/custominvoicing/adapter.go(1 hunks)openmeter/app/custominvoicing/adapter/adapter.go(1 hunks)openmeter/app/custominvoicing/adapter/appconfig.go(1 hunks)openmeter/app/custominvoicing/adapter/customerdata.go(1 hunks)openmeter/app/custominvoicing/app.go(1 hunks)openmeter/app/custominvoicing/customerdata.go(1 hunks)openmeter/app/custominvoicing/factory.go(1 hunks)openmeter/app/custominvoicing/service.go(1 hunks)openmeter/app/custominvoicing/service/customerdata.go(1 hunks)openmeter/app/custominvoicing/service/factory.go(1 hunks)openmeter/app/custominvoicing/service/service.go(1 hunks)openmeter/app/httpdriver/app.go(4 hunks)openmeter/app/httpdriver/customer.go(4 hunks)openmeter/app/httpdriver/handler.go(1 hunks)openmeter/app/httpdriver/mapper.go(5 hunks)openmeter/app/httpdriver/marketplace.go(2 hunks)openmeter/app/registry.go(2 hunks)openmeter/app/sandbox/app.go(1 hunks)openmeter/app/sandbox/config.go(1 hunks)openmeter/app/sandbox/mock.go(2 hunks)openmeter/app/service/app.go(2 hunks)openmeter/app/stripe/entity/app/config.go(1 hunks)openmeter/billing/httpdriver/handler.go(0 hunks)openmeter/billing/httpdriver/profile.go(2 hunks)openmeter/ent/db/migrate/schema.go(1 hunks)openmeter/ent/schema/appcustominvoicing.go(1 hunks)tools/migrate/migrations/20250417145207_app-custominvoicing.down.sql(1 hunks)tools/migrate/migrations/20250417145207_app-custominvoicing.up.sql(1 hunks)
💤 Files with no reviewable changes (1)
- openmeter/billing/httpdriver/handler.go
🧰 Additional context used
🧬 Code Graph Analysis (16)
openmeter/app/sandbox/config.go (3)
openmeter/app/custominvoicing/app.go (1)
Configuration(19-22)openmeter/app/stripe/entity/app/config.go (1)
Configuration(11-13)pkg/models/validator.go (1)
Validate(16-26)
openmeter/app/httpdriver/marketplace.go (1)
openmeter/app/httpdriver/mapper.go (1)
MapAppToAPI(16-48)
openmeter/billing/httpdriver/profile.go (1)
openmeter/app/httpdriver/mapper.go (1)
MapAppToAPI(16-48)
openmeter/app/httpdriver/handler.go (5)
openmeter/app/custominvoicing/service/service.go (1)
Service(13-19)openmeter/app/service.go (1)
Service(9-11)openmeter/billing/service/service.go (1)
Service(23-37)openmeter/app/stripe/service.go (1)
Service(11-16)openmeter/app/stripe/service/service.go (1)
Service(18-26)
cmd/billing-worker/main.go (1)
app/common/app.go (1)
AppRegistry(136-141)
openmeter/app/adapter/marketplace.go (1)
pkg/models/error.go (1)
NewGenericValidationError(138-140)
cmd/billing-worker/wire.go (4)
app/common/app.go (1)
AppRegistry(136-141)app/common/meter.go (1)
Meter(24-27)app/common/streaming.go (1)
Streaming(17-19)openmeter/streaming/connector.go (1)
Connector(37-50)
cmd/server/wire.go (1)
app/common/app.go (1)
AppRegistry(136-141)
openmeter/app/sandbox/mock.go (1)
openmeter/app/app.go (1)
AppConfigUpdate(97-99)
openmeter/app/registry.go (2)
pkg/models/validator.go (1)
Validate(16-26)openmeter/ent/db/appstripe/where.go (1)
APIKey(99-101)
openmeter/app/custominvoicing/adapter.go (8)
pkg/framework/entutils/transaction.go (1)
TxCreator(176-176)openmeter/app/custominvoicing/customerdata.go (4)
GetAppCustomerDataInput(98-98)CustomerData(14-16)UpsertCustomerDataInput(58-61)DeleteAppCustomerDataInput(99-99)openmeter/app/sandbox/app.go (1)
CustomerData(141-141)openmeter/app/appbase.go (1)
AppID(138-141)openmeter/app/custominvoicing/app.go (1)
Configuration(19-22)openmeter/app/sandbox/config.go (1)
Configuration(3-3)openmeter/app/stripe/entity/app/config.go (1)
Configuration(11-13)openmeter/app/custominvoicing/factory.go (1)
UpsertAppConfigurationInput(148-151)
openmeter/app/custominvoicing/service/customerdata.go (4)
openmeter/app/custominvoicing/service.go (1)
Service(9-13)openmeter/app/custominvoicing/service/service.go (1)
Service(13-19)openmeter/app/custominvoicing/customerdata.go (4)
GetAppCustomerDataInput(98-98)CustomerData(14-16)UpsertCustomerDataInput(58-61)DeleteAppCustomerDataInput(99-99)pkg/framework/transaction/transaction.go (2)
Run(31-49)RunWithNoValue(23-28)
openmeter/app/app.go (2)
openmeter/app/appbase.go (1)
AppID(138-141)pkg/models/validator.go (1)
Validate(16-26)
openmeter/app/custominvoicing/adapter/adapter.go (5)
openmeter/app/custominvoicing/service/service.go (2)
Config(21-26)New(44-54)openmeter/app/custominvoicing/adapter.go (1)
Adapter(10-15)openmeter/ent/db/tx.go (1)
Tx(15-106)pkg/framework/entutils/transaction.go (2)
NewTxDriver(47-52)TxDriver(76-88)openmeter/ent/db/expose.go (1)
NewTxClientFromRawConfig(78-174)
openmeter/app/httpdriver/mapper.go (6)
openmeter/app/app.go (1)
App(14-33)openmeter/app/custominvoicing/app.go (2)
App(28-34)Configuration(19-22)openmeter/app/sandbox/app.go (1)
App(35-39)api/api.gen.go (6)
App(983-985)AppTypeStripe(72-72)AppTypeCustomInvoicing(70-70)CustomInvoicingApp(1818-1862)AppStatus(1079-1079)Metadata(4439-4439)openmeter/app/appbase.go (3)
AppTypeStripe(14-14)AppTypeCustomInvoicing(16-16)AppStatus(20-20)openmeter/app/sandbox/config.go (1)
Configuration(3-3)
openmeter/app/custominvoicing/service.go (6)
openmeter/app/custominvoicing/customerdata.go (4)
GetAppCustomerDataInput(98-98)CustomerData(14-16)UpsertCustomerDataInput(58-61)DeleteAppCustomerDataInput(99-99)openmeter/app/app.go (1)
CreateAppInput(148-155)openmeter/app/custominvoicing/factory.go (2)
CreateAppInput(129-134)UpsertAppConfigurationInput(148-151)openmeter/app/appbase.go (2)
AppBase(38-46)AppID(138-141)openmeter/app/registry.go (1)
UninstallAppInput(18-18)openmeter/app/custominvoicing/app.go (1)
Configuration(19-22)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (90)
docker-compose.yaml (1)
126-128: Review mount permissions for Svix initialization scriptAdding
mode: 0444correctly makes thesvix.sqlfile read‑only inside the Postgres container, which should resolve the prior Svix init failures due to permission issues. Please verify that your Docker Compose deployment supports theconfigsandmodedirectives (they require Docker Swarm or Compose spec ≥ 3.3). If you're running standalone compose without Swarm, you may need to use avolumesmount instead.openmeter/ent/db/migrate/schema.go (1)
239-243: Enforce unique constraint on CustomInvoicing apps
The new indexappcustominvoicing_namespace_idcorrectly enforces uniqueness on(namespace, id)for theapp_custom_invoicingstable and matches the corresponding migration.api/spec/src/app/main.tsp (1)
6-10: Import Custom Invoicing TypeSpec definitions
Addingimport "./custominvoicing.tsp"under the// Appssection correctly integrates the new app type alongside Stripe and Sandbox.openmeter/app/appbase.go (1)
14-16: Add CustomInvoicing app type constant
IntroducingAppTypeCustomInvoicingwith the value"custom_invoicing"aligns with the new Custom Invoicing app integration.openmeter/app/sandbox/app.go (1)
65-67: Implementation of UpdateAppConfig is a no-opThe newly added
UpdateAppConfigmethod satisfies the interface requirement but is implemented as a no-op (simply returns nil). This is consistent with the PR objectives of supporting app-specific fields in the update process, while keeping the sandbox app implementation minimal.openmeter/app/sandbox/config.go (1)
3-7: Empty configuration structure is appropriate for sandbox appThe empty
Configurationstruct and theValidatemethod that always returnsnilare appropriate for the sandbox app if it doesn't require any configuration parameters. This is consistent with the pattern used throughout the codebase where each app type has its own configuration structure, though other app types like Stripe have actual configuration fields (e.g.,SecretAPIKey).cmd/billing-worker/main.go (1)
87-87: Refactored to use centralized AppRegistryThe change from
app.AppSandboxProvisioner()toapp.AppRegistry.SandboxProvisioner()aligns with the PR objectives of centralizing app-related services into a singleAppRegistrystruct. This improves code organization and maintainability as the number of supported app types grows.openmeter/app/httpdriver/marketplace.go (2)
153-153: Switched to standalone mapping functionChanged from using instance method
h.appMapper.MapAppToAPI()to standalone functionMapAppToAPI(). This is part of a broader refactoring to use functional mapping approaches consistently across the codebase, which will help support the new CustomInvoicing app type.
210-210: Switched to standalone mapping functionChanged from using instance method
h.appMapper.MapAppToAPI()to standalone functionMapAppToAPI(). This is part of a broader refactoring to use functional mapping approaches consistently across the codebase, which will help support the new CustomInvoicing app type.openmeter/ent/schema/appcustominvoicing.go (1)
34-37: Good addition of a unique index to enforce data integrityAdding a unique index on the combined fields "namespace" and "id" enforces uniqueness at the database level, which is an excellent way to ensure data integrity for the Custom Invoicing app records. This change aligns well with the PR objectives of enabling storage of metadata per customer and enhancing the flexibility of app management.
cmd/server/main.go (2)
130-130: Good refactoring to use the unified AppRegistryThe change from direct method call to using the AppRegistry improves the code organization by centralizing access to app-related services.
152-153: Consistent use of AppRegistry for dependency injectionThese changes properly update the server configuration to use the consolidated AppRegistry struct, making the code more maintainable and aligned with the architectural changes introduced in this PR.
openmeter/billing/httpdriver/profile.go (2)
15-15: Good addition of import for the app HTTP driver packageThe import is required for direct access to the package-level mapping functions after refactoring away from instance methods.
467-480: Refactored to use package-level mapping functionsThe code now properly uses the standalone
MapAppToAPIfunction instead of the instance method. This is part of a good architectural refactoring that centralizes mapping logic and supports the new CustomInvoicing app type.The function handles all app types, including the newly introduced CustomInvoicingApp, as shown in the provided relevant code snippet from
mapper.go.openmeter/app/httpdriver/handler.go (1)
44-44: Clean removal of the appMapper fieldThe handler struct no longer needs an appMapper field since it now uses package-level mapping functions directly. This simplifies the code structure and aligns with the broader refactoring to support the new CustomInvoicing app type across the codebase.
openmeter/app/sandbox/mock.go (1)
186-188: SurfaceUpdateAppConfigcall‑tracking at the instance levelThe instance simply delegates to the parent, which is fine, but once the parent starts counting the calls (see previous comment) you should propagate the expectation flags; otherwise tests that work through an
app.Appinterface obtained from the factory can’t observe the call.No code change required after the parent fix, just mentioning why the delegation matters.
openmeter/app/adapter/marketplace.go (2)
58-58: Enhanced validation error for unsupported installation methods.The error handling has been improved by replacing a generic error with a structured validation error. The new error message clearly indicates that the app doesn't support this installation method and lists the supported methods from the listing metadata.
94-94: Consistent error handling for installation method validation.This change follows the same pattern as the previous one, using structured validation errors for better error reporting. This improvement provides users with more specific feedback about supported installation methods.
cmd/server/wire.go (1)
44-44: Architectural improvement with consolidated app registry.The change replaces direct dependencies on individual app services with a unified
AppRegistry. This is a good architectural improvement that consolidates multiple app-related services (main app service, sandbox provisioner, Stripe, and the new custom invoicing service) under a single registry.openmeter/app/httpdriver/customer.go (3)
12-12: Added import for custom invoicing app package.This import supports the new custom invoicing app functionality being added throughout the codebase.
286-303: Support for handling custom invoicing app type.The implementation correctly adds support for the new custom invoicing app type in the
getCustomerDatafunction. It extracts customer metadata from the API request and creates a correspondingCustomerDatastruct.
348-348: Refactored app mapping calls.The calls to app mapping functions have been refactored from using
h.appMapper.mapXXXAppToAPIto direct calls to standalone functions. This aligns with the broader refactoring away from theAppMapperstruct.Also applies to: 364-364
api/spec/src/app/customer.tsp (2)
25-26: Added custom invoicing app type to CustomerAppData union.This correctly extends the API schema to add support for the new custom invoicing app type in the discriminated union.
87-104: Added CustomInvoicingCustomerAppData model.The new model is well-structured and includes:
- A read-only optional
appproperty of typeCustomInvoicingApp- Inherited base properties from
CustomerAppBasewith the type parameterType.CustomInvoicing- An optional
metadataproperty to store custom invoicing provider dataThe model follows the same patterns as the existing app data models.
openmeter/app/registry.go (2)
27-45: Good addition of validation for app installation with API keyThe validation implementation is thorough and checks all required fields with clear error messages. This is consistent with the validation patterns used elsewhere in the codebase.
57-64: Correct validation fix for AppFactoryInstallAppInputThe validation has been properly corrected to only check for fields that exist in the struct, removing any previous erroneous APIKey check. This change matches the struct definition which does not include an APIKey field.
cmd/billing-worker/wire.go (1)
23-27: Good architectural refactoring to use AppRegistryThe refactoring to use a consolidated
AppRegistryrather than separate app service fields is a good design improvement that:
- Simplifies the
Applicationstruct- Makes it easier to add new app types in the future
- Aligns with the new custom invoicing feature being added
The
AppRegistrypattern will make dependency injection cleaner and more maintainable.api/spec/src/app/custominvoicing.tsp (2)
1-31: Well-documented new CustomInvoicingApp modelThe new
CustomInvoicingAppmodel is clearly defined with:
- Comprehensive documentation explaining its purpose and integration points
- Proper extension of the base app model
- Clear definition of configuration flags with explanatory comments
The model is appropriately typed and follows the established patterns for app types in the system.
23-30: Verify hook behavior documentationThe documentation for both hook flags states "The hook will auto progress to the next state of the invoice." This implies that setting these flags to
truewould prevent auto-progression, but the property names suggest the opposite (skipXxxHook).Please confirm that the behavior is correctly documented - does setting
skipDraftSyncHook: trueprevent auto-progression, or does it enable it?openmeter/app/app.go (4)
24-24: Good interface extension for app-specific configurationAdding the
UpdateAppConfigmethod to theAppinterface enables app-specific configuration updates, which is necessary for supporting the new custom invoicing app type.
97-99: Clean interface design for config updatesThe
AppConfigUpdateinterface is minimal and well-designed, leveraging the existingmodels.Validatorinterface to ensure all implementations include proper validation.
102-109: Flexible update model with polymorphic app configThe addition of the
AppConfigUpdatefield toUpdateAppInputprovides a clean way to handle app-specific configuration updates while maintaining a consistent update API across different app types.
138-143: Proper validation for app config updatesThe conditional validation of
AppConfigUpdatewhen present is correct and follows the established error formatting pattern in the codebase.api/spec/src/app/app.tsp (1)
97-107: Verify existence ofTypeSpec.Rest.Resource.ResourceReplaceModelThe new
AppReplaceUpdateunion relies onTypeSpec.Rest.Resource.ResourceReplaceModel<T>.
Double‑check that:
- The referenced helper actually exists in the currently pinned @typespec/rest version, and
- It is exported at that exact nested path.
A mismatch will surface only at compile time and can be hard to trace.
cmd/billing-worker/wire_gen.go (1)
377-386: Generated file looks correct – no manual action requiredThe new
AppRegistrywiring and cleanup paths compile and follow the existing pattern. Treat this file as read‑only.openmeter/app/custominvoicing/app.go (2)
15-17: Note the intention to implement invoicing interfaces in the future.The commented-out interface assertions indicate planned future implementation of invoicing-related interfaces. This is good documentation of the intended direction, but consider adding a more detailed roadmap comment or creating a ticket to track this work.
40-54: LGTM - Config update logic looks correct.The implementation for
UpdateAppConfigproperly validates the configuration type, runs the validation logic, and persists the changes through the service layer.openmeter/app/custominvoicing/service/factory.go (3)
14-41: LGTM - Well-implemented transactional app creation.The
CreateAppmethod correctly:
- Validates the input
- Uses a transaction for atomic operations
- Creates the app base first
- Sets up the app configuration
- Provides clear error messages for each potential failure point
This ensures data consistency and proper error handling.
43-47: LGTM - Clean transaction handling for app deletion.The implementation properly uses the transaction framework to ensure atomic deletion operations.
49-57: LGTM - App configuration methods look good.Both the
UpsertAppConfigurationandGetAppConfigurationmethods delegate properly to the adapter with appropriate transaction handling for the write operation.app/common/app.go (4)
13-15: LGTM - Appropriate imports for new functionality.The imports for the custom invoicing packages are correctly organized.
29-35: LGTM - Wire set updated correctly.The App wire set is properly updated to include both the new custom invoicing service and the app registry.
106-134: LGTM - Well-structured custom invoicing service initialization.The
NewAppCustomInvoicingServicefunction follows the established pattern for other app services:
- Creates and configures the adapter
- Creates and configures the service
- Sets up the factory with appropriate registration side effects
- Handles errors appropriately at each step
The code is clear and consistent with the codebase's approach to service initialization.
136-155: LGTM - Good approach to service aggregation.The
AppRegistrystruct and its constructor provide a clean way to aggregate multiple app-related services. This approach simplifies dependency injection and makes service access more consistent throughout the codebase.openmeter/app/custominvoicing/adapter.go (3)
10-15: LGTM - Well-designed composite adapter interface.The
Adapterinterface correctly composes the domain-specific interfaces (CustomerDataAdapterandAppConfigAdapter) with transaction capabilities, following the pattern used elsewhere in the codebase.
17-21: LGTM - Clean customer data adapter interface.The
CustomerDataAdapterinterface provides the necessary operations for managing customer data (get, upsert, delete) with appropriate input and output types.
23-27: LGTM - Clean app configuration adapter interface.The
AppConfigAdapterinterface provides the necessary operations for managing app configuration (get, upsert, delete) with appropriate input and output types.openmeter/app/custominvoicing/adapter/adapter.go (3)
16-31: Good validation practice for the configuration struct.The Config struct and its Validate method ensure all required dependencies (Client and Logger) are properly checked before the adapter is initialized. This helps prevent runtime errors from missing dependencies.
44-44: LGTM: Effective compile-time interface check.The variable declaration
var _ appcustominvoicing.Adapter = (*adapter)(nil)ensures at compile time that the adapter struct implements the Adapter interface. This is a good practice that catches interface implementation errors early.
51-59: LGTM: Effective transaction initialization.The Tx method properly hijacks a database transaction and wraps it with the transaction driver. Error handling is appropriate with detailed error messages.
openmeter/app/custominvoicing/adapter/customerdata.go (2)
13-37: LGTM: Well-structured data retrieval with proper error handling.The GetCustomerData method appropriately validates input, uses transactions, and properly handles the case where data is not found by returning an empty struct instead of an error.
53-68: LGTM: Proper soft deletion implementation.The DeleteCustomerData method correctly performs a soft delete by setting the DeletedAt timestamp rather than physically removing the record, which is a good practice for data retention.
openmeter/app/custominvoicing/service.go (2)
9-13: LGTM: Good interface composition.The Service interface appropriately composes more specific interfaces (CustomerDataService and FactoryService), following the Interface Segregation Principle.
15-26: LGTM: Well-defined service interfaces.Both CustomerDataService and FactoryService interfaces define clear, focused methods with appropriate input/output types. The naming is consistent and follows the domain language.
openmeter/app/custominvoicing/adapter/appconfig.go (2)
14-14: LGTM: Proper interface implementation verification.The compile-time check ensures the adapter implements the AppConfigAdapter interface.
66-71: LGTM: Clean domain model mapping.The mapDBToAppConfiguration function provides a clear separation between database entities and domain models.
openmeter/app/custominvoicing/customerdata.go (1)
32-46:⚠️ Potential issueType assertion may panic & blocks pointer usage
input.Data.(CustomerData)will panic ifinput.Datais:
- a pointer (
*CustomerData) – quite common when the HTTP layer unmarshals JSON into a pointer;- any other
app.CustomerDataimpl.Safer pattern:
- data, ok := input.Data.(CustomerData) - if !ok { - return fmt.Errorf("invalid customer data: %v", input.Data) - } + dataPtr, ok := input.Data.(*CustomerData) + if !ok || dataPtr == nil { + return fmt.Errorf("invalid customer data type: %T", input.Data) + } + data := *dataPtrThis avoids a panic, works with both pointer & value inputs, and keeps error messages clear.
Likely an incorrect or invalid review comment.
cmd/server/wire_gen.go (1)
322-333: ConfirmappcustominvoicingServicecleans up correctly
NewAppCustomInvoicingServicemight allocate background goroutines (e.g., periodic sync).
Because the generated code doesn’t record acleanupXfor it, the application may leak resources on shutdown. Please check the factory signature – if it returns a(Service, func(), error)like several other components, wire should capture that cleanup.api/client/javascript/src/client/schemas.ts (8)
2264-2267: LGTM: Properly extended App union typeThe
Appunion type has been correctly extended to include the newCustomInvoicingAppschema type alongside the existing app types.
2359-2359: LGTM: AppType enum extended correctlyThe
AppTypeenum has been properly extended to include 'custom_invoicing' as a new app type value.
3147-3214: LGTM: CustomInvoicingApp schema well-defined and documentedThe new
CustomInvoicingAppschema follows the established pattern for app schemas with appropriate fields and documentation. The app-specific fieldsskipDraftSyncHookandskipIssuingSyncHookare well-documented, explaining their behavior in controlling invoice workflow progression.
3215-3233: LGTM: CustomerAppData schema properly implementedThe
CustomInvoicingCustomerAppDataschema correctly implements the customer app data pattern with appropriate fields including metadata support, which aligns with the PR objective of enabling customer-specific metadata storage.
3234-3243: LGTM: Tax configuration schema addedThe
CustomInvoicingTaxConfigschema provides a simple but flexible approach with a string code field that can be interpreted by the custom invoicing provider.
3415-3415: LGTM: CustomerAppData union type correctly extendedThe
CustomerAppDataunion type has been properly extended to include the newCustomInvoicingCustomerAppDataschema type.
9122-9126: LGTM: Tax configuration extended for custom invoicingThe tax configuration object has been properly extended with a
customInvoicingfield to support custom invoicing tax settings.
9707-9711: LGTM: Type exports added correctlyAll new schema types are properly exported with TypeScript type aliases, maintaining consistency with the rest of the codebase.
openmeter/app/custominvoicing/factory.go (3)
11-46: LGTM: Well-structured capabilities and marketplace listingThe marketplace listing and capabilities are well-defined and align with the PR objectives. The support for
InstallMethodNoCredentialsaligns with the goal of being able to install apps without requiring a secret key.
107-121: LGTM: Well-structured app installation flowThe
InstallAppmethod properly validates input, creates a new app via the service, and returns the instantiated app. The error handling is appropriate with detailed error messages.
123-125: LGTM: Simple and effective UninstallApp implementationThe
UninstallAppmethod correctly delegates app deletion to the custom invoicing service, which follows good separation of concerns.api/openapi.cloud.yaml (12)
810-813: Reference the polymorphic AppReplaceUpdate schema for app updates
The PATCH operation now correctly uses the unifiedAppReplaceUpdateschema to handle update payloads for all app types, including the newly addedcustom_invoicing.
942-945: Mark Stripe API key update endpoint as deprecated
Deprecating theStripeAPIKeyInputendpoint is a clear signal for clients to migrate away. Ensure this is documented in the changelog so integrators are aware of the deprecation timeline.
3143-3145: Extend upsert customer app data to use the new polymorphic item schema
Theitemsarray now referencesCustomerAppDataCreateOrUpdateItem, allowing inclusion ofCustomInvoicingCustomerAppDatavariants.
11116-11122: IncludeCustomInvoicingAppin the App union and discriminator
AddingCustomInvoicingAppin theoneOfand the discriminator mapping ensures the new app type is treated alongside Stripe and Sandbox in all API flows.
11208-11213: DefineCustomInvoicingAppReplaceUpdatein the AppReplaceUpdate union
The polymorphicAppReplaceUpdateschema correctly incorporates the replace/update model for the new custom invoicing app type.
11229-11233: Extend AppType enum withcustom_invoicing
TheAppTypeenum now includescustom_invoicing, ensuring type safety across schemas and request/response payloads.
12394-12491: Define theCustomInvoicingAppschema
The new schema includes all required fields (id,name, timestamps, listing, status, default flags, sync hooks) with appropriatereadOnlyconstraints and metadata.
12492-12540: DefineCustomInvoicingAppReplaceUpdateschema
This covers the replace/update payload for custom invoicing apps, mirroring the structure of other app types while requiring the new sync-hook flags.
12571-12583: DefineCustomInvoicingTaxConfigschema
This schema correctly captures thecodeproperty for provider-specific tax configurations.
19282-19294: Mark Stripe app response fields as read-only
The propertiesstripeAccountId,livemode, andmaskedAPIKeyare now correctly flagged asreadOnly, clarifying that they are managed by the server and not client-supplied.
19405-19435: IntroduceStripeCustomerAppDataCreateOrUpdateItem
This schema properly defines the input model for creating/updating Stripe customer data, requiringstripeCustomerIdwith optional default payment method.
20554-20559: Extend provider-specific tax configs withcustomInvoicing
The union of tax configurations now includescustomInvoicing, allowing custom invoicing providers to supply their own tax code definitions.api/openapi.yaml (9)
809-813: Use polymorphic AppReplaceUpdate for update endpointThe PUT /api/v1/apps/{id} operation now references
AppReplaceUpdate, enabling polymorphic support for different app types (Stripe, Sandbox, CustomInvoicing). This aligns with the PR objectives.
3141-3145: Switch to CreateOrUpdateItem schemaThe
/api/v1/customers/{customerIdOrKey}/appsrequest schema now usesCustomerAppDataCreateOrUpdateItem, which better represents batched create/update operations. This change is correct.
10855-10864: Extend app discriminator with CustomInvoicingAppAdding
CustomInvoicingAppto theoneOfand discriminator mapping ensures the new app type is recognized in polymorphic responses. This looks good.
11042-11053: UpdateAppReplaceUpdateunion for all app typesThe
AppReplaceUpdateschema now includesCustomInvoicingAppReplaceUpdatealongsideStripeAppReplaceUpdateandSandboxAppReplaceUpdateand updates the discriminator. This correctly implements polymorphic replace/update models.
11063-11067: Addcustom_invoicingto AppType enumIncluding
custom_invoicingin theAppTypeenum allows the new app type to be recognized in client/server validations.
11623-11637: Use polymorphic AppReadOrCreateOrUpdateOrDeleteOrQuery for workflow overridesReferencing the unified
AppReadOrCreateOrUpdateOrDeleteOrQueryschema fortaxApp,invoicingApp, andpaymentAppensures all app types (including CustomInvoicing) are supported in workflow overrides.
19722-19735: Mark Stripe output fields as read-onlyAdding
readOnly: truetostripeAccountId,livemode, andmaskedAPIKeycorrectly prevents these fields from being set in requests.
19977-20003: DefineStripeCustomerAppDataCreateOrUpdateItemcorrectlyThe
StripeCustomerAppDataCreateOrUpdateItemschema is well-defined with required fields (type,stripeCustomerId) and optional payment method.
21161-21166: Extend tax configs with CustomInvoicingTaxConfigIncluding
customInvoicingin the provider-specific tax configs union allows tax configuration for the new app type.
turip
force-pushed
the
feat/skaffold-custom-invoicing
branch
from
5f2cfbe to
376d919
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
api/openapi.cloud.yaml (1)
12560-12565:⚠️ Potential issueFix description for customer data
typefield.In
CustomInvoicingCustomerAppData, thetypeproperty's description is incorrect. It should indicate this is the app’s type (not its name) and that it must becustom_invoicing:- description: The app name. + description: The app's type. Must be `custom_invoicing`.api/openapi.yaml (2)
11013-11025:⚠️ Potential issueInconsistent Sandbox reference in read/query union
The newly introducedAppReadOrCreateOrUpdateOrDeleteOrQueryunion referencesSandboxAppdirectly, while Stripe usesStripeAppReadOrCreateOrUpdateOrDeleteOrQuery. This will break clients expecting an operation-specific shape for Sandbox as well.Please apply this diff to maintain consistency:
- oneOf: - - $ref: '#/components/schemas/StripeAppReadOrCreateOrUpdateOrDeleteOrQuery' - - $ref: '#/components/schemas/SandboxApp' - - $ref: '#/components/schemas/CustomInvoicingApp' + oneOf: + - $ref: '#/components/schemas/StripeAppReadOrCreateOrUpdateOrDeleteOrQuery' + - $ref: '#/components/schemas/SandboxAppReadOrCreateOrUpdateOrDeleteOrQuery' + - $ref: '#/components/schemas/CustomInvoicingApp' ... - discriminator: - mapping: - stripe: '#/components/schemas/StripeAppReadOrCreateOrUpdateOrDeleteOrQuery' - sandbox: '#/components/schemas/SandboxApp' + discriminator: + mapping: + stripe: '#/components/schemas/StripeAppReadOrCreateOrUpdateOrDeleteOrQuery' + sandbox: '#/components/schemas/SandboxAppReadOrCreateOrUpdateOrDeleteOrQuery'Ensure the mapping keys and schema references align for all app types.
19899-19938:⚠️ Potential issueProtect secretAPIKey with writeOnly
TheStripeAppReplaceUpdateschema introducessecretAPIKeyas a password field but misseswriteOnly: true. To prevent accidental exposure, mark this field as write-only:secretAPIKey: type: string format: password description: The Stripe API key. + writeOnly: true
🧹 Nitpick comments (1)
api/openapi.yaml (1)
12300-12489: Define CustomInvoicingApp entities
Comprehensive definitions forCustomInvoicingApp,CustomInvoicingAppReplaceUpdate,CustomInvoicingCustomerAppData, andCustomInvoicingTaxConfighave been added.
These schemas include all required properties and adhere to the polymorphic design framework.Nitpick: Consider standardizing the
typeproperty description inCustomInvoicingCustomerAppDatato "The app type" for consistency with other CustomerAppData schemas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
api/client/javascript/src/client/apps.ts(4 hunks)api/client/javascript/src/client/schemas.ts(16 hunks)api/openapi.cloud.yaml(13 hunks)api/openapi.yaml(15 hunks)api/spec/src/app/app.tsp(4 hunks)api/spec/src/app/customer.tsp(3 hunks)api/spec/src/app/custominvoicing.tsp(1 hunks)api/spec/src/app/main.tsp(1 hunks)api/spec/src/app/stripe.tsp(2 hunks)api/spec/src/productcatalog/tax.tsp(2 hunks)app/common/app.go(3 hunks)cmd/billing-worker/main.go(1 hunks)cmd/billing-worker/wire.go(1 hunks)cmd/billing-worker/wire_gen.go(8 hunks)cmd/server/main.go(2 hunks)cmd/server/wire.go(1 hunks)cmd/server/wire_gen.go(11 hunks)docker-compose.yaml(1 hunks)openmeter/app/adapter/marketplace.go(2 hunks)openmeter/app/app.go(4 hunks)openmeter/app/appbase.go(1 hunks)openmeter/app/custominvoicing/adapter.go(1 hunks)openmeter/app/custominvoicing/adapter/adapter.go(1 hunks)openmeter/app/custominvoicing/adapter/appconfig.go(1 hunks)openmeter/app/custominvoicing/adapter/customerdata.go(1 hunks)openmeter/app/custominvoicing/app.go(1 hunks)openmeter/app/custominvoicing/customerdata.go(1 hunks)openmeter/app/custominvoicing/factory.go(1 hunks)openmeter/app/custominvoicing/service.go(1 hunks)openmeter/app/custominvoicing/service/customerdata.go(1 hunks)openmeter/app/custominvoicing/service/factory.go(1 hunks)openmeter/app/custominvoicing/service/service.go(1 hunks)openmeter/app/httpdriver/app.go(4 hunks)openmeter/app/httpdriver/customer.go(4 hunks)openmeter/app/httpdriver/handler.go(1 hunks)openmeter/app/httpdriver/mapper.go(5 hunks)openmeter/app/httpdriver/marketplace.go(2 hunks)openmeter/app/registry.go(2 hunks)openmeter/app/sandbox/app.go(1 hunks)openmeter/app/sandbox/config.go(1 hunks)openmeter/app/sandbox/mock.go(2 hunks)openmeter/app/service/app.go(2 hunks)openmeter/app/stripe/entity/app/config.go(1 hunks)openmeter/billing/httpdriver/handler.go(0 hunks)openmeter/billing/httpdriver/profile.go(2 hunks)openmeter/ent/db/migrate/schema.go(1 hunks)openmeter/ent/schema/appcustominvoicing.go(1 hunks)tools/migrate/migrations/20250417145207_app-custominvoicing.down.sql(1 hunks)tools/migrate/migrations/20250417145207_app-custominvoicing.up.sql(1 hunks)
💤 Files with no reviewable changes (1)
- openmeter/billing/httpdriver/handler.go
✅ Files skipped from review due to trivial changes (4)
- openmeter/app/sandbox/config.go
- cmd/server/wire.go
- api/client/javascript/src/client/apps.ts
- openmeter/app/stripe/entity/app/config.go
🚧 Files skipped from review as they are similar to previous changes (38)
- cmd/billing-worker/main.go
- docker-compose.yaml
- openmeter/app/appbase.go
- api/spec/src/app/main.tsp
- openmeter/app/httpdriver/handler.go
- cmd/server/main.go
- openmeter/ent/schema/appcustominvoicing.go
- tools/migrate/migrations/20250417145207_app-custominvoicing.up.sql
- openmeter/app/service/app.go
- openmeter/billing/httpdriver/profile.go
- tools/migrate/migrations/20250417145207_app-custominvoicing.down.sql
- api/spec/src/productcatalog/tax.tsp
- openmeter/app/httpdriver/app.go
- openmeter/app/adapter/marketplace.go
- openmeter/app/httpdriver/customer.go
- openmeter/app/httpdriver/marketplace.go
- openmeter/app/registry.go
- openmeter/app/sandbox/mock.go
- api/spec/src/app/customer.tsp
- api/spec/src/app/custominvoicing.tsp
- openmeter/app/custominvoicing/service/service.go
- openmeter/app/custominvoicing/adapter.go
- api/spec/src/app/app.tsp
- openmeter/app/custominvoicing/service/factory.go
- openmeter/app/sandbox/app.go
- openmeter/app/httpdriver/mapper.go
- openmeter/app/custominvoicing/service.go
- openmeter/app/custominvoicing/app.go
- openmeter/app/custominvoicing/factory.go
- openmeter/app/custominvoicing/customerdata.go
- openmeter/app/custominvoicing/adapter/appconfig.go
- api/spec/src/app/stripe.tsp
- openmeter/app/custominvoicing/adapter/customerdata.go
- openmeter/app/custominvoicing/adapter/adapter.go
- openmeter/app/custominvoicing/service/customerdata.go
- openmeter/app/app.go
- openmeter/ent/db/migrate/schema.go
- cmd/billing-worker/wire.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/client/javascript/src/client/schemas.ts (1)
api/api.gen.go (12)
AppReplaceUpdate(1074-1076)CustomInvoicingApp(1818-1862)CustomInvoicingAppReplaceUpdate(1868-1894)CustomInvoicingCustomerAppData(1900-1913)CustomInvoicingTaxConfig(1919-1924)CustomerAppDataCreateOrUpdateItem(2041-2043)SandboxAppReplaceUpdate(5662-5678)SandboxCustomerAppData(5684-5694)StripeApp(5723-5767)StripeAppReplaceUpdate(5826-5845)StripeCustomerAppData(5854-5870)StripeCustomerAppDataCreateOrUpdateItem(5876-5889)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (50)
cmd/billing-worker/wire_gen.go (4)
366-376: Good implementation of the new Custom Invoicing service initialization.The initialization follows the same pattern as other app services with proper error handling and cleanup. The service is created after the Stripe service, maintaining a logical initialization order.
377-377: Well-designed registry for app services.The new
AppRegistryaggregates all app-related services into a single container, simplifying service management and access throughout the codebase.
382-382: Appropriate struct field update to use the registry.Replacing individual app service fields with the unified
AppRegistryis a good architectural improvement that centralizes app service management.
298-319: Logical reordering of initialization sequence.The initialization sequence has been reordered to create Kafka namespace-related components before other dependent services, which provides a clearer dependency flow.
app/common/app.go (5)
13-15: Appropriate imports for the new custom invoicing functionality.The imports correctly include all necessary packages for the custom invoicing feature.
34-34: Good addition to the wire set.Adding
NewAppCustomInvoicingServiceto the wire set ensures proper dependency injection for the new service.
106-134: Well-structured implementation of the custom invoicing service constructor.The function follows the established pattern for service initialization with proper error handling at each step. The comment at line 124 about registering with the marketplace as a side-effect is helpful for understanding the code's behavior.
136-141: Clean design of the AppRegistry struct.The struct provides a unified interface to access all app-related services, which is a good architectural pattern.
143-155: Simple and effective registry constructor.The constructor is clean and straightforward, accepting all required services and returning a properly initialized registry.
cmd/server/wire_gen.go (4)
171-190: Appropriate reorganization of namespace initialization.Moving the namespace-related initializations earlier in the flow establishes a clearer dependency hierarchy for components that rely on namespace functionality.
322-331: Consistent initialization of the custom invoicing service.The initialization follows the same pattern as the Stripe service, maintaining consistency across the codebase while properly handling errors and cleanup.
332-332: Good registry creation with all required services.The app registry is created with all necessary services, ensuring a complete and properly initialized registry object.
446-446: Appropriate Application struct update.Replacing individual app service fields with the unified AppRegistry simplifies the Application struct and provides a cleaner interface for accessing app services.
api/openapi.cloud.yaml (9)
810-814: Use new polymorphic AppReplaceUpdate schema.The request body now correctly references the unified
AppReplaceUpdatemodel, aligning with the updated polymorphic pattern for replacing/updating apps.
3142-3145: Incorporation of new CustomerAppDataCreateOrUpdateItem.The array schema for the upsert endpoint now references the polymorphic
CustomerAppDataCreateOrUpdateItem, which includesCustomInvoicingCustomerAppData. This correctly extends support to custom invoicing.
11208-11220: Introduce polymorphic AppReplaceUpdate schema.The new
AppReplaceUpdateobject correctly unifies replace/update operations acrossStripeApp,SandboxApp, andCustomInvoicingApp. This adheres to the new polymorphic design.
11229-11233: Extendtypeenum for apps.Including
custom_invoicingin the app type enumeration ensures the spec supports the new app type.
12803-12816: Union extension for CustomerAppData.The
CustomerAppDataandCustomerAppDataCreateOrUpdateItemschemas now correctly includeCustomInvoicingCustomerAppDatain bothoneOfand discriminator mappings, with updated descriptions. This aligns with the new app type support.
19117-19123: AddSandboxAppReplaceUpdateschema.The
SandboxAppReplaceUpdateobject is properly defined with required fields and atypeenum ofsandbox, fitting into the unified update model.
19280-19293: Mark Stripe app read-only fields.Adding
readOnly: truetostripeAccountId,livemode, andmaskedAPIKeyin theStripeAppschema correctly clarifies that these are output-only properties.
19326-19366: DefineStripeAppReplaceUpdateschema.The
StripeAppReplaceUpdateschema is correctly structured, with optionalsecretAPIKeyand required base fields (name,default,type). This complements the polymorphic replace pattern.
20555-20560: Add custom invoicing to tax configs.The
TaxConfigschema now includes acustomInvoicingproperty referencingCustomInvoicingTaxConfig, correctly extending provider-specific tax options for custom invoicing.api/openapi.yaml (12)
809-815: Consistent polymorphic update schema
ThePUT /api/v1/apps/{id}operation now references the newAppReplaceUpdateschema, correctly introducing a polymorphic union that supports Stripe, Sandbox, and Custom Invoicing app types.
941-947: Verify deprecation placement
Ensure that thedeprecated: trueflag for thePUT /api/v1/apps/{id}/stripe/api-keyendpoint is specified at the operation level (as a direct child of the HTTP verb) rather than nested underrequestBodyorschema.
3141-3147: Batch customer app data update schema
ThePUT /api/v1/customers/{customerIdOrKey}/appsendpoint now expects an array ofCustomerAppDataCreateOrUpdateItem, aligning with the polymorphic design to support all three app types.
10855-10864: Extended App schema to include CustomInvoicingApp
TheAppschema’soneOfunion and discriminator mapping have been updated to includeCustomInvoicingAppwith the correctcustom_invoicingkey. This fully integrates the new app type.
11042-11054: Polymorphic AppReplaceUpdate union defined
TheAppReplaceUpdateschema correctly defines aoneOfunion forStripeAppReplaceUpdate,SandboxAppReplaceUpdate, andCustomInvoicingAppReplaceUpdatewith an appropriate discriminator.
11063-11067: Updatedtypeenum to includecustom_invoicing
The sharedtypeenum now supportscustom_invoicing, ensuring the new app type is recognized across the API.
11622-11638: Use of AppReadOrCreateOrUpdateOrDeleteOrQuery in workflow overrides
ThetaxApp,invoicingApp, andpaymentAppfields now reference the newAppReadOrCreateOrUpdateOrDeleteOrQueryschema, allowing custom invoicing workflows to be overridden correctly.
12709-12738: Extend CustomerAppData unions for custom invoicing
Both theCustomerAppDataandCustomerAppDataCreateOrUpdateItemunions now includeCustomInvoicingCustomerAppData. Discriminator mappings correctly reference the new schema.
19543-19568: Add SandboxAppReplaceUpdate schema
TheSandboxAppReplaceUpdatemodel now includes the previously missingdefaultfield, resolving earlier validation issues.
19722-19736: Mark Stripe app properties as read-only
ThestripeAccountId,livemode, andmaskedAPIKeyproperties inStripeAppReadOrCreateOrUpdateOrDeleteOrQueryare now correctly flagged asreadOnly: true.
19976-20009: Define StripeCustomerAppDataCreateOrUpdateItem
TheStripeCustomerAppDataCreateOrUpdateItemmodel is correctly defined with required fieldstypeandstripeCustomerId, and optionalstripeDefaultPaymentMethodId.
21160-21168: Include CustomInvoicingTaxConfig in tax configurations
The API’s tax config schema now supportscustomInvoicing, integratingCustomInvoicingTaxConfigalongsideStripeTaxConfig.api/client/javascript/src/client/schemas.ts (16)
153-155: Endpoint correctly marked as deprecated.The PR adds a deprecation notice to the Stripe API key update endpoint, which aligns with the PR objective that states "The Stripe app-specific endpoint is marked as deprecated but remains functional."
2265-2268: New CustomInvoicingApp type properly added to App union type.The CustomInvoicingApp is properly added to the App union type, allowing the system to handle the new custom invoicing applications alongside existing Stripe and Sandbox apps.
2330-2334: New AppReplaceUpdate union type facilitates polymorphic updates.The introduction of the
AppReplaceUpdateunion type is a good design decision that enables type-safe updates for different app types. This aligns with the PR objective of supporting app-specific fields without requiring separate app-specific configuration update endpoints.
2344-2344: App type enumeration extended appropriately.Adding 'custom_invoicing' to the AppType enumeration ensures consistent type checking throughout the application.
3132-3262: Comprehensive schema definition for CustomInvoicingApp.The schema defines all necessary properties for the custom invoicing app, including metadata support which aligns with the PR objective of "enabling the storage of metadata per customer." The schema includes appropriate fields for configuration control, such as
skipDraftSyncHookandskipIssuingSyncHook.The descriptive comments on each field provide good documentation for developers.
3430-3441: CustomerAppData types extended for custom invoicing.The changes correctly extend both the
CustomerAppDataandCustomerAppDataCreateOrUpdateItemunion types to include custom invoicing data, facilitating consistent handling of app-specific customer data across different app types.
8094-8119: SandboxAppReplaceUpdate schema added for consistency.Adding the
SandboxAppReplaceUpdateschema provides a consistent pattern for app updates across all supported app types. This follows good design principles by ensuring all app types have similar update models.
8249-8286: StripeAppReplaceUpdate schema added with proper fields.The
StripeAppReplaceUpdateschema is correctly implemented with all necessary fields, including the optionalsecretAPIKeyfield. This maintains backward compatibility while supporting the new unified update pattern.
8319-8343: StripeCustomerAppDataCreateOrUpdateItem schema properly defined.This schema provides a consistent pattern for creating or updating customer app data for Stripe apps, following the same pattern established for other app types.
9230-9234: CustomInvoicing tax configuration added to TaxConfig.The tax configuration for custom invoicing is properly added to the TaxConfig union type, allowing tax information to be specified for custom invoicing apps.
9707-9711: AppReplaceUpdate type export added.The export of the new AppReplaceUpdate type is correctly added, making it available for use in the client API.
9815-9821: Type exports added for all custom invoicing schemas.All required type exports for the custom invoicing schemas are properly added, ensuring they're available for use in the client API.
9830-9831: CustomerAppDataCreateOrUpdateItem type export added.The export of the CustomerAppDataCreateOrUpdateItem type is correctly added, making it available for use in the client API.
10136-10152: Type exports added for app update models.The exports for SandboxAppReplaceUpdate, StripeAppReplaceUpdate, and StripeCustomerAppDataCreateOrUpdateItem types are correctly added, ensuring consistency across the API.
11257-11257: Updated request body schema for updateApp operation.The request body for the updateApp operation now correctly uses the new AppReplaceUpdate union type, aligning with the PR objective of supporting app-specific fields without requiring separate endpoints.
14324-14324: Updated request body schema for upsertCustomerAppData operation.The request body for the upsertCustomerAppData operation now correctly uses an array of the new CustomerAppDataCreateOrUpdateItem union type, maintaining consistency with the updated schema design.
2 participants
Overview
Skaffolds the custom invoicing app. We allow for a metadata per customer, so that any IDs can be persisted there.
The invoicing logic is left unimplemented.
Fixes:
Summary by CodeRabbit
New Features
Improvements
Refactor
Documentation
Database Migration