feat: implement NestJS middleware pipeline#20
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/route-registry-middleware.spec.ts`:
- Around line 179-199: The test registers an UpperCasePipe but never supplies a
request body, so the handler awaits req.body and the pipe never receives
"hello"; fix by feeding the mocked uWS request with the raw body before invoking
the registered handler: simulate the request body delivery on mockUwsReq (e.g.,
resolve its body promise or call the mock onData/onEnd/data callback with the
string "hello") so that when registry.register('POST','/test', handler, { pipes:
[UpperCasePipe] }) is executed and you call
registeredRoutes.get('POST:/test')?.handler!(mockUwsRes, mockUwsReq), the pipe
will receive "hello" and the handler will assert "HELLO". Ensure you use the
same mock hooks your body-parsing code relies on (mockUwsReq.body,
mockUwsReq.onData, mockUwsReq.onEnd, or mockUwsReq.forEach) so the
UpperCasePipe.transform is exercised.
In `@src/platform/route-registry.ts`:
- Around line 506-512: The default error path always sends a 500, losing
original HTTP status when an HttpException (e.g., BadRequestException,
UnauthorizedException) was thrown; update the error branch that currently calls
res.status(500).send(...) to preserve the exception status and response body:
detect if the thrown error is an instance of HttpException (or has
getStatus/getResponse methods), and if so call
res.status(err.getStatus()).send(err.getResponse() || { statusCode:
err.getStatus(), message: err.message }); otherwise keep the generic 500
payload; reference symbols: HttpException, BadRequestException,
UnauthorizedException, res.headersSent, res.status(...).send(...).
- Around line 426-430: The code currently calls pipe.transform with type/body
metadata hardcoded to undefined, preventing Nest pipes from validating; update
the RouteMetadata interface to include an optional bodyMetadata?:
ArgumentMetadata, change executePipes(route: RouteMetadata, ...) (and its
callers) to accept and forward that ArgumentMetadata, and replace the hardcoded
object passed to pipe.transform in executePipes with the provided bodyMetadata
(e.g., { type: 'body', ...route.bodyMetadata }) so pipes like ValidationPipe
receive the real metatype and data; ensure callers that register routes can
optionally supply the new bodyMetadata field.
- Around line 393-397: Guards and pipes may return Observables, but the current
await on guard.canActivate and pipe.transform will treat Observable objects as
truthy; update the execution in the RouteRegistry where guard.canActivate
(referenced as guard.canActivate(context as ExecutionContext)) and
pipe.transform (referenced as pipe.transform(value, metadata, { type:
'transform' })) are called to detect Observable results using isObservable(...)
and, if true, resolve them with lastValueFrom(...) before using the result;
ensure the resolved guard value is coerced to boolean before gating execution
and the resolved pipe value replaces the request body/value.
- Around line 388-391: The loop currently instantiates guards/pipes/filters
directly (e.g., new GuardType(), new PipeType(), new FilterType()), which
bypasses DI; change those instantiations to resolve instances from the Nest
moduleRef by calling this.options.moduleRef.get(GuardType) (and similarly for
PipeType and FilterType) so constructor-injected dependencies are provided;
ensure you follow the same resolution pattern used in GuardExecutor,
PipeExecutor, and ExceptionFilterExecutor and handle null/undefined returns the
same way the other executors do.
In `@src/platform/test-helpers.ts`:
- Around line 1-4: The test helper module test-helpers.ts (imports HttpRequest,
HttpResponse and uWS) is being compiled into production artifacts; either rename
the file to match test patterns (e.g., test-helpers.test.ts) or move it into an
explicitly excluded test directory (e.g., a testing folder), then update
tsconfig.build.json to add that directory (e.g., "src/testing") to the "exclude"
array so the compiler skips it; also verify package.json "files" doesn't include
the helper so it won't be packaged for consumers.
In `@src/platform/uws-platform.adapter.ts`:
- Around line 577-612: The current numeric-to-string mapping in methodMap
silently defaults unknown numeric RequestMethod values to 'GET', which can
mis-register WebDAV or unsupported verbs; update the logic around requestMethod
and methodMap so that when requestMethod is a number you validate it against an
explicit supportedMethods set (e.g.,
['GET','POST','PUT','DELETE','PATCH','OPTIONS','HEAD','ALL']) before calling
routeRegistry.register, and throw an explicit error for any numeric enum value
not in that set (include requestMethod value in the error). Also validate string
requestMethod by uppercasing it (as done) and rejecting any resulting verb not
present in the same supportedMethods set, throwing a clear error instead of
silently mapping to GET; keep references to methodMap, requestMethod, method,
and routeRegistry.register to locate the code to change.
In `@src/platform/uws-request.ts`:
- Around line 76-77: The class currently uses the optional property
transformedBody?: unknown to both store a transformed value and indicate
presence, which treats a legitimate transformed undefined as "not set" and
triggers fallback parsing; add a separate boolean flag (e.g., transformedBodySet
or hasTransformedBody) initialized false, update the setter (where
transformedBody is assigned) to also set that flag to true even when the
assigned value is undefined, and change all reads (the body getter and any
checks) to test the flag instead of checking transformedBody !== undefined;
apply the same change for the other occurrences referenced by the reviewer
(around the methods/properties named transformedBody, any setTransformedBody or
body getter logic at the other locations).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8579b853-dac1-410c-ad4a-6f6b6859884b
📒 Files selected for processing (11)
src/interfaces/http-options.interface.tssrc/platform/http-context.spec.tssrc/platform/http-context.tssrc/platform/index.tssrc/platform/route-registry-middleware.spec.tssrc/platform/route-registry.spec.tssrc/platform/route-registry.tssrc/platform/test-helpers.tssrc/platform/uws-platform.adapter.spec.tssrc/platform/uws-platform.adapter.tssrc/platform/uws-request.ts
VikramAditya33
force-pushed
the
v2-p5
branch
3 times, most recently
from
011722a to
e4b15a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/platform/test-helpers.ts (4)
224-226:listenmock ignores the 2-arg overload and always invokes the callback.
uWS.TemplatedApp.listenhas multiple overloads —(port, cb),(host, port, cb), and(port, options, cb). The mock hard-codes the 3-arg shape, so any production code path callingapp.listen(port, cb)against this mock will pass the callback as_portand silently do nothing. Ifuws-platform.adapter.tsonly ever uses the 3-arg form this is fine, but it's a latent footgun. Consider using a variadic signature that picks the last arg as the callback.- listen: jest.fn((_host: string, _port: number, callback: (socket: unknown) => void) => { - callback(listenSocket); - }), + listen: jest.fn((...args: unknown[]) => { + const callback = args[args.length - 1] as (socket: unknown) => void; + if (typeof callback === 'function') callback(listenSocket); + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/test-helpers.ts` around lines 224 - 226, The listen mock in src/platform/test-helpers.ts currently assumes a 3-arg signature and always calls the callback, which breaks the 2-arg overload used by uWS.TemplatedApp.listen; change the jest.fn for listen to accept variadic args (e.g., ...args), detect the last argument as the callback if it's a function, and invoke it with listenSocket, otherwise return a failure indicator; update references to listenSocket and ensure this mock covers the (port, cb), (host, port, cb), and (port, options, cb) forms so uws-platform.adapter.ts tests behave like production.
196-242:trackRoutesdefaults totrue, which contradicts the opt-in framing in the JSDoc example.The example at line 190 passes
{ trackRoutes: true }explicitly, implying opt-in, but the default is alreadytrue. Either flip the default tofalse(opt-in, matching the example) or drop the explicit flag from the example to avoid confusion for readers. Also,registeredRoutesis always returned even when tracking is disabled — fine, just noting for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/test-helpers.ts` around lines 196 - 242, The default behavior of createMockUwsApp currently sets trackRoutes to true which conflicts with the JSDoc/example that treats route tracking as opt-in; change the default in the createMockUwsApp options destructuring to trackRoutes = false (or alternatively remove the explicit { trackRoutes: true } in the example), ensuring the MockUwsAppOptions default aligns with the JSDoc, and leave registeredRoutes returned (it can remain empty when tracking is disabled).
151-151:tryEndmock ignoreswriteSuccessoption.
writeis wired towriteSuccess, buttryEndalways returns[true, false]regardless. Tests that exercise backpressure throughtryEndhave to override it manually (asuws-response.spec.tsdoes at line 969, 981, 1012). For consistency, consider havingtryEndhonorwriteSuccessas theokflag, e.g.[writeSuccess, false].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/test-helpers.ts` at line 151, The tryEnd mock currently always returns [true, false] which ignores the writeSuccess flag used by the write mock; update the jest.fn for tryEnd in test-helpers so it returns [writeSuccess, false] (i.e., use the same writeSuccess boolean that the write mock reads) so backpressure behavior is consistent with the write mock and existing tests like uws-response.spec.ts no longer need to override tryEnd manually.
78-79: Falsy-coalescing may mask legitimate empty/zero parameter or header values.
params[index] || ''andheaders[name.toLowerCase()] || ''behave fine today because all values are strings, but if a test ever sets a parameter to'0'or''intentionally, the||fallback still yields''— which is what real uWS also returns for missing values, so it's indistinguishable. Considerparams[index] ?? ''to at least preserve nullish semantics clearly. Low priority.- getParameter: jest.fn((index: number) => params[index] || ''), - getHeader: jest.fn((name: string) => headers[name.toLowerCase()] || ''), + getParameter: jest.fn((index: number) => params[index] ?? ''), + getHeader: jest.fn((name: string) => headers[name.toLowerCase()] ?? ''),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/test-helpers.ts` around lines 78 - 79, The mock implementations for getParameter and getHeader use falsy-coalescing which masks legitimate empty or "0" values; update the jest.fn callbacks for getParameter (referencing params and index) and getHeader (referencing headers and name.toLowerCase()) to use nullish coalescing (?? '') instead of || '' so that non-null/defined falsy strings like '0' or '' are preserved while still defaulting to '' for undefined/null missing values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/test-helpers.ts`:
- Around line 131-144: The mock response's onAborted currently invokes the abort
callback synchronously when aborted is true, causing UwsResponse constructor
(and tests) to observe a synchronous abort; update mockRes.onAborted so it
stores callbacks.onAborted = callback but defers invocation (e.g., via
queueMicrotask or setImmediate) when aborted is true, or alternatively remove
automatic invocation and document that tests should call callbacks.onAborted!()
after constructing UwsResponse; change references in tests that rely on
synchronous behavior to explicitly invoke callbacks.onAborted instead of passing
aborted: true.
---
Nitpick comments:
In `@src/platform/test-helpers.ts`:
- Around line 224-226: The listen mock in src/platform/test-helpers.ts currently
assumes a 3-arg signature and always calls the callback, which breaks the 2-arg
overload used by uWS.TemplatedApp.listen; change the jest.fn for listen to
accept variadic args (e.g., ...args), detect the last argument as the callback
if it's a function, and invoke it with listenSocket, otherwise return a failure
indicator; update references to listenSocket and ensure this mock covers the
(port, cb), (host, port, cb), and (port, options, cb) forms so
uws-platform.adapter.ts tests behave like production.
- Around line 196-242: The default behavior of createMockUwsApp currently sets
trackRoutes to true which conflicts with the JSDoc/example that treats route
tracking as opt-in; change the default in the createMockUwsApp options
destructuring to trackRoutes = false (or alternatively remove the explicit {
trackRoutes: true } in the example), ensuring the MockUwsAppOptions default
aligns with the JSDoc, and leave registeredRoutes returned (it can remain empty
when tracking is disabled).
- Line 151: The tryEnd mock currently always returns [true, false] which ignores
the writeSuccess flag used by the write mock; update the jest.fn for tryEnd in
test-helpers so it returns [writeSuccess, false] (i.e., use the same
writeSuccess boolean that the write mock reads) so backpressure behavior is
consistent with the write mock and existing tests like uws-response.spec.ts no
longer need to override tryEnd manually.
- Around line 78-79: The mock implementations for getParameter and getHeader use
falsy-coalescing which masks legitimate empty or "0" values; update the jest.fn
callbacks for getParameter (referencing params and index) and getHeader
(referencing headers and name.toLowerCase()) to use nullish coalescing (?? '')
instead of || '' so that non-null/defined falsy strings like '0' or '' are
preserved while still defaulting to '' for undefined/null missing values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2ee2442-3e7b-41b9-bae7-85eb2869f64b
📒 Files selected for processing (15)
package.jsonsrc/index.tssrc/interfaces/http-options.interface.tssrc/platform/http-context.spec.tssrc/platform/http-context.tssrc/platform/index.tssrc/platform/route-registry-middleware.spec.tssrc/platform/route-registry.spec.tssrc/platform/route-registry.tssrc/platform/test-helpers.tssrc/platform/uws-platform.adapter.spec.tssrc/platform/uws-platform.adapter.tssrc/platform/uws-request.tssrc/platform/uws-response.spec.tstsconfig.build.json
✅ Files skipped from review due to trivial changes (4)
- tsconfig.build.json
- package.json
- src/platform/route-registry-middleware.spec.ts
- src/platform/http-context.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/platform/uws-platform.adapter.spec.ts
- src/platform/index.ts
- src/platform/uws-request.ts
- src/interfaces/http-options.interface.ts
- src/platform/http-context.ts
- src/platform/uws-platform.adapter.ts
- src/platform/route-registry.ts
- src/platform/route-registry.spec.ts
Implement complete NestJS middleware integration with guards, pipes, exception filters, and interceptors. Add HTTP execution context and comprehensive test coverage.
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
1 participant
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.
Implements complete NestJS middleware support for the uWS platform adapter.
What's Implemented
Core Middleware Components
Execution Context
HttpExecutionContext: NestJS execution context implementationswitchToHttp(),switchToRpc(),switchToWs()getArgs(),getArgByIndex(),getClass(),getHandler()Route Registry Enhancements
Test Coverage
Summary by CodeRabbit
Release Notes
New Features
Improvements