Merged
Conversation
hekike
force-pushed
the
feat/feature-unit-config-update
branch
from
30fae49 to
3ae939c
Compare
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 a PATCH /features/{featureId} endpoint and full update flow focused on Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,248,255,0.5)
participant Client
end
rect rgba(224,255,255,0.5)
participant Server as Router
participant Handler as UpdateFeature Handler
end
rect rgba(240,255,240,0.5)
participant Connector as Feature Connector
participant Repo as Feature Repo
participant DB as Database Adapter
end
rect rgba(255,250,240,0.5)
participant Meter as Meter Service
participant Publisher as Event Publisher
end
Client->>Server: PATCH /openmeter/features/{id} (UpdateFeatureRequest)
Server->>Handler: route -> UpdateFeature().With(id).ServeHTTP
Handler->>Handler: parse JSON, resolve namespace, convert to domain inputs
Handler->>Connector: UpdateFeature(inputs)
Connector->>Connector: Validate inputs, GetFeature (ensure exists & not archived)
alt unit_cost is LLM
Connector->>Meter: Lookup meter by feature.MeterSlug
Meter-->>Connector: Meter details
Connector->>Connector: Validate unit_cost with meter
end
Connector->>Repo: UpdateFeature(inputs)
Repo->>DB: Execute update, handle cost type switching
DB-->>Repo: updated row(s) or zero rows
Repo-->>Connector: updated Feature / not found error
Connector->>Publisher: Publish FeatureUpdateEvent
Publisher-->>Connector: ack
Connector-->>Handler: updated Feature
Handler->>Handler: optionally enrich with LLM pricing
Handler-->>Client: 200 OK (Feature)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 3
🧹 Nitpick comments (1)
openmeter/productcatalog/adapter/feature_test.go (1)
309-325: Minor: Variable shadows package name.The local
metervariable shadows the importedmeterpackage. It works because you only need the variable's fields after this point, but it can be confusing. Consider renaming totestMeterto match the pattern used inTestCreateFeature.✏️ Suggested rename
- meter := meter.Meter{ + testMeter := meter.Meter{ ManagedResource: models.ManagedResource{ - ID: "meter-1", + ID: ulid.Make().String(),And update references accordingly (
testMeter.ID, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature_test.go` around lines 309 - 325, The local variable named meter shadows the imported meter package; rename the local variable (defined as meter := meter.Meter{...}) to testMeter (e.g., testMeter := meter.Meter{...}) and update all subsequent references in this test (such as meter.ID, meter.Key, etc.) to use testMeter.ID, testMeter.Key, etc., matching the naming pattern used in TestCreateFeature.
🤖 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/productcatalog/adapter/feature_test.go`:
- Around line 553-572: The loop in this test does not create the meter entity
that testFeature references, causing FK violations; before constructing
dbConnector and calling tc.run, create and persist a meter using the Ent client
(the same pattern used in TestCreateFeature) so that testFeature's &meter.ID
points to a valid DB row—insert the meter via the dbClient/Ent driver after
Schema.Create and before adapter.NewPostgresFeatureRepo/test run, ensuring the
created meter.ID is used by testFeature.
In `@openmeter/productcatalog/adapter/feature.go`:
- Around line 124-137: The code in the feature.UnitCostTypeLLM case leaves old
literal LLM columns around because SetNillable... skips empty inputs; update the
query to explicitly clear any previous literal LLM columns before applying the
new nillable fields: call the corresponding Clear methods (e.g.,
ClearUnitCostLlmProvider, ClearUnitCostLlmModel, ClearUnitCostLlmTokenType or
their exact names on the query) prior to the SetNillableUnitCostLlm* calls, and
also ensure the branch where input.UnitCost.LLM == nil clears those LLM columns
so stale literal values cannot persist; modify the code around query and
SetNillableUnitCostLlmProviderProperty / SetNillableUnitCostLlmProvider /
SetNillableUnitCostLlmModelProperty / SetNillableUnitCostLlmModel /
SetNillableUnitCostLlmTokenTypeProperty / SetNillableUnitCostLlmTokenType
accordingly.
In `@openmeter/productcatalog/feature/connector.go`:
- Around line 253-259: The code currently maps every error from
c.meterService.GetMeterByIDOrSlug to meterpkg.NewMeterNotFoundError which hides
real failures; change the error handling in the block that calls
GetMeterByIDOrSlug (using meterpkg.GetMeterInput) to only convert errors that
are actual "not found" errors (e.g., via a meterpkg.IsNotFound/IsMeterNotFound
helper or by type-asserting the error), and for any other error return/propagate
the original error (or wrap it) instead of NewMeterNotFoundError so the PATCH
path matches CreateFeature's behavior.
---
Nitpick comments:
In `@openmeter/productcatalog/adapter/feature_test.go`:
- Around line 309-325: The local variable named meter shadows the imported meter
package; rename the local variable (defined as meter := meter.Meter{...}) to
testMeter (e.g., testMeter := meter.Meter{...}) and update all subsequent
references in this test (such as meter.ID, meter.Key, etc.) to use testMeter.ID,
testMeter.Key, etc., matching the naming pattern used in TestCreateFeature.
🪄 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: 54e5f166-5056-46b6-aed9-d26e2ed99419
⛔ Files ignored due to path filters (8)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
Makefileapi/api.gen.goapi/spec/packages/aip/src/features/feature.tspapi/spec/packages/aip/src/features/operations.tspapi/spec/packages/legacy/src/productcatalog/routes.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/handler.goapi/v3/handlers/features/update.goapi/v3/server/routes.goopenmeter/llmcost/adapter/price.goopenmeter/llmcost/service.goopenmeter/productcatalog/adapter/feature.goopenmeter/productcatalog/adapter/feature_test.goopenmeter/productcatalog/feature/connector.goopenmeter/productcatalog/feature/event.goopenmeter/productcatalog/feature/repository.goopenmeter/server/server_test.go
💤 Files with no reviewable changes (1)
- api/spec/packages/legacy/src/productcatalog/routes.tsp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/productcatalog/adapter/feature_test.go (3)
309-325: Variablemetershadows the imported package name.The variable declaration
meter := meter.Meter{...}shadows themeterpackage. While this works because the struct literal is evaluated before the variable enters scope, it's confusing for readers and could lead to bugs in future modifications. Compare withTestCreateFeaturewhich usestestMeterto avoid this.♻️ Suggested rename to match existing pattern
- meter := meter.Meter{ + testMeter := meter.Meter{ ManagedResource: models.ManagedResource{ ID: "meter-1", NamespacedModel: models.NamespacedModel{Then update all references from
meter.ID,meter.Namespace, etc. totestMeter.ID,testMeter.Namespace, etc. throughout the function (including lines 331, 571-577).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature_test.go` around lines 309 - 325, The local variable currently declared as "meter := meter.Meter{...}" shadows the imported meter package; rename the local variable to "testMeter" (i.e., create testMeter := meter.Meter{...}) and update every use of that variable (e.g., meter.ID, meter.Namespace, meter.Key, meter.GroupBy, meter.Aggregation, meter.EventType, and any other references inside the test function) to use testMeter.ID, testMeter.Namespace, etc., so the meter package remains unshadowed and the code matches the existing test naming pattern.
406-442: Consider adding a test for LLM-to-Manual type switch.You've got good coverage for Manual→LLM conversion, but the reverse direction (LLM→Manual) isn't tested. This would verify that the LLM fields get cleared properly when switching back to Manual type, matching the adapter's clearing logic for type switches.
💡 Example test case to add after this one
{ name: "Should change unit cost type from LLM to manual", run: func(t *testing.T, connector feature.FeatureRepo) { ctx := context.Background() featureIn := testFeature featureIn.UnitCost = &feature.UnitCost{ Type: feature.UnitCostTypeLLM, LLM: &feature.LLMUnitCost{ Provider: "openai", Model: "gpt-4", TokenType: "input", }, } created, err := connector.CreateFeature(ctx, featureIn) assert.NoError(t, err) assert.Equal(t, feature.UnitCostTypeLLM, created.UnitCost.Type) updated, err := connector.UpdateFeature(ctx, feature.UpdateFeatureInputs{ Namespace: namespace, ID: created.ID, UnitCost: &feature.UnitCost{ Type: feature.UnitCostTypeManual, Manual: &feature.ManualUnitCost{ Amount: alpacadecimal.NewFromFloat(0.02), }, }, }) assert.NoError(t, err) assert.Equal(t, feature.UnitCostTypeManual, updated.UnitCost.Type) assert.Equal(t, "0.02", updated.UnitCost.Manual.Amount.String()) // LLM fields should be cleared assert.Nil(t, updated.UnitCost.LLM) }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature_test.go` around lines 406 - 442, Add a mirror test that verifies switching UnitCost from LLM to Manual: create a feature using CreateFeature with UnitCost.Type = feature.UnitCostTypeLLM and populated LLM fields, then call connector.UpdateFeature with UnitCost.Type = feature.UnitCostTypeManual and a Manual.UnitCost (e.g., Amount alpacadecimal.NewFromFloat(0.02)), assert no error, assert updated.UnitCost.Type == feature.UnitCostTypeManual, assert updated.UnitCost.Manual.Amount matches the new value (string or value check), and assert updated.UnitCost.LLM == nil to ensure LLM fields are cleared; place this new test case alongside the existing "Should change unit cost type from manual to LLM" case in feature_test.go.
545-547: UpdatedAt assertion might not catch regressions.The check
updated.UpdatedAt.After(created.UpdatedAt) || updated.UpdatedAt.Equal(created.UpdatedAt)essentially asserts>=, which is almost always true. If the adapter ever stopped updating the timestamp, this could still pass when operations complete within the same clock tick.A quick
time.Sleep(time.Millisecond)before the update call would make this more robust, or you could compare againstcreated.UpdatedAtdirectly with justAfter().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature_test.go` around lines 545 - 547, The UpdatedAt assertion is too permissive (using After(...) || Equal(...)) and can mask regressions; update the test in feature_test.go around the assertion that checks updated.UpdatedAt vs created.UpdatedAt so it reliably detects timestamp changes: either insert a small pause (e.g., time.Sleep(time.Millisecond)) before invoking the update operation that produces updated, or change the assertion to require updated.UpdatedAt.After(created.UpdatedAt) (strict >) so the test fails if the adapter stops advancing UpdatedAt; locate the assertion referencing updated.UpdatedAt and created.UpdatedAt and apply one of these fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/productcatalog/adapter/feature_test.go`:
- Around line 309-325: The local variable currently declared as "meter :=
meter.Meter{...}" shadows the imported meter package; rename the local variable
to "testMeter" (i.e., create testMeter := meter.Meter{...}) and update every use
of that variable (e.g., meter.ID, meter.Namespace, meter.Key, meter.GroupBy,
meter.Aggregation, meter.EventType, and any other references inside the test
function) to use testMeter.ID, testMeter.Namespace, etc., so the meter package
remains unshadowed and the code matches the existing test naming pattern.
- Around line 406-442: Add a mirror test that verifies switching UnitCost from
LLM to Manual: create a feature using CreateFeature with UnitCost.Type =
feature.UnitCostTypeLLM and populated LLM fields, then call
connector.UpdateFeature with UnitCost.Type = feature.UnitCostTypeManual and a
Manual.UnitCost (e.g., Amount alpacadecimal.NewFromFloat(0.02)), assert no
error, assert updated.UnitCost.Type == feature.UnitCostTypeManual, assert
updated.UnitCost.Manual.Amount matches the new value (string or value check),
and assert updated.UnitCost.LLM == nil to ensure LLM fields are cleared; place
this new test case alongside the existing "Should change unit cost type from
manual to LLM" case in feature_test.go.
- Around line 545-547: The UpdatedAt assertion is too permissive (using
After(...) || Equal(...)) and can mask regressions; update the test in
feature_test.go around the assertion that checks updated.UpdatedAt vs
created.UpdatedAt so it reliably detects timestamp changes: either insert a
small pause (e.g., time.Sleep(time.Millisecond)) before invoking the update
operation that produces updated, or change the assertion to require
updated.UpdatedAt.After(created.UpdatedAt) (strict >) so the test fails if the
adapter stops advancing UpdatedAt; locate the assertion referencing
updated.UpdatedAt and created.UpdatedAt and apply one of these fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85eaac3f-3570-4c38-94cc-3ba53d915e69
📒 Files selected for processing (1)
openmeter/productcatalog/adapter/feature_test.go
hekike
force-pushed
the
feat/feature-unit-config-update
branch
from
0f6debc to
99fa65c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (1)
204-208: Consider adding-vfor consistency with other lint targets.The other
lint-go-*targets all use the-vflag for verbose output. Might be worth adding here too so developers get the same level of feedback:🔧 Suggested tweak
.PHONY: lint-go-head lint-go-head: ## Lint Go code since last commit $(call print-target) - golangci-lint run --new-from-rev=HEAD~1 + golangci-lint run -v --new-from-rev=HEAD~1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 204 - 208, Update the lint-go-head Makefile target to run golangci-lint with verbose output: modify the command in the lint-go-head target (currently using "golangci-lint run --new-from-rev=HEAD~1") to include the -v flag so it matches the other lint-go-* targets and provides consistent, verbose feedback.openmeter/llmcost/service.go (1)
86-92: Consider using theallowedOrderByvariable in the error message to stay DRY.The validation logic looks solid! One small nit: the error message on line 87 hardcodes the allowed values, which duplicates the
allowedOrderByslice. If you ever add a new allowed field, you'd need to update both places.🔧 Optional: Use the variable in the error message
if i.OrderBy != "" && !slices.Contains(allowedOrderBy, i.OrderBy) { - errs = append(errs, fmt.Errorf("invalid order_by: %q (allowed: id, provider.id, model.id, effective_from, effective_to)", i.OrderBy)) + errs = append(errs, fmt.Errorf("invalid order_by: %q (allowed: %v)", i.OrderBy, allowedOrderBy)) }Totally fine to keep as-is if you prefer the more human-readable format in the error message though!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/service.go` around lines 86 - 92, The error message for invalid order_by hardcodes allowed values; change it to use the allowedOrderBy slice so it's not duplicated: when validating i.OrderBy (the block that appends to errs with fmt.Errorf), format the allowedOrderBy values (e.g., join them into a string) and include that constructed string in the error message instead of the hardcoded list so future updates to allowedOrderBy are reflected automatically.openmeter/productcatalog/feature/event.go (1)
68-113: Consider extracting shared event helpers to reduce drift.Create/update/archive events repeat the same constructor/metadata/validation scaffolding. A small shared helper would make this easier to maintain and less error-prone over time.
As per coding guidelines,
**/*.go: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/feature/event.go` around lines 68 - 113, The create/update/archive feature events all duplicate constructor, metadata and validation scaffolding—refactor by extracting a shared helper (e.g., a function like buildFeatureEventMetadata or a small base type FeatureEventBase) and reuse it from NewFeatureUpdateEvent, FeatureUpdateEvent.EventMetadata, EventName and Validate; specifically, move common logic that composes the resourcePath, builds metadata (ID/Source/Subject/Time via ulid.Make and metadata.ComposeResourcePath) and the nil/Validate checks for e.Feature into shared functions (e.g., ValidateFeaturePresence(feature *Feature) error and BuildFeatureMetadata(feature *Feature, version string) metadata.EventMetadata) and call those from FeatureUpdateEvent methods so constructors (NewFeatureUpdateEvent), EventMetadata(), EventName() and Validate() become thin wrappers that delegate to the shared helpers.
🤖 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/productcatalog/feature/event.go`:
- Around line 90-99: FeatureUpdateEvent.EventMetadata() currently dereferences
e.Feature (calling metadata.ComposeResourcePath(e.Feature.Namespace, ...,
e.Feature.ID)) which can panic because NewCloudEvent() calls EventMetadata()
before ev.Validate(); add a nil check in FeatureUpdateEvent.EventMetadata(): if
e.Feature is nil, return a safe EventMetadata (e.g., ID ulid.Make().String(),
Time zero value or e.Feature.UpdatedAt guarded, and Source/Subject set to a
sensible empty or placeholder resource path via metadata.ComposeResourcePath("",
metadata.EntityFeature, "")) so the method never dereferences e.Feature and is
resilient to the call order from NewCloudEvent(); ensure you reference
FeatureUpdateEvent.EventMetadata, e.Feature, and metadata.ComposeResourcePath
when making the change.
---
Nitpick comments:
In `@Makefile`:
- Around line 204-208: Update the lint-go-head Makefile target to run
golangci-lint with verbose output: modify the command in the lint-go-head target
(currently using "golangci-lint run --new-from-rev=HEAD~1") to include the -v
flag so it matches the other lint-go-* targets and provides consistent, verbose
feedback.
In `@openmeter/llmcost/service.go`:
- Around line 86-92: The error message for invalid order_by hardcodes allowed
values; change it to use the allowedOrderBy slice so it's not duplicated: when
validating i.OrderBy (the block that appends to errs with fmt.Errorf), format
the allowedOrderBy values (e.g., join them into a string) and include that
constructed string in the error message instead of the hardcoded list so future
updates to allowedOrderBy are reflected automatically.
In `@openmeter/productcatalog/feature/event.go`:
- Around line 68-113: The create/update/archive feature events all duplicate
constructor, metadata and validation scaffolding—refactor by extracting a shared
helper (e.g., a function like buildFeatureEventMetadata or a small base type
FeatureEventBase) and reuse it from NewFeatureUpdateEvent,
FeatureUpdateEvent.EventMetadata, EventName and Validate; specifically, move
common logic that composes the resourcePath, builds metadata
(ID/Source/Subject/Time via ulid.Make and metadata.ComposeResourcePath) and the
nil/Validate checks for e.Feature into shared functions (e.g.,
ValidateFeaturePresence(feature *Feature) error and BuildFeatureMetadata(feature
*Feature, version string) metadata.EventMetadata) and call those from
FeatureUpdateEvent methods so constructors (NewFeatureUpdateEvent),
EventMetadata(), EventName() and Validate() become thin wrappers that delegate
to the shared helpers.
🪄 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: ccd8a7f9-ec38-46d9-9ad8-d4893fb17a79
⛔ Files ignored due to path filters (8)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
Makefileapi/api.gen.goapi/spec/packages/aip/src/features/feature.tspapi/spec/packages/aip/src/features/operations.tspapi/spec/packages/legacy/src/productcatalog/routes.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/handler.goapi/v3/handlers/features/update.goapi/v3/server/routes.goopenmeter/llmcost/adapter/price.goopenmeter/llmcost/service.goopenmeter/productcatalog/adapter/feature.goopenmeter/productcatalog/adapter/feature_test.goopenmeter/productcatalog/feature/connector.goopenmeter/productcatalog/feature/event.goopenmeter/productcatalog/feature/repository.goopenmeter/server/server_test.go
💤 Files with no reviewable changes (1)
- api/spec/packages/legacy/src/productcatalog/routes.tsp
✅ Files skipped from review due to trivial changes (4)
- api/api.gen.go
- api/spec/packages/aip/src/features/feature.tsp
- api/v3/handlers/features/convert.go
- openmeter/productcatalog/adapter/feature_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- openmeter/llmcost/adapter/price.go
- api/spec/packages/aip/src/features/operations.tsp
- openmeter/productcatalog/feature/repository.go
- openmeter/server/server_test.go
- openmeter/productcatalog/adapter/feature.go
- api/v3/handlers/features/update.go
- openmeter/productcatalog/feature/connector.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/productcatalog/feature/connector.go (1)
241-265: Minor: Redundant nil check onUnitCostSince
input.Validate()at line 230 already ensuresUnitCostis non-nil, thisif input.UnitCost != nilcheck will always be true. You could remove the conditional and just run the validation directly.That said, this is harmless defensive coding - if the validation logic ever changes in the future, this check would catch it. Totally fine to keep it as-is if you prefer the safety net!
♻️ Optional simplification
- // Validate unit cost if provided - if input.UnitCost != nil { - if err := input.UnitCost.Validate(); err != nil { - return Feature{}, models.NewGenericValidationError(err) - } + // Validate unit cost + if err := input.UnitCost.Validate(); err != nil { + return Feature{}, models.NewGenericValidationError(err) + } - if input.UnitCost.Type == UnitCostTypeLLM { - if feat.MeterSlug == nil { - return Feature{}, models.NewGenericValidationError( - fmt.Errorf("LLM unit cost requires a meter to be associated with the feature"), - ) - } + if input.UnitCost.Type == UnitCostTypeLLM { + if feat.MeterSlug == nil { + return Feature{}, models.NewGenericValidationError( + fmt.Errorf("LLM unit cost requires a meter to be associated with the feature"), + ) + } - meter, err := c.meterService.GetMeterByIDOrSlug(ctx, meterpkg.GetMeterInput{ - Namespace: input.Namespace, - IDOrSlug: *feat.MeterSlug, - }) - if err != nil { - return Feature{}, err - } + meter, err := c.meterService.GetMeterByIDOrSlug(ctx, meterpkg.GetMeterInput{ + Namespace: input.Namespace, + IDOrSlug: *feat.MeterSlug, + }) + if err != nil { + return Feature{}, err + } - if err := input.UnitCost.ValidateWithMeter(meter); err != nil { - return Feature{}, models.NewGenericValidationError(err) - } + if err := input.UnitCost.ValidateWithMeter(meter); err != nil { + return Feature{}, models.NewGenericValidationError(err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/feature/connector.go` around lines 241 - 265, The nil-check around input.UnitCost is redundant because input.Validate() already guarantees UnitCost is non-nil; remove the outer "if input.UnitCost != nil" guard and unindent the UnitCost validation block so you always call input.UnitCost.Validate(), the LLM-specific branch (checking feat.MeterSlug, calling c.meterService.GetMeterByIDOrSlug) and input.UnitCost.ValidateWithMeter(meter) directly; keep the existing error handling (models.NewGenericValidationError and returning errors) and the references to feat.MeterSlug, c.meterService.GetMeterByIDOrSlug, UnitCost.Validate and UnitCost.ValidateWithMeter to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/productcatalog/feature/connector.go`:
- Around line 241-265: The nil-check around input.UnitCost is redundant because
input.Validate() already guarantees UnitCost is non-nil; remove the outer "if
input.UnitCost != nil" guard and unindent the UnitCost validation block so you
always call input.UnitCost.Validate(), the LLM-specific branch (checking
feat.MeterSlug, calling c.meterService.GetMeterByIDOrSlug) and
input.UnitCost.ValidateWithMeter(meter) directly; keep the existing error
handling (models.NewGenericValidationError and returning errors) and the
references to feat.MeterSlug, c.meterService.GetMeterByIDOrSlug,
UnitCost.Validate and UnitCost.ValidateWithMeter to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 687293ef-f83a-427c-b166-acc07a1af1e4
📒 Files selected for processing (1)
openmeter/productcatalog/feature/connector.go
hekike
changed the title
hekike
changed the title
Use a dedicated FeatureUpdateRequest model instead of Shared.UpdateRequest<Feature> so that only unit_cost is exposed in the PATCH endpoint, excluding name, description, and labels from the update request. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The TestUpdateFeature test cases were failing with a foreign key constraint violation because the meter referenced by the feature was not created in the database before attempting to create the feature. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hekike
force-pushed
the
feat/feature-unit-config-update
branch
from
152aaf8 to
055033f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openmeter/productcatalog/adapter/feature.go (1)
124-137:⚠️ Potential issue | 🟠 MajorClear the old LLM fields before writing a new LLM config.
In the
llm -> llmpath,lo.EmptyableToPtr("")turns into nil and the generatedSetNillable...helpers skip nil inputs. If a field is intentionally cleared, or moved from a literal value to a...Property, the previous column survives and the feature can end up with a mixed old+new pricing config.🩹 Suggested fix
case feature.UnitCostTypeLLM: // Clear manual fields, set LLM fields query = query. ClearUnitCostManualAmount(). + ClearUnitCostLlmProviderProperty(). + ClearUnitCostLlmProvider(). + ClearUnitCostLlmModelProperty(). + ClearUnitCostLlmModel(). + ClearUnitCostLlmTokenTypeProperty(). + ClearUnitCostLlmTokenType(). SetUnitCostType(string(input.UnitCost.Type)) if input.UnitCost.LLM != nil { query = query. SetNillableUnitCostLlmProviderProperty(lo.EmptyableToPtr(input.UnitCost.LLM.ProviderProperty)).Expected result: the generated
SetNillable...helpers only mutate when the pointer is non-nil, which is why the explicitClear...calls are still needed here.#!/bin/bash set -euo pipefail echo "1) Generated Ent update helpers for LLM unit-cost fields" fd 'feature.*update.*\.go$' openmeter/ent/db -x rg -n -C2 'SetNillableUnitCostLlm|ClearUnitCostLlm' {} echo echo "2) Current LLM update branch" sed -n '124,140p' openmeter/productcatalog/adapter/feature.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature.go` around lines 124 - 137, The LLM branch currently only calls SetNillableUnitCostLlm* helpers which skip nils, so old LLM columns can linger; before setting the new LLM fields in the feature.UnitCostTypeLLM case you must explicitly clear the previous LLM columns on the query—call the ClearUnitCostLlm* helpers (e.g. ClearUnitCostLlmProviderProperty, ClearUnitCostLlmProvider, ClearUnitCostLlmModelProperty, ClearUnitCostLlmModel, ClearUnitCostLlmTokenTypeProperty, ClearUnitCostLlmTokenType or a ClearUnitCostLlm aggregate if available) on the same query prior to the SetNillableUnitCostLlm* calls to ensure stale values are removed.openmeter/productcatalog/feature/event.go (1)
90-98:⚠️ Potential issue | 🟠 MajorGuard
EventMetadata()against a nil feature.
EventMetadata()dereferencese.Featuredirectly. In the publish path, metadata is built before validation, so a bad event panics here instead of returning the"feature is required"error fromValidate().🩹 Suggested fix
func (e FeatureUpdateEvent) EventMetadata() metadata.EventMetadata { + if e.Feature == nil { + resourcePath := metadata.ComposeResourcePath("", metadata.EntityFeature, "") + + return metadata.EventMetadata{ + ID: ulid.Make().String(), + Source: resourcePath, + Subject: resourcePath, + } + } + resourcePath := metadata.ComposeResourcePath(e.Feature.Namespace, metadata.EntityFeature, e.Feature.ID) return metadata.EventMetadata{Expected result: the marshaling flow should show
EventMetadata()being called beforeValidate(), which makes this nil guard necessary.#!/bin/bash set -euo pipefail echo "1) FeatureUpdateEvent metadata + validation" sed -n '82,113p' openmeter/productcatalog/feature/event.go echo echo "2) CloudEvent construction order" sed -n '80,130p' openmeter/watermill/marshaler/marshaler.goBased on learnings, In OpenMeter, the CloudEvents
subjectfield is mandatory for the application's business logic, even though it's optional in the CloudEvents specification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/feature/event.go` around lines 90 - 98, FeatureUpdateEvent.EventMetadata currently dereferences e.Feature and panics for nil; add a nil guard at the start of EventMetadata (check if e.Feature == nil) and if nil build and return metadata.EventMetadata with a generated ID and safe defaults for Source and Subject (empty string or ""), and a sensible Time value (e.g., time.Now() or time.Time{}), otherwise compute resourcePath via metadata.ComposeResourcePath(e.Feature.Namespace, metadata.EntityFeature, e.Feature.ID) and populate Source, Subject and Time from e.Feature as before; keep using ulid.Make().String() for ID so callers (like the marshaler) don't panic when metadata is constructed before Validate().
🧹 Nitpick comments (3)
openmeter/llmcost/service.go (1)
75-87: Keep allowed sort fields as a single source of truth.
allowedOrderByis defined once, but the error text repeats the same list manually. Consider deriving the message fromallowedOrderByso future changes don’t drift.♻️ Suggested refactor
import ( "context" "errors" "fmt" "slices" + "strings" "time" @@ var allowedOrderBy = []string{"id", "provider.id", "model.id", "effective_from", "effective_to"} +var allowedOrderByText = strings.Join(allowedOrderBy, ", ") @@ if i.OrderBy != "" && !slices.Contains(allowedOrderBy, i.OrderBy) { - errs = append(errs, fmt.Errorf("invalid order_by: %q (allowed: id, provider.id, model.id, effective_from, effective_to)", i.OrderBy)) + errs = append(errs, fmt.Errorf("invalid order_by: %q (allowed: %s)", i.OrderBy, allowedOrderByText)) }As per coding guidelines, in Go code readability and maintainability should be prioritized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/service.go` around lines 75 - 87, The error message in ListPricesInput.Validate duplicates the allowed fields list instead of using the canonical allowedOrderBy slice; update the validation to construct the error text from allowedOrderBy (e.g., join allowedOrderBy into a comma-separated string) when building the fmt.Errorf message, keeping the existing check that uses slices.Contains and referencing allowedOrderBy, ListPricesInput.Validate and the OrderBy field so future changes to allowedOrderBy are automatically reflected in the error message.openmeter/llmcost/adapter/price.go (1)
62-63: Nice fix—just update the comment to match behavior.Line 62 says the default is ascending, but Line 63 now applies the requested
orderdirection when provided. A quick comment tweak would avoid confusion for future readers.As per coding guidelines, in Go code readability and maintainability should be prioritized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/adapter/price.go` around lines 62 - 63, Update the misleading comment above the query.Order call in price.go to state that the code applies the provided order direction to both model ID and ID (using pricedb.ByModelID(order...) and pricedb.ByID(order...)), and only falls back to the default database ordering when no order argument is supplied; reference the query.Order line and the pricedb.ByModelID/pricedb.ByID helpers so future readers understand the actual behavior.openmeter/productcatalog/adapter/feature_test.go (1)
407-442: Please add onellm -> llmcleanup case here.Nice coverage overall. The table still misses the case where an existing LLM config switches from a literal field to the matching
...Property(or clears a field entirely) and then re-fetches. That’s the branch that protects the adapter’s column-clearing behavior and would catch stale-value regressions quickly.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature_test.go` around lines 407 - 442, Add a new table case that starts with an LLM unit cost (create via connector.CreateFeature with feature.UnitCost.Type = feature.UnitCostTypeLLM and literal LLM fields set, e.g. Model and TokenType), then call connector.UpdateFeature with feature.UpdateFeatureInputs supplying the LLM using the matching "...Property" fields (or omitting/clearing a literal field) to simulate switching from literal to property/cleared values; assert no error, then assert updated.UnitCost.Type == feature.UnitCostTypeLLM, updated.UnitCost.LLM reflects the new property/cleared values (e.g. ModelProperty populated and literal Model nil/empty, TokenType cleared), and ensure any stale literal fields were removed (nil) to cover the adapter’s column-clearing 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/productcatalog/adapter/feature.go`:
- Around line 100-105: The update query that begins with c.db.Feature.Update()
currently only uses dbfeature.ArchivedAtIsNil(), causing features scheduled to
archive later to be treated as inactive; replace that predicate with the shared
“active feature” predicate used elsewhere in this adapter (the same predicate
used by the feature read/reload path) so the Update matches features where
ArchivedAt IS NULL OR ArchivedAt > now() (or call the central helper, e.g.,
dbfeature.Active() if available), and ensure the reload path that re-fetches the
feature after the update uses the identical predicate as well.
---
Duplicate comments:
In `@openmeter/productcatalog/adapter/feature.go`:
- Around line 124-137: The LLM branch currently only calls
SetNillableUnitCostLlm* helpers which skip nils, so old LLM columns can linger;
before setting the new LLM fields in the feature.UnitCostTypeLLM case you must
explicitly clear the previous LLM columns on the query—call the
ClearUnitCostLlm* helpers (e.g. ClearUnitCostLlmProviderProperty,
ClearUnitCostLlmProvider, ClearUnitCostLlmModelProperty, ClearUnitCostLlmModel,
ClearUnitCostLlmTokenTypeProperty, ClearUnitCostLlmTokenType or a
ClearUnitCostLlm aggregate if available) on the same query prior to the
SetNillableUnitCostLlm* calls to ensure stale values are removed.
In `@openmeter/productcatalog/feature/event.go`:
- Around line 90-98: FeatureUpdateEvent.EventMetadata currently dereferences
e.Feature and panics for nil; add a nil guard at the start of EventMetadata
(check if e.Feature == nil) and if nil build and return metadata.EventMetadata
with a generated ID and safe defaults for Source and Subject (empty string or
""), and a sensible Time value (e.g., time.Now() or time.Time{}), otherwise
compute resourcePath via metadata.ComposeResourcePath(e.Feature.Namespace,
metadata.EntityFeature, e.Feature.ID) and populate Source, Subject and Time from
e.Feature as before; keep using ulid.Make().String() for ID so callers (like the
marshaler) don't panic when metadata is constructed before Validate().
---
Nitpick comments:
In `@openmeter/llmcost/adapter/price.go`:
- Around line 62-63: Update the misleading comment above the query.Order call in
price.go to state that the code applies the provided order direction to both
model ID and ID (using pricedb.ByModelID(order...) and pricedb.ByID(order...)),
and only falls back to the default database ordering when no order argument is
supplied; reference the query.Order line and the pricedb.ByModelID/pricedb.ByID
helpers so future readers understand the actual behavior.
In `@openmeter/llmcost/service.go`:
- Around line 75-87: The error message in ListPricesInput.Validate duplicates
the allowed fields list instead of using the canonical allowedOrderBy slice;
update the validation to construct the error text from allowedOrderBy (e.g.,
join allowedOrderBy into a comma-separated string) when building the fmt.Errorf
message, keeping the existing check that uses slices.Contains and referencing
allowedOrderBy, ListPricesInput.Validate and the OrderBy field so future changes
to allowedOrderBy are automatically reflected in the error message.
In `@openmeter/productcatalog/adapter/feature_test.go`:
- Around line 407-442: Add a new table case that starts with an LLM unit cost
(create via connector.CreateFeature with feature.UnitCost.Type =
feature.UnitCostTypeLLM and literal LLM fields set, e.g. Model and TokenType),
then call connector.UpdateFeature with feature.UpdateFeatureInputs supplying the
LLM using the matching "...Property" fields (or omitting/clearing a literal
field) to simulate switching from literal to property/cleared values; assert no
error, then assert updated.UnitCost.Type == feature.UnitCostTypeLLM,
updated.UnitCost.LLM reflects the new property/cleared values (e.g.
ModelProperty populated and literal Model nil/empty, TokenType cleared), and
ensure any stale literal fields were removed (nil) to cover the adapter’s
column-clearing behavior.
🪄 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: c4433160-2bff-4041-98d8-f683a271ae6c
⛔ Files ignored due to path filters (8)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
Makefileapi/api.gen.goapi/spec/packages/aip/src/features/feature.tspapi/spec/packages/aip/src/features/operations.tspapi/spec/packages/legacy/src/productcatalog/routes.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/handler.goapi/v3/handlers/features/update.goapi/v3/server/routes.goopenmeter/llmcost/adapter/price.goopenmeter/llmcost/service.goopenmeter/productcatalog/adapter/feature.goopenmeter/productcatalog/adapter/feature_test.goopenmeter/productcatalog/feature/connector.goopenmeter/productcatalog/feature/event.goopenmeter/productcatalog/feature/repository.goopenmeter/server/server_test.go
💤 Files with no reviewable changes (1)
- api/spec/packages/legacy/src/productcatalog/routes.tsp
✅ Files skipped from review due to trivial changes (4)
- api/spec/packages/aip/src/features/feature.tsp
- openmeter/productcatalog/feature/repository.go
- api/api.gen.go
- api/v3/handlers/features/convert.go
🚧 Files skipped from review as they are similar to previous changes (5)
- api/v3/handlers/features/handler.go
- api/v3/server/routes.go
- api/spec/packages/aip/src/features/operations.tsp
- openmeter/server/server_test.go
- Makefile
turip
approved these changes
Mar 26, 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
2 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.
Summary
Add a
PATCH /api/v3/openmeter/features/{featureId}endpoint to update the unit cost configuration on features.UpdateFeatureon connector, adapter, repository with validation and event publishingmanual(fixed per-unit cost) andllm(dynamic LLM cost lookup) unit cost typesValidation
v1 PATCH /api/v1/features/{id}returns 405v3 PATCH /api/v3/openmeter/features/{id}with manual unit costv3 GET /api/v3/openmeter/features/{id}confirms update persistedv3 POST /api/v3/openmeter/features(create) still worksv3 GET /api/v3/openmeter/features(list) still worksgo build ./...compiles cleanlygo vetpassesmake lint-gopassesKey files
api/spec/packages/aip/src/features/operations.tsp— v3 update operationapi/spec/packages/aip/src/features/feature.tsp—unit_costvisibility includesLifecycle.Updateapi/v3/handlers/features/update.go— v3 update handlerapi/v3/handlers/features/convert.go—convertUpdateRequestToDomainopenmeter/productcatalog/feature/connector.go—UpdateFeatureservice method + validationopenmeter/productcatalog/adapter/feature.go—UpdateFeaturerepository implementationopenmeter/productcatalog/adapter/feature_test.go— comprehensive adapter testsSummary by CodeRabbit
New Features
Improvements
Tests