Introduce validation middleware#160
Conversation
imdhemy
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward. The general direction is useful, but I think the API needs one adjustment before this lands.
The key DevX goal should be:
- Framework users wire validation once with their configured validator.
- That validator already includes built-in and custom constraint validators.
- Endpoint code then creates middleware by passing only request
validationRules.
So the preferred API shape is:
const validateRequest = createValidationMiddleware({ validate });
export const routes = [
Route({
method: 'POST',
path: '/users',
middleware: [
validateRequest({
email: ['notBlank', 'email'],
}),
],
handler: async scope => {
scope.response.body = { ok: true };
},
}),
];If custom response formatting is needed, the mapper can be wired once too:
const validateRequest = createValidationMiddleware({
validate,
mapViolations,
});This keeps dependency injection explicit, supports custom constraints through the configured validator, avoids module mocking, and keeps endpoint-level code clean.
One important naming point: ValidationRules are the rules the request must satisfy, while validator constraints are the mechanics that perform validation. The middleware API should use validationRules for the endpoint argument to avoid mixing those concepts.
|
|
||
| export type Validator = (payload: Payload, rules: ValidationRules, options?: ValidateOptions) => Violation[]; | ||
|
|
||
| export type ViolationMapper = (violations: Violation[]) => Record<string, string[]>; |
There was a problem hiding this comment.
I would avoid adding ViolationMapper here.
src/validator/types.ts is exported through the validator public API, so this widens the framework API without proving that ViolationMapper is a user-facing contract.
If this type is only needed by the middleware factory, declare it in validation-middleware.ts. If we intentionally want users to customize violation mapping, expose it through the createValidationMiddleware options shape instead of adding a broad shared type by default.
This PR should stay focused on the middleware helper; removing barrels or reorganizing all validator types can be a separate refactor.
| import { type HttpMiddleware } from '@/Http'; | ||
| import { type ValidationRules, type Validator, type ViolationMapper } from '@/validator/types'; | ||
|
|
||
| export function validationMiddleware( |
There was a problem hiding this comment.
The composition idea is good, but I think the API should make the one-time wiring explicit.
Instead of:
validationMiddleware(validate, mapViolations)(constraints)prefer:
createValidationMiddleware({
validate,
mapViolations,
})(validationRules)validate should be the user’s configured validator, which already includes built-in and custom constraint validators. That avoids hardcoding built-ins here while still giving endpoint code a simple API.
mapViolations can default to flattenViolations, since that is the framework default behavior required by the issue.
| export function validationMiddleware( | ||
| validate: Validator, | ||
| mapViolations: ViolationMapper, | ||
| ): (constraints: ValidationRules) => HttpMiddleware { |
There was a problem hiding this comment.
Please rename this endpoint-level parameter from constraints to validationRules.
In this module there are two different concepts:
validationRules: the request rules an endpoint requires- constraint validators: the validation mechanics configured inside the validator
Using constraints here makes the public API ambiguous, especially because custom constraint validators are configured when creating the validator, not when creating endpoint middleware.
| mapViolations: ViolationMapper, | ||
| ): (constraints: ValidationRules) => HttpMiddleware { | ||
| return function createMiddleware(constraints: ValidationRules): HttpMiddleware { | ||
| return async function middleware(scope, next): Promise<void> { |
There was a problem hiding this comment.
This is contextually typed through the returned HttpMiddleware, so explicit scope and next types are not required for type safety.
I’m fine leaving it as-is, but if we want readability we can type them explicitly using the existing HTTP middleware types.
|
|
||
| describe('Validation middleware', () => { | ||
| test('continues to the next middleware when no constrains are violated', async () => { | ||
| const validate: Validator = vi.fn().mockReturnValue({}); |
There was a problem hiding this comment.
This mock violates the Validator contract.
Validator returns Violation[], so the passing case should return [], not {}. Returning {} makes the test pass accidentally through .length being undefined, and it weakens the test as an example of the API.
Hello,
In this PR, I introduced a validation middleware.Initially, I placed it under the
http/middlewarenamespace because it is HTTP-related and functions as middleware. However, I later moved it to thevalidatornamespace, as I felt it conceptually belongs there more. I don’t have a strong preference yet regarding its exact placement, but my current decision is to keep it undervalidator.Please let me know if you have a different opinion.