🔧 fix: expose guard() schemas to plugins via getFlattenedRoutes() #1542
Conversation
plugins - Add protected getFlattenedRoutes() method that merges standaloneValidator schemas into direct hook properties - This allows plugins to access schemas defined in guard() blocks - Implement mergeStandaloneValidators(), mergeSchemaProperty(), and mergeResponseSchema() helper methods - Handle complex schema merging including status code response objects - Add comprehensive test suite covering various guard schema scenarios
|
Caution Review failedFailed to post review comments WalkthroughAdds internals to merge guard-provided standalone validator schemas into per-route hook schemas, introduces protected route-flattening, adjusts error handling to honor error.toResponse(), and adds tests for flattened routes and toResponse-driven error flows. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Elysia App
participant Flattener as getFlattenedRoutes()
participant Guard as Guard Layer(s)
participant Merger as mergeStandaloneValidators()
participant Output as FlattenedRoute
Note right of App: used for metadata export / tests
App->>Flattener: collect routes (with guards & hooks)
Flattener->>Guard: read standaloneValidator(s)
Flattener->>Merger: provide route.hooks + standaloneValidator
Merger->>Merger: merge properties (body, headers, query, params, cookie)
Merger->>Merger: mergeResponseSchema for response status maps
Merger-->>Flattener: merged hook schemas
Flattener-->>Output: return flattened route with combined hooks
sequenceDiagram
autonumber
participant Request as Incoming Request
participant Router as Router/Handler
participant Error as Thrown/Returned Error
participant toResp as error.toResponse()
participant Map as mapResponse()
participant After as afterResponse()
participant Final as Final Response
Request->>Router: handle -> throws/returns Error
Router->>Error: detect Error
alt error has toResponse() && not validation/transform errors
Router->>toResp: call toResponse()
toResp->>Map: produce mapped response
Map->>After: run afterResponse()
After->>Final: return mapped response (early)
else
Router->>Router: existing onError / fallback JSON error payload
Router->>Final: return fallback response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(2 hunks)test/core/flattened-routes.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/core/flattened-routes.test.ts (1)
src/index.ts (1)
t(8275-8275)
src/index.ts (2)
src/types.ts (2)
InternalRoute(1878-1886)AnyLocalHook(1680-1680)src/schema.ts (1)
mergeObjectSchemas(1403-1474)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(2 hunks)test/core/flattened-routes.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
🔇 Additional comments (2)
test/core/flattened-routes.test.ts (2)
1-5: LGTM! Clean test structure.The imports are minimal and appropriate, and the test suite follows Bun's testing conventions.
39-49: Reference equality check assumes optimization.The test correctly verifies that routes without guards are returned unchanged. Using
.toBe()(reference equality) at line 48 assumes the implementation optimizes by returning the same object reference rather than cloning. This is a reasonable assumption, but note that if the implementation changes to always return new route objects, this assertion would need updating.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/core/flattened-routes.test.ts (2)
108-164: Consider verifying response[200] for completeness.The standaloneValidator preservation test is thorough and addresses the past review feedback well. One minor enhancement: since the route provides a plain response schema (lines 124-127), and based on the merging behavior tested at lines 230-265, this should be merged as
response[200].Consider adding an assertion to verify this:
// Verify schemas were also flattened into direct properties expect(protectedRoute?.hooks.headers).toBeDefined() expect(protectedRoute?.hooks.headers.type).toBe('object') expect(protectedRoute?.hooks.body).toBeDefined() expect(protectedRoute?.hooks.body.type).toBe('object') expect(protectedRoute?.hooks.response).toBeDefined() + expect(protectedRoute?.hooks.response[200]).toBeDefined() + expect(protectedRoute?.hooks.response[200].type).toBe('object') + expect(protectedRoute?.hooks.response[200].properties).toHaveProperty('success') expect(protectedRoute?.hooks.response[401]).toBeDefined() expect(protectedRoute?.hooks.response[500]).toBeDefined()
79-106: Optional: Consider testing schema type conflicts explicitly.This test validates that guard headers and route body coexist (no conflict between different schema types). While the test at lines 230-265 shows route response precedence over guard response, you might want to add a test that explicitly covers the case where both guard and route define the same schema type (e.g., both define
body).This would clarify the expected precedence or merging behavior for non-response schema types:
it('route-level schema takes precedence over guard for same type', () => { const app = new Elysia().guard( { body: t.Object({ guardField: t.String() }) }, (app) => app.post('/override', ({ body }) => body, { body: t.Object({ routeField: t.String() }) }) ) // @ts-expect-error - accessing protected method for testing const flatRoutes = app.getFlattenedRoutes() const route = flatRoutes.find((r) => r.path === '/override') expect(route?.hooks.body).toBeDefined() // Verify which schema wins or how they're merged expect(route?.hooks.body.type).toBe('object') // Add assertions based on expected behavior })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/core/flattened-routes.test.ts(1 hunks)
🔇 Additional comments (1)
test/core/flattened-routes.test.ts (1)
1-442: Excellent test coverage addressing past feedback!The test suite is comprehensive and well-structured. It covers all the key scenarios including:
- Basic guard schema merging across all schema types (body, headers, query, params, cookie, response)
- Nested guards and guard+route schema combinations
- Edge cases: unions, intersects, t.Any, and schemas without type properties
- Status-code map merging with correct precedence rules
- String schema aliases (TRef)
Most importantly, the standaloneValidator preservation test (lines 108-164) directly addresses the previous review feedback. Nice work!
MarcelOlsen
force-pushed
the
fix/expose-guard-schemas-clean
branch
from
569074e to
78e2115
Compare
|
Nice work! But if I understand this correctly, this would only work with Elysia.t? |
|
Thanks for the feedback! I see your point about keeping this at the OpenAPI level. My thinking was that other plugins (Swagger, Scalar, GraphQL schema generators) might also need access to guard schemas, so putting the merging logic in core would let them all reuse it instead of duplicating. But you know the ecosystem better than I do - if OpenAPI is the only plugin that needs this, or if each plugin should handle its own merging, I'm happy to move the logic there. Would keep core simpler. Also wondering about the "only work with Elysia.t" question - the implementation uses TypeBox schema merging, but the flattening itself works with any validator. Should this support other schema validators, or is TypeBox-only fine? Let me know how you'd prefer to handle this and I'll refactor accordingly. |
2 participants
This resolves #1490
(This has been previously worked on in #1533, but I've messed up my local git instance)
Problem
Guard schemas defined via
guard()are not accessible to plugins like OpenAPI. Routes appear in documentation but lack request body and parameter schemas.Current Behavior
requestBodyschemaRoot Cause
Guard schemas are stored in
route.hooks.standaloneValidator[]arrays. Plugins only read direct schema properties likeroute.hooks.body.Solution
Add
getFlattenedRoutes()method that mergesstandaloneValidatorarrays into direct hook properties.Changes
New Methods:
getFlattenedRoutes(): Returns routes with merged schemas (protected)mergeStandaloneValidators(): Flattens standaloneValidator arraysmergeSchemaProperty(): Handles TypeBox schema mergingmergeResponseSchema(): Handles response status codesTests:
test/core/flattened-routes.test.ts: 7 tests covering all schema types and edge casesBackward Compatibility
getGlobalRoutes()unchanged - existing behavior preservedapp.getFlattenedRoutes?.() ?? app.getGlobalRoutes()for compatibilityUsage
Plugins can now access guard schemas:
Demo
Reproduction case and full documentation: https://github.com/MarcelOlsen/elysia-guard-openapi-fix-demo
Related PR
OpenAPI plugin PR: elysiajs/elysia-openapi
Testing
bun test test/core/flattened-routes.test.tsAll 7 tests pass.
Summary by CodeRabbit
Refactor
Behavior
Tests