feat(productcatalog): support custom currencies#4738
Open
GAlexIHU wants to merge 6 commits into
Open
Conversation
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
Contributor
📝 WalkthroughWalkthroughThis change adds custom currency identities and rate-card currency overrides across API contracts, persistence, product-catalog services, serialization, subscription validation, compatibility handling, dependency wiring, and migrations. ChangesCustom currency support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
GAlexIHU
marked this pull request as ready for review
July 17, 2026 14:48
![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: 12
🧹 Nitpick comments (3)
openmeter/productcatalog/currencyresolver_test.go (1)
184-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid adding the local
must...wrapper.Please inline these builder calls or use an existing currency fixture; this helper only hides a short operation.
As per coding guidelines, do not add local wrappers such as
mustwhen existing helpers or direct operations suffice.🤖 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 `@openmeter/productcatalog/currencyresolver_test.go` around lines 184 - 193, Remove the local mustFiatCurrency helper and replace its call sites with direct currencyx.NewCurrencyBuilder(...).WithCode(...).Build() usage or an existing currency fixture. Preserve the current fiat currency construction and error assertions at each call site.Source: Coding guidelines
openmeter/productcatalog/addon/serializer_test.go (1)
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the usage-based serialization branch too.
The new serializer switches separately on flat-fee and usage-based rate cards, but this round-trip only exercises flat-fee. A table case using
UsageBasedRateCardwould catch regressions in that branch.As per path instructions, tests should comprehensively cover introduced behavior.
Also applies to: 67-73
🤖 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 `@openmeter/productcatalog/addon/serializer_test.go` around lines 32 - 43, Extend the serializer round-trip test cases in the relevant test function to include a UsageBasedRateCard fixture alongside the existing FlatFeeRateCard case. Populate the usage-based card with the required metadata and usage pricing fields, then assert it round-trips successfully so the separate usage-based serialization branch is covered.Source: Path instructions
pkg/currencyx/code.go (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSkip custom length check for empty inputs to keep validation errors tidy.
Since an empty string will evaluate
IsFiat()asfalse, the code currently emits two errors when no currency is provided: "currency code is required" and "custom currency code must be between 3 and 64 characters".We can skip the latter check if the code is completely empty to keep the returned error slightly more concise!
💡 Suggested refactor
- if !c.IsFiat() { + if c != "" && !c.IsFiat() { if length := len(code); length < CustomCurrencyCodeMinLength || length > CustomCurrencyCodeMaxLength { errs = append(errs, fmt.Errorf("custom currency code must be between %d and %d characters", CustomCurrencyCodeMinLength, CustomCurrencyCodeMaxLength)) } }🤖 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/currencyx/code.go` around lines 39 - 44, Update the custom currency length validation in the IsFiat check to run only when code is non-empty, so empty input produces only the existing required-code error. Preserve the current 3–64 character validation for non-empty custom currency codes.
🤖 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 `@openmeter/currencies/currency.go`:
- Around line 83-87: The managed custom currency ID validation error in the
currency validation method is missing field context. Update the `c.IsCustom() &&
c.ID == ""` branch to wrap its error with the `code:` field prefix, matching the
existing code validation failure, while preserving collection through
`errors.Join(errs...)` and `models.NewNillableGenericValidationError`.
In `@openmeter/productcatalog/addon.go`:
- Around line 90-96: Update the currency validation branch in the addon
validation method to wrap and retain the concrete error returned by
m.Currency.Validate() alongside ErrCurrencyInvalid, while preserving the
existing currency field selector and collecting the error in errs. Keep the
nil-currency handling unchanged.
In `@openmeter/productcatalog/addon/service.go`:
- Around line 410-424: The currency-preservation logic around persistedCurrency
and updatedCurrency must only replace code-only currency values, not an
explicitly managed update identity. Before calling rateCard.ChangeMeta, detect
when updatedCurrency is already a ManagedCurrency with a non-empty ID and skip
preservation; retain the existing same-code behavior for code-only updates and
continue preserving the persisted identity there.
In `@openmeter/productcatalog/currencyresolver.go`:
- Around line 150-153: Update the existing.IsCustom branch in the currency
resolution flow to avoid returning ManagedCurrency values solely because GetID()
is non-empty; re-resolve managed custom currencies by their ID or verify their
concrete type belongs to the requested namespace before returning. Preserve the
current fast path only for currencies proven to match namespace, and add a test
covering a managed custom currency from another namespace.
In `@openmeter/productcatalog/currencyresolver/resolver_test.go`:
- Around line 22-27: Update mockCurrencyService.ListCurrencies to honor
params.Page and the requested page size instead of returning all currencies. Add
a regression test with two archived currencies before an active matching
currency, ensuring Resolve still finds the active result. Update Resolve to
query active currencies or continue paging until a valid match is found.
In `@openmeter/productcatalog/currencyresolver/resolver.go`:
- Around line 45-68: The currency lookup around ListCurrencies currently
paginates before excluding archived entries, so active matches can be missed.
Push the DeletedAt-is-nil predicate into the ListCurrencies filter for custom
currencies, or continue fetching pages until activeCurrencies establishes a
definitive not-found or unique result; preserve the existing uniqueness and
error handling behavior.
In `@openmeter/productcatalog/errors.go`:
- Around line 265-315: Rename the new error-code constants in this section and
the additionally referenced declaration from the ErrCode... form to the required
ErrorCode... form, including ErrorCodeRateCardCurrencyOverrideNotAllowed,
ErrorCodeRateCardCurrencyOverrideRedundant,
ErrorCodeRateCardCurrencyRequiresPrice, ErrorCodePlanMultipleFiatCurrencies,
ErrorCodeCurrencyNotFound, and ErrorCodeCurrencyCostBasisNotFound. Update every
corresponding reference while leaving the exported validation issue variables
unchanged.
In `@openmeter/productcatalog/plan.go`:
- Around line 382-388: Update the currency validation branches in the plan
validation method so both missing-currency and Currency.Validate() failures are
wrapped with the “currency” field context. Preserve the underlying validation
error as the cause when wrapping the Currency.Validate() result, rather than
replacing it with ErrCurrencyInvalid.
In `@openmeter/productcatalog/plan/ratecard.go`:
- Around line 103-224: Remove the map-based currency rewriting from RateCard
MarshalJSON and UnmarshalJSON. In MarshalJSON, delegate directly to json.Marshal
on r.RateCard; in UnmarshalJSON, unmarshal serialized.RateCard directly into the
type-specific rateCard selected by serialized.Type. Delete
marshalRateCardForJSON, rateCardJSONWithoutCurrency, and
setRateCardCurrencyIdentity, while preserving existing error handling and
managed-field assignment.
- Around line 103-224: Remove the map-based currency rewriting from
RateCard.MarshalJSON and RateCard.UnmarshalJSON. Delegate serialization and
deserialization directly to json.Marshal and json.Unmarshal on the concrete
FlatFeeRateCard and UsageBasedRateCard values, then delete
marshalRateCardForJSON, rateCardJSONWithoutCurrency, and
setRateCardCurrencyIdentity while preserving the existing type selection and
managed-field handling.
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 924-928: Strengthen the assertions in the stored rate-card test
around storedRateCard.Currency and EffectiveCurrency to validate the managed
currency resource identity, not only its code. Assert both values are
currencyx.ManagedCurrency instances and compare their IDs with the expected
custom managed currency ID, preserving the existing code checks and ensuring
hydration cannot resolve a different resource sharing CREDITS.
In `@openmeter/productcatalog/planaddon/service/planaddon.go`:
- Around line 148-150: Update the create flow around ValidateWith to first run
the full plan-addon validation, mirroring the update path, before executing the
resolver-backed currency validation. Ensure invalid phases, keys, quantities,
cadence, and overlays are rejected before persistence while retaining the
existing currency-only check afterward.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/serializer_test.go`:
- Around line 32-43: Extend the serializer round-trip test cases in the relevant
test function to include a UsageBasedRateCard fixture alongside the existing
FlatFeeRateCard case. Populate the usage-based card with the required metadata
and usage pricing fields, then assert it round-trips successfully so the
separate usage-based serialization branch is covered.
In `@openmeter/productcatalog/currencyresolver_test.go`:
- Around line 184-193: Remove the local mustFiatCurrency helper and replace its
call sites with direct currencyx.NewCurrencyBuilder(...).WithCode(...).Build()
usage or an existing currency fixture. Preserve the current fiat currency
construction and error assertions at each call site.
In `@pkg/currencyx/code.go`:
- Around line 39-44: Update the custom currency length validation in the IsFiat
check to run only when code is non-empty, so empty input produces only the
existing required-code error. Preserve the current 3–64 character validation for
non-empty custom currency codes.
🪄 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: 37f5e766-91f8-4e02-be44-f8cad9ee161c
⛔ Files ignored due to path filters (37)
api/v3/openapi.yamlis excluded by!**/openapi.yamlopenmeter/ent/db/addon.gois excluded by!**/ent/db/**openmeter/ent/db/addon/addon.gois excluded by!**/ent/db/**openmeter/ent/db/addon/where.gois excluded by!**/ent/db/**openmeter/ent/db/addon_create.gois excluded by!**/ent/db/**openmeter/ent/db/addon_query.gois excluded by!**/ent/db/**openmeter/ent/db/addon_update.gois excluded by!**/ent/db/**openmeter/ent/db/addonratecard.gois excluded by!**/ent/db/**openmeter/ent/db/addonratecard/addonratecard.gois excluded by!**/ent/db/**openmeter/ent/db/addonratecard/where.gois excluded by!**/ent/db/**openmeter/ent/db/addonratecard_create.gois excluded by!**/ent/db/**openmeter/ent/db/addonratecard_query.gois excluded by!**/ent/db/**openmeter/ent/db/addonratecard_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency/customcurrency.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency/where.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_create.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_query.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/plan.gois excluded by!**/ent/db/**openmeter/ent/db/plan/plan.gois excluded by!**/ent/db/**openmeter/ent/db/plan/where.gois excluded by!**/ent/db/**openmeter/ent/db/plan_create.gois excluded by!**/ent/db/**openmeter/ent/db/plan_query.gois excluded by!**/ent/db/**openmeter/ent/db/plan_update.gois excluded by!**/ent/db/**openmeter/ent/db/planratecard.gois excluded by!**/ent/db/**openmeter/ent/db/planratecard/planratecard.gois excluded by!**/ent/db/**openmeter/ent/db/planratecard/where.gois excluded by!**/ent/db/**openmeter/ent/db/planratecard_create.gois excluded by!**/ent/db/**openmeter/ent/db/planratecard_query.gois excluded by!**/ent/db/**openmeter/ent/db/planratecard_update.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (117)
api/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/currencies/currency.tspapi/spec/packages/aip/src/productcatalog/plan.tspapi/spec/packages/aip/src/productcatalog/ratecard.tspapi/v3/api.gen.goapi/v3/client/models_plans.goapi/v3/client/models_shared.goapi/v3/handlers/addons/convert.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goapp/common/openmeter_billingworker.goapp/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/billing/derived.gen.goopenmeter/billing/models/stddetailedline/derived.gen.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/creditsonly_test.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/billing/worker/subscriptionsync/service/sync_test.goopenmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/adapter/mapping.goopenmeter/currencies/adapter/mapping_test.goopenmeter/currencies/currency.goopenmeter/currencies/currency_test.goopenmeter/ent/schema/addon.goopenmeter/ent/schema/custom_currencies.goopenmeter/ent/schema/productcatalog.goopenmeter/ent/schema/ratecard.goopenmeter/productcatalog/adapter/feature_test.goopenmeter/productcatalog/addon.goopenmeter/productcatalog/addon/adapter/adapter_test.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/adapter/mapping.goopenmeter/productcatalog/addon/assert.goopenmeter/productcatalog/addon/httpdriver/addon.goopenmeter/productcatalog/addon/httpdriver/mapping.goopenmeter/productcatalog/addon/ratecard.goopenmeter/productcatalog/addon/serializer.goopenmeter/productcatalog/addon/serializer_test.goopenmeter/productcatalog/addon/service.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/addon_test.goopenmeter/productcatalog/currency_test.goopenmeter/productcatalog/currencyresolver.goopenmeter/productcatalog/currencyresolver/resolver.goopenmeter/productcatalog/currencyresolver/resolver_test.goopenmeter/productcatalog/currencyresolver_test.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/plan.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/productcatalog/plan/adapter/phase.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/assert.goopenmeter/productcatalog/plan/httpdriver/mapping.goopenmeter/productcatalog/plan/httpdriver/plan.goopenmeter/productcatalog/plan/plan.goopenmeter/productcatalog/plan/ratecard.goopenmeter/productcatalog/plan/ratecard_test.goopenmeter/productcatalog/plan/serializer.goopenmeter/productcatalog/plan/serializer_test.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/plan_test.goopenmeter/productcatalog/planaddon.goopenmeter/productcatalog/planaddon/adapter/planaddon.goopenmeter/productcatalog/planaddon/httpdriver/planaddon.goopenmeter/productcatalog/planaddon/service.goopenmeter/productcatalog/planaddon/service/planaddon.goopenmeter/productcatalog/planaddon/service/service.goopenmeter/productcatalog/planaddon/service/service_test.goopenmeter/productcatalog/planaddon_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/ratecard_test.goopenmeter/productcatalog/subscription/http/mapping.goopenmeter/productcatalog/subscription/plan.goopenmeter/productcatalog/testutils/addon.goopenmeter/productcatalog/testutils/env.goopenmeter/productcatalog/testutils/plan.goopenmeter/subscription/addon/diff/restore_test.goopenmeter/subscription/addon/events_test.goopenmeter/subscription/addon/repo/subscriptionaddon.goopenmeter/subscription/serialize_test.goopenmeter/subscription/service/servicevalidation.goopenmeter/subscription/service/servicevalidation_test.goopenmeter/subscription/testutils/addon.goopenmeter/subscription/testutils/builder.goopenmeter/subscription/testutils/service.goopenmeter/subscription/workflow/service/addon_test.goopenmeter/subscription/workflow/service/subscription_test.gopkg/currencyx/code.gopkg/currencyx/code_test.gopkg/currencyx/currency.gotest/billing/subscription_suite.gotest/billing/subscription_test.gotest/customer/customer.gotest/customer/subject.gotest/customer/testenv.gotest/subscription/scenario_editaligned_test.gotest/subscription/scenario_editcancel_test.gotest/subscription/scenario_entinnextphase_test.gotest/subscription/scenario_firstofmonth_test.gotools/migrate/migrations/20260717143312_add_rate_card_currency.down.sqltools/migrate/migrations/20260717143312_add_rate_card_currency.up.sqltools/migrate/migrations/20260717143818_add_product_catalog_currency_references.down.sqltools/migrate/migrations/20260717143818_add_product_catalog_currency_references.up.sqltools/migrate/productcatalog_currency_references_test.go
Comment on lines
+83
to
+87
| if c.IsCustom() && c.ID == "" { | ||
| errs = append(errs, errors.New("managed custom currency ID is required")) | ||
| } | ||
|
|
||
| return models.NewNillableGenericValidationError(errors.Join(errs...)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the managed-ID validation failure field-scoped.
The code failure is wrapped with code:, but the managed-ID failure is appended as a plain error. Prefix and wrap it consistently so validation consumers retain the failing field.
Proposed fix
if c.IsCustom() && c.ID == "" {
- errs = append(errs, errors.New("managed custom currency ID is required"))
+ errs = append(errs, fmt.Errorf("id: %w", errors.New("managed custom currency ID is required")))
}As per coding guidelines, “For Validate() error methods, collect all validation failures and return models.NewNillableGenericValidationError(errors.Join(errs...)), preserving field context with wrapped errors.”
📝 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.
Suggested change
| if c.IsCustom() && c.ID == "" { | |
| errs = append(errs, errors.New("managed custom currency ID is required")) | |
| } | |
| return models.NewNillableGenericValidationError(errors.Join(errs...)) | |
| if c.IsCustom() && c.ID == "" { | |
| errs = append(errs, fmt.Errorf("id: %w", errors.New("managed custom currency ID is required"))) | |
| } | |
| return models.NewNillableGenericValidationError(errors.Join(errs...)) |
🤖 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 `@openmeter/currencies/currency.go` around lines 83 - 87, The managed custom
currency ID validation error in the currency validation method is missing field
context. Update the `c.IsCustom() && c.ID == ""` branch to wrap its error with
the `code:` field prefix, matching the existing code validation failure, while
preserving collection through `errors.Join(errs...)` and
`models.NewNillableGenericValidationError`.
Source: Coding guidelines
Comment on lines
+90
to
+96
| if m.Currency == nil { | ||
| errs = append(errs, ErrCurrencyInvalid) | ||
| } else if err := m.Currency.Validate(); err != nil { | ||
| errs = append(errs, models.ErrorWithFieldPrefix( | ||
| models.NewFieldSelectorGroup(models.NewFieldSelector("currency")), | ||
| ErrCurrencyInvalid, | ||
| )) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the concrete currency validation error.
m.Currency.Validate() is discarded, so callers only receive the generic ErrCurrencyInvalid and lose the underlying failure details. Wrap err alongside the catalog issue while retaining the currency field selector.
As per coding guidelines, Validate() methods must collect all failures and preserve field context with wrapped errors.
🤖 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 `@openmeter/productcatalog/addon.go` around lines 90 - 96, Update the currency
validation branch in the addon validation method to wrap and retain the concrete
error returned by m.Currency.Validate() alongside ErrCurrencyInvalid, while
preserving the existing currency field selector and collecting the error in
errs. Keep the nil-currency handling unchanged.
Source: Coding guidelines
Comment on lines
+410
to
+424
| persistedCurrency := persistedRateCard.AsMeta().Currency | ||
| updatedCurrency := rateCard.AsMeta().Currency | ||
| managedCurrency, ok := persistedCurrency.(currencyx.ManagedCurrency) | ||
| if !ok || !managedCurrency.IsCustom() || managedCurrency.GetID() == "" { | ||
| continue | ||
| } | ||
| if updatedCurrency == nil || managedCurrency.GetCode() != updatedCurrency.GetCode() { | ||
| continue | ||
| } | ||
|
|
||
| if err := rateCard.ChangeMeta(func(meta productcatalog.RateCardMeta) (productcatalog.RateCardMeta, error) { | ||
| meta.Currency = persistedCurrency | ||
| return meta, nil | ||
| }); err != nil { | ||
| return fmt.Errorf("preserving rate card currency identity [ratecard.key=%s]: %w", rateCard.Key(), err) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not overwrite an explicitly managed update identity.
Any updated currency with the same code is replaced by the persisted identity, even when it is already a different ManagedCurrency. That silently retargets an intentional update back to the old resource ID. Preserve only code-only identities:
Proposed guard
updatedCurrency := rateCard.AsMeta().Currency
managedCurrency, ok := persistedCurrency.(currencyx.ManagedCurrency)
if !ok || !managedCurrency.IsCustom() || managedCurrency.GetID() == "" {
continue
}
+ if updatedManaged, ok := updatedCurrency.(currencyx.ManagedCurrency); ok && updatedManaged.GetID() != "" {
+ continue
+ }
if updatedCurrency == nil || managedCurrency.GetCode() != updatedCurrency.GetCode() {
continue
}This also matches the managed-identity stability contract exercised in openmeter/productcatalog/currencyresolver_test.go.
📝 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.
Suggested change
| persistedCurrency := persistedRateCard.AsMeta().Currency | |
| updatedCurrency := rateCard.AsMeta().Currency | |
| managedCurrency, ok := persistedCurrency.(currencyx.ManagedCurrency) | |
| if !ok || !managedCurrency.IsCustom() || managedCurrency.GetID() == "" { | |
| continue | |
| } | |
| if updatedCurrency == nil || managedCurrency.GetCode() != updatedCurrency.GetCode() { | |
| continue | |
| } | |
| if err := rateCard.ChangeMeta(func(meta productcatalog.RateCardMeta) (productcatalog.RateCardMeta, error) { | |
| meta.Currency = persistedCurrency | |
| return meta, nil | |
| }); err != nil { | |
| return fmt.Errorf("preserving rate card currency identity [ratecard.key=%s]: %w", rateCard.Key(), err) | |
| persistedCurrency := persistedRateCard.AsMeta().Currency | |
| updatedCurrency := rateCard.AsMeta().Currency | |
| managedCurrency, ok := persistedCurrency.(currencyx.ManagedCurrency) | |
| if !ok || !managedCurrency.IsCustom() || managedCurrency.GetID() == "" { | |
| continue | |
| } | |
| if updatedManaged, ok := updatedCurrency.(currencyx.ManagedCurrency); ok && updatedManaged.GetID() != "" { | |
| continue | |
| } | |
| if updatedCurrency == nil || managedCurrency.GetCode() != updatedCurrency.GetCode() { | |
| continue | |
| } | |
| if err := rateCard.ChangeMeta(func(meta productcatalog.RateCardMeta) (productcatalog.RateCardMeta, error) { | |
| meta.Currency = persistedCurrency | |
| return meta, nil | |
| }); err != nil { | |
| return fmt.Errorf("preserving rate card currency identity [ratecard.key=%s]: %w", rateCard.Key(), err) |
🤖 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 `@openmeter/productcatalog/addon/service.go` around lines 410 - 424, The
currency-preservation logic around persistedCurrency and updatedCurrency must
only replace code-only currency values, not an explicitly managed update
identity. Before calling rateCard.ChangeMeta, detect when updatedCurrency is
already a ManagedCurrency with a non-empty ID and skip preservation; retain the
existing same-code behavior for code-only updates and continue preserving the
persisted identity there.
Comment on lines
+150
to
+153
| if existing.IsCustom() { | ||
| if managed, ok := existing.(currencyx.ManagedCurrency); ok && managed.GetID() != "" { | ||
| return existing, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline pkg/currencyx openmeter/currencies openmeter/ent/schema \
--items all --match 'ManagedCurrency|Currency|CustomCurrency'
rg -n -C4 \
'\bManagedCurrency\b|GetNamespace|CustomCurrencyID|custom_currency_id' \
pkg/currencyx openmeter/currencies openmeter/ent/schemaRepository: openmeterio/openmeter
Length of output: 19441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' openmeter/productcatalog/currencyresolver.go
printf '\n---\n'
sed -n '1,220p' pkg/currencyx/currency.go
printf '\n---\n'
sed -n '1,200p' openmeter/currencies/currency.go
printf '\n---\n'
sed -n '1,200p' openmeter/currencies/adapter/mapping.go
printf '\n---\n'
sed -n '1,220p' openmeter/ent/schema/productcatalog.goRepository: openmeterio/openmeter
Length of output: 21075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' openmeter/ent/schema/custom_currencies.go
printf '\n---\n'
rg -n -C3 'Namespace|custom_currency_id|Ref\("plans"\)|Field\("custom_currency_id"\)|UniqueResourceMixin|NamespacedID' openmeter/ent/schema openmeter/productcatalog openmeter/currenciesRepository: openmeterio/openmeter
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' openmeter/ent/schema/custom_currencies.goRepository: openmeterio/openmeter
Length of output: 2937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "type .* struct .*ManagedCurrency|_ currencyx.ManagedCurrency|ManagedCurrency =|GetNamespace\(" pkg openmeter | head -n 200Repository: openmeterio/openmeter
Length of output: 12295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' openmeter/productcatalog/currencyresolver_test.go
printf '\n---\n'
sed -n '1,220p' pkg/models/model.go
printf '\n---\n'
sed -n '1,220p' pkg/framework/entutils/mixins.goRepository: openmeterio/openmeter
Length of output: 16495
Avoid short-circuiting managed custom currencies here. ManagedCurrency only carries an ID, so this returns any custom currency object as-is without proving it belongs to namespace. Re-resolve managed identities by ID or compare the namespace on the concrete type, and add a cross-namespace test.
🤖 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 `@openmeter/productcatalog/currencyresolver.go` around lines 150 - 153, Update
the existing.IsCustom branch in the currency resolution flow to avoid returning
ManagedCurrency values solely because GetID() is non-empty; re-resolve managed
custom currencies by their ID or verify their concrete type belongs to the
requested namespace before returning. Preserve the current fast path only for
currencies proven to match namespace, and add a test covering a managed custom
currency from another namespace.
Comment on lines
+22
to
+27
| func (m mockCurrencyService) ListCurrencies(_ context.Context, params currencies.ListCurrenciesInput) (pagination.Result[currencies.Currency], error) { | ||
| return pagination.Result[currencies.Currency]{ | ||
| Page: params.Page, | ||
| TotalCount: len(m.currencies), | ||
| Items: m.currencies, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the mock enforce pagination so this behavior is actually covered.
Resolve requests only two matches, but this mock returns every row. With two archived rows before an active one, production can return not-found while this test still passes. Please paginate the mock and add that case, then make the resolver query active rows or continue paging.
As per path instructions, suggest missing tests when “the current tests would pass despite a concrete regression in the changed code.”
Also applies to: 42-67
🤖 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 `@openmeter/productcatalog/currencyresolver/resolver_test.go` around lines 22 -
27, Update mockCurrencyService.ListCurrencies to honor params.Page and the
requested page size instead of returning all currencies. Add a regression test
with two archived currencies before an active matching currency, ensuring
Resolve still finds the active result. Update Resolve to query active currencies
or continue paging until a valid match is found.
Source: Path instructions
Comment on lines
+265
to
+315
| const ErrCodeRateCardCurrencyOverrideNotAllowed models.ErrorCode = "rate_card_currency_override_not_allowed" | ||
|
|
||
| var ErrRateCardCurrencyOverrideNotAllowed = models.NewValidationIssue( | ||
| ErrCodeRateCardCurrencyOverrideNotAllowed, | ||
| "currency override is not allowed when the default currency is custom", | ||
| models.WithFieldString("currency"), | ||
| models.WithWarningSeverity(), | ||
| commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), | ||
| ) | ||
|
|
||
| const ErrCodeRateCardCurrencyOverrideRedundant models.ErrorCode = "rate_card_currency_override_redundant" | ||
|
|
||
| var ErrRateCardCurrencyOverrideRedundant = models.NewValidationIssue( | ||
| ErrCodeRateCardCurrencyOverrideRedundant, | ||
| "currency override must differ from the default currency", | ||
| models.WithFieldString("currency"), | ||
| models.WithWarningSeverity(), | ||
| commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), | ||
| ) | ||
|
|
||
| const ErrCodeRateCardCurrencyRequiresPrice models.ErrorCode = "rate_card_currency_requires_price" | ||
|
|
||
| var ErrRateCardCurrencyRequiresPrice = models.NewValidationIssue( | ||
| ErrCodeRateCardCurrencyRequiresPrice, | ||
| "currency override requires a price", | ||
| models.WithFieldString("currency"), | ||
| models.WithWarningSeverity(), | ||
| commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), | ||
| ) | ||
|
|
||
| const ErrCodePlanMultipleFiatCurrencies models.ErrorCode = "plan_multiple_fiat_currencies" | ||
|
|
||
| var ErrPlanMultipleFiatCurrencies = models.NewValidationIssue( | ||
| ErrCodePlanMultipleFiatCurrencies, | ||
| "a catalog resource cannot contain more than one fiat currency", | ||
| models.WithFieldString("currency"), | ||
| models.WithWarningSeverity(), | ||
| commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), | ||
| ) | ||
|
|
||
| const ErrCodeCurrencyNotFound models.ErrorCode = "currency_not_found" | ||
|
|
||
| var ErrCurrencyNotFound = models.NewValidationIssue( | ||
| ErrCodeCurrencyNotFound, | ||
| "currency does not exist", | ||
| models.WithFieldString("currency"), | ||
| models.WithCriticalSeverity(), | ||
| commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), | ||
| ) | ||
|
|
||
| const ErrCodeCurrencyCostBasisNotFound models.ErrorCode = "currency_cost_basis_not_found" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required <Type><Value> names for these error codes.
These new models.ErrorCode constants use ErrCode.... Please rename them to the type-prefixed form, such as ErrorCodeRateCardCurrencyOverrideNotAllowed, before the exported API stabilizes.
As per coding guidelines, Go string enum constants must use the <Type><Value> naming form.
Also applies to: 472-472
🤖 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 `@openmeter/productcatalog/errors.go` around lines 265 - 315, Rename the new
error-code constants in this section and the additionally referenced declaration
from the ErrCode... form to the required ErrorCode... form, including
ErrorCodeRateCardCurrencyOverrideNotAllowed,
ErrorCodeRateCardCurrencyOverrideRedundant,
ErrorCodeRateCardCurrencyRequiresPrice, ErrorCodePlanMultipleFiatCurrencies,
ErrorCodeCurrencyNotFound, and ErrorCodeCurrencyCostBasisNotFound. Update every
corresponding reference while leaving the exported validation issue variables
unchanged.
Source: Coding guidelines
Comment on lines
+382
to
+388
| if p.Currency == nil { | ||
| errs = append(errs, ErrCurrencyInvalid) | ||
| } else if err := p.Currency.Validate(); err != nil { | ||
| errs = append(errs, models.ErrorWithFieldPrefix( | ||
| models.NewFieldSelectorGroup(models.NewFieldSelector("currency")), | ||
| ErrCurrencyInvalid, | ||
| )) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the currency validation cause and field context.
Line 383 returns an unscoped error, while Line 387 discards the error from Currency.Validate(). Keep both under currency and retain the underlying cause.
Proposed fix
+ currencyField := models.NewFieldSelectorGroup(models.NewFieldSelector("currency"))
if p.Currency == nil {
- errs = append(errs, ErrCurrencyInvalid)
+ errs = append(errs, models.ErrorWithFieldPrefix(currencyField, ErrCurrencyInvalid))
} else if err := p.Currency.Validate(); err != nil {
errs = append(errs, models.ErrorWithFieldPrefix(
- models.NewFieldSelectorGroup(models.NewFieldSelector("currency")),
- ErrCurrencyInvalid,
+ currencyField,
+ errors.Join(ErrCurrencyInvalid, err),
))
}As per coding guidelines, Validate methods must preserve field context with wrapped errors.
📝 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.
Suggested change
| if p.Currency == nil { | |
| errs = append(errs, ErrCurrencyInvalid) | |
| } else if err := p.Currency.Validate(); err != nil { | |
| errs = append(errs, models.ErrorWithFieldPrefix( | |
| models.NewFieldSelectorGroup(models.NewFieldSelector("currency")), | |
| ErrCurrencyInvalid, | |
| )) | |
| currencyField := models.NewFieldSelectorGroup(models.NewFieldSelector("currency")) | |
| if p.Currency == nil { | |
| errs = append(errs, models.ErrorWithFieldPrefix(currencyField, ErrCurrencyInvalid)) | |
| } else if err := p.Currency.Validate(); err != nil { | |
| errs = append(errs, models.ErrorWithFieldPrefix( | |
| currencyField, | |
| errors.Join(ErrCurrencyInvalid, err), | |
| )) |
🤖 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 `@openmeter/productcatalog/plan.go` around lines 382 - 388, Update the currency
validation branches in the plan validation method so both missing-currency and
Currency.Validate() failures are wrapped with the “currency” field context.
Preserve the underlying validation error as the cause when wrapping the
Currency.Validate() result, rather than replacing it with ErrCurrencyInvalid.
Source: Coding guidelines
Comment on lines
+103
to
+224
| rateCard, err := marshalRateCardForJSON(r.RateCard) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| serde := struct { | ||
| productcatalog.RateCardSerde | ||
| productcatalog.RateCard | ||
| RateCard json.RawMessage `json:"RateCard"` | ||
| RateCardManagedFields | ||
| }{ | ||
| RateCardSerde: productcatalog.RateCardSerde{ | ||
| Type: r.Type(), | ||
| }, | ||
| RateCard: r.RateCard, | ||
| RateCard: rateCard, | ||
| RateCardManagedFields: r.RateCardManagedFields, | ||
| } | ||
|
|
||
| return json.Marshal(serde) | ||
| } | ||
|
|
||
| func (r *RateCard) UnmarshalJSON(b []byte) error { | ||
| var s productcatalog.RateCardSerde | ||
| err := json.Unmarshal(b, &s) | ||
| var serialized struct { | ||
| productcatalog.RateCardSerde | ||
| RateCard json.RawMessage `json:"RateCard"` | ||
| RateCardManagedFields | ||
| } | ||
| err := json.Unmarshal(b, &serialized) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to JSON deserialize RateCard type: %w", err) | ||
| } | ||
|
|
||
| serde := struct { | ||
| productcatalog.RateCard | ||
| RateCardManagedFields | ||
| }{ | ||
| RateCardManagedFields: r.RateCardManagedFields, | ||
| RateCard: r.RateCard, | ||
| } | ||
|
|
||
| switch s.Type { | ||
| var rateCard productcatalog.RateCard | ||
| switch serialized.Type { | ||
| case productcatalog.FlatFeeRateCardType: | ||
| serde.RateCard = &productcatalog.FlatFeeRateCard{} | ||
| rateCard = &productcatalog.FlatFeeRateCard{} | ||
| case productcatalog.UsageBasedRateCardType: | ||
| serde.RateCard = &productcatalog.UsageBasedRateCard{} | ||
| rateCard = &productcatalog.UsageBasedRateCard{} | ||
| default: | ||
| return fmt.Errorf("invalid RateCard type: %s", s.Type) | ||
| return fmt.Errorf("invalid RateCard type: %s", serialized.Type) | ||
| } | ||
|
|
||
| err = json.Unmarshal(b, &serde) | ||
| var currencyData struct { | ||
| Currency *currencyx.Code `json:"currency,omitempty"` | ||
| } | ||
| if err = json.Unmarshal(serialized.RateCard, ¤cyData); err != nil { | ||
| return fmt.Errorf("failed to JSON deserialize RateCard currency: %w", err) | ||
| } | ||
|
|
||
| rateCardData, err := rateCardJSONWithoutCurrency(serialized.RateCard) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to JSON deserialize UsageBasedRateCard: %w", err) | ||
| return fmt.Errorf("failed to JSON deserialize RateCard: %w", err) | ||
| } | ||
| if err = json.Unmarshal(rateCardData, rateCard); err != nil { | ||
| return fmt.Errorf("failed to JSON deserialize RateCard: %w", err) | ||
| } | ||
|
|
||
| if currencyData.Currency != nil { | ||
| if err = setRateCardCurrencyIdentity(rateCard, *currencyData.Currency); err != nil { | ||
| return fmt.Errorf("failed to set RateCard currency: %w", err) | ||
| } | ||
| } | ||
|
|
||
| r.RateCardManagedFields = serialized.RateCardManagedFields | ||
| r.RateCard = rateCard | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| r.RateCardManagedFields = serde.RateCardManagedFields | ||
| r.RateCard = serde.RateCard | ||
| func marshalRateCardForJSON(rateCard productcatalog.RateCard) (json.RawMessage, error) { | ||
| currency := rateCard.AsMeta().Currency | ||
| rateCardWithoutCurrency := rateCard.Clone() | ||
| if err := setRateCardCurrencyIdentity(rateCardWithoutCurrency, nil); err != nil { | ||
| return nil, fmt.Errorf("failed to prepare RateCard for JSON serialization: %w", err) | ||
| } | ||
|
|
||
| data, err := json.Marshal(rateCardWithoutCurrency) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to JSON serialize RateCard: %w", err) | ||
| } | ||
|
|
||
| var fields map[string]json.RawMessage | ||
| if err = json.Unmarshal(data, &fields); err != nil { | ||
| return nil, fmt.Errorf("failed to rewrite RateCard currency: %w", err) | ||
| } | ||
|
|
||
| if currency != nil { | ||
| fields["currency"], err = json.Marshal(currency.GetCode()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to JSON serialize RateCard currency: %w", err) | ||
| } | ||
| } else { | ||
| delete(fields, "currency") | ||
| } | ||
|
|
||
| data, err = json.Marshal(fields) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to JSON serialize RateCard: %w", err) | ||
| } | ||
|
|
||
| return data, nil | ||
| } | ||
|
|
||
| func rateCardJSONWithoutCurrency(data json.RawMessage) (json.RawMessage, error) { | ||
| var fields map[string]json.RawMessage | ||
| if err := json.Unmarshal(data, &fields); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| delete(fields, "currency") | ||
|
|
||
| return json.Marshal(fields) | ||
| } | ||
|
|
||
| func setRateCardCurrencyIdentity(rateCard productcatalog.RateCard, currency currencyx.CurrencyIdentity) error { | ||
| switch rateCard := rateCard.(type) { | ||
| case *productcatalog.FlatFeeRateCard: | ||
| rateCard.Currency = currency | ||
| case *productcatalog.UsageBasedRateCard: | ||
| rateCard.Currency = currency | ||
| default: | ||
| return fmt.Errorf("unsupported RateCard type: %T", rateCard) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove redundant map-based JSON rewriting.
The complex map[string]json.RawMessage manipulation to extract and inject the currency field is unnecessary. The underlying FlatFeeRateCard and UsageBasedRateCard types already correctly handle the serialization and deserialization of the currency field via field shadowing in their own MarshalJSON and UnmarshalJSON methods (in openmeter/productcatalog/ratecard.go).
You can safely delegate directly to the standard json.Marshal and json.Unmarshal functions, allowing you to delete marshalRateCardForJSON, rateCardJSONWithoutCurrency, and setRateCardCurrencyIdentity.
♻️ Proposed simplification
func (r *RateCard) MarshalJSON() ([]byte, error) {
- rateCard, err := marshalRateCardForJSON(r.RateCard)
- if err != nil {
- return nil, err
- }
-
serde := struct {
productcatalog.RateCardSerde
- RateCard json.RawMessage `json:"RateCard"`
+ RateCard productcatalog.RateCard `json:"RateCard"`
RateCardManagedFields
}{
RateCardSerde: productcatalog.RateCardSerde{
Type: r.Type(),
},
- RateCard: rateCard,
+ RateCard: r.RateCard,
RateCardManagedFields: r.RateCardManagedFields,
}
return json.Marshal(serde)
}
func (r *RateCard) UnmarshalJSON(b []byte) error {
var serialized struct {
productcatalog.RateCardSerde
RateCard json.RawMessage `json:"RateCard"`
RateCardManagedFields
}
err := json.Unmarshal(b, &serialized)
if err != nil {
return fmt.Errorf("failed to JSON deserialize RateCard type: %w", err)
}
var rateCard productcatalog.RateCard
switch serialized.Type {
case productcatalog.FlatFeeRateCardType:
rateCard = &productcatalog.FlatFeeRateCard{}
case productcatalog.UsageBasedRateCardType:
rateCard = &productcatalog.UsageBasedRateCard{}
default:
return fmt.Errorf("invalid RateCard type: %s", serialized.Type)
}
- var currencyData struct {
- Currency *currencyx.Code `json:"currency,omitempty"`
- }
- if err = json.Unmarshal(serialized.RateCard, ¤cyData); err != nil {
- return fmt.Errorf("failed to JSON deserialize RateCard currency: %w", err)
- }
-
- rateCardData, err := rateCardJSONWithoutCurrency(serialized.RateCard)
- if err != nil {
- return fmt.Errorf("failed to JSON deserialize RateCard: %w", err)
- }
- if err = json.Unmarshal(rateCardData, rateCard); err != nil {
+ if err = json.Unmarshal(serialized.RateCard, rateCard); err != nil {
return fmt.Errorf("failed to JSON deserialize RateCard: %w", err)
}
- if currencyData.Currency != nil {
- if err = setRateCardCurrencyIdentity(rateCard, *currencyData.Currency); err != nil {
- return fmt.Errorf("failed to set RateCard currency: %w", err)
- }
- }
-
r.RateCardManagedFields = serialized.RateCardManagedFields
r.RateCard = rateCard
return nil
}
-
-func marshalRateCardForJSON(rateCard productcatalog.RateCard) (json.RawMessage, error) {
-...
-func rateCardJSONWithoutCurrency(data json.RawMessage) (json.RawMessage, error) {
-...
-func setRateCardCurrencyIdentity(rateCard productcatalog.RateCard, currency currencyx.CurrencyIdentity) 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 `@openmeter/productcatalog/plan/ratecard.go` around lines 103 - 224, Remove the
map-based currency rewriting from RateCard MarshalJSON and UnmarshalJSON. In
MarshalJSON, delegate directly to json.Marshal on r.RateCard; in UnmarshalJSON,
unmarshal serialized.RateCard directly into the type-specific rateCard selected
by serialized.Type. Delete marshalRateCardForJSON, rateCardJSONWithoutCurrency,
and setRateCardCurrencyIdentity, while preserving existing error handling and
managed-field assignment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove redundant map-based JSON rewriting.
The complex map[string]json.RawMessage manipulation to extract and inject the currency field is unnecessary. The underlying FlatFeeRateCard and UsageBasedRateCard types already correctly handle the serialization and deserialization of the currency field via field shadowing in their own MarshalJSON and UnmarshalJSON methods (in openmeter/productcatalog/ratecard.go).
You can safely delegate directly to the standard json.Marshal and json.Unmarshal functions, allowing you to delete marshalRateCardForJSON, rateCardJSONWithoutCurrency, and setRateCardCurrencyIdentity.
♻️ Proposed simplification
func (r *RateCard) MarshalJSON() ([]byte, error) {
- rateCard, err := marshalRateCardForJSON(r.RateCard)
- if err != nil {
- return nil, err
- }
-
serde := struct {
productcatalog.RateCardSerde
- RateCard json.RawMessage `json:"RateCard"`
+ RateCard productcatalog.RateCard `json:"RateCard"`
RateCardManagedFields
}{
RateCardSerde: productcatalog.RateCardSerde{
Type: r.Type(),
},
- RateCard: rateCard,
+ RateCard: r.RateCard,
RateCardManagedFields: r.RateCardManagedFields,
}
return json.Marshal(serde)
}
func (r *RateCard) UnmarshalJSON(b []byte) error {
var serialized struct {
productcatalog.RateCardSerde
RateCard json.RawMessage `json:"RateCard"`
RateCardManagedFields
}
err := json.Unmarshal(b, &serialized)
if err != nil {
return fmt.Errorf("failed to JSON deserialize RateCard type: %w", err)
}
var rateCard productcatalog.RateCard
switch serialized.Type {
case productcatalog.FlatFeeRateCardType:
rateCard = &productcatalog.FlatFeeRateCard{}
case productcatalog.UsageBasedRateCardType:
rateCard = &productcatalog.UsageBasedRateCard{}
default:
return fmt.Errorf("invalid RateCard type: %s", serialized.Type)
}
- var currencyData struct {
- Currency *currencyx.Code `json:"currency,omitempty"`
- }
- if err = json.Unmarshal(serialized.RateCard, ¤cyData); err != nil {
- return fmt.Errorf("failed to JSON deserialize RateCard currency: %w", err)
- }
-
- rateCardData, err := rateCardJSONWithoutCurrency(serialized.RateCard)
- if err != nil {
- return fmt.Errorf("failed to JSON deserialize RateCard: %w", err)
- }
- if err = json.Unmarshal(rateCardData, rateCard); err != nil {
+ if err = json.Unmarshal(serialized.RateCard, rateCard); err != nil {
return fmt.Errorf("failed to JSON deserialize RateCard: %w", err)
}
- if currencyData.Currency != nil {
- if err = setRateCardCurrencyIdentity(rateCard, *currencyData.Currency); err != nil {
- return fmt.Errorf("failed to set RateCard currency: %w", err)
- }
- }
-
r.RateCardManagedFields = serialized.RateCardManagedFields
r.RateCard = rateCard
return nil
}
-
-func marshalRateCardForJSON(rateCard productcatalog.RateCard) (json.RawMessage, error) {
-...
-func rateCardJSONWithoutCurrency(data json.RawMessage) (json.RawMessage, error) {
-...
-func setRateCardCurrencyIdentity(rateCard productcatalog.RateCard, currency currencyx.CurrencyIdentity) error {
-...📝 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.
Suggested change
| rateCard, err := marshalRateCardForJSON(r.RateCard) | |
| if err != nil { | |
| return nil, err | |
| } | |
| serde := struct { | |
| productcatalog.RateCardSerde | |
| productcatalog.RateCard | |
| RateCard json.RawMessage `json:"RateCard"` | |
| RateCardManagedFields | |
| }{ | |
| RateCardSerde: productcatalog.RateCardSerde{ | |
| Type: r.Type(), | |
| }, | |
| RateCard: r.RateCard, | |
| RateCard: rateCard, | |
| RateCardManagedFields: r.RateCardManagedFields, | |
| } | |
| return json.Marshal(serde) | |
| } | |
| func (r *RateCard) UnmarshalJSON(b []byte) error { | |
| var s productcatalog.RateCardSerde | |
| err := json.Unmarshal(b, &s) | |
| var serialized struct { | |
| productcatalog.RateCardSerde | |
| RateCard json.RawMessage `json:"RateCard"` | |
| RateCardManagedFields | |
| } | |
| err := json.Unmarshal(b, &serialized) | |
| if err != nil { | |
| return fmt.Errorf("failed to JSON deserialize RateCard type: %w", err) | |
| } | |
| serde := struct { | |
| productcatalog.RateCard | |
| RateCardManagedFields | |
| }{ | |
| RateCardManagedFields: r.RateCardManagedFields, | |
| RateCard: r.RateCard, | |
| } | |
| switch s.Type { | |
| var rateCard productcatalog.RateCard | |
| switch serialized.Type { | |
| case productcatalog.FlatFeeRateCardType: | |
| serde.RateCard = &productcatalog.FlatFeeRateCard{} | |
| rateCard = &productcatalog.FlatFeeRateCard{} | |
| case productcatalog.UsageBasedRateCardType: | |
| serde.RateCard = &productcatalog.UsageBasedRateCard{} | |
| rateCard = &productcatalog.UsageBasedRateCard{} | |
| default: | |
| return fmt.Errorf("invalid RateCard type: %s", s.Type) | |
| return fmt.Errorf("invalid RateCard type: %s", serialized.Type) | |
| } | |
| err = json.Unmarshal(b, &serde) | |
| var currencyData struct { | |
| Currency *currencyx.Code `json:"currency,omitempty"` | |
| } | |
| if err = json.Unmarshal(serialized.RateCard, ¤cyData); err != nil { | |
| return fmt.Errorf("failed to JSON deserialize RateCard currency: %w", err) | |
| } | |
| rateCardData, err := rateCardJSONWithoutCurrency(serialized.RateCard) | |
| if err != nil { | |
| return fmt.Errorf("failed to JSON deserialize UsageBasedRateCard: %w", err) | |
| return fmt.Errorf("failed to JSON deserialize RateCard: %w", err) | |
| } | |
| if err = json.Unmarshal(rateCardData, rateCard); err != nil { | |
| return fmt.Errorf("failed to JSON deserialize RateCard: %w", err) | |
| } | |
| if currencyData.Currency != nil { | |
| if err = setRateCardCurrencyIdentity(rateCard, *currencyData.Currency); err != nil { | |
| return fmt.Errorf("failed to set RateCard currency: %w", err) | |
| } | |
| } | |
| r.RateCardManagedFields = serialized.RateCardManagedFields | |
| r.RateCard = rateCard | |
| return nil | |
| } | |
| r.RateCardManagedFields = serde.RateCardManagedFields | |
| r.RateCard = serde.RateCard | |
| func marshalRateCardForJSON(rateCard productcatalog.RateCard) (json.RawMessage, error) { | |
| currency := rateCard.AsMeta().Currency | |
| rateCardWithoutCurrency := rateCard.Clone() | |
| if err := setRateCardCurrencyIdentity(rateCardWithoutCurrency, nil); err != nil { | |
| return nil, fmt.Errorf("failed to prepare RateCard for JSON serialization: %w", err) | |
| } | |
| data, err := json.Marshal(rateCardWithoutCurrency) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to JSON serialize RateCard: %w", err) | |
| } | |
| var fields map[string]json.RawMessage | |
| if err = json.Unmarshal(data, &fields); err != nil { | |
| return nil, fmt.Errorf("failed to rewrite RateCard currency: %w", err) | |
| } | |
| if currency != nil { | |
| fields["currency"], err = json.Marshal(currency.GetCode()) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to JSON serialize RateCard currency: %w", err) | |
| } | |
| } else { | |
| delete(fields, "currency") | |
| } | |
| data, err = json.Marshal(fields) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to JSON serialize RateCard: %w", err) | |
| } | |
| return data, nil | |
| } | |
| func rateCardJSONWithoutCurrency(data json.RawMessage) (json.RawMessage, error) { | |
| var fields map[string]json.RawMessage | |
| if err := json.Unmarshal(data, &fields); err != nil { | |
| return nil, err | |
| } | |
| delete(fields, "currency") | |
| return json.Marshal(fields) | |
| } | |
| func setRateCardCurrencyIdentity(rateCard productcatalog.RateCard, currency currencyx.CurrencyIdentity) error { | |
| switch rateCard := rateCard.(type) { | |
| case *productcatalog.FlatFeeRateCard: | |
| rateCard.Currency = currency | |
| case *productcatalog.UsageBasedRateCard: | |
| rateCard.Currency = currency | |
| default: | |
| return fmt.Errorf("unsupported RateCard type: %T", rateCard) | |
| } | |
| func (r *RateCard) MarshalJSON() ([]byte, error) { | |
| serde := struct { | |
| productcatalog.RateCardSerde | |
| RateCard productcatalog.RateCard `json:"RateCard"` | |
| RateCardManagedFields | |
| }{ | |
| RateCardSerde: productcatalog.RateCardSerde{ | |
| Type: r.Type(), | |
| }, | |
| RateCard: r.RateCard, | |
| RateCardManagedFields: r.RateCardManagedFields, | |
| } | |
| return json.Marshal(serde) | |
| } | |
| func (r *RateCard) UnmarshalJSON(b []byte) error { | |
| var serialized struct { | |
| productcatalog.RateCardSerde | |
| RateCard json.RawMessage `json:"RateCard"` | |
| RateCardManagedFields | |
| } | |
| err := json.Unmarshal(b, &serialized) | |
| if err != nil { | |
| return fmt.Errorf("failed to JSON deserialize RateCard type: %w", err) | |
| } | |
| var rateCard productcatalog.RateCard | |
| switch serialized.Type { | |
| case productcatalog.FlatFeeRateCardType: | |
| rateCard = &productcatalog.FlatFeeRateCard{} | |
| case productcatalog.UsageBasedRateCardType: | |
| rateCard = &productcatalog.UsageBasedRateCard{} | |
| default: | |
| return fmt.Errorf("invalid RateCard type: %s", serialized.Type) | |
| } | |
| if err = json.Unmarshal(serialized.RateCard, rateCard); err != nil { | |
| return fmt.Errorf("failed to JSON deserialize RateCard: %w", err) | |
| } | |
| r.RateCardManagedFields = serialized.RateCardManagedFields | |
| r.RateCard = rateCard | |
| return nil | |
| } |
🤖 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 `@openmeter/productcatalog/plan/ratecard.go` around lines 103 - 224, Remove the
map-based currency rewriting from RateCard.MarshalJSON and
RateCard.UnmarshalJSON. Delegate serialization and deserialization directly to
json.Marshal and json.Unmarshal on the concrete FlatFeeRateCard and
UsageBasedRateCard values, then delete marshalRateCardForJSON,
rateCardJSONWithoutCurrency, and setRateCardCurrencyIdentity while preserving
the existing type selection and managed-field handling.
Comment on lines
+924
to
+928
| storedRateCard := stored.Phases[0].RateCards[0].AsMeta() | ||
| require.NotNil(t, storedRateCard.Price, "the custom currency must qualify a real price") | ||
| require.NotNil(t, storedRateCard.Currency) | ||
| require.Equal(t, customCurrency, storedRateCard.Currency.GetCode()) | ||
| require.Equal(t, customCurrency, storedRateCard.EffectiveCurrency(stored.Currency).GetCode()) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the managed currency identity, not just its code.
These assertions still pass if hydration regresses to a code-only identity or resolves a different resource reusing CREDITS. Check currencyx.ManagedCurrency and its ID.
Proposed assertion
require.NotNil(t, storedRateCard.Currency)
+ storedCurrency, ok := storedRateCard.Currency.(currencyx.ManagedCurrency)
+ require.True(t, ok, "the persisted override must hydrate as a managed currency")
+ require.Equal(t, managedCurrency.ID, storedCurrency.GetID())
require.Equal(t, customCurrency, storedRateCard.Currency.GetCode())As per path instructions, suggest coverage when the current test would pass despite a concrete regression in changed behavior.
📝 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.
Suggested change
| storedRateCard := stored.Phases[0].RateCards[0].AsMeta() | |
| require.NotNil(t, storedRateCard.Price, "the custom currency must qualify a real price") | |
| require.NotNil(t, storedRateCard.Currency) | |
| require.Equal(t, customCurrency, storedRateCard.Currency.GetCode()) | |
| require.Equal(t, customCurrency, storedRateCard.EffectiveCurrency(stored.Currency).GetCode()) | |
| storedRateCard := stored.Phases[0].RateCards[0].AsMeta() | |
| require.NotNil(t, storedRateCard.Price, "the custom currency must qualify a real price") | |
| require.NotNil(t, storedRateCard.Currency) | |
| storedCurrency, ok := storedRateCard.Currency.(currencyx.ManagedCurrency) | |
| require.True(t, ok, "the persisted override must hydrate as a managed currency") | |
| require.Equal(t, managedCurrency.ID, storedCurrency.GetID()) | |
| require.Equal(t, customCurrency, storedRateCard.Currency.GetCode()) | |
| require.Equal(t, customCurrency, storedRateCard.EffectiveCurrency(stored.Currency).GetCode()) |
🤖 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 `@openmeter/productcatalog/plan/service/service_test.go` around lines 924 -
928, Strengthen the assertions in the stored rate-card test around
storedRateCard.Currency and EffectiveCurrency to validate the managed currency
resource identity, not only its code. Assert both values are
currencyx.ManagedCurrency instances and compare their IDs with the expected
custom managed currency ID, preserving the existing code checks and ensuring
hydration cannot resolve a different resource sharing CREDITS.
Source: Path instructions
Comment on lines
+148
to
+150
| if err = pa.ValidateWith( | ||
| productcatalog.ValidatePlanAddonRateCardCurrencies(), | ||
| productcatalog.ValidatePlanAddonWithCurrencies(ctx, params.Namespace, s.currencyResolver), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Run full assignment validation before the currency-only check.
This path never calls pa.Validate(). Since the currency validator intentionally ignores unknown starting phases, invalid phase keys, quantities, cadence, or overlays can reach persistence. Please mirror the update path, then run the resolver-backed validation.
Proposed fix
- if err = pa.ValidateWith(
- productcatalog.ValidatePlanAddonRateCardCurrencies(),
- productcatalog.ValidatePlanAddonWithCurrencies(ctx, params.Namespace, s.currencyResolver),
- ); err != nil {
+ if err = pa.Validate(); err != nil {
+ return nil, models.NewGenericValidationError(
+ fmt.Errorf("invalid plan add-on assignment [namespace=%s plan.id=%s addon.id=%s]: %w",
+ params.Namespace, params.PlanID, params.AddonID, err),
+ )
+ }
+
+ if err = pa.ValidateWith(
+ productcatalog.ValidatePlanAddonWithCurrencies(ctx, params.Namespace, s.currencyResolver),
+ ); err != 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.
Suggested change
| if err = pa.ValidateWith( | |
| productcatalog.ValidatePlanAddonRateCardCurrencies(), | |
| productcatalog.ValidatePlanAddonWithCurrencies(ctx, params.Namespace, s.currencyResolver), | |
| if err = pa.Validate(); err != nil { | |
| return nil, models.NewGenericValidationError( | |
| fmt.Errorf("invalid plan add-on assignment [namespace=%s plan.id=%s addon.id=%s]: %w", | |
| params.Namespace, params.PlanID, params.AddonID, err), | |
| ) | |
| } | |
| if err = pa.ValidateWith( | |
| productcatalog.ValidatePlanAddonWithCurrencies(ctx, params.Namespace, s.currencyResolver), | |
| ); err != nil { |
🤖 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 `@openmeter/productcatalog/planaddon/service/planaddon.go` around lines 148 -
150, Update the create flow around ValidateWith to first run the full plan-addon
validation, mirroring the update path, before executing the resolver-backed
currency validation. Ensure invalid phases, keys, quantities, cadence, and
overlays are rejected before persistence while retaining the existing
currency-only check afterward.
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 `@test/productcatalog/custom_currency_test.go`:
- Around line 28-31: Update newTestEnv to set POSTGRES_HOST to 127.0.0.1 before
calling pctestutils.NewTestEnv(t), ensuring the shared database-backed test
setup uses the explicit PostgreSQL host.
🪄 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: 3743cd13-8049-4302-a3ed-351a095cba2c
📒 Files selected for processing (4)
api/v3/handlers/addons/convert.goapi/v3/handlers/addons/convert_test.goe2e/v1readsurface_currencyoverride_test.gotest/productcatalog/custom_currency_test.go
Comment on lines
+28
to
+31
| func newTestEnv(t *testing.T) *testEnv { | ||
| t.Helper() | ||
|
|
||
| env := &testEnv{TestEnv: pctestutils.NewTestEnv(t)} |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Set the PostgreSQL host in the shared test setup.
These database-backed tests should pin the host before creating TestEnv.
Proposed fix
func newTestEnv(t *testing.T) *testEnv {
t.Helper()
+ t.Setenv("POSTGRES_HOST", "127.0.0.1")
env := &testEnv{TestEnv: pctestutils.NewTestEnv(t)}As per coding guidelines, “For database-touching OpenMeter Go tests, explicitly set POSTGRES_HOST=127.0.0.1.”
📝 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.
Suggested change
| func newTestEnv(t *testing.T) *testEnv { | |
| t.Helper() | |
| env := &testEnv{TestEnv: pctestutils.NewTestEnv(t)} | |
| func newTestEnv(t *testing.T) *testEnv { | |
| t.Helper() | |
| t.Setenv("POSTGRES_HOST", "127.0.0.1") | |
| env := &testEnv{TestEnv: pctestutils.NewTestEnv(t)} |
🤖 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 `@test/productcatalog/custom_currency_test.go` around lines 28 - 31, Update
newTestEnv to set POSTGRES_HOST to 127.0.0.1 before calling
pctestutils.NewTestEnv(t), ensuring the shared database-backed test setup uses
the explicit PostgreSQL host.
Source: Coding guidelines
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.
What changed
Database migration
Validation
go test -tags=dynamic ./openmeter/productcatalog/... ./openmeter/subscription/servicego test -tags=dynamic ./tools/migrate -run '^TestProductCatalogCurrencyReferencesMigration$' -count=1go vet -tags=dynamic ./openmeter/productcatalog/... ./openmeter/subscription/service ./tools/migrateBuilt on the currency domain contract introduced by #4701.
Summary by CodeRabbit