fix: validate handler constructors#4741
Conversation
📝 WalkthroughWalkthroughHandler constructors now validate required dependencies and return errors. Router and server initialization propagate failures, subscription handlers add typed-nil validation, and API v3 credits use a dedicated disabled handler when disabled. ChangesHandler construction and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if c.ChargeService == nil { | ||
| return errors.New("charge service is required") | ||
| } |
There was a problem hiding this comment.
The main server passes BillingRegistry.ChargesServiceOrNil(), and the v3 server already treats a nil charge service as a valid configuration by omitting its handler. This new unconditional check makes that same deployment fail in NewRouter before any routes are registered.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/server/router/router.go
Line: 178-180
Comment:
**Optional Charge Service Blocks Startup**
The main server passes `BillingRegistry.ChargesServiceOrNil()`, and the v3 server already treats a nil charge service as a valid configuration by omitting its handler. This new unconditional check makes that same deployment fail in `NewRouter` before any routes are registered.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/server/router/router.go (1)
178-253: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the typed-nil-safe check for interface deps.
== nilonly catches nil interfaces, so a nil concrete pointer wrapped in an interface can still slip through and leave the handler broken. Reuse theisNilpattern already used in the subscription handlers for the interface-typed fields in:
openmeter/server/router/router.go#L148-L295openmeter/app/custominvoicing/httpdriver/handler.goopenmeter/ingest/httpdriver/handler.goopenmeter/meter/httphandler/handler.goopenmeter/meterevent/httphandler/handler.goopenmeter/notification/httpdriver/handler.goopenmeter/portal/httphandler/handler.goopenmeter/productcatalog/addon/httpdriver/driver.goopenmeter/productcatalog/driver/feature.goopenmeter/productcatalog/plan/httpdriver/driver.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/server/router/router.go` around lines 178 - 253, Replace plain == nil dependency checks with the existing typed-nil-safe isNil pattern for interface-typed fields in openmeter/server/router/router.go lines 178-253, openmeter/app/custominvoicing/httpdriver/handler.go lines 47-57, openmeter/ingest/httpdriver/handler.go lines 42-52, openmeter/meter/httphandler/handler.go lines 63-82, openmeter/meterevent/httphandler/handler.go lines 45-55, openmeter/notification/httpdriver/handler.go lines 69-82, openmeter/portal/httphandler/handler.go lines 49-62, openmeter/productcatalog/addon/httpdriver/driver.go lines 50-60, openmeter/productcatalog/driver/feature.go lines 48-64, and openmeter/productcatalog/plan/httpdriver/driver.go lines 51-61; preserve each existing validation error and control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/productcatalog/subscription/http/handler.go`:
- Line 71: Update the Validate() error return containing errors.Join(errs...) to
wrap the joined validation failures with
models.NewNillableGenericValidationError. Keep the individual validation errors
wrapped with their existing field context before joining them.
In `@openmeter/server/router/router.go`:
- Around line 214-216: Gate the GrantRepo validation and grant handler
initialization on config.Credits.Enabled. When credits are disabled, leave grant
wiring disabled, including the paths around the referenced grant setup, while
preserving the existing required-repository validation and initialization when
credits are enabled.
- Around line 178-181: Update the router validation method containing the
ChargeService check to accumulate all missing-dependency validation errors in an
errs collection instead of returning immediately. Apply the same pattern to the
checks around the referenced validation blocks, wrap each failure with its field
context, and return
models.NewNillableGenericValidationError(errors.Join(errs...)) after all checks
complete; preserve nil when no validation errors exist.
- Around line 178-181: Update NewRouter’s ChargeService validation to detect
both a nil interface and an interface containing a typed-nil concrete pointer,
rejecting either with the existing “charge service is required” error before
handler setup. Apply the same typed-nil-safe validation pattern to any
comparable interface-valued dependencies validated in NewRouter.
---
Outside diff comments:
In `@openmeter/server/router/router.go`:
- Around line 178-253: Replace plain == nil dependency checks with the existing
typed-nil-safe isNil pattern for interface-typed fields in
openmeter/server/router/router.go lines 178-253,
openmeter/app/custominvoicing/httpdriver/handler.go lines 47-57,
openmeter/ingest/httpdriver/handler.go lines 42-52,
openmeter/meter/httphandler/handler.go lines 63-82,
openmeter/meterevent/httphandler/handler.go lines 45-55,
openmeter/notification/httpdriver/handler.go lines 69-82,
openmeter/portal/httphandler/handler.go lines 49-62,
openmeter/productcatalog/addon/httpdriver/driver.go lines 50-60,
openmeter/productcatalog/driver/feature.go lines 48-64, and
openmeter/productcatalog/plan/httpdriver/driver.go lines 51-61; preserve each
existing validation error and control flow.
🪄 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: b97fc03f-7880-4058-bca4-31d886cd6959
📒 Files selected for processing (29)
api/v3/handlers/currencies/handler.goapi/v3/server/server.goopenmeter/app/custominvoicing/httpdriver/handler.goopenmeter/app/httpdriver/handler.goopenmeter/app/stripe/httpdriver/handler.goopenmeter/billing/httpdriver/handler.goopenmeter/credit/driver/grant.goopenmeter/customer/httpdriver/handler.goopenmeter/debug/httpdriver/metrics.goopenmeter/entitlement/driver/entitlement.goopenmeter/entitlement/driver/metered.goopenmeter/entitlement/driver/v2/handler.goopenmeter/ingest/httpdriver/handler.goopenmeter/ingest/httpdriver/ingest_test.goopenmeter/meter/httphandler/handler.goopenmeter/meterevent/httphandler/handler.goopenmeter/notification/httpdriver/handler.goopenmeter/portal/httphandler/handler.goopenmeter/productcatalog/addon/httpdriver/driver.goopenmeter/productcatalog/driver/feature.goopenmeter/productcatalog/plan/httpdriver/driver.goopenmeter/productcatalog/planaddon/httpdriver/driver.goopenmeter/productcatalog/subscription/http/handler.goopenmeter/productcatalog/subscription/http/handler_test.goopenmeter/progressmanager/httpdriver/handler.goopenmeter/server/router/router.goopenmeter/subject/httphandler/handler.goopenmeter/subscription/addon/http/handler.goopenmeter/subscription/addon/http/handler_test.go
| if namespaceDecoder == nil { | ||
| errs = append(errs, errors.New("namespace decoder is required")) | ||
| } | ||
| if appService == nil { | ||
| errs = append(errs, errors.New("app service is required")) | ||
| } | ||
| if appStripeService == nil { | ||
| errs = append(errs, errors.New("app stripe service is required")) | ||
| } | ||
| if billingService == nil { | ||
| errs = append(errs, errors.New("billing service is required")) | ||
| } | ||
| if customerService == nil { | ||
| errs = append(errs, errors.New("customer service is required")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make constructor validation consistently reject typed-nil interfaces.
Direct interface comparisons with nil still permit nil-backed handlers, while the subscription configuration already handles this case correctly.
openmeter/app/httpdriver/handler.go#L77-L90: use typed-nil-aware checks for each interface dependency.openmeter/credit/driver/grant.go#L51-L62: reject typed-nil connector, repository, decoder, and service values.openmeter/entitlement/driver/entitlement.go#L57-L67: reject typed-nil entitlement dependencies.openmeter/entitlement/driver/v2/handler.go#L45-L55: reject typed-nil v2 entitlement dependencies.openmeter/productcatalog/planaddon/httpdriver/driver.go#L50-L55: reject typed-nil decoder and service values.
📍 Affects 5 files
openmeter/app/httpdriver/handler.go#L77-L90(this comment)openmeter/credit/driver/grant.go#L51-L62openmeter/entitlement/driver/entitlement.go#L57-L67openmeter/entitlement/driver/v2/handler.go#L45-L55openmeter/productcatalog/planaddon/httpdriver/driver.go#L50-L55
| errs = append(errs, errors.New("logger is required")) | ||
| } | ||
|
|
||
| return errors.Join(errs...) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return the repository’s generic validation error type.
This currently returns the bare joined error. Please wrap it with models.NewNillableGenericValidationError(errors.Join(errs...)) and preserve field context in the individual errors.
As per coding guidelines, “For Validate() error methods, collect all validation failures and return models.NewNillableGenericValidationError(errors.Join(errs...)), preserving field context with wrapped errors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/productcatalog/subscription/http/handler.go` at line 71, Update the
Validate() error return containing errors.Join(errs...) to wrap the joined
validation failures with models.NewNillableGenericValidationError. Keep the
individual validation errors wrapped with their existing field context before
joining them.
Source: Coding guidelines
| if c.ChargeService == nil { | ||
| return errors.New("charge service is required") | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Aggregate all router validation failures.
The added checks return immediately, so startup reports missing dependencies one at a time. Collect wrapped field errors and return models.NewNillableGenericValidationError(errors.Join(errs...)).
As per coding guidelines, “For Validate() error methods, collect all validation failures and return models.NewNillableGenericValidationError(errors.Join(errs...)), preserving field context with wrapped errors.”
Also applies to: 214-253, 282-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/server/router/router.go` around lines 178 - 181, Update the router
validation method containing the ChargeService check to accumulate all
missing-dependency validation errors in an errs collection instead of returning
immediately. Apply the same pattern to the checks around the referenced
validation blocks, wrap each failure with its field context, and return
models.NewNillableGenericValidationError(errors.Join(errs...)) after all checks
complete; preserve nil when no validation errors exist.
Source: Coding guidelines
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the router validation code.
file="openmeter/server/router/router.go"
wc -l "$file"
sed -n '1,320p' "$file"Repository: openmeterio/openmeter
Length of output: 12473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="openmeter/server/router/router.go"
# Show the constructor and any validation calls around it.
rg -n "func NewRouter|Validate\(\)|NewRouter\(" "$file" -n -A80 -B40
# Map the interface shapes used in the config so we can see whether they can be backed by nil pointers.
rg -n "type (Service|Handler|Connector|Facade|Decoder|Repo|Logger|PlanSubscriptionService|MeteredEntitlementHandler|ChargeService)" \
openmeter/server/router openmeter/billing openmeter/openmeter -g '*.go'Repository: openmeterio/openmeter
Length of output: 12153
Make router validation typed-nil-safe. Interface-valued deps like ChargeService can still hold a nil concrete pointer, so NewRouter may accept broken wiring and fail later during handler setup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/server/router/router.go` around lines 178 - 181, Update NewRouter’s
ChargeService validation to detect both a nil interface and an interface
containing a typed-nil concrete pointer, rejecting either with the existing
“charge service is required” error before handler setup. Apply the same
typed-nil-safe validation pattern to any comparable interface-valued
dependencies validated in NewRouter.
| if c.GrantRepo == nil { | ||
| return errors.New("grant repo is required") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep grant wiring disabled when credits are disabled.
GrantRepo is now required and the grant handler is always initialized, including when credits are disabled. Gate both paths on config.Credits.Enabled.
As per coding guidelines, “When credits.enabled is false, keep credit handlers, ledger hooks, and namespace/default-account provisioning disabled.”
Also applies to: 434-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/server/router/router.go` around lines 214 - 216, Gate the GrantRepo
validation and grant handler initialization on config.Credits.Enabled. When
credits are disabled, leave grant wiring disabled, including the paths around
the referenced grant setup, while preserving the existing required-repository
validation and initialization when credits are enabled.
Source: Coding guidelines
1 participant
Summary
This makes HTTP handler initialization fail fast when required domain services are not wired. These handlers represent product, billing, entitlement, subscription, customer, notification, and related API capabilities; if one of their required services is missing, the server should reject startup instead of accepting traffic and failing later on a request path with an internal error.
The change moves that guarantee into the handler constructors themselves, so the invariant holds even when handlers are built outside the main router. Router initialization now propagates those constructor errors with handler context.
It also removes an unused billing handler constructor dependency on the Stripe app service, keeping the required wiring surface aligned with what the billing HTTP handler actually needs.
Release note
Bug fix: validate handler initialization dependencies at startup to avoid request-time failures from incomplete service wiring.
Greptile Summary
This PR makes HTTP handler setup fail fast when required services are missing. The main changes are:
Confidence Score: 5/5
The latest changes do not introduce a separate blocking issue.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: return precondition failed when cre..." | Re-trigger Greptile
Context used (3)
Summary by CodeRabbit
Bug Fixes
Tests