feat(api): v3 feature handlers and unit config#3986
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds per-feature unit-cost support (manual and LLM), changes Feature meter representation to an explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Features Handler
participant NS as NamespaceResolver
participant MeterSvc as MeterService
participant FeatureSvc as FeatureConnector
participant DB as Database
Client->>API: POST /features (body with meter slug & filters)
API->>NS: Resolve namespace
NS-->>API: namespace
alt meter provided
API->>MeterSvc: GetMeterByIDOrSlug(slug)
MeterSvc-->>API: meter (GroupBy dims)
API->>API: validateMeterFilters(filters, meter)
end
API->>FeatureSvc: CreateFeature(inputs with meter id, unit_cost)
FeatureSvc->>DB: Insert feature
DB-->>FeatureSvc: created feature
FeatureSvc-->>API: feature (with unit_cost)
API-->>Client: 201 Created (Feature JSON)
sequenceDiagram
participant Client
participant API as Features Handler
participant NS as NamespaceResolver
participant FeatureSvc as FeatureConnector
participant LLMCost as LLMCostService
Client->>API: GET /features/{id}
API->>NS: Resolve namespace
NS-->>API: namespace
API->>FeatureSvc: GetFeature(namespace, id)
FeatureSvc-->>API: feature (may include unit_cost + meter filters)
API->>API: convertFeatureToAPI(feature)
alt unit_cost.type == "llm" and llmcost service available
API->>API: extract provider/model/token from meter filters or static fields
API->>LLMCost: ResolvePrice(provider, model, token_type)
LLMCost-->>API: pricing (per-token rates)
API->>API: enrichFeatureResponseWithPricing(pricing)
end
API-->>Client: 200 OK (Feature JSON, pricing if resolved)
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 |
…fixes - Reference meters by required ID instead of key in v3 Feature API - Remove unsupported description field from Feature model - Resolve meter ID to slug in CreateFeature handler via meter service - Fix unnecessary type conversions flagged by linter - Fix test to check error returns instead of discarding them Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The FeatureLLMUnitCost and pricing field descriptions implied that only meter group-by properties were supported. Updated to accurately describe that both static values and property references work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… costs AsFeatureLLMUnitCost() silently succeeds on manual union variants, causing FromFeatureLLMUnitCost() to overwrite the manual cost data. Add discriminator check before attempting the LLM cast. Added test for the manual unit cost no-op case that exposed the bug. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validate filter dimension keys against the meter's GroupBy and reject unknown filter operators at the API layer before reaching the domain. Fix remaining unnecessary type conversions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hekike
force-pushed
the
feat/feature-meter-id
branch
from
44dcd78 to
48fc691
Compare
hekike
changed the title
![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 (2)
openmeter/productcatalog/driver/feature.go (1)
130-143: Consider removing the extra meter lookup in the HTTP layer.Right now meter resolution happens here and again in the connector, which adds an avoidable round-trip on create. You can pass the incoming meter reference through and let connector-side validation/normalization do the single lookup.
♻️ Suggested simplification
- // Resolve meter slug to meter ID - var meterID *string - if parsedBody.MeterSlug != nil { - m, err := h.meterService.GetMeterByIDOrSlug(ctx, meter.GetMeterInput{ - Namespace: ns, - IDOrSlug: *parsedBody.MeterSlug, - }) - if err != nil { - return emptyFeature, err - } - meterID = &m.ID - } - - return MapFeatureCreateInputsRequest(ns, parsedBody, meterID) + return MapFeatureCreateInputsRequest(ns, parsedBody, parsedBody.MeterSlug)As per coding guidelines: “Performance should be a priority in critical code paths… database operations… should be vetted for potential performance bottlenecks.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/driver/feature.go` around lines 130 - 143, Remove the redundant meter lookup in the HTTP layer: stop calling h.meterService.GetMeterByIDOrSlug and avoid creating meterID here (the block referencing parsedBody.MeterSlug, m.ID and meterID). Instead pass the incoming meter reference from parsedBody through into MapFeatureCreateInputsRequest (or change MapFeatureCreateInputsRequest signature to accept the raw meter slug/ref) and let the connector-side validation/normalization perform the single lookup; update any call sites relying on meterID accordingly so the connector handles resolution/validation.api/v3/handlers/features/convert.go (1)
280-286: Consider returningnilwhen there are no labels.This helper always allocates a map, so
labelswill serialize as{}even when the feature has no metadata. Returningnilfor empty input preserves theomitemptybehavior on the API model.♻️ Possible tweak
func convertMetadataToLabels(metadata models.Metadata) *api.Labels { + if len(metadata) == 0 { + return nil + } + labels := make(api.Labels) for k, v := range metadata { labels[k] = v } return &labels }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert.go` around lines 280 - 286, The convertMetadataToLabels helper always allocates and returns a non-nil api.Labels map, causing empty metadata to serialize as {}; change convertMetadataToLabels(models.Metadata) to return nil when metadata is empty: check if metadata is nil or has length 0 and return nil, otherwise allocate api.Labels, copy entries from metadata into it and return &labels; this preserves omitempty on the API model while still returning a pointer to api.Labels when there are entries.
🤖 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/packages/aip/src/features/unitcost.tsp`:
- Around line 31-82: FeatureLLMUnitCost currently allows invalid selector
combinations and leaks server-computed fields into request shapes; fix by
modeling selectors as strict unions and separating request/response fields:
replace the optional pairs (provider/provider_property), (model/model_property),
and (token_type/token_type_property) with explicit union types (e.g.,
ProviderSelector = { provider: string } | { provider_property: string },
ModelSelector = { model: string } | { model_property: string },
TokenTypeSelector = { token_type: string } | { token_type_property: string })
and use those union types as the selector properties on FeatureLLMUnitCost so
the compiler enforces mutual exclusivity and that one must be set; and
remove/move the pricing field out of the input/request shape by marking it
response-only (use your TypeSpec visibility/readOnly mechanism or create
separate FeatureLLMUnitCostCreate vs FeatureLLMUnitCostResponse models) so
server-computed pricing does not appear in generated create requests.
- Around line 84-91: The shared FeatureLLMUnitCost model exposes the computed
pricing field in create requests; split it into two types (e.g.,
FeatureLLMUnitCostCreate and FeatureLLMUnitCostRead), remove the pricing
property from the Create variant (keep pricing?: FeatureLLMUnitCostPricing only
on the Read variant), and update any usages: use FeatureLLMUnitCostCreate in
input types like CreateFeatureRequest/Feature create paths and
FeatureLLMUnitCostRead in response/Feature read paths so clients cannot send the
server-computed pricing field; update any references to FeatureLLMUnitCost,
pricing, Feature, and CreateFeatureRequest accordingly.
In `@api/v3/handlers/features/convert_test.go`:
- Around line 224-236: The test uses a non-ULID MeterID ("tokens_total"), which
hides ID-format regressions; update the fixture used to create feature f by
setting MeterID to a ULID-shaped string (e.g., a valid 26-char ULID) and adjust
the assertion that compares result.Meter.Id (in the convertFeatureToAPI test) to
expect that same ULID (api.ULID("<your-ulid>")) so the v3 ID-based flow is
exercised; locate the MeterID field in the test fixture and the subsequent
assert.Equal against api.ULID to make the values consistent.
In `@api/v3/handlers/features/convert.go`:
- Around line 270-277: The converter convertFilterStringToAPIMapItem is missing
several FilterString fields (contains, ncontains, and/or) so
api.QueryFilterStringMapItem loses operators on round-trip; update the returned
struct in convertFilterStringToAPIMapItem to also set Contains (from
f.Contains), Ncontains (from f.Ncontains or f.Ncontains depending on exact field
name), And (from f.And) and Or (from f.Or) so all supported operators in
filter.FilterString are preserved when serializing to
api.QueryFilterStringMapItem.
In `@api/v3/handlers/features/create.go`:
- Line 90: The current validation calls ValidateQueryFilterStringMapItem(v,
"meter", "filters", k) which only rejects unknown operators; update the handler
to validate incoming filter operators against the exact operator set accepted by
the downstream filter converter (or perform a dry-run conversion) to prevent
runtime failures. Replace or augment the call to
ValidateQueryFilterStringMapItem for the "meter" -> "filters" -> k entry by
either (a) invoking the downstream converter/validator used at query execution
time (e.g., perform a dry-run with the converter you use to build executable
filters) and surface any conversion errors, or (b) explicitly check the operator
tokens in v against the converter’s supported operator list so only supported
operators pass validation. Ensure the error message references the same
"meter"/"filters"/k context when returning validation errors.
- Around line 41-44: Before calling h.meterService.GetMeterByIDOrSlug, validate
that body.Meter.Id is non-empty and return a validation error immediately if it
is empty to avoid the DB lookup; update the create feature handler to perform
this pre-check on body.Meter.Id and only call GetMeterByIDOrSlug when the
id/slug is present, using the same error response path you use for other
validation errors.
In `@api/v3/handlers/features/error_encoder.go`:
- Line 18: Change the HTTP status mapping for feature.ForbiddenError from
BadRequest to Forbidden: update the call to
commonhttp.HandleErrorIfTypeMatches[*feature.ForbiddenError](ctx,
http.StatusBadRequest, err, w) to use http.StatusForbidden instead so
ForbiddenError returns 403; keep the rest of the call/site unchanged.
In `@api/v3/handlers/features/get.go`:
- Line 38: The GET handler currently calls h.connector.GetFeature(...,
feature.IncludeArchivedFeatureFalse) which excludes soft-deleted features;
change the call to include archived features instead (e.g., pass
feature.IncludeArchivedFeatureTrue or remove the exclusion flag according to the
GetFeature signature) so GET returns soft-deleted entities for consistency with
other v3 read endpoints; update the invocation of GetFeature in the handler to
use the "include archived" option and run tests to ensure behavior matches other
read paths.
In `@api/v3/server/server.go`:
- Line 213: The code currently constructs the features handler unconditionally
via featureshandler.New(resolveNamespace, config.FeatureConnector, ...) which
will later panic if config.FeatureConnector is nil; add a fail-fast check during
server initialization to validate config.FeatureConnector is non-nil (e.g.,
before calling featureshandler.New) and return an error or log/fatal with a
clear message if it's missing so the server won't start with a nil
FeatureConnector; ensure the check references config.FeatureConnector and the
handler creation site (featureshandler.New) so maintainers can locate and fix
the issue.
---
Nitpick comments:
In `@api/v3/handlers/features/convert.go`:
- Around line 280-286: The convertMetadataToLabels helper always allocates and
returns a non-nil api.Labels map, causing empty metadata to serialize as {};
change convertMetadataToLabels(models.Metadata) to return nil when metadata is
empty: check if metadata is nil or has length 0 and return nil, otherwise
allocate api.Labels, copy entries from metadata into it and return &labels; this
preserves omitempty on the API model while still returning a pointer to
api.Labels when there are entries.
In `@openmeter/productcatalog/driver/feature.go`:
- Around line 130-143: Remove the redundant meter lookup in the HTTP layer: stop
calling h.meterService.GetMeterByIDOrSlug and avoid creating meterID here (the
block referencing parsedBody.MeterSlug, m.ID and meterID). Instead pass the
incoming meter reference from parsedBody through into
MapFeatureCreateInputsRequest (or change MapFeatureCreateInputsRequest signature
to accept the raw meter slug/ref) and let the connector-side
validation/normalization perform the single lookup; update any call sites
relying on meterID accordingly so the connector handles resolution/validation.
🪄 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: 3618ede3-0819-4fe3-9710-c66f0d0f840b
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
api/spec/packages/aip/src/features/feature.tspapi/spec/packages/aip/src/features/index.tspapi/spec/packages/aip/src/features/unitcost.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/features/create.goapi/v3/handlers/features/create_test.goapi/v3/handlers/features/delete.goapi/v3/handlers/features/error_encoder.goapi/v3/handlers/features/get.goapi/v3/handlers/features/handler.goapi/v3/handlers/features/list.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/productcatalog/driver/feature.goopenmeter/productcatalog/driver/parser.goopenmeter/server/router/router.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/v3/handlers/features/convert.go (2)
202-204: Silently ignored error onFromFeatureLLMUnitCost.Line 203 discards the error from
FromFeatureLLMUnitCost. If this fails, the pricing won't be set and there's no indication why. Consider at least logging the error for debugging purposes.🔧 Suggested fix
- _ = resp.UnitCost.FromFeatureLLMUnitCost(llmCost) + if err := resp.UnitCost.FromFeatureLLMUnitCost(llmCost); err != nil { + // Log error but don't fail - pricing is optional enrichment + return + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert.go` around lines 202 - 204, The call to FromFeatureLLMUnitCost is currently ignoring its returned error (in the block setting llmCost.Pricing and calling resp.UnitCost.FromFeatureLLMUnitCost(llmCost)), so failures are silent; change this to capture the error, check it, and handle it (at minimum log via the existing logger or return the error) so pricing failures are visible—e.g., assign the result to err, and if err != nil, call the appropriate logger or propagate the error back to the caller from the surrounding function.
91-113: Consider adding a nil guard foru.LLM.At line 93, you access
u.LLM.ProviderPropertywithout first checking ifu.LLMis nil. While domain validation (perUnitCost.Validate()) should ensureLLMis set whenType == UnitCostTypeLLM, a defensive nil check would make this more robust against data corruption or bypassed validation.🛡️ Optional defensive check
case feature.UnitCostTypeLLM: + if u.LLM == nil { + return out, fmt.Errorf("LLM configuration is nil for LLM unit cost type") + } llmCost := api.FeatureLLMUnitCost{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert.go` around lines 91 - 113, The conversion branch for UnitCostTypeLLM dereferences u.LLM without a nil check; add a defensive guard at the start of the case (e.g., if u.LLM == nil { return out, fmt.Errorf("missing LLM data for UnitCostTypeLLM") }) to avoid panics, then proceed to populate api.FeatureLLMUnitCost and call out.FromFeatureLLMUnitCost as before; reference symbols: u.LLM, UnitCostTypeLLM, api.FeatureLLMUnitCost, out.FromFeatureLLMUnitCost (no other changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/features/convert.go`:
- Around line 202-204: The call to FromFeatureLLMUnitCost is currently ignoring
its returned error (in the block setting llmCost.Pricing and calling
resp.UnitCost.FromFeatureLLMUnitCost(llmCost)), so failures are silent; change
this to capture the error, check it, and handle it (at minimum log via the
existing logger or return the error) so pricing failures are visible—e.g.,
assign the result to err, and if err != nil, call the appropriate logger or
propagate the error back to the caller from the surrounding function.
- Around line 91-113: The conversion branch for UnitCostTypeLLM dereferences
u.LLM without a nil check; add a defensive guard at the start of the case (e.g.,
if u.LLM == nil { return out, fmt.Errorf("missing LLM data for UnitCostTypeLLM")
}) to avoid panics, then proceed to populate api.FeatureLLMUnitCost and call
out.FromFeatureLLMUnitCost as before; reference symbols: u.LLM, UnitCostTypeLLM,
api.FeatureLLMUnitCost, out.FromFeatureLLMUnitCost (no other changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10cda57a-d0a0-4f25-8c22-f2bfca356203
📒 Files selected for processing (5)
api/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/features/create.goapi/v3/handlers/features/error_encoder.goapi/v3/server/server.go
✅ Files skipped from review due to trivial changes (1)
- api/v3/handlers/features/convert_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/v3/handlers/features/error_encoder.go
- api/v3/server/server.go
tothandras
requested changes
Mar 21, 2026
tothandras
requested changes
Mar 21, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/api.gen.go (1)
2284-2290:⚠️ Potential issue | 🟠 MajorMake map-item filters self-recursive.
These new feature shapes still hang
meter.filtersoffQueryFilterStringMapItem, but that filter type recurses throughQueryFilterStringforand/orinstead of recursing to itself. A nested case like{"or":[{"exists":false},{"eq":"x"}]}falls off the typed path and only survives viaAdditionalProperties, which is pretty easy for conversion/validation code to miss. I’d point those branches back to the map-item filter schema in the OpenAPI source and regenerate.As per coding guidelines,
Never manually edit generated files marked with '// Code generated by X, DO NOT EDIT.' headers.Also applies to: 2421-2427
🤖 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 2284 - 2290, The generated API types currently have QueryFilterStringMapItem pointing to QueryFilterString for its "and"/"or" recursion, which loses typed map-item branches (e.g., meter.filters entries); update the OpenAPI schema so the map-item filter schema references itself for nested "and"/"or" (i.e., make QueryFilterStringMapItem recursive) and then regenerate the code rather than editing api/v3/api.gen.go directly; locate the map item schema that produces QueryFilterStringMapItem and change its "and"/"or" sub-schemas to reference the map-item schema, then run the codegen to propagate the fix (also apply same change for the other affected schema that generated lines ~2421-2427).
🧹 Nitpick comments (1)
pkg/filter/filter.go (1)
50-63: Could we add a quick round-trip table test for this helper?Since escaping logic is easy to regress, a small test matrix (
"","%","_","\\", mixed strings) validatingReverseContainsPattern(ptr(ContainsPattern(x))) == xwould make this safer long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/filter/filter.go` around lines 50 - 63, Add a small table-driven unit test that verifies round-trip escaping between ContainsPattern and ReverseContainsPattern: for each test case (e.g., "", "%", "_", "\\", "foo%bar", "a_b\\c", and mixed strings) call like := ContainsPattern(tc), then got := ReverseContainsPattern(like) and assert that dereferenced got equals the original tc (handle nil pointers consistently). Put the test in the package tests for these functions (e.g., add a TestContainsPatternRoundTrip using a slice of inputs and t.Run subtests) and include clear failure messages indicating the input that failed.
🤖 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/v3/api.gen.go`:
- Around line 1622-1625: The generated API includes a response-only Pricing
field on BillingFeatureLLMUnitCost (and CreateFeatureRequest.UnitCost reusing
that union), allowing clients to echo server-resolved pricing into create
requests; fix this by updating the source schema/IDL (not api/v3/api.gen.go) to
separate request vs response shapes—e.g., introduce a create-only variant of
BillingFeatureLLMUnitCost (or remove pricing from the
CreateFeatureRequest.UnitCost union) so the create path does not carry Pricing,
then regenerate the API; ensure you update the corresponding schema entries that
generate the sections around the BillingFeatureLLMUnitCost/Pricing and the
CreateFeatureRequest unit cost union (addresses the duplicated spots including
the other generated block around lines referenced in the comment).
---
Outside diff comments:
In `@api/v3/api.gen.go`:
- Around line 2284-2290: The generated API types currently have
QueryFilterStringMapItem pointing to QueryFilterString for its "and"/"or"
recursion, which loses typed map-item branches (e.g., meter.filters entries);
update the OpenAPI schema so the map-item filter schema references itself for
nested "and"/"or" (i.e., make QueryFilterStringMapItem recursive) and then
regenerate the code rather than editing api/v3/api.gen.go directly; locate the
map item schema that produces QueryFilterStringMapItem and change its "and"/"or"
sub-schemas to reference the map-item schema, then run the codegen to propagate
the fix (also apply same change for the other affected schema that generated
lines ~2421-2427).
---
Nitpick comments:
In `@pkg/filter/filter.go`:
- Around line 50-63: Add a small table-driven unit test that verifies round-trip
escaping between ContainsPattern and ReverseContainsPattern: for each test case
(e.g., "", "%", "_", "\\", "foo%bar", "a_b\\c", and mixed strings) call like :=
ContainsPattern(tc), then got := ReverseContainsPattern(like) and assert that
dereferenced got equals the original tc (handle nil pointers consistently). Put
the test in the package tests for these functions (e.g., add a
TestContainsPatternRoundTrip using a slice of inputs and t.Run subtests) and
include clear failure messages indicating the input that failed.
🪄 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: 46eb78ee-deee-44a3-ae6f-29e1b4fb8aaa
⛔ Files ignored due to path filters (12)
api/v3/openapi.yamlis excluded by!**/openapi.yamlgo.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/feature.gois excluded by!**/ent/db/**openmeter/ent/db/feature/feature.gois excluded by!**/ent/db/**openmeter/ent/db/feature/where.gois excluded by!**/ent/db/**openmeter/ent/db/feature_create.gois excluded by!**/ent/db/**openmeter/ent/db/feature_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (12)
api/spec/packages/aip/src/features/feature.tspapi/spec/packages/aip/src/features/unitcost.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goopenmeter/ent/schema/feature.goopenmeter/productcatalog/adapter/feature.goopenmeter/productcatalog/feature/connector.goopenmeter/productcatalog/feature/feature.gopkg/filter/filter.gotools/migrate/migrations/20260322041807_add-feature-description.down.sqltools/migrate/migrations/20260322041807_add-feature-description.up.sql
✅ Files skipped from review due to trivial changes (6)
- tools/migrate/migrations/20260322041807_add-feature-description.up.sql
- openmeter/ent/schema/feature.go
- openmeter/productcatalog/feature/feature.go
- openmeter/productcatalog/feature/connector.go
- api/v3/handlers/features/convert.go
- api/v3/handlers/features/convert_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/packages/aip/src/features/feature.tsp
- api/spec/packages/aip/src/features/unitcost.tsp
Comment on lines
+1622
to
+1625
| // Pricing Resolved per-token pricing from the LLM cost database. | ||
| // Populated in responses when the provider and model can be determined, | ||
| // either from static values or from meter group-by filters with exact matches. | ||
| Pricing *BillingFeatureLLMUnitCostPricing `json:"pricing,omitempty"` |
Contributor
There was a problem hiding this comment.
Keep resolved LLM pricing out of create requests.
BillingFeatureLLMUnitCost now carries pricing, and CreateFeatureRequest.UnitCost reuses that same union. That means a client can echo a fetched feature back into create and send server-resolved pricing even though openmeter/productcatalog/feature/unitcost.go:26-66 has no input field for it. I’d split the request/response schemas, or use a create-only variant without pricing, then regenerate this file.
As per coding guidelines, Never manually edit generated files marked with '// Code generated by X, DO NOT EDIT.' headers.
Also applies to: 2297-2300
🤖 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 1622 - 1625, The generated API includes a
response-only Pricing field on BillingFeatureLLMUnitCost (and
CreateFeatureRequest.UnitCost reusing that union), allowing clients to echo
server-resolved pricing into create requests; fix this by updating the source
schema/IDL (not api/v3/api.gen.go) to separate request vs response shapes—e.g.,
introduce a create-only variant of BillingFeatureLLMUnitCost (or remove pricing
from the CreateFeatureRequest.UnitCost union) so the create path does not carry
Pricing, then regenerate the API; ensure you update the corresponding schema
entries that generate the sections around the BillingFeatureLLMUnitCost/Pricing
and the CreateFeatureRequest unit cost union (addresses the duplicated spots
including the other generated block around lines referenced in the comment).
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.
Feature Meter ID
Summary
Migrate features from referencing meters by slug to referencing by ID. Adds v3 feature CRUD handlers (
/api/v3/openmeter/features) that use meter ID natively, while preserving full v1 API backward compatibility.What
/api/v3/openmeter/featuresthat reference meters by ID instead of slug, with structured filter syntax (eq,in,neq, etc.).meter_idcolumn to thefeaturestable with a foreign key tometers, add reverse edge from meters to features.meterSlugcontinues to be populated in v1 responses by eager-loading the meter edge on read, and by populating from the resolved meter on create.manualandllmunit cost types, with LLM pricing resolution from the cost database.Why
Meter slugs can change; referencing by immutable ID is more robust. The v3 API provides a cleaner contract (structured filters, labels instead of metadata, snake_case fields) while the v1 API remains unchanged.
CodeRabbit Review Fixes
Addressed valid review comments:
error_encoder.go): Changedhttp.StatusBadRequesttohttp.StatusForbidden(403) forForbiddenError.convert.go):convertFilterStringToAPIMapItemwas missingContains,Ncontains,And,Orfields. Added reverse conversion includingreverseContainsPatternhelper and recursiveAnd/Orconversion.server.go): Added validation inConfig.Validate()alongside all other required service checks to prevent runtime panics.create.go): Validatebody.Meter.Idis non-empty before callingGetMeterByIDOrSlug(defense-in-depth; OpenAPI schema also validates ULID format).convert_test.go): Replaced"tokens_total"with a valid ULID to better exercise the v3 ID-based flow.convert.go):convertMetadataToLabelsnow returnsnilfor empty metadata, preservingomitemptyso empty metadata doesn't serialize as{}.Validation Results
All tests performed against
http://localhost:8888with the server running from thefeat/feature-meter-idbranch.V3 Feature API (
/api/v3/openmeter/features)meter.idPOSTmeter.idmeter.filtersPOSTPOSTmeterfield in responsePOSTPOSTPOSTPOSTGET /{id}GET /{id}GET{data, meta}responseDELETE /{id}Filter Operators Validation
All filter operators tested end-to-end (API -> Postgres storage -> read-back -> ClickHouse query path):
eq{"$eq": "GET"}SelectWhereExpr->EQneq{"$ne": ...}SelectWhereExpr->NEin{"$in": [...]}SelectWhereExpr->Innin{"$nin": [...]}SelectWhereExpr->NotInexists{"$exists": true}SelectWhereExpr->IsNotNull/IsNullcontains{"$like": "%api%"}SelectWhereExpr->Likencontains{"$nlike": "%admin%"}SelectWhereExpr->NotLikeand{"$and": [...]}SelectWhereExpr-> recursiveAndor{"$or": [...]}SelectWhereExpr-> recursiveOrStorage uses dual columns:
meter_group_by_filters(legacymap[string]string, only for simpleeqfilters) andadvanced_meter_group_by_filters(fullFilterStringJSON). Reader prefersadvanced_meter_group_by_filterswhen available, falls back to legacy column.Cross-API Compatibility
meter.idpopulated frommeter_idcolumnmeterSlugpopulated via eager-loaded meter edgeBug Fixes Verified
http.StatusForbiddenconvertFilterStringToAPIMapItemConfig.Validate(){}convertMetadataToLabelsreturns nil for emptySummary by CodeRabbit