Extract benchmark feature guard as middleware#2094
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors benchmark feature-flag access validation into a guard object retrieved via a factory function, aiming to centralize/standardize benchmark access checks for future extensibility (OPS-3873).
Changes:
- Replace direct
assertBenchmarkFeatureEnabled(...)calls withgetBenchmarkFeatureGuard().assertBenchmarkFeatureEnabled(...)in the benchmark controller. - Convert the benchmark feature guard from a standalone function to a
BenchmarkFeatureGuardobject with an updated method signature includingorganizationId. - Introduce
benchmark-feature-guard-factory.tsto provide the guard instance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/server/api/src/app/benchmark/benchmark.controller.ts | Updates all benchmark endpoints to use the guard factory and pass organizationId into the guard call. |
| packages/server/api/src/app/benchmark/benchmark-feature-guard.ts | Refactors the guard into a typed object and updates the guard API to include organizationId. |
| packages/server/api/src/app/benchmark/benchmark-feature-guard-factory.ts | Adds a factory function that returns the guard implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/server/api/src/app/benchmark/benchmark-feature-guard.ts
Outdated
Show resolved
Hide resolved
Replace per-handler assertBenchmarkFeatureEnabled calls with a single
app.addHook('preHandler', ...) registered at the plugin scope, following
the established hook pattern used across the codebase.
Made-with: Cursor
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
packages/server/api/test/unit/benchmark/benchmark-feature-guard.test.ts
Dismissed
Show dismissed
Hide dismissed
| logger.info( | ||
| 'Benchmark access denied: FINOPS_BENCHMARK_ENABLED flag is not enabled', | ||
| { provider, projectId }, | ||
| { projectId: request.principal.projectId }, |
There was a problem hiding this comment.
I see no good reason to log the provider, as this guard checks only the value of system env variable
There was a problem hiding this comment.
As we discussed, we don't need this factory.
cezudas
changed the title
|
Centralizes benchmark access validation for better extensibility.
Part of OPS-3873.