🔧 fix: expose guard() schemas to plugins via getFlattenedRoutes() #1533
Conversation
Fixes guard() schemas not being accessible to plugins like OpenAPI. Changes: - Add getFlattenedRoutes() method that merges standaloneValidator arrays into direct hook properties - Add mergeStandaloneValidators() helper for schema merging - Add mergeSchemaProperty() for TypeBox object merging - Add mergeResponseSchema() for response status code handling - Add comprehensive test suite (7 tests, all passing) This allows plugins to access guard() schemas that were previously stored only in standaloneValidator arrays and invisible to external consumers. Backward compatible - getGlobalRoutes() behavior unchanged.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds internal utilities to flatten routes by merging guard/group standalone validator schemas into route hook properties and response maps; includes tests validating merge behaviors across body, headers, query, params, cookie, and status-code response shapes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Routes as RouteDefs
participant Merger as getFlattenedRoutes
participant Schema as SchemaFragments
Note over Routes,Merger: Collect ancestor standaloneValidators for each route
Routes->>Merger: route + [standaloneValidators]
Merger->>Schema: extract parts (body, headers, query, params, cookie, response)
alt simple property (body/headers/query/params/cookie)
Merger->>Merger: mergeSchemaProperty(prop) %%[`#8FB9A8`]%%
else response merging
Merger->>Merger: mergeResponseSchema(response) %%[`#F2D388`]%%
end
Merger-->>Routes: flattened route with merged hook properties and unified response map
Routes-->>App: application uses flattened routes (runtime and docs/spec generation)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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
🧹 Nitpick comments (3)
test/core/flattened-routes.test.ts (2)
6-37: Consider verifying standaloneValidator preservation.According to the PR description, "The standaloneValidator arrays are preserved; merged properties are exposed alongside them." This test should verify that
signUpRoute?.hooks.standaloneValidatorstill exists after flattening to ensure the original data isn't lost.// Verify it's an object schema with the expected properties expect(signUpRoute?.hooks.body.type).toBe('object') expect(signUpRoute?.hooks.body.properties).toHaveProperty('username') expect(signUpRoute?.hooks.body.properties).toHaveProperty('password') + + // Verify standaloneValidator is preserved + expect(signUpRoute?.hooks.standaloneValidator).toBeDefined() + expect(signUpRoute?.hooks.standaloneValidator?.length).toBeGreaterThan(0)
5-171: Add tests for schema merging edge cases.The current test suite covers happy paths well but is missing tests for important edge cases in the merging logic:
- Schema merging conflicts: When both guard and direct route define the same schema (e.g., both have
body), test that they merge correctly- String schema references: Test behavior when schemas are string references to models
- Non-object schemas: Test that non-object schemas create Intersect types correctly
- Complex response schemas: Test merging of response schemas with multiple status codes
These edge cases are particularly important since
mergeSchemaPropertyandmergeResponseSchemahave specific logic to handle them.Example test for schema merging conflict:
it('merges body schemas from guard and direct route', () => { const app = new Elysia().guard( { body: t.Object({ token: t.String() }) }, (app) => app.post('/data', ({ body }) => body, { body: t.Object({ data: t.String() }) }) ) // @ts-expect-error - accessing protected method for testing const flatRoutes = app.getFlattenedRoutes() const dataRoute = flatRoutes.find((r) => r.path === '/data') // Should have both token and data properties expect(dataRoute?.hooks.body.properties).toHaveProperty('token') expect(dataRoute?.hooks.body.properties).toHaveProperty('data') })src/index.ts (1)
417-420: Consider documenting string reference override behavior.When either schema is a string reference, the function returns
incoming, potentially discarding a validexistingschema. For example:
existing = t.Object({ id: t.String() })incoming = 'UserModel'- Result:
'UserModel'(loses the inline object schema)If this override behavior is intentional, consider adding a JSDoc comment explaining the rationale. If not, you might want to log a warning or handle this case differently.
// If either is a string reference, we can't merge - use incoming if (typeof existing === 'string' || typeof incoming === 'string') { + // String references cannot be merged with schemas, so incoming takes precedence return incoming }
📜 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(8256-8256)
src/index.ts (2)
src/types.ts (2)
InternalRoute(1878-1886)AnyLocalHook(1680-1680)src/schema.ts (1)
mergeObjectSchemas(1403-1474)
🔇 Additional comments (3)
src/index.ts (3)
337-354: LGTM!The
getFlattenedRoutes()implementation is clean and efficient. The early return optimization for routes without standaloneValidator is good, and the shallow copy with hook replacement preserves the original route data properly.
359-405: LGTM!The
mergeStandaloneValidators()method correctly iterates through the standaloneValidator array and delegates to appropriate merge functions for each schema type. The conditional checks prevent unnecessary merging operations.
52-53: LGTM!The
mergeObjectSchemasimport is correctly placed and used inmergeSchemaProperty()for schema composition.
… spec Fixes guard() schemas not appearing in generated OpenAPI documentation. Changes: - Replace app.getGlobalRoutes() with app.getFlattenedRoutes?.() ?? app.getGlobalRoutes() - Maintains backward compatibility with older Elysia versions This allows the OpenAPI plugin to access guard() schemas that are now exposed via Elysia's getFlattenedRoutes() method. Requires: elysiajs/elysia#1533
Addresses feedback from coderabbitai on PR elysiajs#1533. **Problem:** When merging a status-code map with a plain schema, the original implementation would discard the entire status-code map, losing schemas for error responses (404, 500, etc.). **Solution:** Updated mergeResponseSchema logic to preserve status codes while treating plain schemas as 200: - Both status maps → merge with incoming winning conflicts - Status map + plain schema → treat plain as 200, preserve other codes - Both plain schemas → use incoming **Testing:** Added test case verifying 404/500 schemas are preserved when route defines a plain 200 response schema.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
MarcelOlsen
force-pushed
the
fix/expose-guard-schemas-to-plugins
branch
from
32f640f to
8cdde76
Compare
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)
- test/core/flattened-routes.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (3)
src/types.ts (2)
InternalRoute(1878-1886)AnyLocalHook(1680-1680)src/schema.ts (1)
mergeObjectSchemas(1403-1474)src/type-system/index.ts (1)
t(673-673)
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 (1)
src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (2)
src/types.ts (2)
InternalRoute(1878-1886)AnyLocalHook(1680-1680)src/schema.ts (1)
mergeObjectSchemas(1403-1474)
🪛 GitHub Actions: Build and Test
src/index.ts
[error] 491-491: src/index.ts(491,10): error TS7053: Element implicitly has an 'any' type because expression of type '200' can't be used to index type 'TSchema | { [status: number]: TSchema; } | { [status: number]: string | TSchema; }'. Property '200' does not exist on type 'TSchema | { [status: number]: TSchema; } | { [status: number]: string | TSchema; }'.
🔇 Additional comments (4)
src/index.ts (4)
52-53: LGTM: Import added correctly.The
mergeObjectSchemasimport is properly added and used in themergeSchemaPropertymethod (line 423).
337-354: LGTM: Clean implementation of getFlattenedRoutes().The method correctly:
- Maintains backward compatibility by preserving routes without standaloneValidator
- Returns a new route object with merged hooks rather than mutating the original
- Matches the return type of
getGlobalRoutes()
359-405: LGTM: Solid implementation of validator merging.The method correctly:
- Creates a shallow copy to avoid mutation
- Optimizes with early return for empty validators
- Delegates to specialized merge methods for each schema type
- Preserves the original standaloneValidator array
410-437: LGTM: Schema property merging is well-designed.The method handles all cases appropriately:
- Early returns for undefined values
- Correctly rejects merging string references
- Uses
mergeObjectSchemasutility for object schemas- Properly wraps non-object schemas in
Intersect
|
@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 (1)
src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (3)
src/types.ts (2)
InternalRoute(1878-1886)AnyLocalHook(1680-1680)src/schema.ts (1)
mergeObjectSchemas(1403-1474)src/type-system/index.ts (1)
t(673-673)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
488-495: Guard plain schema unconditionally overwrites route's status-200 response.When
existingis a plain schema (route-specific) andincomingis a status-code map (guard), Line 493 unconditionally sets200: existing, discardingincoming[200]if it already exists. This breaks route-defined 200-status responses. The past review flagged this exact issue and marked it as "✅ Addressed in commit 4a68446," but the fix is not present in the current code.Apply this diff to preserve route-defined status-code entries:
if (existingIsSchema && !incomingIsSchema) { - return { - ...incoming, - 200: existing - } + return (incoming as Record<number, TSchema | string>)[200] === undefined + ? { + ...incoming, + 200: existing + } + : incoming }
🧹 Nitpick comments (1)
src/index.ts (1)
469-470: Fragile type detection for TSchema vs status-code map.Using
'type' in existingto distinguish between a plain TSchema and a status-code object is unreliable:
- A status-code map could theoretically have a numeric key named
type(unlikely but not impossible)- Not all TSchema instances guarantee a
typeproperty (refs, unions, etc.)Consider using TypeBox's
Kindsymbol for more robust detection:-const existingIsSchema = 'type' in existing -const incomingIsSchema = 'type' in incoming +const existingIsSchema = Kind in existing +const incomingIsSchema = Kind in incoming
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(2 hunks)
🔇 Additional comments (5)
src/index.ts (5)
52-53: Verify imports are used correctly.The new imports
queryCoercionsandmergeObjectSchemasare added but not directly visible in the changed code segments. Confirm these are indeed used in the new merge helper methods.
337-354: LGTM: Clean implementation of route flattening.The
getFlattenedRoutes()method correctly maps routes and merges standalone validators only when present, preserving the original route structure when no merging is needed.
497-501: LGTM: Status-code map merge gives route precedence.When both schemas are status-code maps, spreading
...incomingfirst then...existingcorrectly gives the accumulated route-specific state precedence over guard defaults. This matches the expected semantics where route definitions override guards.
410-437: Verify mergeObjectSchemas behavior for edge cases.The logic handles string references, object merging, and Intersect creation. Confirm that
mergeObjectSchemasgracefully handles:
- Empty object schemas
- Circular references
- Incompatible object shapes
If
mergeObjectSchemasreturns{ schema: undefined, notObjects: [] }, Line 436 returnsundefined—verify this is the intended behavior when both inputs are objects but cannot be merged.
359-405: Validate merge iteration order matches guard application order.
mergeStandaloneValidatorsiterates thestandaloneValidatorarray in order, merging each entry into the accumulated state. Confirm that the array order matches the intended precedence:
- Route-specific validators should come first (higher precedence)
- Guard/group validators should come later (fill gaps only)
Looking at the
guard()implementation (lines 4780-4792), guard schemas are appended to the array, suggesting they come after route validators. Verify this ordering is correct and that the merge helpers give appropriate precedence to earlier entries (route) over later ones (guards).
src/index.ts
Outdated
| // If both are plain schemas, incoming overrides | ||
| if (existingIsSchema && incomingIsSchema) { | ||
| return incoming | ||
| } |
There was a problem hiding this comment.
Route-specific plain response schemas are overwritten by guards.
When both schemas are plain (not status-code maps), incoming overrides existing. Since existing is the accumulated state starting with the route's direct schema and incoming is the guard schema being merged, this gives guards precedence over route-specific definitions. For plugins consuming flattened routes, route-specific schemas should take precedence.
Apply this diff:
// If both are plain schemas, incoming overrides
if (existingIsSchema && incomingIsSchema) {
- return incoming
+ return existing
}🤖 Prompt for AI Agents
In src/index.ts around lines 472 to 475, the current logic returns incoming when
both existingIsSchema and incomingIsSchema are true, causing guard schemas to
overwrite route-specific plain schemas; change this so that when both are plain
schemas the existing schema (the route's accumulated/direct schema) is preserved
by returning existing instead of incoming, ensuring route-specific definitions
take precedence over guard-provided schemas.
|
closing, because i've reopened form a fresh branch ;) |
1 participant
This resolves #1490
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 (to be created)
Testing
bun test test/core/flattened-routes.test.tsAll 7 tests pass.
Summary by CodeRabbit
Chores
Tests