feat(ent): Add custom currencies to the database schema#3888
Merged
Conversation
mark-vass-konghq
added
the
release-note/feature
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds new Ent schemas for custom currencies and cost-basis history with a TimeMixin and immutable fields, plus SQL migrations that create/drop the related tables, indexes, and foreign-key relationships (including a separate effective_from history table). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 9
🧹 Nitpick comments (8)
api/spec/src/v3/currencies/cost-bases/cost-basis.tsp (1)
9-12: Quick note on the@friendlyNameThe friendly name is
"BillingCostBasis"but the model isCostBasisunder theCurrenciesnamespace, not underBilling. This is totally fine if "Billing" is the intended domain prefix for generated code, just wanted to flag the naming mismatch in case it's unintentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp` around lines 9 - 12, The model CostBasis in the Currencies namespace has a `@friendlyName` of "BillingCostBasis", creating a naming mismatch; confirm intent and if unintended, update the `@friendlyName` to match the model/domain (e.g., "CurrenciesCostBasis" or "CostBasis") or move/rename the model to the Billing domain; locate the CostBasis model declaration and the `@friendlyName` attribute and make the name consistent with the intended generated-code prefix.tools/migrate/migrations/20260220154446_add_custom_currency.up.sql (1)
10-11: Redundant unique indexes on primary key columnsLines 11 and 26 create unique indexes on
idcolumns that are already primary keys. PKs inherently have unique indexes. This is likely auto-generated by Ent so probably not worth fighting, but worth knowing they add a tiny bit of storage/write overhead for no benefit.Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260220154446_add_custom_currency.up.sql` around lines 10 - 11, Remove the redundant UNIQUE indexes on the primary key columns: delete the CREATE UNIQUE INDEX "customcurrency_id" ON "custom_currencies" ("id"); statement (and the similar duplicate around lines 25-26) from the migration so you don't create a second unique index on the PK; ensure there are no other statements depending on those index names and re-generate or re-run the migration after confirming the primary key constraint alone provides the necessary uniqueness.openmeter/ent/schema/custom_currencies.go (1)
17-25: Good catch adding aTimeMixin—this is new and solid!There's no existing
TimeMixinin the codebase, so you're introducing something useful. That said, I did notice thatcreated_atfield definitions are duplicated in a few places (likecustomer.goandnotification.go). Consider using this mixin in those files going forward to keep things consistent and DRY. No need to refactor existing code as part of this PR, but it's worth thinking about as the pattern grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 17 - 25, You added a new TimeMixin with TimeMixin.Fields() that defines the created_at field, but created_at is still duplicated in other schemas (e.g., customer.go and notification.go); update those schema files to remove their local created_at field declarations and instead include the TimeMixin in their mixins() by referencing TimeMixin (or embed mixin.Schema where appropriate) so the created_at column is defined centrally by TimeMixin and not duplicated across Customer and Notification schema definitions.api/spec/src/v3/currencies/operations.tsp (1)
40-43: RedundantCurrencies.namespace prefix on the request bodySince we're already inside
namespace Currencies,Currencies.CustomCurrencycan just beCustomCurrency. Tiny nit but it's cleaner.✨ Suggested fix
create( `@header` contentType: "application/json", - `@body` body: Currencies.CustomCurrency, + `@body` body: CustomCurrency, ): Shared.CreateResponse<CustomCurrency> | Common.ErrorResponses;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/operations.tsp` around lines 40 - 43, Inside the Currencies namespace the create method's body type uses a redundant qualified name; change the parameter type from Currencies.CustomCurrency to CustomCurrency in the create(...) signature (the method named create in this file), leaving the return type Shared.CreateResponse<CustomCurrency> unchanged so the local type is used without the unnecessary Currencies. prefix.api/v3/server/routes.go (1)
143-159: Stubs look good — just tracking the follow-upThe delegation to
unimplementedis the right pattern for work in progress, and all four route methods are wired correctly. Once the actual handlers are ready, you'll want to add acurrenciesHandler(or similar) to theServerstruct and swap these out, following the same pattern ass.metersHandler,s.billingProfilesHandler, etc.Want me to open a follow-up issue to track implementing the real handler?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/server/routes.go` around lines 143 - 159, These route methods currently delegate to unimplemented stubs; add a concrete handler field (e.g., currenciesHandler) to the Server struct and replace the unimplemented calls in ListCurrencies, CreateCustomCurrency, CreateCostBasis, and GetCostBases to delegate to s.currenciesHandler.ListCurrencies, s.currenciesHandler.CreateCustomCurrency, s.currenciesHandler.CreateCostBasis, and s.currenciesHandler.GetCostBases respectively (matching the existing pattern used for s.metersHandler and s.billingProfilesHandler), and ensure the Server constructor/initializer wires an instance of the new currenciesHandler so the routes call real implementations instead of unimplemented.*.api/spec/src/v3/currencies/custom.tsp (1)
1-1: Add explicitimport "./base.tsp"for consistency
custom.tspusesCurrencyBaseandCustomCurrencyCodewhich are defined inbase.tsp, but that file isn't imported here. It works through the shared compilation unit, butcurrency.tspdoes this explicitly — it'd be great to keep things consistent so the dependency is clear at a glance.✨ Suggested fix
import "../shared/index.tsp"; +import "./base.tsp";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/custom.tsp` at line 1, Add an explicit import of the base file in custom.tsp so the dependencies are obvious: update api/spec/src/v3/currencies/custom.tsp to import "./base.tsp" in addition to "../shared/index.tsp" so types used in this file (CurrencyBase and CustomCurrencyCode) are directly referenced; ensure the import is placed at the top of the file before any usage of CurrencyBase or CustomCurrencyCode.api/v3/api.gen.go (1)
2048-2052: Request body types include read-only fields (id,created_at)
CreateCustomCurrencyJSONRequestBody = BillingCustomCurrencyandCreateCostBasisJSONRequestBody = BillingCostBasisboth include server-generated fields (id,created_at) that are@visibility(Lifecycle.Read)in the TypeSpec source. Clients can technically send these in the request payload, where they'll just be silently ignored.This is a TypeSpec-level design choice — other endpoints in this API use dedicated "create request" types (e.g.,
CreateCustomerRequest,CreateBillingProfileRequest) that exclude read-only fields. Using a dedicated create type forCustomCurrencyandCostBasiswould make the contract clearer for API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 2048 - 2052, The request body aliases CreateCustomCurrencyJSONRequestBody and CreateCostBasisJSONRequestBody currently point to BillingCustomCurrency and BillingCostBasis (which include read-only fields like id and created_at); change the API contract to use dedicated create-only types that exclude read-only fields by defining new TypeSpec create types (e.g., CreateCustomCurrencyRequest and CreateCostBasisRequest) and update the generated aliases to point to these new types instead of BillingCustomCurrency/BillingCostBasis; then regenerate api.gen.go so CreateCustomCurrencyJSONRequestBody -> CreateCustomCurrencyRequest and CreateCostBasisJSONRequestBody -> CreateCostBasisRequest, ensuring server-generated fields remain omitted from create payloads.api/spec/src/v3/currencies/base.tsp (1)
1-4: Add explicitimport "@typespec/openapi"for clarity
base.tspuses theTypeSpec.OpenAPInamespace but only imports@typespec/openapi3. While this currently works becausecurrency.tsp(which does import@typespec/openapi) depends on this file, it creates an implicit transitive dependency. Making the import explicit would make the file self-contained and clearer about its actual dependencies.Suggested fix
+import "@typespec/openapi"; import "@typespec/openapi3"; import "../shared/index.tsp";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/base.tsp` around lines 1 - 4, base.tsp relies on the TypeSpec.OpenAPI namespace but only imports "@typespec/openapi3", creating a transitive dependency; fix it by adding an explicit top-level import of "@typespec/openapi" in base.tsp so the file is self-contained and still uses using TypeSpec.OpenAPI; ensure the new import appears alongside the existing imports before the using statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/spec/src/v3/currencies/base.tsp`:
- Line 12: The `@friendlyName` on CurrencyBase is misleadingly named
"BillingCustomCurrencyBase" even though both Currency and CustomCurrency extend
it; rename the decorator value to "BillingCurrencyBase" on the CurrencyBase type
(update the `@friendlyName`("BillingCustomCurrencyBase") to
`@friendlyName`("BillingCurrencyBase")) and then update all references/usages of
that friendly name elsewhere (tests, docs, or type refs) to the new
"BillingCurrencyBase" so names remain consistent with the shared base used by
Currency and CustomCurrency.
- Around line 26-30: The currency "symbol" field lacks length constraints which
can lead to DB/UI issues; update the symbol declaration in base.tsp (the symbol
field with `@visibility`(Lifecycle.Create, Lifecycle.Read)) to include length
decorators, e.g. add `@minLength`(1) and `@maxLength`(3) (or another agreed upper
bound) so the symbol is constrained to 1–3 characters to match typical currency
symbols.
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp`:
- Around line 14-18: The docstring for fiat_code is misleading—it's a fiat
currency code (e.g., "USD") not the parent custom currency ID—so update the
comment on fiat_code to clearly state its purpose (e.g., "The fiat currency code
for the exchange rate" or "The fiat currency code (e.g., 'USD') used for the
exchange rate") and optionally mention that the parent custom currency relation
is provided via the currencyId path parameter; modify the comment above
fiat_code in the cost-basis type accordingly.
In `@api/spec/src/v3/currencies/cost-bases/operations.tsp`:
- Around line 20-22: Update the doc comment to match the operation and return
type: change the comment for the get_cost_bases operation from "Get a cost
basis." to a plural form like "Get cost bases." to reflect that get_cost_bases
returns CostBases (a list); locate the comment above the get_cost_bases
operation in operations.tsp and make the wording consistent with the operation
name and return type.
In `@api/spec/src/v3/currencies/currency.tsp`:
- Around line 18-23: The doc comment for the `code` field is misleading because
`code: Shared.CurrencyCode | CustomCurrencyCode` can be a custom code when
`is_custom` is true; update the comment on `code` to state that it is an ISO
4217 currency code for standard currencies (Shared.CurrencyCode) or a custom
currency code when `is_custom` is true (CustomCurrencyCode), so API consumers
understand both cases and the relation to the `is_custom` flag.
In `@api/spec/src/v3/currencies/operations.tsp`:
- Around line 18-24: CurrenciesResponse currently only has `@pageItems` data:
Currency[] and no pagination meta or list query params; align it with other list
endpoints by adding a meta: PaginatedMeta (or Meta PaginatedMeta) field to the
CurrenciesResponse model and update the corresponding list operation to accept
the same page query params used by other endpoints so responses include
pagination metadata; if returning the full set is intentional instead, add a
short comment on the CurrenciesResponse model explaining that the currency list
is intentionally non-paginated to prevent future accidental changes.
In `@api/v3/api.gen.go`:
- Around line 882-885: The generated comment for FiatCode is incorrect: update
the upstream TypeSpec for the cost-basis spec (the field named FiatCode of type
CurrencyCode) so its doc/comment describes a currency code (e.g., "The currency
code this cost basis belongs to, e.g. 'USD' or 'EUR'") instead of "currency id",
then re-run the code generator to update api.gen.go; target the TypeSpec field
named FiatCode (type CurrencyCode) when making the change and regenerate
artifacts.
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 27-50: Add a unique database index on the currency code by
implementing the CustomCurrency Indexes() method: in the ent schema for
CustomCurrency, add an Indexes() []ent.Index that returns a unique index (e.g.,
index.Unique("custom_currency_code_idx").Fields("code")) so the "code" field
(defined in Fields()) is enforced unique at the DB level; reference the
CustomCurrency type and the "code" field when adding the index.
In `@tools/migrate/migrations/20260220154446_add_custom_currency.up.sql`:
- Around line 2-11: Add a unique constraint on the custom_currencies.code
column: update the Ent schema field for "code" to include .Unique() (so the
generated migration will create a unique index) and regenerate the migration;
then update the SQL migration to create a UNIQUE index on "code" in the
"custom_currencies" table instead of leaving it non-unique (ensure the index
name is distinct, e.g., customcurrency_code). This fixes duplicates by enforcing
uniqueness at the DB level for the code column referenced by the schema field
"code".
---
Nitpick comments:
In `@api/spec/src/v3/currencies/base.tsp`:
- Around line 1-4: base.tsp relies on the TypeSpec.OpenAPI namespace but only
imports "@typespec/openapi3", creating a transitive dependency; fix it by adding
an explicit top-level import of "@typespec/openapi" in base.tsp so the file is
self-contained and still uses using TypeSpec.OpenAPI; ensure the new import
appears alongside the existing imports before the using statement.
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp`:
- Around line 9-12: The model CostBasis in the Currencies namespace has a
`@friendlyName` of "BillingCostBasis", creating a naming mismatch; confirm intent
and if unintended, update the `@friendlyName` to match the model/domain (e.g.,
"CurrenciesCostBasis" or "CostBasis") or move/rename the model to the Billing
domain; locate the CostBasis model declaration and the `@friendlyName` attribute
and make the name consistent with the intended generated-code prefix.
In `@api/spec/src/v3/currencies/custom.tsp`:
- Line 1: Add an explicit import of the base file in custom.tsp so the
dependencies are obvious: update api/spec/src/v3/currencies/custom.tsp to import
"./base.tsp" in addition to "../shared/index.tsp" so types used in this file
(CurrencyBase and CustomCurrencyCode) are directly referenced; ensure the import
is placed at the top of the file before any usage of CurrencyBase or
CustomCurrencyCode.
In `@api/spec/src/v3/currencies/operations.tsp`:
- Around line 40-43: Inside the Currencies namespace the create method's body
type uses a redundant qualified name; change the parameter type from
Currencies.CustomCurrency to CustomCurrency in the create(...) signature (the
method named create in this file), leaving the return type
Shared.CreateResponse<CustomCurrency> unchanged so the local type is used
without the unnecessary Currencies. prefix.
In `@api/v3/api.gen.go`:
- Around line 2048-2052: The request body aliases
CreateCustomCurrencyJSONRequestBody and CreateCostBasisJSONRequestBody currently
point to BillingCustomCurrency and BillingCostBasis (which include read-only
fields like id and created_at); change the API contract to use dedicated
create-only types that exclude read-only fields by defining new TypeSpec create
types (e.g., CreateCustomCurrencyRequest and CreateCostBasisRequest) and update
the generated aliases to point to these new types instead of
BillingCustomCurrency/BillingCostBasis; then regenerate api.gen.go so
CreateCustomCurrencyJSONRequestBody -> CreateCustomCurrencyRequest and
CreateCostBasisJSONRequestBody -> CreateCostBasisRequest, ensuring
server-generated fields remain omitted from create payloads.
In `@api/v3/server/routes.go`:
- Around line 143-159: These route methods currently delegate to unimplemented
stubs; add a concrete handler field (e.g., currenciesHandler) to the Server
struct and replace the unimplemented calls in ListCurrencies,
CreateCustomCurrency, CreateCostBasis, and GetCostBases to delegate to
s.currenciesHandler.ListCurrencies, s.currenciesHandler.CreateCustomCurrency,
s.currenciesHandler.CreateCostBasis, and s.currenciesHandler.GetCostBases
respectively (matching the existing pattern used for s.metersHandler and
s.billingProfilesHandler), and ensure the Server constructor/initializer wires
an instance of the new currenciesHandler so the routes call real implementations
instead of unimplemented.*.
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 17-25: You added a new TimeMixin with TimeMixin.Fields() that
defines the created_at field, but created_at is still duplicated in other
schemas (e.g., customer.go and notification.go); update those schema files to
remove their local created_at field declarations and instead include the
TimeMixin in their mixins() by referencing TimeMixin (or embed mixin.Schema
where appropriate) so the created_at column is defined centrally by TimeMixin
and not duplicated across Customer and Notification schema definitions.
In `@tools/migrate/migrations/20260220154446_add_custom_currency.up.sql`:
- Around line 10-11: Remove the redundant UNIQUE indexes on the primary key
columns: delete the CREATE UNIQUE INDEX "customcurrency_id" ON
"custom_currencies" ("id"); statement (and the similar duplicate around lines
25-26) from the migration so you don't create a second unique index on the PK;
ensure there are no other statements depending on those index names and
re-generate or re-run the migration after confirming the primary key constraint
alone provides the necessary uniqueness.
| @maxLength(24) | ||
| scalar CustomCurrencyCode extends string; | ||
|
|
||
| @friendlyName("BillingCustomCurrencyBase") |
Contributor
There was a problem hiding this comment.
@friendlyName("BillingCustomCurrencyBase") is a bit misleading
Both Currency and CustomCurrency spread CurrencyBase, so calling it BillingCustomCurrencyBase implies it's custom-only when it's actually the shared base for all currencies. BillingCurrencyBase would be clearer.
✨ Suggested fix
-@friendlyName("BillingCustomCurrencyBase")
+@friendlyName("BillingCurrencyBase")
model CurrencyBase {📝 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
| @friendlyName("BillingCustomCurrencyBase") | |
| `@friendlyName`("BillingCurrencyBase") | |
| model CurrencyBase { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/src/v3/currencies/base.tsp` at line 12, The `@friendlyName` on
CurrencyBase is misleadingly named "BillingCustomCurrencyBase" even though both
Currency and CustomCurrency extend it; rename the decorator value to
"BillingCurrencyBase" on the CurrencyBase type (update the
`@friendlyName`("BillingCustomCurrencyBase") to
`@friendlyName`("BillingCurrencyBase")) and then update all references/usages of
that friendly name elsewhere (tests, docs, or type refs) to the new
"BillingCurrencyBase" so names remain consistent with the shared base used by
Currency and CustomCurrency.
Comment on lines
+26
to
+30
| /** | ||
| * Symbol representation of the currency. | ||
| */ | ||
| @visibility(Lifecycle.Create, Lifecycle.Read) | ||
| symbol: string; |
Contributor
There was a problem hiding this comment.
symbol field is missing a length constraint
name has @maxLength(256) but symbol has no bounds at all. Currency symbols are typically 1–3 characters — an unbounded string could cause surprises at the DB layer or in UI rendering. At minimum a reasonable @maxLength would be nice.
✨ Suggested fix
/**
* Symbol representation of the currency.
*/
`@visibility`(Lifecycle.Create, Lifecycle.Read)
+ `@minLength`(1)
+ `@maxLength`(10)
symbol: string;📝 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
| /** | |
| * Symbol representation of the currency. | |
| */ | |
| @visibility(Lifecycle.Create, Lifecycle.Read) | |
| symbol: string; | |
| /** | |
| * Symbol representation of the currency. | |
| */ | |
| `@visibility`(Lifecycle.Create, Lifecycle.Read) | |
| `@minLength`(1) | |
| `@maxLength`(10) | |
| symbol: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/src/v3/currencies/base.tsp` around lines 26 - 30, The currency
"symbol" field lacks length constraints which can lead to DB/UI issues; update
the symbol declaration in base.tsp (the symbol field with
`@visibility`(Lifecycle.Create, Lifecycle.Read)) to include length decorators,
e.g. add `@minLength`(1) and `@maxLength`(3) (or another agreed upper bound) so the
symbol is constrained to 1–3 characters to match typical currency symbols.
2014854 to
e92a87a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/ent/schema/custom_currencies.go (1)
27-50:CustomCurrencystill missing a unique index oncode.Without a unique constraint on
code, duplicate currency codes can be inserted, which would undermine data integrity. This was flagged in a prior review and still needs to be addressed.Suggested fix
func (CustomCurrency) Edges() []ent.Edge { return []ent.Edge{ edge.To("cost_basis_history", CurrencyCostBasis.Type), } } + +func (CustomCurrency) Indexes() []ent.Index { + return []ent.Index{ + index.Fields("code").Unique(), + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 27 - 50, The schema lacks a unique index for the "code" field on CustomCurrency, so add an Indexes() method on the CustomCurrency schema that returns an ent.Index built with index.Fields("code").Unique() (i.e., implement func (CustomCurrency) Indexes() []ent.Index { return []ent.Index{ index.Fields("code").Unique() } }), keeping the existing Fields() definition for "code" intact so duplicate currency codes are prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 27-50: CustomCurrency and CurrencyCostBasis need tenant scoping:
add entutils.NamespaceMixin{} to the Mixin() return for both schemas (in
addition to existing entutils.IDMixin{} and TimeMixin{}) and add compound
indexes on the namespace field plus the unique identifying fields following the
pattern used in Subject, Subscription, Meter (e.g., include "namespace" in any
unique/index definitions for code/name on CustomCurrency and the relevant fields
on CurrencyCostBasis) so tenant isolation is enforced; update the Edges/Fields'
schema files for CustomCurrency and CurrencyCostBasis accordingly.
---
Duplicate comments:
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 27-50: The schema lacks a unique index for the "code" field on
CustomCurrency, so add an Indexes() method on the CustomCurrency schema that
returns an ent.Index built with index.Fields("code").Unique() (i.e., implement
func (CustomCurrency) Indexes() []ent.Index { return []ent.Index{
index.Fields("code").Unique() } }), keeping the existing Fields() definition for
"code" intact so duplicate currency codes are prevented.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
go.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/where.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_create.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_delete.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_query.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_update.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_delete.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/ent.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
openmeter/ent/schema/custom_currencies.gotools/migrate/migrations/20260220154446_add_custom_currency.down.sqltools/migrate/migrations/20260220154446_add_custom_currency.up.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/migrate/migrations/20260220154446_add_custom_currency.up.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tools/migrate/migrations/20260223180500_add_custom_currency.up.sql (1)
5-5:codestill missing a unique constraint — this was flagged in a previous review round and doesn't appear to have been addressed yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260223180500_add_custom_currency.up.sql` at line 5, The migration creates the custom_currency table but the code column lacks a unique constraint; update the migration to enforce uniqueness by either changing the column definition to "code" character varying NOT NULL UNIQUE or adding an explicit statement like ALTER TABLE custom_currency ADD CONSTRAINT custom_currency_code_key UNIQUE (code), ensuring the constraint name is descriptive and the change is applied in this up migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/migrate/migrations/20260223180500_add_custom_currency.up.sql`:
- Around line 10-11: The migration adds redundant UNIQUE indexes on primary key
columns; remove the CREATE UNIQUE INDEX statements for "customcurrency_id" on
table "custom_currencies" and the similar "currencycostbasis_id" index (lines
referenced) so you don't duplicate the automatically-created PK indexes and
avoid extra write overhead; locate and delete the CREATE UNIQUE INDEX
"customcurrency_id" ... ("id") and the CREATE UNIQUE INDEX
"currencycostbasis_id" ... statements from the migration file, leaving the
PRIMARY KEY definitions intact.
- Around line 19-24: The foreign key column custom_currency_cost_basis_history
on table currency_cost_bases lacks a dedicated index, causing sequential scans
when checking/deleting referenced rows in custom_currencies; add a standalone
index on currency_cost_bases(custom_currency_cost_basis_history) (e.g., name it
currencycostbasis_custom_currency_cost_basis_history_idx) so FK lookups and
cascades can use an index rather than the existing composite index
currencycostbasis_fiat_code_effective_from_custom_currency_cost.
---
Duplicate comments:
In `@tools/migrate/migrations/20260223180500_add_custom_currency.up.sql`:
- Line 5: The migration creates the custom_currency table but the code column
lacks a unique constraint; update the migration to enforce uniqueness by either
changing the column definition to "code" character varying NOT NULL UNIQUE or
adding an explicit statement like ALTER TABLE custom_currency ADD CONSTRAINT
custom_currency_code_key UNIQUE (code), ensuring the constraint name is
descriptive and the change is applied in this up migration.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
tools/migrate/migrations/20260223180500_add_custom_currency.down.sqltools/migrate/migrations/20260223180500_add_custom_currency.up.sql
Comment on lines
+10
to
+11
| -- create index "customcurrency_id" to table: "custom_currencies" | ||
| CREATE UNIQUE INDEX "customcurrency_id" ON "custom_currencies" ("id"); |
Contributor
There was a problem hiding this comment.
Redundant unique indexes on id — these are already covered by the PRIMARY KEY.
PostgreSQL automatically creates a unique index when a PRIMARY KEY is defined; there's no need to manually create indexes on those columns, as doing so would just duplicate the automatically-created index. Both customcurrency_id and currencycostbasis_id indexes can be dropped — they add write overhead without any benefit.
🗑️ Suggested fix
-- create index "customcurrency_id" to table: "custom_currencies"
-CREATE UNIQUE INDEX "customcurrency_id" ON "custom_currencies" ("id"); -- create index "currencycostbasis_id" to table: "currency_cost_bases"
-CREATE UNIQUE INDEX "currencycostbasis_id" ON "currency_cost_bases" ("id");Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/migrate/migrations/20260223180500_add_custom_currency.up.sql` around
lines 10 - 11, The migration adds redundant UNIQUE indexes on primary key
columns; remove the CREATE UNIQUE INDEX statements for "customcurrency_id" on
table "custom_currencies" and the similar "currencycostbasis_id" index (lines
referenced) so you don't duplicate the automatically-created PK indexes and
avoid extra write overhead; locate and delete the CREATE UNIQUE INDEX
"customcurrency_id" ... ("id") and the CREATE UNIQUE INDEX
"currencycostbasis_id" ... statements from the migration file, leaving the
PRIMARY KEY definitions intact.
Comment on lines
+19
to
+24
| "custom_currency_cost_basis_history" character(26) NOT NULL, | ||
| PRIMARY KEY ("id"), | ||
| CONSTRAINT "currency_cost_bases_custom_currencies_cost_basis_history" FOREIGN KEY ("custom_currency_cost_basis_history") REFERENCES "custom_currencies" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION | ||
| ); | ||
| -- create index "currencycostbasis_fiat_code_effective_from_custom_currency_cost" to table: "currency_cost_bases" | ||
| CREATE UNIQUE INDEX "currencycostbasis_fiat_code_effective_from_custom_currency_cost" ON "currency_cost_bases" ("fiat_code", "effective_from", "custom_currency_cost_basis_history"); |
Contributor
There was a problem hiding this comment.
Missing index on the FK column custom_currency_cost_basis_history.
The only index that covers this column is the composite one on (fiat_code, effective_from, custom_currency_cost_basis_history) where it's the third element. PostgreSQL can't use that index for FK enforcement or lookups filtered solely by custom_currency_cost_basis_history, resulting in a sequential scan on currency_cost_bases when the parent custom_currencies row is checked. Worth adding a dedicated index:
⚡ Suggested fix
+-- create index on FK column "custom_currency_cost_basis_history"
+CREATE INDEX "currencycostbasis_custom_currency" ON "currency_cost_bases" ("custom_currency_cost_basis_history");📝 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
| "custom_currency_cost_basis_history" character(26) NOT NULL, | |
| PRIMARY KEY ("id"), | |
| CONSTRAINT "currency_cost_bases_custom_currencies_cost_basis_history" FOREIGN KEY ("custom_currency_cost_basis_history") REFERENCES "custom_currencies" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION | |
| ); | |
| -- create index "currencycostbasis_fiat_code_effective_from_custom_currency_cost" to table: "currency_cost_bases" | |
| CREATE UNIQUE INDEX "currencycostbasis_fiat_code_effective_from_custom_currency_cost" ON "currency_cost_bases" ("fiat_code", "effective_from", "custom_currency_cost_basis_history"); | |
| "custom_currency_cost_basis_history" character(26) NOT NULL, | |
| PRIMARY KEY ("id"), | |
| CONSTRAINT "currency_cost_bases_custom_currencies_cost_basis_history" FOREIGN KEY ("custom_currency_cost_basis_history") REFERENCES "custom_currencies" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION | |
| ); | |
| -- create index "currencycostbasis_fiat_code_effective_from_custom_currency_cost" to table: "currency_cost_bases" | |
| CREATE UNIQUE INDEX "currencycostbasis_fiat_code_effective_from_custom_currency_cost" ON "currency_cost_bases" ("fiat_code", "effective_from", "custom_currency_cost_basis_history"); | |
| -- create index on FK column "custom_currency_cost_basis_history" | |
| CREATE INDEX "currencycostbasis_custom_currency" ON "currency_cost_bases" ("custom_currency_cost_basis_history"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/migrate/migrations/20260223180500_add_custom_currency.up.sql` around
lines 19 - 24, The foreign key column custom_currency_cost_basis_history on
table currency_cost_bases lacks a dedicated index, causing sequential scans when
checking/deleting referenced rows in custom_currencies; add a standalone index
on currency_cost_bases(custom_currency_cost_basis_history) (e.g., name it
currencycostbasis_custom_currency_cost_basis_history_idx) so FK lookups and
cascades can use an index rather than the existing composite index
currencycostbasis_fiat_code_effective_from_custom_currency_cost.
c7f5eb5 to
9e5ab00
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/ent/schema/custom_currencies.go (1)
31-60:⚠️ Potential issue | 🟠 MajorMissing tenant scoping on both schemas (repeat).
Still noNamespaceMixinor namespace-aware indexes here, so cross-tenant isolation can’t be enforced. This mirrors the earlier review note; please addentutils.NamespaceMixin{}and includenamespacein unique/index definitions for both schemas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 31 - 60, Add tenant scoping by including entutils.NamespaceMixin{} in the Mixin() results for both CustomCurrency and CurrencyCostBasis (update CustomCurrency.Mixin and CurrencyCostBasis.Mixin). Also modify the unique/index definitions so they are namespace-aware: make indexes/unique constraints include the namespace column together with the entity key(s) (e.g., combine namespace with the "code" field in CustomCurrency and with whatever identifying fields in CurrencyCostBasis) so each constraint is enforced per-tenant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 31-60: Add tenant scoping by including entutils.NamespaceMixin{}
in the Mixin() results for both CustomCurrency and CurrencyCostBasis (update
CustomCurrency.Mixin and CurrencyCostBasis.Mixin). Also modify the unique/index
definitions so they are namespace-aware: make indexes/unique constraints include
the namespace column together with the entity key(s) (e.g., combine namespace
with the "code" field in CustomCurrency and with whatever identifying fields in
CurrencyCostBasis) so each constraint is enforced per-tenant.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
go.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/where.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_create.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_delete.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_query.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_update.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_delete.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/ent.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
openmeter/ent/schema/custom_currencies.gotools/migrate/migrations/20260223180500_add_custom_currency.down.sqltools/migrate/migrations/20260223180500_add_custom_currency.up.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/migrate/migrations/20260223180500_add_custom_currency.up.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
openmeter/ent/schema/custom_currencies.go (1)
31-35:⚠️ Potential issue | 🟠 MajorAdd tenant scoping (NamespaceMixin + namespace‑scoped uniqueness).
Right now these schemas look tenant‑agnostic, which can lead to cross‑namespace collisions and unindexed tenant filters. I’d add
entutils.NamespaceMixin{}and scope uniqueness/indexes bynamespace(matching the existing multi‑tenant patterns elsewhere).🔧 Possible direction
func (CustomCurrency) Mixin() []ent.Mixin { return []ent.Mixin{ entutils.IDMixin{}, + entutils.NamespaceMixin{}, TimeMixin{}, } } func (CustomCurrency) Fields() []ent.Field { return []ent.Field{ - field.String("code").NotEmpty().MinLen(3).MaxLen(24).Unique().Immutable(), + field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable(), field.String("name").NotEmpty().MaxLen(100).Immutable(), field.String("symbol").NotEmpty().MaxLen(10).Immutable(), } } +func (CustomCurrency) Indexes() []ent.Index { + return []ent.Index{ + index.Fields("namespace", "code").Unique(), + } +} func (CurrencyCostBasis) Mixin() []ent.Mixin { return []ent.Mixin{ entutils.IDMixin{}, + entutils.NamespaceMixin{}, TimeMixin{}, } } func (CurrencyCostBasis) Indexes() []ent.Index { return []ent.Index{ - index.Edges("currency").Fields("fiat_code").Unique(), + index.Edges("currency").Fields("namespace", "fiat_code").Unique(), } }As per coding guidelines, “Performance should be a priority in critical code paths… database operations… should be vetted for potential performance bottlenecks.”
Also applies to: 38-40, 56-60, 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 31 - 35, Add tenant scoping to the CustomCurrency ent schema by updating the CustomCurrency Mixin to include entutils.NamespaceMixin{} (i.e., return entutils.IDMixin{}, TimeMixin{}, entutils.NamespaceMixin{}), and update any schema-level uniqueness/index declarations in the CustomCurrency schema (and the other listed schemas at lines ~38-40, 56-60, 83-86) to include the namespace field so indexes/unique constraints are namespace-scoped (e.g., make unique/index definitions include "namespace" plus the existing fields) to prevent cross-namespace collisions and allow efficient tenant filtering.
🧹 Nitpick comments (2)
tools/migrate/migrations/20260224135901_add_effective_from_schema.up.sql (1)
8-17: Avoid redundant PK index and add an FK index.
The PK already creates a unique index onid, so the extra unique index is redundant. Also, indexingcurrency_cost_basis_effective_from_historywill help joins and deletes.Suggested index tweak
- CREATE UNIQUE INDEX "currencycostbasiseffectivefrom_id" ON "currency_cost_basis_effective_froms" ("id"); + CREATE INDEX "currencycostbasiseffectivefrom_history_idx" + ON "currency_cost_basis_effective_froms" ("currency_cost_basis_effective_from_history");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260224135901_add_effective_from_schema.up.sql` around lines 8 - 17, Remove the redundant unique index "currencycostbasiseffectivefrom_id" since PRIMARY KEY on "id" in table "currency_cost_basis_effective_froms" already creates that unique index; instead add a non-unique index on the foreign key column "currency_cost_basis_effective_from_history" to speed joins/deletes (create index on "currency_cost_basis_effective_froms" ("currency_cost_basis_effective_from_history")). Ensure the CREATE UNIQUE INDEX line is deleted and replace with a CREATE INDEX targeting the FK column.openmeter/ent/schema/custom_currencies.go (1)
21-24: Preferclock.Nowforcreated_atdefaults.Using
time.Nowhere bypasses the project’s time‑freeze/drift logic. Swapping toclock.Nowkeeps testability and time behavior consistent.🕒 Suggested tweak
-import ( - "time" +import ( "entgo.io/ent" "entgo.io/ent/dialect" "entgo.io/ent/schema/edge" "entgo.io/ent/schema/field" "entgo.io/ent/schema/index" "entgo.io/ent/schema/mixin" "github.com/alpacahq/alpacadecimal" + "github.com/openmeterio/openmeter/pkg/clock" "github.com/openmeterio/openmeter/pkg/framework/entutils" ) func (TimeMixin) Fields() []ent.Field { return []ent.Field{ - field.Time("created_at").Default(time.Now).Immutable(), + field.Time("created_at").Default(clock.Now).Immutable(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 21 - 24, The TimeMixin.Fields implementation uses time.Now for the created_at default; change the field default to use the project's clock helper (clock.Now) instead to respect time-freeze/drift logic: update the default in TimeMixin.Fields (the field.Time("created_at").Default(...) call) to call clock.Now and add the appropriate import for the clock package where custom_currencies.go defines TimeMixin so tests and runtime follow the centralized clock behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 63-69: The schema currently enforces a unique key on the
currency+fiat pair which blocks adding new historical rates; update the
CurrencyCostBasis schema to either remove that uniqueness or expand the unique
index to include effective_from so multiple rows per currency+fiat with
different effective_from are allowed; locate the uniqueness definition in the
CurrencyCostBasis Indexes() (or schema index block) and either drop the unique
constraint on (currency, fiat_code) or change it to (currency, fiat_code,
effective_from) and keep fiat_code, rate, and Immutable() fields as-is to
preserve types.
In `@tools/migrate/migrations/20260224135901_add_effective_from_schema.down.sql`:
- Around line 1-10: The down migration currently drops the history table then
adds a NOT NULL effective_from column which will fail or lose data; instead
modify the down migration to (1) add "effective_from" to currency_cost_bases as
nullable (ALTER TABLE ... ADD COLUMN "effective_from" timestamptz NULL), (2)
backfill effective_from from the history table
currency_cost_basis_effective_froms (UPDATE currency_cost_bases SET
effective_from = <choose intended value from
currency_cost_basis_effective_froms, e.g. the latest/earliest matching row>
using the PK/FK relationship), (3) set the column NOT NULL (ALTER TABLE ...
ALTER COLUMN "effective_from" SET NOT NULL) once all rows are populated, and
only then (4) DROP TABLE "currency_cost_basis_effective_froms" and DROP/CREATE
the related indexes ("currencycostbasiseffectivefrom_id",
"custom_currencies_code_key",
"currencycostbasis_fiat_code_custom_currency_cost_basis_history") as needed;
ensure the backfill uses the correct join keys between currency_cost_bases and
currency_cost_basis_effective_froms.
In `@tools/migrate/migrations/20260224135901_add_effective_from_schema.up.sql`:
- Around line 1-2: Before dropping the effective_from column, create the new
table currency_cost_basis_effective_froms and backfill it with all existing
effective_from values from currency_cost_bases (preserving ids and timestamps),
verify the migration by checking row counts and non-null values, then in a
subsequent step drop the effective_from column from currency_cost_bases;
specifically: add a CREATE TABLE for currency_cost_basis_effective_froms, run an
INSERT ... SELECT to copy currency_cost_bases.id, effective_from (and any
required metadata), validate the copied data, and only then run the ALTER TABLE
"currency_cost_bases" DROP COLUMN "effective_from".
---
Duplicate comments:
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 31-35: Add tenant scoping to the CustomCurrency ent schema by
updating the CustomCurrency Mixin to include entutils.NamespaceMixin{} (i.e.,
return entutils.IDMixin{}, TimeMixin{}, entutils.NamespaceMixin{}), and update
any schema-level uniqueness/index declarations in the CustomCurrency schema (and
the other listed schemas at lines ~38-40, 56-60, 83-86) to include the namespace
field so indexes/unique constraints are namespace-scoped (e.g., make
unique/index definitions include "namespace" plus the existing fields) to
prevent cross-namespace collisions and allow efficient tenant filtering.
---
Nitpick comments:
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 21-24: The TimeMixin.Fields implementation uses time.Now for the
created_at default; change the field default to use the project's clock helper
(clock.Now) instead to respect time-freeze/drift logic: update the default in
TimeMixin.Fields (the field.Time("created_at").Default(...) call) to call
clock.Now and add the appropriate import for the clock package where
custom_currencies.go defines TimeMixin so tests and runtime follow the
centralized clock behavior.
In `@tools/migrate/migrations/20260224135901_add_effective_from_schema.up.sql`:
- Around line 8-17: Remove the redundant unique index
"currencycostbasiseffectivefrom_id" since PRIMARY KEY on "id" in table
"currency_cost_basis_effective_froms" already creates that unique index; instead
add a non-unique index on the foreign key column
"currency_cost_basis_effective_from_history" to speed joins/deletes (create
index on "currency_cost_basis_effective_froms"
("currency_cost_basis_effective_from_history")). Ensure the CREATE UNIQUE INDEX
line is deleted and replace with a CREATE INDEX targeting the FK column.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/where.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_create.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_query.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_update.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom/currencycostbasiseffectivefrom.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom/where.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom_create.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom_delete.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom_query.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasiseffectivefrom_update.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
openmeter/ent/schema/custom_currencies.gotools/migrate/migrations/20260224135901_add_effective_from_schema.down.sqltools/migrate/migrations/20260224135901_add_effective_from_schema.up.sql
Comment on lines
+1
to
+10
| -- reverse: create index "currencycostbasiseffectivefrom_id" to table: "currency_cost_basis_effective_froms" | ||
| DROP INDEX "currencycostbasiseffectivefrom_id"; | ||
| -- reverse: create "currency_cost_basis_effective_froms" table | ||
| DROP TABLE "currency_cost_basis_effective_froms"; | ||
| -- reverse: create index "custom_currencies_code_key" to table: "custom_currencies" | ||
| DROP INDEX "custom_currencies_code_key"; | ||
| -- reverse: create index "currencycostbasis_fiat_code_custom_currency_cost_basis_history" to table: "currency_cost_bases" | ||
| DROP INDEX "currencycostbasis_fiat_code_custom_currency_cost_basis_history"; | ||
| -- reverse: modify "currency_cost_bases" table | ||
| ALTER TABLE "currency_cost_bases" ADD COLUMN "effective_from" timestamptz NOT NULL; |
Contributor
There was a problem hiding this comment.
Rollback will lose data and may fail on non-empty tables.
Dropping the history table before restoring effective_from, then adding a NOT NULL column with no default, will either lose data or fail if rows exist. Please add the column nullable, backfill from history (choose the intended value), set NOT NULL, then drop the history table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/migrate/migrations/20260224135901_add_effective_from_schema.down.sql`
around lines 1 - 10, The down migration currently drops the history table then
adds a NOT NULL effective_from column which will fail or lose data; instead
modify the down migration to (1) add "effective_from" to currency_cost_bases as
nullable (ALTER TABLE ... ADD COLUMN "effective_from" timestamptz NULL), (2)
backfill effective_from from the history table
currency_cost_basis_effective_froms (UPDATE currency_cost_bases SET
effective_from = <choose intended value from
currency_cost_basis_effective_froms, e.g. the latest/earliest matching row>
using the PK/FK relationship), (3) set the column NOT NULL (ALTER TABLE ...
ALTER COLUMN "effective_from" SET NOT NULL) once all rows are populated, and
only then (4) DROP TABLE "currency_cost_basis_effective_froms" and DROP/CREATE
the related indexes ("currencycostbasiseffectivefrom_id",
"custom_currencies_code_key",
"currencycostbasis_fiat_code_custom_currency_cost_basis_history") as needed;
ensure the backfill uses the correct join keys between currency_cost_bases and
currency_cost_basis_effective_froms.
Comment on lines
+1
to
+2
| -- modify "currency_cost_bases" table | ||
| ALTER TABLE "currency_cost_bases" DROP COLUMN "effective_from"; |
Contributor
There was a problem hiding this comment.
Backfill before dropping effective_from to avoid data loss.
Dropping the column at Line 2 will wipe any existing effective_from values. Please create and backfill currency_cost_basis_effective_froms first, then drop the column once data is safely migrated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/migrate/migrations/20260224135901_add_effective_from_schema.up.sql`
around lines 1 - 2, Before dropping the effective_from column, create the new
table currency_cost_basis_effective_froms and backfill it with all existing
effective_from values from currency_cost_bases (preserving ids and timestamps),
verify the migration by checking row counts and non-null values, then in a
subsequent step drop the effective_from column from currency_cost_bases;
specifically: add a CREATE TABLE for currency_cost_basis_effective_froms, run an
INSERT ... SELECT to copy currency_cost_bases.id, effective_from (and any
required metadata), validate the copied data, and only then run the ALTER TABLE
"currency_cost_bases" DROP COLUMN "effective_from".
588dc17 to
4223e19
Compare
tothandras
reviewed
Feb 25, 2026
Contributor
There was a problem hiding this comment.
We've merged the schema, did anything change or are these leftovers to be removed?
Contributor
Author
There was a problem hiding this comment.
No, it's just a missing rebase
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
81218fb to
0c492f3
Compare
tothandras
previously requested changes
Mar 2, 2026
Comment on lines
+17
to
+25
| type TimeMixin struct { | ||
| mixin.Schema | ||
| } | ||
|
|
||
| func (TimeMixin) Fields() []ent.Field { | ||
| return []ent.Field{ | ||
| field.Time("created_at").Default(time.Now).Immutable(), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Use the built-in entutils.TimeMixin, even if updated_at and deleted_at are not exposed on the APIs.
|
|
||
| func (CurrencyCostBasisEffectiveFrom) Fields() []ent.Field { | ||
| return []ent.Field{ | ||
| field.Time("effective_from").Immutable(), |
Contributor
There was a problem hiding this comment.
Let's move effective_from to the CurrencyCostBasis model for now, and later we can decide if we want to change it to this pattern.
turip
reviewed
Mar 2, 2026
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
3 times, most recently
from
7514cba to
c8b2e0d
Compare
tothandras
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
6717db1 to
8cfc1f3
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
6af5848 to
1553d1e
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
from
1553d1e to
9a79c79
Compare
turip
reviewed
Mar 3, 2026
Comment on lines
+29
to
+30
| field.String("name").NotEmpty().MaxLen(100).Immutable(), | ||
| field.String("symbol").NotEmpty().MaxLen(10).Immutable(), |
Member
There was a problem hiding this comment.
Suggested change
| field.String("name").NotEmpty().MaxLen(100).Immutable(), | |
| field.String("symbol").NotEmpty().MaxLen(10).Immutable(), | |
| field.String("name").NotEmpty(), | |
| field.String("symbol").NotEmpty(), |
|
|
||
| func (CurrencyCostBasis) Fields() []ent.Field { | ||
| return []ent.Field{ | ||
| field.String("fiat_code").NotEmpty().MinLen(3).MaxLen(3).Immutable(), |
Member
There was a problem hiding this comment.
Let's use something like this
field.String("currency").
GoType(currencyx.Code("")).
| return []ent.Edge{ | ||
| edge.To("cost_basis_history", CurrencyCostBasis.Type), | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Let's add a unique index to prevent duplicate codes. Example:
index.Fields("namespace", "key").
Annotations(
entsql.IndexWhere("deleted_at IS NULL"),
).
Unique(),
Member
There was a problem hiding this comment.
let's add one more index at least on namespace, id UNIQUE
|
|
||
| func (CustomCurrency) Edges() []ent.Edge { | ||
| return []ent.Edge{ | ||
| edge.To("cost_basis_history", CurrencyCostBasis.Type), |
Member
There was a problem hiding this comment.
let's add cascade deletion: Annotations(entsql.OnDelete(entsql.Cascade)),
|
|
||
| func (CurrencyCostBasis) Indexes() []ent.Index { | ||
| return []ent.Index{ | ||
| index.Edges("currency").Fields("fiat_code", "effective_from").Unique(), |
Member
There was a problem hiding this comment.
Suggested change
| index.Edges("currency").Fields("fiat_code", "effective_from").Unique(), | |
| index.Fields("namespace", "currency_code_id", "fiat_code", "effective_from"). | |
| Annotations( | |
| entsql.IndexWhere("deleted_at IS NULL"), | |
| ). | |
| Unique(), |
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
from
9a79c79 to
589747a
Compare
turip
reviewed
Mar 3, 2026
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
from
0393cba to
8587407
Compare
turip
approved these changes
Mar 3, 2026
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
3 participants
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.
Features
Summary by CodeRabbit
New Features
Database