feat(api): make usage attribution optional for customers #3264
Conversation
📝 WalkthroughWalkthroughMade usageAttribution optional and allowed SubjectKeys arrays to be empty across spec, OpenAPI, JS typings/zod, Go API models/mappings, and tests; updated mappings and tests to use pointer/optional forms accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/spec/src/customer/customer.tsp (1)
154-156: Typo in user-facing docs: 'susbcription' → 'subscription'Visible in generated API docs; please fix.
- * Filter customers by the plan key of their susbcription. + * Filter customers by the plan key of their subscription.
🧹 Nitpick comments (5)
api/spec/src/customer/customer.tsp (1)
256-270: Doc/spec mismatch: comments say 'multiple subjects' while model enforces max 1Current docs promise multiple subjects, but
@maxItems(1)restricts to one. Also, given minItems was removed, clarify that an empty array is allowed to explicitly clear attribution. Proposed doc-only tweak:@@ -/** - * Mapping to attribute metered usage to the customer. - * One customer can have multiple subjects, - * but one subject can only belong to one customer. - */ +/** + * Mapping to attribute metered usage to the customer. + * Currently limited to a single subject per customer (maxItems(1)). + * Roadmap: allow multiple subjects; one subject can only belong to one customer. + */ @friendlyName("CustomerUsageAttribution") model CustomerUsageAttribution { /** * The subjects that are attributed to the customer. - * @TODO allow multiple subjects + * Empty array is allowed to explicitly clear attribution. + * @TODO allow multiple subjects */ @summary("SubjectKeys") @maxItems(1) subjectKeys: string[]; }api/client/javascript/src/zod/index.ts (1)
6705-6707: Guard against empty/whitespace-only subject keys (array items)With the array length constraint relaxed, empty strings like "" or " " can slip through. If empty keys are not meaningful server-side, add item-level trimming + non-empty validation.
If allowed by the contract, apply:
- subjectKeys: zod - .array(zod.coerce.string()) + subjectKeys: zod + .array(zod.coerce.string().trim().min(1, 'subjectKey cannot be empty')) .max(createCustomerBodyUsageAttributionSubjectKeysMax)If empty strings are intentionally allowed, feel free to ignore.
e2e/productcatalog_test.go (1)
46-49: Pointer migration looks right; add coverage for nil/empty casesThe switch to passing a pointer aligns with the client model change. Please add coverage for:
- Creating a customer without usageAttribution (field omitted).
- Creating a customer with usageAttribution but subjectKeys as an empty array.
This will validate the optionality end-to-end and prevent regressions around "null vs []" encoding on the response.
Example additions (outside the selected lines):
t.Run("Should create a customer without usage attribution", func(t *testing.T) { res, err := client.CreateCustomerWithResponse(ctx, api.CreateCustomerJSONRequestBody{ Name: "No Attribution", Currency: lo.ToPtr(api.CurrencyCode("USD")), }) require.NoError(t, err) require.Equal(t, 201, res.StatusCode(), "body: %s", res.Body) c := res.JSON201 require.NotNil(t, c) // If server chooses to omit when empty: assert.Nil(t, c.UsageAttribution) // If server chooses to always present but empty, use this instead: // require.NotNil(t, c.UsageAttribution) // assert.Equal(t, 0, len(c.UsageAttribution.SubjectKeys)) }) t.Run("Should accept empty subject keys", func(t *testing.T) { res, err := client.CreateCustomerWithResponse(ctx, api.CreateCustomerJSONRequestBody{ Name: "Empty Keys", Currency: lo.ToPtr(api.CurrencyCode("USD")), UsageAttribution: &api.CustomerUsageAttribution{ SubjectKeys: []string{}, }, }) require.NoError(t, err) require.Equal(t, 201, res.StatusCode(), "body: %s", res.Body) c := res.JSON201 require.NotNil(t, c) // See server behavior notes above })openmeter/customer/httpdriver/apimapping.go (2)
22-27: Optionality handled correctly; consider a tiny helper to dedupe mappingThe pointer gate for body.UsageAttribution is correct. You can remove duplication across create/replace by extracting a small helper:
Outside the selected lines:
func mapUsageAttribution(in *api.CustomerUsageAttribution) customer.CustomerUsageAttribution { if in == nil { return customer.CustomerUsageAttribution{} } // If you want to normalize nil -> empty slice for consistency: keys := in.SubjectKeys if keys == nil { keys = []string{} } return customer.CustomerUsageAttribution{SubjectKeys: keys} }Then here:
- var usageAttribution *customer.CustomerUsageAttribution - if body.UsageAttribution != nil { - usageAttribution = &customer.CustomerUsageAttribution{ - SubjectKeys: body.UsageAttribution.SubjectKeys, - } - } ... - UsageAttribution: lo.FromPtrOr(usageAttribution, customer.CustomerUsageAttribution{}), + UsageAttribution: mapUsageAttribution(body.UsageAttribution),Also applies to: 33-33
47-53: Mirror the helper in replace/update to avoid driftSame suggestion as create-path: use a shared mapper for UsageAttribution so both code paths stay in sync if normalization rules change later.
Apply the same change here:
- var usageAttribution *customer.CustomerUsageAttribution - if body.UsageAttribution != nil { - usageAttribution = &customer.CustomerUsageAttribution{ - SubjectKeys: body.UsageAttribution.SubjectKeys, - } - } ... - UsageAttribution: lo.FromPtrOr(usageAttribution, customer.CustomerUsageAttribution{}), + UsageAttribution: mapUsageAttribution(body.UsageAttribution),Also applies to: 58-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
api/client/javascript/src/client/schemas.ts(3 hunks)api/client/javascript/src/zod/index.ts(3 hunks)api/openapi.cloud.yaml(0 hunks)api/openapi.yaml(0 hunks)api/spec/src/customer/customer.tsp(1 hunks)e2e/productcatalog_test.go(4 hunks)openmeter/customer/httpdriver/apimapping.go(3 hunks)
💤 Files with no reviewable changes (2)
- api/openapi.yaml
- api/openapi.cloud.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/productcatalog_test.go (4)
api/client/javascript/src/client/schemas.ts (1)
CustomerUsageAttribution(10872-10873)api/api.gen.go (1)
CustomerUsageAttribution(2350-2353)openmeter/billing/invoice.go (1)
CustomerUsageAttribution(655-655)api/client/go/client.gen.go (1)
CustomerUsageAttribution(2180-2183)
openmeter/customer/httpdriver/apimapping.go (4)
api/api.gen.go (3)
CustomerUsageAttribution(2350-2353)Metadata(4487-4487)Annotations(1008-1008)openmeter/billing/invoice.go (1)
CustomerUsageAttribution(655-655)openmeter/customer/customer.go (2)
CustomerUsageAttribution(149-151)CustomerMutate(68-78)pkg/models/metadata.go (1)
Metadata(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
api/spec/src/customer/customer.tsp (1)
211-211: LGTM: usageAttribution is now optional as intendedThis aligns with the PR objective to make usage attribution optional across the API surface.
api/client/javascript/src/client/schemas.ts (2)
4113-4117: No unsafeusageAttributionreads found in this repo
I ran a grep across all.tsfiles for any direct property access onusageAttribution(e.g.usageAttribution.fooorusageAttribution.subjectKeys[0]) and found no occurrences. Since this file only contains generated type definitions and there’s no downstream consumption of those fields in this client library, there are no unsafe reads to guard against here. Feel free to ignore this suggestion.
4024-4028: No custom serializers detected; undefined fields will be omittedMarking
usageAttributionas optional is correct. I verified that inapi/client/javascript/src/clientthere are no customJSON.stringify,toJSON, orserialize*routines that would convertundefinedtonull, nor any explicitusageAttribution: nullassignments. By default,JSON.stringifyomits properties withundefinedvalues, so clients will not send this field unless it’s explicitly defined.e2e/productcatalog_test.go (3)
71-74: LGTM: pointer usage matches API/client changesConsistent with the new pointer-based UsageAttribution type.
105-108: LGTM: pointer usage matches API/client changesConsistent with the new pointer-based UsageAttribution type.
950-953: LGTM: pointer usage matches API/client changesConsistent with the new pointer-based UsageAttribution type.
| * Usage Attribution | ||
| * @description Mapping to attribute metered usage to the customer | ||
| */ | ||
| usageAttribution: components['schemas']['CustomerUsageAttribution'] | ||
| usageAttribution?: components['schemas']['CustomerUsageAttribution'] | ||
| /** | ||
| * Primary Email |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: usageAttribution made optional in Customer schema
The optional marker aligns with the PR goal and upstream OpenAPI changes. No issues in the JSDoc or typing. Heads-up: this is potentially a breaking change for TypeScript consumers that previously assumed the field is always present; consider a release note for SDK users to guard reads (optional chaining or defaults).
Run the script below to verify repo-wide consistency (TS client, Zod, OpenAPI). Expected: only optional occurrences in TS client, Zod uses .optional(), and OpenAPI no longer lists it under required.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) TS client: confirm exactly 3 optional occurrences in schemas.ts"
rg -n --type=ts $'\\busageAttribution\\?\\s*:' api/client/javascript/src/client/schemas.ts | tee /dev/stderr | wc -l
echo "2) TS client: detect any non-optional property declarations elsewhere (should be none)"
rg -n --type=ts $'\\busageAttribution\\s*:' api/client/javascript | rg -n -v '\\?\\s*:' || true
echo "3) Zod schema: ensure .optional() is applied"
rg -n -C2 $'usageAttribution\\s*:\\s*z\\..*\\.optional\\(\\)' api/client/javascript/src/zod/index.ts
echo "4) OpenAPI specs: eyeball that usageAttribution is not under required"
rg -n -C2 $'usageAttribution' api/openapi*.yamlLength of output: 373
To ensure we accurately verify the change across all relevant layers, let’s rerun the checks without relying on tee and capture everything directly:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Optional fields in TS client schemas.ts:"
rg -n --type=ts '\busageAttribution\?' api/client/javascript/src/client/schemas.ts
echo
echo "Count of optional occurrences:"
rg -c --type=ts '\busageAttribution\?' api/client/javascript/src/client/schemas.ts
echo
echo "2) Non-optional declarations in TS client schemas.ts (should be none):"
rg -n --type=ts '\busageAttribution\s*:' api/client/javascript/src/client/schemas.ts | rg -v '\?'
echo
echo "3) Project-wide non-optional occurrences (excluding the above file):"
rg -n --type=ts '\busageAttribution\s*:' api/client/javascript | rg -v '\?' || true
echo
echo "4) Zod schema should have .optional() on usageAttribution:"
rg -n -C2 'usageAttribution.*\.optional\(' api/client/javascript/src/zod/index.ts
echo
echo "5) OpenAPI spec – check if usageAttribution is NOT listed under any required arrays:"
echo " Required arrays around usageAttribution:"
rg -n -C2 'required:' api/openapi*.yaml
echo
echo " All usages of usageAttribution in specs:"
rg -n 'usageAttribution' api/openapi*.yamlLength of output: 1739
To complete the verification—especially ensuring Zod and OpenAPI are updated correctly—let’s recheck without exiting on missing matches:
🏁 Script executed:
#!/bin/bash
# Don’t exit on non-zero so we can see all results
set +e
echo "1) TS client non-optional occurrences in schemas.ts (should only be optional):"
rg -n --type=ts '\busageAttribution\s*:' api/client/javascript/src/client/schemas.ts
echo
echo "2) Zod schema – all usageAttribution entries:"
rg -n -C1 'usageAttribution:' api/client/javascript/src/zod/index.ts
echo
echo "3) Zod schema – look for .optional() on usageAttribution:"
rg -n 'usageAttribution.*\.optional\(' api/client/javascript/src/zod/index.ts || echo "→ No .optional() found"
echo
echo "4) OpenAPI spec – show required arrays around usageAttribution:"
rg -n -C2 'required:' api/openapi*.yaml
echo
echo "5) OpenAPI spec – all usageAttribution references:"
rg -n 'usageAttribution' api/openapi*.yamlLength of output: 271358
usageAttribution optionality not applied end-to-end; Zod schema and OpenAPI spec still treat it as required
TS client schemas.ts
- Line 2774:
usageAttribution: components['schemas']['CustomerUsageAttribution']remains non-optional; change tousageAttribution?: …to match lines 3921, 4027, 4116.
Zod schema (src/client/javascript/src/zod/index.ts)
- Around lines 6702, 6932, 12897: each
usageAttribution: zodmust be followed by.optional()(e.g.
usageAttribution: zod.object({ … }).optional()).
OpenAPI spec (api/openapi.yaml & openapi.cloud.yaml)
- In the
BillingInvoiceCustomerExtendedDetailsschema (api/openapi.yaml line 12919 and api/openapi.cloud.yaml around line 12812), removeusageAttributionfrom therequired:array so it’s truly optional.
These omissions will break TypeScript’s runtime validation and documentation consistency.
🤖 Prompt for AI Agents
In api/client/javascript/src/client/schemas.ts around lines 3918 and
specifically at the referenced TypeScript schema (also note line ~2774), change
the property declaration from a required usageAttribution:
components['schemas']['CustomerUsageAttribution'] to an optional
usageAttribution?: components['schemas']['CustomerUsageAttribution']; in
src/client/javascript/src/zod/index.ts at the listed spots (~lines 6702, 6932,
12897) append .optional() to each usageAttribution zod schema (e.g.,
usageAttribution: zod.object({...}).optional()); and in api/openapi.yaml and
api/openapi.cloud.yaml (around lines 12919 and ~12812 respectively) remove
usageAttribution from the required array of the
BillingInvoiceCustomerExtendedDetails schema so the field is no longer listed as
required. Ensure all three changes are consistent so TypeScript types, runtime
zod validation, and OpenAPI documentation mark usageAttribution as optional.
| .describe( | ||
| 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.' | ||
| ) | ||
| .optional() | ||
| .describe('Mapping to attribute metered usage to the customer'), | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update doc to match max=1 and clarify update semantics (omit vs clear)
The same “multiple subjects” wording appears here while the schema caps at 1. Also, since this is an update model and the field is now optional, it’s worth documenting the difference between omitting the field (no-op) and sending an empty array (clear).
Apply this doc tweak (preferably at the source spec so codegen picks it up everywhere):
- .describe(
- 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.'
- )
+ .describe(
+ 'Mapping to attribute metered usage to the customer.\nA customer can be attributed to at most one subject,\nand a subject can belong to at most one customer.'
+ )
@@
- .describe('Mapping to attribute metered usage to the customer'),
+ .describe('Mapping to attribute metered usage to the customer (optional; omit to leave unchanged, send an empty array to clear any existing attribution)'),To ensure client/server semantics are aligned (undefined = no change, [] = clear), run:
Expected:
- Server update mapping treats omitted UsageAttribution as no-op and empty slice as clear.
- Max is consistently 1 across spec/code.
- No remaining
.min(1)on subjectKeys in Zod.
🏁 Script executed:
#!/bin/bash
# Inspect Go API mapping and tests for clearing vs no-op semantics
rg -nC3 -g '!**/node_modules/**' -P 'UsageAttribution\b' openmeter/customer/httpdriver apis e2e
# Confirm subject keys max is 1 across sources
rg -nC1 -P 'SubjectKeys.*(maxItems|Max)\s*[:=]\s*1' -S api
# Check JS client zod for any lingering `.min(1)` on subjectKeys
rg -n -C1 -P 'subjectKeys:\s*zod\s*\.\s*array\([^)]*\)\s*\.min\(1\)' api/client/javascript/src/zodLength of output: 6323
Fix UsageAttribution update semantics & docs
The current update mapping in openmeter/customer/httpdriver/apimapping.go treats an omitted UsageAttribution as a clear operation (it defaults to an empty struct), whereas an explicit empty slice should be the only way to clear existing attribution. We need to:
• Change the update logic so that:
body.UsageAttribution == nil⇒ no-op (preserve the customer’s existingUsageAttribution)body.UsageAttribution != nil && len(body.UsageAttribution.SubjectKeys) == 0⇒ clear any existing attributionbody.UsageAttribution != nil && len(body.UsageAttribution.SubjectKeys) == 1⇒ set to the new subject key
• Adjust the Zod schema documentation (in api/client/javascript/src/zod/index.ts around line 6939) to:
- Correctly state “A customer can be attributed to at most one subject…” instead of the old “One customer can have multiple subjects…”
- Clarify the update semantics:
omit ⇒ no change
[] ⇒ clear existing attribution
• Ensure the OpenAPI/source spec is updated so codegen picks up this description everywhere.
Locations to update:
- openmeter/customer/httpdriver/apimapping.go (update-customer mapping logic)
- api/client/javascript/src/zod/index.ts (around the existing
.describe(...)forsubjectKeys)
Suggested Zod doc diff:
- .describe(
- 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.'
- )
+ .describe(
+ 'Mapping to attribute metered usage to the customer.\nA customer can be attributed to at most one subject,\nand a subject can belong to at most one customer.'
+ )
@@
- .describe('Mapping to attribute metered usage to the customer'),
+ .describe(
+ 'Mapping to attribute metered usage to the customer ' +
+ '(optional; omit to leave unchanged, send an empty array to clear any existing attribution)'
+ ),And in apimapping.go, switch from lo.FromPtrOr(usageAttribution, customer.CustomerUsageAttribution{}) to logic that preserves the prior value when body.UsageAttribution is nil.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .describe( | |
| 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.' | |
| ) | |
| .optional() | |
| .describe('Mapping to attribute metered usage to the customer'), | |
| }) | |
| .describe( | |
| 'Mapping to attribute metered usage to the customer.\nA customer can be attributed to at most one subject,\nand a subject can belong to at most one customer.' | |
| ) | |
| .optional() | |
| .describe( | |
| 'Mapping to attribute metered usage to the customer ' + | |
| '(optional; omit to leave unchanged, send an empty array to clear any existing attribution)' | |
| ), | |
| }) |
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 6939-6944, update the
.describe(...) text to state that "A customer can be attributed to at most one
subject (a subject can only belong to one customer)" and add concise update
semantics: "omit ⇒ no change; [] ⇒ clear existing attribution; [subjectKey] ⇒
set to the provided subject", then ensure the OpenAPI/source spec description is
updated so generated clients inherit this wording; additionally, in
openmeter/customer/httpdriver/apimapping.go replace the current
lo.FromPtrOr(usageAttribution, customer.CustomerUsageAttribution{}) behavior
with explicit logic: if body.UsageAttribution == nil => preserve existing
customer.UsageAttribution (no-op); if body.UsageAttribution != nil &&
len(body.UsageAttribution.SubjectKeys) == 0 => clear existing attribution; if
body.UsageAttribution != nil && len(body.UsageAttribution.SubjectKeys) == 1 =>
set to that subject key, and ensure any other lengths are validated/rejected as
appropriate.
| .describe( | ||
| 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.' | ||
| ) | ||
| .optional() | ||
| .describe('Mapping to attribute metered usage to the customer'), |
There was a problem hiding this comment.
Stripe checkout body: fix doc to reflect single-subject cap and optionality
Same doc mismatch here; also mark the final description as optional to match the schema.
Apply:
- .describe(
- 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.'
- )
+ .describe(
+ 'Mapping to attribute metered usage to the customer.\nA customer can be attributed to at most one subject,\nand a subject can belong to at most one customer.'
+ )
@@
- .describe('Mapping to attribute metered usage to the customer'),
+ .describe('Mapping to attribute metered usage to the customer (optional)'),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .describe( | |
| 'Mapping to attribute metered usage to the customer.\nOne customer can have multiple subjects,\nbut one subject can only belong to one customer.' | |
| ) | |
| .optional() | |
| .describe('Mapping to attribute metered usage to the customer'), | |
| .describe( | |
| 'Mapping to attribute metered usage to the customer.\nA customer can be attributed to at most one subject,\nand a subject can belong to at most one customer.' | |
| ) | |
| .optional() | |
| .describe('Mapping to attribute metered usage to the customer (optional)'), |
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 12908 to 12912, the final
.describe(...) text is inconsistent with the schema (it repeats mapping text and
doesn't reflect that a subject can only belong to one customer) and the field is
missing .optional(); update the description to clearly state the
single-subject-per-customer constraint and that the field is optional, and add
.optional() to the final chained call so the schema and docs match.
| */ | ||
| @summary("Usage Attribution") | ||
| usageAttribution: CustomerUsageAttribution; | ||
| usageAttribution?: CustomerUsageAttribution; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify omit vs empty semantics for updates (replace) and clearing behavior
With usageAttribution? optional and subjectKeys allowing empty arrays, please confirm/document that:
- Omitting
usageAttributionon replace/update keeps the existing value unchanged. - Providing
usageAttribution: { subjectKeys: [] }explicitly clears attribution.
If that’s the intended contract, consider stating it in model docs to avoid client confusion. Script below helps verify server mapping and test coverage.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate API mapping for create/replace update:"
fd -t f apimapping.go
echo
# Inspect mapping functions and how nil vs empty is handled
for f in $(fd -t f apimapping.go); do
echo "=== $f ==="
rg -n -C4 'MapCustomer(Create|ReplaceUpdate)' "$f" || true
rg -n -C3 'UsageAttribution' "$f" || true
done
echo
echo "Check e2e tests around UsageAttribution pointer/clearing semantics:"
rg -n -C3 'CreateCustomerJSONRequestBody' --type go || true
rg -n -C3 'UsageAttribution.*\[\]' --type go || true # explicit empty array
rg -n -C3 'UsageAttribution: nil' --type go || true # omitted / nil pointer
echo
echo "Confirm OpenAPI required lists do not include usageAttribution anymore:"
rg -n -C2 'usageAttribution' api/openapi*.yaml || trueLength of output: 13951
Update usageAttribution replace semantics, spec & tests
– Mapping (openmeter/customer/httpdriver/apimapping.go lines 22–33 & 47–59) always calls
var usageAttribution *customer.CustomerUsageAttribution
if body.UsageAttribution != nil {
usageAttribution = &customer.CustomerUsageAttribution{ SubjectKeys: body.UsageAttribution.SubjectKeys }
}
…
UsageAttribution: lo.FromPtrOr(usageAttribution, customer.CustomerUsageAttribution{}),which means any ReplaceUpdate will reset usageAttribution to the zero‐value (nil slice) if omitted, and to an empty slice if explicitly passed as { subjectKeys: [] }. There is no “preserve existing” path.
– OpenAPI spec still marks usageAttribution as required in both api/openapi.yaml (around line 12922) and api/openapi.cloud.yaml (around line 12814). Remove it from the required lists so that ReplaceUpdate truly allows omission.
– Update model docs (api/spec/src/customer/customer.tsp, around line 211) to state that Replace/PUT semantics always replace the existing attribution, and that:
• Omitting usageAttribution clears it to zero (no subjectKeys)
• Passing usageAttribution: { subjectKeys: [] } also clears it (empty‐slice)
– Add test coverage:
• In e2e/productcatalog_test.go, add a ReplaceUpdate case with no usageAttribution and assert that after update the customer’s usageAttribution.subjectKeys is empty/nil.
• Add another test with usageAttribution: { subjectKeys: [] } to verify explicit‐empty clears as expected.
tothandras
force-pushed
the
feat/api-customer-usage-attribution
branch
from
43aad85 to
419be3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
openmeter/customer/httpdriver/apimapping.go (3)
22-27: Copy SubjectKeys to avoid aliasing request memoryWhen mapping input to domain, copy the slice so later mutations to the API struct (or reuse by middleware) cannot leak into domain state.
Apply this diff:
- SubjectKeys: body.UsageAttribution.SubjectKeys, + // copy to avoid aliasing the request slice + SubjectKeys: append([]string(nil), body.UsageAttribution.SubjectKeys...),
47-52: DRY the mapping and copy the slice (same as create path)This block duplicates the create-path logic. Consider extracting a small helper to keep behavior consistent and to copy the slice here as well.
Apply this diff to copy the slice:
- SubjectKeys: body.UsageAttribution.SubjectKeys, + // copy to avoid aliasing the request slice + SubjectKeys: append([]string(nil), body.UsageAttribution.SubjectKeys...),Optionally, introduce a helper (outside this hunk) and reuse in both places:
// helper (suggested) func mapUsageAttributionFromAPI(src *api.CustomerUsageAttribution) (customer.CustomerUsageAttribution, bool) { if src == nil { return customer.CustomerUsageAttribution{}, false } keys := src.SubjectKeys // normalize nil to empty if you prefer stricter domain invariants // if keys == nil { keys = []string{} } return customer.CustomerUsageAttribution{ SubjectKeys: append([]string(nil), keys...), }, true }
141-146: Good: omit usageAttribution when empty; tighten comment and copy sliceThis resolves the previous “subjectKeys: null” risk by omitting the whole object when there are no keys. Two small tweaks:
- The comment mentions “ensure non-nil slice” but we’re already guarding on len>0; reword for precision.
- Copy the slice to avoid aliasing domain memory.
Apply this diff:
- // Only include UsageAttribution when there are subject keys and ensure non-nil slice + // Only include usageAttribution when there are subject keys if len(c.UsageAttribution.SubjectKeys) > 0 { apiCustomer.UsageAttribution = &api.CustomerUsageAttribution{ - SubjectKeys: c.UsageAttribution.SubjectKeys, + // copy to avoid aliasing the domain slice + SubjectKeys: append([]string(nil), c.UsageAttribution.SubjectKeys...), } }If you’d like, I can add an e2e/assertion ensuring responses never contain
"subjectKeys": nulland thatusageAttributionis omitted when empty. Want me to draft that?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
openmeter/customer/httpdriver/apimapping.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openmeter/customer/httpdriver/apimapping.go (4)
api/api.gen.go (3)
CustomerUsageAttribution(2350-2353)Metadata(4487-4487)Annotations(1008-1008)openmeter/billing/invoice.go (1)
CustomerUsageAttribution(655-655)api/client/go/client.gen.go (3)
CustomerUsageAttribution(2180-2183)Metadata(4075-4075)Annotations(923-923)openmeter/customer/customer.go (2)
CustomerUsageAttribution(149-151)CustomerMutate(68-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
openmeter/customer/httpdriver/apimapping.go (3)
33-33: Create: zero-value default for UsageAttribution is appropriateDefaulting to an empty value when the field is omitted keeps domain mutate non-nil and aligns with create semantics.
129-139: API customer base-field mapping looks goodThe base fields (ids, timestamps, metadata/annotations, etc.) are mapped correctly and defensively via helpers.
58-58: No changes needed: UpdateCustomer uses PUT with replace semanticsI verified that the
UpdateCustomerendpoint is wired as a PUT, not PATCH, so omittingusageAttributionshould indeed clear it under full-replace semantics. Your use oflo.FromPtrOr(usageAttribution, customer.CustomerUsageAttribution{})is therefore correct and intentional.• In openmeter/server/router/customer.go lines 40–43, the route is defined as
(PUT /api/v1/customer/customers/{customerId})pointing toUpdateCustomer
• In openmeter/customer/httpdriver/customer.go line 210,CustomerMutateis built viaMapCustomerReplaceUpdateNo refactoring required.
Summary by CodeRabbit