fear(api): v3 feature cost#3932
Merged
Merged
Conversation
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 new feature-cost API surface: TypeSpec models and POST /openmeter/features/{featureId}/cost endpoint, generated server wiring, a feature cost handler with domain→API converters, and a centralized meters query package (params, errors, dimensions, customer resolution). Also adds ParseOptionalBody and wires CostService/FeatureConnector into server config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Gateway"
participant Handler as "FeatureCost Handler"
participant Feature as "Feature Service"
participant Meter as "Meter Service"
participant Customer as "Customer Service"
participant Cost as "Cost Service"
participant Converter as "Response Converter"
Client->>API: POST /openmeter/features/{featureId}/cost\n(MeterQueryRequest)
API->>Handler: QueryFeatureCost(featureId, request)
Handler->>Feature: Get feature by ID
Feature-->>Handler: feature (includes meter id/slug)
Handler->>Meter: Resolve meter by id/slug
Meter-->>Handler: meter details
Handler->>Handler: BuildQueryParams(meter, request)
alt request contains customer IDs
Handler->>Customer: ListCustomers(ids)
Customer-->>Handler: customers
end
Handler->>Cost: QueryCost(params)
Cost-->>Handler: CostQueryResult
Handler->>Converter: ConvertCostQueryResultToAPI(result)
Converter-->>Handler: FeatureCostQueryResult
Handler-->>API: 200 + FeatureCostQueryResult
API-->>Client: HTTP 200 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 4
🧹 Nitpick comments (3)
api/v3/handlers/query/convert.go (1)
96-104: Potential panic if map is empty.The
firstKeyfunction will panic with an index out of range if called with an empty map sincekeys[0]is accessed unconditionally. Given thatAdditionalPropertieslength is checked before calling this (lines 46, 72), you're safe in current usage, but it's a bit fragile.Consider adding a guard:
🛡️ Defensive fix
func firstKey[V any](m map[string]V) string { keys := make([]string, 0, len(m)) for k := range m { keys = append(keys, k) } + if len(keys) == 0 { + return "" + } sort.Strings(keys) return keys[0] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/query/convert.go` around lines 96 - 104, The helper firstKey[V any](m map[string]V) can panic on empty maps because it returns keys[0] unconditionally; update firstKey to defensively handle empty input (e.g., return an empty string or a sentinel value) when len(m)==0 before sorting/returning, and ensure callers (code that checks AdditionalProperties) can handle the empty result; this keeps the function safe even if future call sites omit the pre-check.api/spec/src/v3/features/cost.tsp (1)
49-64: Consider extracting a shared dimensions model.Lines 49-64 repeat the same
subject/customer_id+ open string-map shape that meter query rows already use. That already turns into another generated helper/type pair inapi/v3/api.gen.go, and future dimension changes can now drift between the two responses. A shared named model would keep them in lockstep.As per coding guidelines: Review the TypeSpec code for conformity with TypeSpec best practices, considering that multiple codegeneration toolchains depend on the TypeSpec code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/features/cost.tsp` around lines 49 - 64, Extract the inline dimensions object into a shared named TypeSpec model (e.g., SharedDimensions or DimensionsModel) that declares subject?: string, customer_id?: string and an open string-indexer for additional keys, then replace the inline dimensions property in the current type (the dimensions block shown) with a reference to that new shared model and update any other types that currently repeat the same shape (e.g., the meter query rows type) to reference this single model so codegen stays consistent.api/v3/handlers/featurecost/query.go (1)
79-82: Use the resolved feature ID for the cost service call.Line 81 passes the raw request value even after
GetFeaturehas resolved the canonical feature. Usingfeat.IDkeeps the handoff tocost.QueryFeatureCostInputexplicit and avoids forwarding a key/alias if this handler continues to accept id-or-key lookups.♻️ Small cleanup
result, err := h.costService.QueryFeatureCost(ctx, cost.QueryFeatureCostInput{ Namespace: req.Namespace, - FeatureID: req.FeatureID, + FeatureID: feat.ID, QueryParams: params, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/featurecost/query.go` around lines 79 - 82, The handler currently forwards the original request's FeatureID to cost.QueryFeatureCostInput even after resolving the canonical feature via GetFeature; update the call to use the resolved feature ID (feat.ID) instead of req.FeatureID so cost.QueryFeatureCostInput receives the canonical identifier (refer to GetFeature, feat.ID, QueryFeatureCost and cost.QueryFeatureCostInput) and avoid passing a potential alias/key through.
🤖 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/features/operations.tsp`:
- Around line 27-29: The spec currently types the path param as Shared.ULID but
the handler resolves via GetFeature using an idOrKey string; update the
operation signature to reflect that the path accepts an id-or-key string (e.g.,
change the `@path` param from featureId: Shared.ULID to featureId: string or a new
Shared.FeatureIdOrKey type) so the documented contract matches the runtime
behavior used by GetFeature in the feature cost query handler; alternatively, if
you prefer ID-only, change the handler (GetFeature call in the feature cost
query logic) to validate/accept only ULIDs and return NotFound for non-ULID
inputs—choose and apply one consistent change across the operation signature and
the feature cost query handling.
In `@api/v3/handlers/featurecost/query.go`:
- Line 52: The read handler currently calls h.featureConnector.GetFeature with
feature.IncludeArchivedFeatureFalse which prevents returning archived
(soft-deleted) features; update the call in the feature cost read path to allow
archived features (e.g., use feature.IncludeArchivedFeatureTrue or the
equivalent "include archived" option) so GetFeature can return archived entities
for read-only cost queries, and ensure any IsDeleted checks remain only in
create/update/upsert code paths rather than here; locate the call to
h.featureConnector.GetFeature in this handler and change the archived-flag
argument accordingly.
In `@api/v3/server/server.go`:
- Around line 216-219: The featureCostH variable can remain nil when
config.CostService or config.FeatureConnector are missing, but routes.go calls
s.featureCostHandler.QueryFeatureCost unconditionally; either enforce the
dependencies in Config.Validate() to require config.CostService and
config.FeatureConnector (and fail fast) or initialize featureCostH to an
explicit fallback Handler that implements the same interface and returns a
501/404 API error for QueryFeatureCost; update the initialization site where
featurecosthandler.New(...) is called to create the real handler when deps exist
or assign the fallback handler when they don’t, and/or add validation in
Config.Validate() to ensure config.CostService and config.FeatureConnector are
present so featurecosthandler.New is always invoked.
In `@openmeter/server/server.go`:
- Around line 126-127: Decide whether CostService and FeatureConnector are
required; if required, update Validate() to return an error when
config.RouterConfig.CostService or config.RouterConfig.FeatureConnector is nil
and ensure featureCostHandler is always constructed when those fields exist (so
s.featureCostHandler is never nil); if optional, add a nil-check in the route
that calls s.featureCostHandler.QueryFeatureCost() (e.g., in the handler invoked
by routes.go:191) and return an appropriate error response or 501/404 when
s.featureCostHandler is nil instead of calling QueryFeatureCost(); reference the
symbols CostService, FeatureConnector, Validate(), featureCostHandler, and
QueryFeatureCost() when making the change.
---
Nitpick comments:
In `@api/spec/src/v3/features/cost.tsp`:
- Around line 49-64: Extract the inline dimensions object into a shared named
TypeSpec model (e.g., SharedDimensions or DimensionsModel) that declares
subject?: string, customer_id?: string and an open string-indexer for additional
keys, then replace the inline dimensions property in the current type (the
dimensions block shown) with a reference to that new shared model and update any
other types that currently repeat the same shape (e.g., the meter query rows
type) to reference this single model so codegen stays consistent.
In `@api/v3/handlers/featurecost/query.go`:
- Around line 79-82: The handler currently forwards the original request's
FeatureID to cost.QueryFeatureCostInput even after resolving the canonical
feature via GetFeature; update the call to use the resolved feature ID (feat.ID)
instead of req.FeatureID so cost.QueryFeatureCostInput receives the canonical
identifier (refer to GetFeature, feat.ID, QueryFeatureCost and
cost.QueryFeatureCostInput) and avoid passing a potential alias/key through.
In `@api/v3/handlers/query/convert.go`:
- Around line 96-104: The helper firstKey[V any](m map[string]V) can panic on
empty maps because it returns keys[0] unconditionally; update firstKey to
defensively handle empty input (e.g., return an empty string or a sentinel
value) when len(m)==0 before sorting/returning, and ensure callers (code that
checks AdditionalProperties) can handle the empty result; this keeps the
function safe even if future call sites omit the pre-check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dc8c415-aa1e-45b2-89d8-c471cab98be7
⛔ Files ignored due to path filters (2)
api/v3/openapi.yamlis excluded by!**/openapi.yamlgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (22)
api/spec/src/v3/features/cost.tspapi/spec/src/v3/features/index.tspapi/spec/src/v3/features/operations.tspapi/spec/src/v3/openmeter.tspapi/spec/src/v3/shared/consts.tspapi/v3/api.gen.goapi/v3/handlers/featurecost/convert.goapi/v3/handlers/featurecost/handler.goapi/v3/handlers/featurecost/query.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/dimensions.goapi/v3/handlers/meters/errors.goapi/v3/handlers/meters/query.goapi/v3/handlers/query/convert.goapi/v3/handlers/query/customers.goapi/v3/handlers/query/dimensions.goapi/v3/handlers/query/errors.goapi/v3/handlers/query/params.goapi/v3/request/body.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/server/server.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/meters/errors.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/handlers/query/convert_test.go (1)
69-141: Add positive cases for the other schema-supported operators.These suites currently prove
eqandinstay valid, but the generated query model also exposescontains,ncontains,neq, andnin. If validation starts rejecting one of those later, this file won’t catch it.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.Also applies to: 143-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/query/convert_test.go` around lines 69 - 141, Add positive tests to TestValidateQueryFilterString so the other supported operators (contains, ncontains, neq, nin) are validated as acceptable: add test cases that set api.QueryFilterString{Contains: lo.ToPtr("val")}, {Ncontains: lo.ToPtr("val")}, {Neq: lo.ToPtr("val")}, and {Nin: &[]string{"x","y"}} with wantErr=false, and mirror at least one nested case (e.g., inside And or Or) to ensure ValidateQueryFilterString accepts those operators both at top-level and in nested filters; reference the ValidateQueryFilterString function and the api.QueryFilterString struct to locate where to append these cases.
🤖 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/handlers/query/params_test.go`:
- Around line 266-284: The test TestCustomersToStreaming only checks length;
update it to assert the mapped fields for each returned item to lock the
conversion behavior: call CustomersToStreaming(customers) and assert that
result[0].ID == "c1" and result[0].Namespace == "ns" (and similarly for
result[1] with "c2"), or otherwise compare the slice to an expected slice of
streaming items to validate both values and order; use the existing require
assertions (require.Equal or require.DeepEqual) to fail clearly if mapping or
ordering changes.
- Around line 169-195: The test's fake resolver ignores its inputs so it doesn't
verify BuildQueryParams passed the correct namespace or IDs; modify the resolver
used in the "resolves customers and adds to group by" test to capture the
namespace and id slice arguments into local variables (e.g., capturedNS,
capturedIDs) and after calling BuildQueryParams add assertions that capturedNS
equals the expected namespace ("test-ns") and capturedIDs equals
[]string{"c1","c2"}; keep the existing assertions on params (FilterCustomer
length and GroupBy) so you validate both inputs to and outputs from
BuildQueryParams and reference the resolver closure and BuildQueryParams in the
test.
---
Nitpick comments:
In `@api/v3/handlers/query/convert_test.go`:
- Around line 69-141: Add positive tests to TestValidateQueryFilterString so the
other supported operators (contains, ncontains, neq, nin) are validated as
acceptable: add test cases that set api.QueryFilterString{Contains:
lo.ToPtr("val")}, {Ncontains: lo.ToPtr("val")}, {Neq: lo.ToPtr("val")}, and
{Nin: &[]string{"x","y"}} with wantErr=false, and mirror at least one nested
case (e.g., inside And or Or) to ensure ValidateQueryFilterString accepts those
operators both at top-level and in nested filters; reference the
ValidateQueryFilterString function and the api.QueryFilterString struct to
locate where to append these cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5d085cd-fa2c-4ce7-890d-1e23b9726abb
📒 Files selected for processing (3)
api/v3/handlers/query/convert_test.goapi/v3/handlers/query/dimensions_test.goapi/v3/handlers/query/params_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
api/v3/handlers/featurecost/query.go (1)
52-52:⚠️ Potential issue | 🟠 MajorAllow archived features on this cost query path.
Using
feature.IncludeArchivedFeatureFalsemakes historical cost queries stop working right after a feature is archived. This is a read-only handler, so archived features should still resolve here.💡 Suggested fix
- feat, err := h.featureConnector.GetFeature(ctx, req.Namespace, req.FeatureID, feature.IncludeArchivedFeatureFalse) + feat, err := h.featureConnector.GetFeature(ctx, req.Namespace, req.FeatureID, feature.IncludeArchivedFeatureTrue)Based on learnings: GET operations may return soft-deleted entities and
IsDeleted()checks should only be performed for CREATE, UPDATE, and UPSERT operations; apply the same read-vs-write split to analogous handlers underapi/v3/handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/featurecost/query.go` at line 52, The cost query handler currently forces exclusion of archived features by calling h.featureConnector.GetFeature with feature.IncludeArchivedFeatureFalse; change this call to allow archived features (e.g., use feature.IncludeArchivedFeatureTrue or the variant that includes archived) so GET/read operations resolve soft-deleted entities, and ensure no IsDeleted()/soft-delete blocking logic is applied in this read handler (leave IsDeleted checks only for CREATE/UPDATE/UPSERT write paths).api/v3/handlers/meters/query/params_test.go (2)
169-195:⚠️ Potential issue | 🟡 MinorCapture the resolver inputs too.
This still passes if
BuildQueryParamssends the wrong namespace or customer IDs to the resolver. It’d be worth capturing both in the fake and asserting them after the call, then keeping the existing output checks.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/meters/query/params_test.go` around lines 169 - 195, Modify the test's fake resolver in the t.Run block so it records the incoming namespace and ids parameters (e.g., capture them into local slices/vars) when BuildQueryParams calls the resolver, then after calling BuildQueryParams assert that the recorded namespace equals "test-ns" and the recorded ids equal []string{"c1","c2"} in addition to the existing assertions on params.FilterCustomer length and params.GroupBy containing DimensionCustomerID; keep the resolver return value the same and preserve existing output checks.
282-284:⚠️ Potential issue | 🟡 MinorCheck the converted customer data too.
Right now this only locks in the result length. It’d be nicer to assert a couple of stable fields per item, or compare against an expected slice, so the test fails on bad mapping or reordering.
As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/meters/query/params_test.go` around lines 282 - 284, The test only asserts length for CustomersToStreaming; update it to validate the converted customer data too by comparing each result item against expected values (e.g., stable fields like ID, Name, Email, CreatedAt or whatever mapping fields CustomersToStreaming produces) or by asserting equality against an expected slice of converted structs; ensure you verify ordering as well (use require.Equal/require.EqualValues or cmp to compare slices) and update the test in params_test.go to fail on incorrect mapping/reordering.
🤖 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/handlers/featurecost/convert.go`:
- Around line 13-19: The function ConvertCostQueryResultToAPI must not return a
half-populated api.FeatureCostQueryResult when result == nil; update the nil
branch to return a fully valid empty response: set From/To from body, set
Currency to body.Currency if provided (fallback to a sensible default like
"USD"), and initialize Data to an empty slice/map (not nil) so the response
contract is satisfied; modify ConvertCostQueryResultToAPI to populate these
fields accordingly (referencing ConvertCostQueryResultToAPI,
api.FeatureCostQueryResult, cost.CostQueryResult, and api.MeterQueryRequest).
In `@api/v3/handlers/meters/query/convert.go`:
- Around line 112-133: ExtractStringsFromQueryFilter currently ignores
f.AdditionalProperties so unknown JSON operators silently pass; update the
function (ExtractStringsFromQueryFilter) to inspect f.AdditionalProperties after
the existing checks and if it contains any keys other than "eq" or "in" return
NewUnsupportedFilterOperatorError(fieldPath...), ensuring those unknown
operators are rejected the same way as Neq/Nin/etc.; this preserves existing
behavior for Eq/In while enforcing validation of unknown operators.
In `@api/v3/handlers/meters/query/customers.go`:
- Around line 24-28: The ListCustomers call in the resolver is excluding
soft-deleted customers by setting IncludeDeleted: false; change the
ListCustomersInput to set IncludeDeleted: true so read-only query paths (the
customer resolution in meters/feature-cost queries) can resolve historical data
for deleted customers; locate the customerService.ListCustomers(...) call and
update the IncludeDeleted field on the ListCustomersInput struct to true (ensure
this change only applies to read resolvers, preserving false for
create/update/upsert paths).
In `@api/v3/handlers/meters/query/params_test.go`:
- Around line 212-215: The test currently only checks that params.FilterGroupBy
contains the key "region" but not its mapped value; update the params_test.go
case that calls BuildQueryParams to also assert the extracted filter value for
the "region" entry (from params.FilterGroupBy) matches the expected value from
the test body (e.g., use require.Equal or require.ElementsMatch on
params.FilterGroupBy["region"] or the corresponding slice) so the test fails if
the value is missing or incorrect; locate the assertion near the
BuildQueryParams call and add the value check referencing BuildQueryParams and
params.FilterGroupBy.
---
Duplicate comments:
In `@api/v3/handlers/featurecost/query.go`:
- Line 52: The cost query handler currently forces exclusion of archived
features by calling h.featureConnector.GetFeature with
feature.IncludeArchivedFeatureFalse; change this call to allow archived features
(e.g., use feature.IncludeArchivedFeatureTrue or the variant that includes
archived) so GET/read operations resolve soft-deleted entities, and ensure no
IsDeleted()/soft-delete blocking logic is applied in this read handler (leave
IsDeleted checks only for CREATE/UPDATE/UPSERT write paths).
In `@api/v3/handlers/meters/query/params_test.go`:
- Around line 169-195: Modify the test's fake resolver in the t.Run block so it
records the incoming namespace and ids parameters (e.g., capture them into local
slices/vars) when BuildQueryParams calls the resolver, then after calling
BuildQueryParams assert that the recorded namespace equals "test-ns" and the
recorded ids equal []string{"c1","c2"} in addition to the existing assertions on
params.FilterCustomer length and params.GroupBy containing DimensionCustomerID;
keep the resolver return value the same and preserve existing output checks.
- Around line 282-284: The test only asserts length for CustomersToStreaming;
update it to validate the converted customer data too by comparing each result
item against expected values (e.g., stable fields like ID, Name, Email,
CreatedAt or whatever mapping fields CustomersToStreaming produces) or by
asserting equality against an expected slice of converted structs; ensure you
verify ordering as well (use require.Equal/require.EqualValues or cmp to compare
slices) and update the test in params_test.go to fail on incorrect
mapping/reordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d12c6486-3788-40b9-afdf-5e8a4e57275d
📒 Files selected for processing (13)
api/v3/handlers/featurecost/convert.goapi/v3/handlers/featurecost/query.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/dimensions.goapi/v3/handlers/meters/query.goapi/v3/handlers/meters/query/convert.goapi/v3/handlers/meters/query/convert_test.goapi/v3/handlers/meters/query/customers.goapi/v3/handlers/meters/query/dimensions.goapi/v3/handlers/meters/query/dimensions_test.goapi/v3/handlers/meters/query/errors.goapi/v3/handlers/meters/query/params.goapi/v3/handlers/meters/query/params_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/v3/handlers/meters/convert.go
- api/v3/handlers/meters/dimensions.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v3/handlers/meters/query/convert.go (1)
33-38: Consider using a custom error type for consistency.
ConvertISO8601DurationToWindowSizeusesNewInvalidWindowSizeErrorfor unknown input, but this function uses a barefmt.Errorf. Using a consistent custom error type would make error handling more uniform for callers.♻️ Suggested fix
func ConvertWindowSizeToISO8601Duration(ws meter.WindowSize) (string, error) { if d, ok := windowSizeToISO8601[ws]; ok { return d, nil } - return "", fmt.Errorf("unknown WindowSize: %q", ws) + return "", NewInvalidWindowSizeError(string(ws)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/meters/query/convert.go` around lines 33 - 38, The ConvertWindowSizeToISO8601Duration function should return the same custom error type used elsewhere for unknown window sizes to keep error handling consistent; replace fmt.Errorf("unknown WindowSize: %q", ws) with a call to the existing NewInvalidWindowSizeError (used by ConvertISO8601DurationToWindowSize) so callers receive the same error type for invalid WindowSize inputs and adjust imports if necessary.
🤖 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/meters/query/convert.go`:
- Around line 33-38: The ConvertWindowSizeToISO8601Duration function should
return the same custom error type used elsewhere for unknown window sizes to
keep error handling consistent; replace fmt.Errorf("unknown WindowSize: %q", ws)
with a call to the existing NewInvalidWindowSizeError (used by
ConvertISO8601DurationToWindowSize) so callers receive the same error type for
invalid WindowSize inputs and adjust imports if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32413966-283b-47fb-a4a8-8b4026ec886a
📒 Files selected for processing (4)
api/v3/handlers/featurecost/convert.goapi/v3/handlers/featurecost/query.goapi/v3/handlers/meters/query/convert.goapi/v3/handlers/meters/query/customers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v3/handlers/featurecost/query.go
tothandras
previously approved these changes
Mar 10, 2026
tothandras
force-pushed
the
feat/feature-cost-v3
branch
from
4862c38 to
3580841
Compare
tothandras
approved these changes
Mar 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
api/v3/handlers/featurecost/query.go (1)
50-55:⚠️ Potential issue | 🟠 MajorThis read path should include archived features too.
Using
feature.IncludeArchivedFeatureFalsehere means historical cost queries start failing as soon as the feature is archived, even though this handler only reads data. I'd switch this lookup to the archived-inclusive option so the query stays consistent with the rest of the read-side soft-delete behavior.Based on learnings: "In customer-related handlers, GET operations (e.g., GetCustomer, GetCustomerBilling) may return soft-deleted entities and should not validate IsDeleted(). Ensure that IsDeleted() checks are only performed for CREATE, UPDATE, and UPSERT operations to prevent modifications to deleted entities. Apply this guidance to all analogous handler functions under api/v3/handlers to maintain consistent soft-delete semantics across read vs write paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/featurecost/query.go` around lines 50 - 55, The GetFeature call in the QueryFeatureCost handler currently uses feature.IncludeArchivedFeatureFalse which prevents reading archived (soft-deleted) features; change the lookup to use the archived-inclusive option (e.g., feature.IncludeArchivedFeatureTrue or the project's "include archived" flag) so QueryFeatureCost's call to h.featureConnector.GetFeature(...) returns archived features for read-only queries, and ensure you do not add any IsDeleted() validation in this read path (IsDeleted checks should remain only for CREATE/UPDATE/UPSERT handlers).api/spec/src/v3/features/operations.tsp (1)
26-29:⚠️ Potential issue | 🟡 MinorKeep
featureIdaligned with what the handler actually accepts.Right now the Go handler reads this path value as a plain string and forwards it straight into feature lookup, so documenting it as
Shared.ULIDstill advertises a narrower contract than the implementation enforces. If feature keys are intentionally supported here, this should be modeled as an id-or-key string type instead of ULID-only.As per coding guidelines: "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/features/operations.tsp` around lines 26 - 29, The documented path param for the operation queryCost is inaccurate: the handler accepts a plain string (ULID or feature key) and forwards it to lookup, so change the `@path` featureId type from Shared.ULID to a type that models "id-or-key" (either a simple string or a shared union type like Shared.IDOrKey) so the spec matches the implementation; update the operation signature for queryCost to use that id-or-key string type and, if needed, add/rename the shared type (e.g., Shared.IDOrKey) so other usages stay consistent.
🤖 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/features/cost.tsp`:
- Around line 17-27: The API model declares cost as nullable but leaves currency
required; update the spec so currency is optional whenever cost can be
unresolved: change the field definition for currency in the Cost type (symbol
"currency" in api/spec/src/v3/features/cost.tsp) to be optional/nullable to
match the runtime behavior in openmeter/cost/adapter/compute.go (see
CostQueryRow where Currency is only set when pricing resolves), or alternatively
ensure upstream code always populates CostQueryRow.Currency for unresolved cost
rows before regenerating the API; make the change in the cost type so the spec
and implementation are in parity.
In `@api/v3/api.gen.go`:
- Around line 4412-4414: The generated handler QueryFeatureCost is being exposed
even when featureCostHandler can be nil; fix by updating the handwritten
routing/DI so the route is only registered when featureCostHandler is non-nil
(or change the server method that calls featureCostHandler to return a
controlled 4xx/5xx error when nil), mirror the existing /service DI wiring used
for other optional handlers (the featureCostHandler wiring in server.go), then
regenerate the api/*.gen.go files so QueryFeatureCost (and the other listed
generated endpoints) are in sync with the DI change.
---
Duplicate comments:
In `@api/spec/src/v3/features/operations.tsp`:
- Around line 26-29: The documented path param for the operation queryCost is
inaccurate: the handler accepts a plain string (ULID or feature key) and
forwards it to lookup, so change the `@path` featureId type from Shared.ULID to a
type that models "id-or-key" (either a simple string or a shared union type like
Shared.IDOrKey) so the spec matches the implementation; update the operation
signature for queryCost to use that id-or-key string type and, if needed,
add/rename the shared type (e.g., Shared.IDOrKey) so other usages stay
consistent.
In `@api/v3/handlers/featurecost/query.go`:
- Around line 50-55: The GetFeature call in the QueryFeatureCost handler
currently uses feature.IncludeArchivedFeatureFalse which prevents reading
archived (soft-deleted) features; change the lookup to use the
archived-inclusive option (e.g., feature.IncludeArchivedFeatureTrue or the
project's "include archived" flag) so QueryFeatureCost's call to
h.featureConnector.GetFeature(...) returns archived features for read-only
queries, and ensure you do not add any IsDeleted() validation in this read path
(IsDeleted checks should remain only for CREATE/UPDATE/UPSERT handlers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bfb6ff3-5d54-4bf5-953a-53f9a8d59fb2
⛔ Files ignored due to path filters (3)
api/client/javascript/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!api/client/**api/spec/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (7)
api/spec/src/v3/features/cost.tspapi/spec/src/v3/features/operations.tspapi/spec/src/v3/konnect.tspapi/spec/src/v3/openmeter.tspapi/v3/api.gen.goapi/v3/handlers/featurecost/convert.goapi/v3/handlers/featurecost/query.go
Comment on lines
+4412
to
+4414
| // Query feature cost | ||
| // (POST /openmeter/features/{featureId}/cost/query) | ||
| QueryFeatureCost(w http.ResponseWriter, r *http.Request, featureId ULID) |
Contributor
There was a problem hiding this comment.
This endpoint is exposed even when its handler can be nil.
api/v3/server/server.go:226-236 only sets featureCostHandler when both CostService and FeatureConnector are present, but api/v3/server/routes.go:185-192 dereferences it unconditionally. With this generated interface/wrapper/route in place, deployments missing those deps will still register /openmeter/features/{featureId}/cost/query and panic on the first call instead of returning a controlled error. I’d guard the handwritten server method or only expose the route when the handler exists, then regenerate this file.
Based on learnings: follow the /service DI wiring conventions; and as per coding guidelines, **/*.gen.go files should be regenerated rather than hand-edited.
Also applies to: 4607-4611, 5244-5267, 6083-6085
🤖 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 4412 - 4414, The generated handler
QueryFeatureCost is being exposed even when featureCostHandler can be nil; fix
by updating the handwritten routing/DI so the route is only registered when
featureCostHandler is non-nil (or change the server method that calls
featureCostHandler to return a controlled 4xx/5xx error when nil), mirror the
existing /service DI wiring used for other optional handlers (the
featureCostHandler wiring in server.go), then regenerate the api/*.gen.go files
so QueryFeatureCost (and the other listed generated endpoints) are in sync with
the DI change.
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 by CodeRabbit