feat(api): support removing customer billing data via v3 PUTs - #4885
Draft
tothandras wants to merge 1 commit into
Draft
feat(api): support removing customer billing data via v3 PUTs#4885tothandras wants to merge 1 commit into
tothandras wants to merge 1 commit into
Conversation
tothandras
added
release-note/bug-fix
Contributor
📝 WalkthroughWalkthroughCustomer billing and app-data upserts now support omitted, ChangesCustomer billing tri-state updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UpdateCustomerBilling
participant applyAppData
participant CustomerDataStore
participant buildAppData
Client->>UpdateCustomerBilling: Send nullable billing profile and app data
UpdateCustomerBilling->>applyAppData: Apply omitted, null, or replacement app data
applyAppData->>CustomerDataStore: Delete or upsert customer data
UpdateCustomerBilling->>buildAppData: Build response app data
buildAppData-->>Client: Return resolved billing profile and app data
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
e2e/customer_billing_appdata_v3_test.go (1)
33-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add
given/when/thenintent comments to the setup subtest.This multi-step subtest installs an app, creates a profile and a customer, then pins the profile. The sibling e2e test in this PR (
e2e/billinginvoice_override_test.go) opens each lifecycle subtest withgiven,when, andthencomments. Matching that here would make the fixture chain easier to follow. The rest of the file reads well, and the tri-state coverage is thorough.As per coding guidelines: "Begin non-trivial service or lifecycle subtests with concise
given,when, andthenintent comments."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/customer_billing_appdata_v3_test.go` around lines 33 - 79, The setup subtest currently lacks lifecycle intent comments. Add concise given, when, and then comments around the setup flow in the runRequired callback, covering the external invoicing app/profile/customer preparation and the billing-profile assignment, matching the style used by the sibling lifecycle test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/spec/packages/aip/src/customers/billing.tsp`:
- Around line 28-50: The optional-field documentation in
CustomerBillingDataUpsertRequest incorrectly implies that every field accepts
null. Update the comments in
api/spec/packages/aip/src/customers/billing.tsp:28-50 to state that
null-to-unset behavior applies only to billing_profile and nested app-data
fields that are nullable; do not document app_data itself as nullable.
Regenerate the corresponding documentation in
api/spec/packages/aip-client-javascript/src/models/types.ts:3422-3437 from the
corrected TypeSpec source, with no direct schema change required there.
In `@api/spec/packages/aip/src/shared/request.tsp`:
- Around line 31-37: The UpsertNullableRequest template currently emits the same
friendly name as UpsertRequest, creating component-name collisions. Update the
friendlyName declaration on UpsertNullableRequest to use a distinct
nullable-specific generated name, while preserving its visibility and
nullable-property decorators.
In `@api/v3/handlers/customers/billing/app_data.go`:
- Around line 70-90: Update validateAppData to accept a field-prefix parameter
and use it when constructing the invalid parameter field for Stripe Customer ID,
preserving the standalone app-data path. Update applyAppData and each calling
handler to pass the appropriate prefix, including app_data.stripe for the
billing endpoint so validation errors match the request body paths.
In `@api/v3/handlers/customers/billing/update_billing.go`:
- Around line 140-158: Update the Stripe pinning validation in the billing
update handler so pinning is rejected when appData.Stripe.IsNull(), returning
the same 400 invalid-parameter response used for missing Stripe data before
applyAppData or UpsertCustomerOverride runs. Preserve the existing customer-data
lookup for unspecified Stripe data, and add a handler or end-to-end regression
covering deletion followed by pinning null.
---
Nitpick comments:
In `@e2e/customer_billing_appdata_v3_test.go`:
- Around line 33-79: The setup subtest currently lacks lifecycle intent
comments. Add concise given, when, and then comments around the setup flow in
the runRequired callback, covering the external invoicing app/profile/customer
preparation and the billing-profile assignment, matching the style used by the
sibling lifecycle test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f6c2887-fdd9-45d9-a346-e94a5d59e1a8
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
api/spec/AGENTS.mdapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/tests/wire.spec.tsapi/spec/packages/aip/lib/decorators.jsapi/spec/packages/aip/src/customers/billing.tspapi/spec/packages/aip/src/customers/operations.tspapi/spec/packages/aip/src/shared/request.tspapi/v3/api.gen.goapi/v3/client/models_customers.goapi/v3/handlers/customers/billing/app_data.goapi/v3/handlers/customers/billing/app_data_test.goapi/v3/handlers/customers/billing/get_billing.goapi/v3/handlers/customers/billing/update_billing.goapi/v3/handlers/customers/billing/update_billing_app_data.goe2e/billinginvoice_override_test.goe2e/billinginvoices_v3_test.goe2e/customer_billing_appdata_v3_test.go
Comment on lines
+31
to
+37
| @friendlyName("Upsert{name}Request", T) | ||
| @withVisibility(Lifecycle.Create, Lifecycle.Update) | ||
| @withNullableOptionalProperties | ||
| model UpsertNullableRequest<T extends {}> is DefaultKeyVisibility< | ||
| T, | ||
| Lifecycle.Read | ||
| >; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every instantiation of both templates and check for a shared T.
rg -n --glob '*.tsp' 'Shared\.Upsert(Nullable)?Request<' .Repository: openmeterio/openmeter
Length of output: 1042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files containing UpsertRequest definitions/usage:"
rg -n --glob '*.tsp' 'UpsertNullableRequest|UpsertRequest|FriendlyName|model Upsert' api/spec/packages/aip/src
echo
echo "request.tsp around definitions:"
cat -n api/spec/packages/aip/src/shared/request.tsp | sed -n '1,80p'
echo
echo "customer files around usages:"
cat -n api/spec/packages/aip/src/customers/operations.tsp | sed -n '70,150p'
cat -n api/spec/packages/aip/src/customers/billing.tsp | sed -n '1,90p'Repository: openmeterio/openmeter
Length of output: 8568
Heads up: keep the nullable upsert template name distinct.
UpsertRequest and UpsertNullableRequest both emit Upsert{name}Request, but only Apps.AppCustomerData uses the nullable variant. If any model is passed to both templates later, generate a shared component name, so give this template a distinct friendly name or ensure duplicate generation fails loudly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/spec/packages/aip/src/shared/request.tsp` around lines 31 - 37, The
UpsertNullableRequest template currently emits the same friendly name as
UpsertRequest, creating component-name collisions. Update the friendlyName
declaration on UpsertNullableRequest to use a distinct nullable-specific
generated name, while preserving its visibility and nullable-property
decorators.
Source: Path instructions
Comment on lines
140
to
158
| pinningProfile := request.BillingProfile.IsSpecified() && !request.BillingProfile.IsNull() | ||
| if pinningProfile && application.GetType() == app.AppTypeStripe && !appData.Stripe.IsSpecified() { | ||
| if _, err := application.GetCustomerData(ctx, app.GetAppInstanceCustomerDataInput{ | ||
| CustomerID: request.CustomerID, | ||
| }); err != nil { | ||
| if app.IsAppCustomerPreConditionError(err) { | ||
| return resp, apierrors.NewBadRequestError(ctx, fmt.Errorf("stripe data is required"), apierrors.InvalidParameters{ | ||
| apierrors.InvalidParameter{ | ||
| Field: "app_data.stripe", | ||
| Rule: "required", | ||
| Reason: "Stripe data is required", | ||
| Source: apierrors.InvalidParamSourceBody, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| if request.AppData.ExternalInvoicing.Labels != nil { | ||
| appData = appcustominvoicing.CustomerData{ | ||
| Metadata: models.Metadata(*request.AppData.ExternalInvoicing.Labels), | ||
| } | ||
| } | ||
| return resp, err | ||
| } | ||
| case app.AppTypeSandbox: | ||
| appData = appsandbox.CustomerData{} | ||
| default: | ||
| return resp, apierrors.NewInternalError(ctx, fmt.Errorf("unsupported app type: %s", application.GetType())) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Verify nullable state semantics for the declared dependency version.
curl -fsSL https://raw.githubusercontent.com/oapi-codegen/nullable/v1.2.0/nullable.go |
rg -n -C 3 'func \(.*Nullable.*\) Is(Specified|Null)'Repository: openmeterio/openmeter
Length of output: 483
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate and inspect the relevant handler and data application code.
fd -i 'update_billing\.go$' .
file="$(fd -i 'update_billing\.go$' . | head -n 1)"
printf '\n--- %s lines 1-240 ---\n' "$file"
sed -n '1,240p' "$file" | nl -ba
printf '\n--- search applyAppData and stripe handling ---\n'
rg -n "applyAppData|AppData|BillingProfile|IsAppCustomerPreConditionError|stripe" -S . --glob '!vendor/**' --glob '!node_modules/**' | head -n 200Repository: openmeterio/openmeter
Length of output: 328
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate and inspect the relevant handler and data application code.
file="$(fd -i 'update_billing\.go$' . | head -n 1)"
printf '%s\n' "$file"
awk '{ printf "%5d: %s\n", NR, $0 }' "$file"
printf '\n--- search applyAppData and stripe handling ---\n'
rg -n "applyAppData|AppData|BillingProfile|IsAppCustomerPreConditionError|stripe" -S . --glob '!vendor/**' --glob '!node_modules/**' | head -n 250Repository: openmeterio/openmeter
Length of output: 31418
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' "$file"
awk '{ printf "%5d: %s\n", NR, $0 }' "$file"
printf '\n--- applyAppData and app data API/model ---\n'
rg -n "func applyAppData|type UpsertAppCustomerDataRequest|type AppCustomer|IsSpecified|IsNull|GetCustomerData|AppTypeStripe" -S api openmeter --glob '!vendor/**' --glob '!node_modules/**' | head -n 240Repository: openmeterio/openmeter
Length of output: 196
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="$(fd -i 'update_billing\.go$' . | head -n 1)"
printf '%s\n' "$file"
awk '{ printf "%5d: %s\n", NR, $0 }' "$file"
printf '\n--- applyAppData and app data API/model ---\n'
rg -n "func applyAppData|type UpsertAppCustomerDataRequest|type AppCustomer|AppCustomerDataRequest|stripe Null|GetCustomerData|AppTypeStripe|UpdateAppData|UpsertApp" -S api openmeter --glob '!vendor/**' --glob '!node_modules/**' | head -n 320Repository: openmeterio/openmeter
Length of output: 29275
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
api_file="$(fd -i '^api\.gen\.go$' . | awk '/\/api\/v3\/api\.gen\.go$/ { print; exit }')"
printf '%s\n' "$api_file"
sed -n '7640,7725p' "$api_file"
printf '\n--- applicable app_data handler implementation ---\n'
awk '{ printf "%5d: %s\n", NR, $0 }' api/v3/handlers/customers/billing/app_data.go
printf '\n--- relevant generated model declaration regions ---\n'
rg -n "func \\(t \\*UpsertAppCustomerDataRequest\".*Nullable|func .*UpsertAppCustomerDataRequest.*IsNull|func .*UpsertAppCustomerDataRequest.*IsSpecified|type UpsertAppCustomerDataRequest \\*" api/v3/api.gen.go openmeter -SRepository: openmeterio/openmeter
Length of output: 10166
Reject app_data.stripe: null when pinning a Stripe profile.
profile.stripe: null deletes Stripe customer data, and pinning it back does not recheck for that data. If pinningProfile && appType == stripe, handle appData.Stripe.IsNull() as a 400 instead of proceeding to applyAppData and UpsertCustomerOverride. Add a handler or e2e regression for this sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v3/handlers/customers/billing/update_billing.go` around lines 140 - 158,
Update the Stripe pinning validation in the billing update handler so pinning is
rejected when appData.Stripe.IsNull(), returning the same 400 invalid-parameter
response used for missing Stripe data before applyAppData or
UpsertCustomerOverride runs. Preserve the existing customer-data lookup for
unspecified Stripe data, and add a handler or end-to-end regression covering
deletion followed by pinning null.
tothandras
force-pushed
the
fix/api-v3-billingprofiles
branch
from
August 8, 2026 12:02
18e9b86 to
701c332
Compare
tothandras
changed the title
PUT /customers/{id}/billing and /billing/app-data now replace the
stored state: a provided value replaces it, while omitting an optional
field (or setting it to null, which is equivalent) removes it — the
billing profile override is unpinned and the resolved app's customer
data is deleted. Invalid stripe data returns 400 on every profile
type. The nullable request shape comes from the new
Shared.UpsertNullableRequest<T> template backed by the
@withNullableOptionalProperties decorator, so SDK clients can state
removal explicitly with null.
tothandras
force-pushed
the
fix/api-v3-billingprofiles
branch
from
August 8, 2026 12:10
701c332 to
a5bdbbe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v3/handlers/customers/billing/app_data.go`:
- Around line 71-100: Introduce an ApplyAppDataInput struct containing the
related parameters currently passed to applyAppData, implement its Validate()
error method using the existing validation logic, and change applyAppData to
accept this input. Validate the input before the mutation switch, aggregate
validation failures with errors.Join, and return
models.NewNillableGenericValidationError(...); update all callers to construct
the named input.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1d40f7c-1a29-4b65-b2fc-7853c7b69da6
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (12)
api/spec/AGENTS.mdapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/billing.tspapi/spec/packages/aip/src/shared/request.tspapi/v3/api.gen.goapi/v3/client/models_customers.goapi/v3/handlers/customers/billing/app_data.goapi/v3/handlers/customers/billing/app_data_test.goapi/v3/handlers/customers/billing/update_billing.goapi/v3/handlers/customers/billing/update_billing_app_data.goe2e/customer_billing_appdata_v3_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- api/v3/handlers/customers/billing/update_billing_app_data.go
- api/spec/packages/aip/src/shared/request.tsp
- api/spec/AGENTS.md
- e2e/customer_billing_appdata_v3_test.go
- api/v3/handlers/customers/billing/update_billing.go
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
- api/v3/handlers/customers/billing/app_data_test.go
- api/spec/packages/aip-client-javascript/src/models/types.ts
- api/v3/api.gen.go
Comment on lines
+71
to
+100
| func validateAppData(ctx context.Context, fieldPrefix string, data api.UpsertAppCustomerDataRequest) error { | ||
| if data.Stripe.IsSpecified() && !data.Stripe.IsNull() { | ||
| stripeData, err := data.Stripe.Get() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if stripeData.CustomerId == nil { | ||
| return apierrors.NewBadRequestError(ctx, fmt.Errorf("stripe customer id is required"), apierrors.InvalidParameters{ | ||
| apierrors.InvalidParameter{ | ||
| Field: fieldPrefix + "stripe.customer_id", | ||
| Rule: "required", | ||
| Reason: "Stripe Customer ID is required", | ||
| Source: apierrors.InvalidParamSourceBody, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // applyAppData replaces the customer's data for the resolved payment app: a | ||
| // provided value is validated and stored, while an omitted or explicitly null | ||
| // field deletes the existing data. Valid fields for apps other than the | ||
| // resolved one are ignored. | ||
| func applyAppData(ctx context.Context, application app.App, customerID customer.CustomerID, fieldPrefix string, data api.UpsertAppCustomerDataRequest) error { | ||
| if err := validateAppData(ctx, fieldPrefix, data); err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a named input for applyAppData.
applyAppData accepts five related parameters and performs validation plus persistent mutation. Define ApplyAppDataInput, implement models.Validator, and validate it before the mutation switch. Collect validation errors with errors.Join and return models.NewNillableGenericValidationError(...).
As per coding guidelines, “For non-trivial operations with multiple related parameters, define a named <Operation>Input struct implementing models.Validator through Validate() error.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v3/handlers/customers/billing/app_data.go` around lines 71 - 100,
Introduce an ApplyAppDataInput struct containing the related parameters
currently passed to applyAppData, implement its Validate() error method using
the existing validation logic, and change applyAppData to accept this input.
Validate the input before the mutation switch, aggregate validation failures
with errors.Join, and return models.NewNillableGenericValidationError(...);
update all callers to construct the named input.
Source: Coding guidelines
tothandras
marked this pull request as draft
August 8, 2026 12:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PUT /customers/{id}/billing and /billing/app-data now replace the stored state: a provided value replaces it, while omitting an optional field (or setting it to
null, which is equivalent) removes it — the billing profile override is unpinned and the resolved app's customer data is deleted. Invalid stripe data returns 400 on every profile type.The nullable request shape comes from the new
Shared.UpsertNullableRequest<T>template backed by the@withNullableOptionalPropertiesdecorator, so SDK clients can state removal explicitly withnull:Notes:
app.DeleteCustomerData,billing.DeleteCustomerOverride), both idempotent — removing absent data is a 200 no-op.unit_cost).🤖 Generated with Claude Code
Greptile Summary
The PR changes the v3 customer billing PUT endpoints to replacement semantics, allowing omitted or explicitly null values to remove profile overrides and app customer data.
Confidence Score: 4/5
The PR is not yet safe to merge because a failed billing-profile override mutation can still leave the earlier app-data replacement committed.
The full billing PUT performs app customer-data mutation before independently upserting or deleting the profile override, so a later override failure returns an error after persisting only part of the requested replacement.
Files Needing Attention: api/v3/handlers/customers/billing/update_billing.go
Important Files Changed
Sequence Diagram
Reviews (3): Last reviewed commit: "feat(api): support removing customer bil..." | Re-trigger Greptile
Context used (3)
Summary by CodeRabbit