feat: v3 invoice update endpoint#4639
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds invoice update support across the TypeSpec contracts, generated JavaScript SDK, v3 Go handlers, and end-to-end tests. It also adds a ChangesInvoice Update Feature
Set Membership Helper
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant UpdateBillingInvoice
participant BillingService
Client->>Server: PUT /api/v3/openmeter/billing/invoices/{invoiceId}
Server->>UpdateBillingInvoice: delegate request
UpdateBillingInvoice->>BillingService: GetInvoiceById
UpdateBillingInvoice->>BillingService: UpdateStandardInvoice
BillingService-->>UpdateBillingInvoice: updated invoice
UpdateBillingInvoice-->>Server: JSON response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
borosr
added
kind/feature
borosr
force-pushed
the
feat/v3-invoice-api-update
branch
from
0199f3c to
565037a
Compare
Greptile SummaryThis PR introduces a
Confidence Score: 4/5Safe to merge after fixing the stale DueAfter value that persists when switching from send_invoice to charge_automatically The merge logic in mergeInvoiceWorkflowFromAPI correctly handles send_invoice and resets DueAfter, but forgets to clear it when the caller switches to charge_automatically. The stale duration then surfaces in the response under invoicing.due_after, returning incorrect state to clients. Everything else — immutable field preservation, tombstoning of omitted lines, duplicate-ID detection, and the 404 guard for gathering invoices — looks correct and is well-tested. api/v3/handlers/billinginvoices/convert.go — the charge_automatically branch in mergeInvoiceWorkflowFromAPI needs to reset DueAfter Important Files Changed
|
![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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/spec/packages/aip/src/shared/resource.tsp (1)
77-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
ResourceReference<T>.idread/create-only unless you want update payloads to grow across all consumers. This also reaches non-invoice update shapes likeProductCatalog.RateCard.featureandProductCatalog.RateCardTaxConfig.code, soUpdateResourceReferencewill show up anywhere those refs are editable.🤖 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/resource.tsp` around lines 77 - 83, The `ResourceReference<T>.id` field in `ResourceReference` is currently visible for updates, which causes `UpdateResourceReference`-based payloads to expand across all consumers. Update the `@visibility` on `id` to keep it read/create-only, and ensure any generated update shapes for references like `ProductCatalog.RateCard.feature` and `ProductCatalog.RateCardTaxConfig.code` do not include `id` as an editable field.
🧹 Nitpick comments (5)
e2e/billinginvoices_v3_test.go (2)
887-914: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMinor:
TaxIdisn't preserved by the supplier mapping.Given the documented "replace, not merge" semantics for
UpdateSupplier, omittingTaxIdhere would silently clear it if the supplier ever has one set. Currently harmless since the test profile's supplier has no tax ID, but worth a note for future-proofing if this helper gets reused with a supplier that has tax data.🤖 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/billinginvoices_v3_test.go` around lines 887 - 914, The supplier round-trip helper in updateSupplierFromInvoiceParty drops TaxId, which can unintentionally clear an existing tax ID because UpdateSupplier replaces the full object. Update updateSupplierFromInvoiceParty to copy the invoice party’s TaxId into the returned apiv3.UpdateSupplier alongside Id and Name, and keep the address mapping unchanged.
719-745: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBeef up the success-path assertions to cover the actual mutable fields.
The "Should update the standard invoice via v3 PUT" case only asserts
IdandDescriptionon the response (lines 743-744), even though the request also carriesCustomer,Supplier, and an always-emptyWorkflow: apiv3.UpdateInvoiceWorkflowSettings{}. Since workflow/supplier/customer merging is a core part of what this PR ships, a regression inmergeStandardInvoiceFromAPI's handling of those fields (e.g., accidentally resetting workflow settings when the request submits a zero-value struct) wouldn't be caught by this test. Consider asserting that the returned invoice's supplier/customer match what was sent, and that workflow settings come back as expected (even if that's "reset to defaults" — pin down the expected value).As per path instructions,
"Make sure the tests are comprehensive and cover the changes."— supplier/customer/workflow merging is one of the changes this test is meant to exercise.✅ Example additional assertions
stdInv, err := updatedInv.AsBillingInvoiceStandard() require.NoError(t, err) assert.Equal(t, invoice.Id, stdInv.Id) assert.Equal(t, newDescription, lo.FromPtr(stdInv.Description)) + assert.Equal(t, *invoice.Customer.Name, stdInv.Customer.Name) + assert.Equal(t, updateSupplierFromInvoiceParty(invoice.Supplier).Name, stdInv.Supplier.Name)🤖 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/billinginvoices_v3_test.go` around lines 719 - 745, The v3 PUT success-path test in the billing invoice update case only checks Id and Description, so it won’t catch regressions in mutable fields handled by mergeStandardInvoiceFromAPI. Extend the assertions in the “Should update the standard invoice via v3 PUT” test to verify the returned standard invoice’s Customer, Supplier, and WorkflowSettings match the request/expected merged state, using the existing updateReq, updatedInv, and AsBillingInvoiceStandard symbols to locate the assertions.api/spec/packages/aip/src/shared/request.tsp (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify
UpdateRequestNested's full-object nested-replace semantics.Unlike
UpdateRequest<T>(which wraps fields inOptionalProperties),UpdateRequestNested<T>does not — so nested sub-object fields with defaults (e.g.auto_advance,draft_periodon invoicing settings) become required in the generated update payload. That means updating a nested section likeworkflowrequires supplying the whole sub-object, not a partial patch — a meaningful difference from the top-levelUpdateRequestpattern that isn't documented anywhere on this model.Worth a short docstring note explaining this full-replace-of-nested-section behavior so future readers don't assume partial-patch semantics apply here too.
As per coding guidelines: "Add docstrings to domain helpers when the name compresses important business semantics, and describe the observable business contract rather than implementation mechanics."
🤖 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 34 - 42, Add a short docstring note on UpdateRequestNested<T> clarifying that it is a full-replace update shape for nested sections, not a partial patch like UpdateRequest<T>. Explain that because it omits OptionalProperties, callers updating a nested object (for example via workflow) must supply the complete sub-object including any defaulted fields such as auto_advance and draft_period. Keep the note alongside the UpdateRequestNested model and reference its relationship to FilterVisibility/DefaultKeyVisibility so readers understand the contract.Source: Coding guidelines
api/spec/packages/aip-client-javascript/src/models/schemas.ts (2)
1697-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGeneric
updateResourceReferencecarries a hardcoded "TaxCode reference." description.Given the stack outline calls this out as a reusable
ResourceReferencevisibility helper, baking a TaxCode-specific description into the shared generated type is misleading if it's ever reused for a different resource kind (feature key, etc.). Worth checkingresource.tsp/request.tsp(not in this batch) for whether each instantiation should carry its own contextual doc via the property, not the shared model.🤖 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-client-javascript/src/models/schemas.ts` around lines 1697 - 1702, The shared updateResourceReference schema is incorrectly hardcoded with a TaxCode-specific description, which makes this reusable helper misleading. Update the schema in updateResourceReference so it uses a generic description or no description at all, and move any resource-specific wording to the instantiating property/documentation in resource.tsp or request.tsp where the reference is actually used.
4144-4145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
array/array_2are pretty opaque export names.
export const array = z.array(updatePriceTier)and laterarray_2 = z.array(updateInvoiceLine)(line 5554) are exported public symbols with no descriptive naming — likely a symptom of the TypeSpec models lacking@friendlyNameon these anonymous array properties. Not a functional bug, but it's rough for anyone consuming the generated SDK directly and risks future name collisions as more anonymous arrays get generated.🤖 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-client-javascript/src/models/schemas.ts` around lines 4144 - 4145, The generated public export name is opaque here, so update the schema generation around the anonymous array model used by array and array_2 to produce descriptive symbols instead of generic names. Add or propagate a friendly name for the underlying TypeSpec array properties so the exported z.array(...) constants in schemas.ts get stable, meaningful identifiers, and make the same adjustment for both the updatePriceTier and updateInvoiceLine cases to avoid future collisions.
🤖 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-client-javascript/src/models/types.ts`:
- Around line 3789-3790: The generated types file has a duplicate top-level
Array alias, causing a TypeScript TS2300 conflict. Update the upstream
TypeSpec/generator source for the affected model aliases (the Array exports for
UpdatePriceTier and UpdateInvoiceStandardLine) so only one unique name is
emitted, and regenerate the client instead of editing types.ts directly.
In `@api/spec/packages/aip/src/invoices/invoice.tsp`:
- Around line 344-354: Make the `id` fields read-only in update payloads because
the backend ignores them anyway: in `InvoiceCustomer` and `Supplier`, remove
`Lifecycle.Update` from the `id` visibility so `UpdateInvoiceCustomer` and
`UpdateSupplier` no longer expose a mutable `id`. Update the AIP models around
`InvoiceCustomer` and `Supplier` to keep `id` only under read visibility,
matching the behavior of `mergeInvoiceCustomerFromAPI` and
`mergeInvoiceSupplierFromAPI` that preserve the existing ID regardless of
request input.
In `@api/v3/handlers/billinginvoices/convert.go`:
- Around line 574-577: The error from
AsUpdateBillingWorkflowPaymentSendInvoiceSettings in the billing invoice
conversion flow is being returned as a generic error, which makes malformed
request payloads surface like a server failure. Update the error handling in the
same pattern used elsewhere in this function by wrapping that failure as
billing.ValidationError so client-side bad input still maps to a 400. Keep the
change local to the payment send-invoice settings branch in convert.go and
preserve the existing error context text.
- Around line 614-643: The duplicate invoice line ID issue in the
line-processing loop lets the same existing line be merged and appended more
than once because mergeStandardLineFromAPI mutates a shared billing.StandardLine
and the current checks only validate per-line state. Add a duplicate-ID
rejection step in NewStandardInvoiceLines before this loop (or during iteration)
using the existing linesByID/foundLines flow so repeated non-empty stdLine.Id
values are detected and վերադարձ an error instead of appending duplicates; keep
the change localized around NewStandardInvoiceLines, mergeStandardLineFromAPI,
and StandardInvoiceLines.Validate.
---
Outside diff comments:
In `@api/spec/packages/aip/src/shared/resource.tsp`:
- Around line 77-83: The `ResourceReference<T>.id` field in `ResourceReference`
is currently visible for updates, which causes `UpdateResourceReference`-based
payloads to expand across all consumers. Update the `@visibility` on `id` to
keep it read/create-only, and ensure any generated update shapes for references
like `ProductCatalog.RateCard.feature` and
`ProductCatalog.RateCardTaxConfig.code` do not include `id` as an editable
field.
---
Nitpick comments:
In `@api/spec/packages/aip-client-javascript/src/models/schemas.ts`:
- Around line 1697-1702: The shared updateResourceReference schema is
incorrectly hardcoded with a TaxCode-specific description, which makes this
reusable helper misleading. Update the schema in updateResourceReference so it
uses a generic description or no description at all, and move any
resource-specific wording to the instantiating property/documentation in
resource.tsp or request.tsp where the reference is actually used.
- Around line 4144-4145: The generated public export name is opaque here, so
update the schema generation around the anonymous array model used by array and
array_2 to produce descriptive symbols instead of generic names. Add or
propagate a friendly name for the underlying TypeSpec array properties so the
exported z.array(...) constants in schemas.ts get stable, meaningful
identifiers, and make the same adjustment for both the updatePriceTier and
updateInvoiceLine cases to avoid future collisions.
In `@api/spec/packages/aip/src/shared/request.tsp`:
- Around line 34-42: Add a short docstring note on UpdateRequestNested<T>
clarifying that it is a full-replace update shape for nested sections, not a
partial patch like UpdateRequest<T>. Explain that because it omits
OptionalProperties, callers updating a nested object (for example via workflow)
must supply the complete sub-object including any defaulted fields such as
auto_advance and draft_period. Keep the note alongside the UpdateRequestNested
model and reference its relationship to FilterVisibility/DefaultKeyVisibility so
readers understand the contract.
In `@e2e/billinginvoices_v3_test.go`:
- Around line 887-914: The supplier round-trip helper in
updateSupplierFromInvoiceParty drops TaxId, which can unintentionally clear an
existing tax ID because UpdateSupplier replaces the full object. Update
updateSupplierFromInvoiceParty to copy the invoice party’s TaxId into the
returned apiv3.UpdateSupplier alongside Id and Name, and keep the address
mapping unchanged.
- Around line 719-745: The v3 PUT success-path test in the billing invoice
update case only checks Id and Description, so it won’t catch regressions in
mutable fields handled by mergeStandardInvoiceFromAPI. Extend the assertions in
the “Should update the standard invoice via v3 PUT” test to verify the returned
standard invoice’s Customer, Supplier, and WorkflowSettings match the
request/expected merged state, using the existing updateReq, updatedInv, and
AsBillingInvoiceStandard symbols to locate the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62acc145-d17e-4b88-8e8d-3473519fafc5
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (19)
api/spec/packages/aip-client-javascript/README.mdapi/spec/packages/aip-client-javascript/src/funcs/invoices.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/invoices.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/src/sdk/invoices.tsapi/spec/packages/aip/src/invoices/invoice.tspapi/spec/packages/aip/src/invoices/operations.tspapi/spec/packages/aip/src/shared/request.tspapi/spec/packages/aip/src/shared/resource.tspapi/v3/api.gen.goapi/v3/handlers/billinginvoices/convert.goapi/v3/handlers/billinginvoices/convert_test.goapi/v3/handlers/billinginvoices/handler.goapi/v3/handlers/billinginvoices/update.goapi/v3/server/routes.goe2e/billinginvoices_v3_test.goe2e/v3helpers_test.go
| export type Array = UpdatePriceTier[] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm both duplicate "export type Array" occurrences and check for any others in the file
rg -n '^export type Array' api/spec/packages/aip-client-javascript/src/models/types.tsRepository: openmeterio/openmeter
Length of output: 255
Rename one of the generated Array aliases upstream — api/spec/packages/aip-client-javascript/src/models/types.ts has two top-level export type Array declarations (UpdatePriceTier[] and UpdateInvoiceStandardLine[]), so TypeScript will hit TS2300. Since this is generator output, fix the TypeSpec source rather than hand-editing the generated file.
🤖 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-client-javascript/src/models/types.ts` around lines
3789 - 3790, The generated types file has a duplicate top-level Array alias,
causing a TypeScript TS2300 conflict. Update the upstream TypeSpec/generator
source for the affected model aliases (the Array exports for UpdatePriceTier and
UpdateInvoiceStandardLine) so only one unique name is emitted, and regenerate
the client instead of editing types.ts directly.
Source: Path instructions
There was a problem hiding this comment.
@tothandras should I do something with this?
There was a problem hiding this comment.
@borosr Yeah, this one's worth a fix — since both Array aliases are auto-generated from anonymous array types in the TypeSpec source, they collide once compiled. Since api/spec/packages/aip-client-javascript/** is generator output, the fix needs to happen upstream in the .tsp files (likely api/spec/packages/aip/src/invoices/invoice.tsp, where the tiers: UpdatePriceTier[] and lines: UpdateInvoiceStandardLine[] properties are defined), not directly in types.ts.
The usual way to dodge this with the generator is to give the array type an explicit named alias in TypeSpec (e.g. model UpdatePriceTierList is Array<UpdatePriceTier>; or similar) instead of leaving it as an inline T[], so the generator emits a unique name instead of falling back to the generic Array label for both. Once that's done, a generate run should produce non-colliding names in types.ts.
Worth flagging to @tothandras for a look since it touches the TypeSpec generator config/pattern rather than just this PR's Go code.
no rush, just don't want a TS2300 surprise sneaking into a build 🐇
| @withVisibility(Lifecycle.Read, Lifecycle.Update) | ||
| model InvoiceCustomer { | ||
| ...PickProperties< | ||
| Customers.Customer, | ||
| "id" | "name" | "usage_attribution" | "billing_address" | ||
| "name" | "usage_attribution" | "billing_address" | ||
| >; | ||
|
|
||
| #suppress "@openmeter/api-spec-aip/doc-decorator" "shared model" | ||
| @visibility(Lifecycle.Read, Lifecycle.Update) | ||
| id: Shared.ULID; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update-visible id fields that the backend silently ignores.
Both InvoiceCustomer.id (required) and Supplier.id (optional) are marked Lifecycle.Update, so they'll show up in UpdateInvoiceCustomer/UpdateSupplier request bodies. But per the Go conversion tests, the backend explicitly discards whatever value is sent here and keeps the existing ID: mergeInvoiceCustomerFromAPI and mergeInvoiceSupplierFromAPI preserve CustomerID/ID regardless of the payload's id.
That's a confusing contract — especially for InvoiceCustomer.id, which is required in the update body, forcing every caller to supply a value that has zero effect. Worth making these Read-only in the update view (drop Lifecycle.Update from id) so the generated types don't imply the field is mutable.
🔧 Proposed fix
`@friendlyName`("BillingInvoiceCustomer")
`@withVisibility`(Lifecycle.Read, Lifecycle.Update)
model InvoiceCustomer {
...PickProperties<
Customers.Customer,
"name" | "usage_attribution" | "billing_address"
>;
`#suppress` "`@openmeter/api-spec-aip/doc-decorator`" "shared model"
- `@visibility`(Lifecycle.Read, Lifecycle.Update)
+ `@visibility`(Lifecycle.Read)
id: Shared.ULID; `@friendlyName`("BillingSupplier")
`@withVisibility`(Lifecycle.Read, Lifecycle.Update)
model Supplier {
...OmitProperties<BillingParty, "id" | "key">;
/**
* Unique identifier for the party.
*/
- `@visibility`(Lifecycle.Read, Lifecycle.Update)
+ `@visibility`(Lifecycle.Read)
id?: string;
}Also applies to: 373-382
🤖 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/invoices/invoice.tsp` around lines 344 - 354, Make
the `id` fields read-only in update payloads because the backend ignores them
anyway: in `InvoiceCustomer` and `Supplier`, remove `Lifecycle.Update` from the
`id` visibility so `UpdateInvoiceCustomer` and `UpdateSupplier` no longer expose
a mutable `id`. Update the AIP models around `InvoiceCustomer` and `Supplier` to
keep `id` only under read visibility, matching the behavior of
`mergeInvoiceCustomerFromAPI` and `mergeInvoiceSupplierFromAPI` that preserve
the existing ID regardless of request input.
There was a problem hiding this comment.
@tothandras I needed this for the update, should I do it differently?
There was a problem hiding this comment.
@borosr Good question! I think the original suggestion still stands as the cleanest fix here.
The key difference is between the lines collection and the singleton customer/supplier snapshots:
- For
InvoiceStandardLine.id, keepingLifecycle.Updatemakes sense because the backend actually uses it — to match against existing lines, decide whether to create a new one, or delete lines omitted from the request. - For
InvoiceCustomer.idandSupplier.id, there's nothing to match against — there's only one customer/supplier snapshot per invoice, andmergeInvoiceCustomerFromAPI/mergeInvoiceSupplierFromAPIalways keep the existing ID no matter what's sent. So exposingidas updatable there just invites confusion (and forces callers to send a required-but-ignored field forInvoiceCustomer).
So I'd go with dropping Lifecycle.Update from those two id fields (keep Lifecycle.Read only), while leaving InvoiceStandardLine.id as-is since that one genuinely drives update behavior. That keeps the generated SDK types honest about what's actually mutable.
`@friendlyName`("BillingInvoiceCustomer")
`@withVisibility`(Lifecycle.Read, Lifecycle.Update)
model InvoiceCustomer {
...PickProperties<
Customers.Customer,
"name" | "usage_attribution" | "billing_address"
>;
`#suppress` "`@openmeter/api-spec-aip/doc-decorator`" "shared model"
- `@visibility`(Lifecycle.Read, Lifecycle.Update)
+ `@visibility`(Lifecycle.Read)
id: Shared.ULID; `@friendlyName`("BillingSupplier")
`@withVisibility`(Lifecycle.Read, Lifecycle.Update)
model Supplier {
...OmitProperties<BillingParty, "id" | "key">;
/**
* Unique identifier for the party.
*/
- `@visibility`(Lifecycle.Read, Lifecycle.Update)
+ `@visibility`(Lifecycle.Read)
id?: string;
}Let's see what @tothandras thinks too, but this should keep the spec in parity with the actual merge behavior without losing anything you need for the update flow.
🧠 Learnings used
Learnt from: tothandras
Repo: openmeterio/openmeter PR: 3859
File: api/spec/src/v3/currencies/cost-basis/cost_basis.tsp:21-22
Timestamp: 2026-02-19T10:10:51.323Z
Learning: In TypeSpec API specs, prefer using Shared.Numeric for financial values (e.g., exchange rates, amounts, prices) instead of native numeric types like decimal128 or float64. Shared.Numeric is a project-standard string-based scalar with arbitrary precision defined in api/spec/src/v3/shared/properties.tsp. Apply this guideline to relevant cost/spec fields (e.g., in api/spec/src/v3/currencies/cost-basis/cost_basis.tsp) and, more broadly, to all TypeSpec files under api/spec to ensure consistent precision handling and serialization.
There was a problem hiding this comment.
I checked this change, but the result is not identical, so I won't apply these changes.
There was a problem hiding this comment.
@borosr Totally fair — thanks for actually testing it out rather than just taking the suggestion at face value. If you ever want to dig into why the output diverges (e.g., something in the OpenAPI snapshot or generated SDK types), feel free to loop me in with more details and I'm happy to take another look. Otherwise, I'll leave it as-is — your call here.
(=^･ω･^=)
🧠 Learnings used
Learnt from: tothandras
Repo: openmeterio/openmeter PR: 3859
File: api/spec/src/v3/currencies/cost-basis/cost_basis.tsp:21-22
Timestamp: 2026-02-19T10:10:51.323Z
Learning: In TypeSpec API specs, prefer using Shared.Numeric for financial values (e.g., exchange rates, amounts, prices) instead of native numeric types like decimal128 or float64. Shared.Numeric is a project-standard string-based scalar with arbitrary precision defined in api/spec/src/v3/shared/properties.tsp. Apply this guideline to relevant cost/spec fields (e.g., in api/spec/src/v3/currencies/cost-basis/cost_basis.tsp) and, more broadly, to all TypeSpec files under api/spec to ensure consistent precision handling and serialization.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/set/set.go (1)
36-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
RLockinstead ofLockfor a read-only check.
Hasonly reads the map, so grabbing the write lock is heavier than it needs to be — unnecessarily blocks concurrent readers.IsEmptyright below already does this correctly withRLock/RUnlock, so let's stay consistent.⚡ Proposed fix
func (s *Set[T]) Has(item T) bool { - s.mu.Lock() - defer s.mu.Unlock() + s.mu.RLock() + defer s.mu.RUnlock() _, exists := s.content[item] return exists }🤖 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 `@pkg/set/set.go` around lines 36 - 43, The Has method on Set[T] is performing a read-only map lookup while taking the exclusive lock, which blocks other readers unnecessarily. Update Has to use the read lock path instead, matching the IsEmpty method’s RLock/RUnlock pattern, and keep the existing mutex-protected access to s.content so concurrent reads remain safe.Source: Path instructions
🤖 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.
Nitpick comments:
In `@pkg/set/set.go`:
- Around line 36-43: The Has method on Set[T] is performing a read-only map
lookup while taking the exclusive lock, which blocks other readers
unnecessarily. Update Has to use the read lock path instead, matching the
IsEmpty method’s RLock/RUnlock pattern, and keep the existing mutex-protected
access to s.content so concurrent reads remain safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 775c00b3-c1a2-4fa8-b637-c8fec16b3b10
📒 Files selected for processing (2)
api/v3/handlers/billinginvoices/convert.gopkg/set/set.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v3/handlers/billinginvoices/convert.go
| case "charge_automatically": | ||
| existing.Config.Payment.CollectionMethod = billing.CollectionMethodChargeAutomatically |
There was a problem hiding this comment.
DueAfter persists after switching to charge_automatically
When the payment method is changed from send_invoice to charge_automatically, existing.Config.Invoicing.DueAfter is never cleared. Because toAPIWorkflow always emits DueAfter under the invoicing section (lo.EmptyableToPtr(config.Invoicing.DueAfter.String())), the GET response after the update will still show invoicing.due_after with the old duration even though the payment method is now charge_automatically. The fix is to reset DueAfter to its zero value in the charge_automatically branch.
| case "charge_automatically": | |
| existing.Config.Payment.CollectionMethod = billing.CollectionMethodChargeAutomatically | |
| case "charge_automatically": | |
| existing.Config.Payment.CollectionMethod = billing.CollectionMethodChargeAutomatically | |
| existing.Config.Invoicing.DueAfter = billing.DefaultWorkflowConfig.Invoicing.DueAfter |
Prompt To Fix With AI
This is a comment left during a code review.
Path: api/v3/handlers/billinginvoices/convert.go
Line: 571-572
Comment:
**Stale `DueAfter` persists after switching to `charge_automatically`**
When the payment method is changed from `send_invoice` to `charge_automatically`, `existing.Config.Invoicing.DueAfter` is never cleared. Because `toAPIWorkflow` always emits `DueAfter` under the invoicing section (`lo.EmptyableToPtr(config.Invoicing.DueAfter.String())`), the GET response after the update will still show `invoicing.due_after` with the old duration even though the payment method is now `charge_automatically`. The fix is to reset `DueAfter` to its zero value in the `charge_automatically` branch.
```suggestion
case "charge_automatically":
existing.Config.Payment.CollectionMethod = billing.CollectionMethodChargeAutomatically
existing.Config.Invoicing.DueAfter = billing.DefaultWorkflowConfig.Invoicing.DueAfter
```
How can I resolve this? If you propose a fix, please make it concise.
2 participants
What
Adds a v3 (AIP) endpoint to update billing invoices:
PUT /openmeter/billing/invoices/{invoiceId}(update-invoice). Only the invoice's mutable fields are editable —description,labels,supplier,customersnapshot,workflowsettings (invoicing auto-advance/draft period, payment collection method/due date), and top-levellines(create, edit, or delete by ID). Everything else — status, totals, currency, timestamps, detailed lines, customer/supplier IDs — stays read-only.Why
v3 previously only exposed read access to invoices (
list-invoices,get-invoice); any edit had to go through the legacy v1 API (PUT /api/v1/billing/invoices/{id}). This brings invoice mutation into v3, scoped down to a well-defined mutable-field set rather than a full v1 port, and follows the sameShared.UpsertRequest/UpdateResponsepattern already used byupdate-billing-profileso the v3 CRUD surface stays consistent.Domain constraints are inherited, not reimplemented: only standard invoices are supported (v3 doesn't expose gathering invoices), and only draft invoices can be updated — both enforced by the existing billing state machine and adapter-level immutability guards (e.g. customer ID can't change even if a client echoes a different value back).
How
api/spec/packages/aip/src/invoices/{invoice,operations}.tsp): addedLifecycle.Create, Lifecycle.Updatevisibility to the mutable fields, added anid?/service_period/newinvoice_atfield toInvoiceStandardLineplus Update visibility on rate card fields, and added theupdateoperation.api/v3/handlers/billinginvoices/): newupdate.gocallsbilling.Service.UpdateStandardInvoicewith anEditFnthat merges the request onto the loaded invoice;convert.gogained merge helpers (supplier/customer/workflow/lines) ported from the v1httpdrivermerge logic, explicitly preserving identity fields (customer ID, supplier ID) instead of overwriting them.Handlerinterface, new route delegate inapi/v3/server/routes.go(handler was already instantiated, noserver.gochange needed).Verified via
make gen-api && make generate,go build/go vet/go teston the touched packages, and a full-repogolangci-lintrun — all clean.Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
PUT /openmeter/billing/invoices/{invoiceId}.