-
Notifications
You must be signed in to change notification settings - Fork 3
Expresskit validator #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/validator/index.ts
Outdated
// If there's no schema defined for this status code, or it's a 204, or data is undefined, don't send content | ||
const hasSchema = | ||
config.response.content[statusCode as number]?.schema !== undefined; | ||
if (!hasSchema || statusCode === 204 || data === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should leave control over the 204 code to consumers?
} | ||
}); | ||
|
||
if (ctx.config.openApiRegistry?.enabled && openapiRegistry) { | ||
const openApiSchema = openapiRegistry.getOpenApiSchema(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculating openApiSchema
can take a long time and block the application from starting. Is it possible to wrap these lines in setImmediate(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now schema is calculated on route add to registry
import jwt from 'jsonwebtoken'; | ||
|
||
// Add OpenAPI security scheme metadata to your auth handler | ||
const jwtAuthHandler = bearerAuth('myJwtAuth')(function authenticate(req, res, next) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work correctly with global declared appAuthHandler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
||
The `res` object in your handler is enhanced with the following methods: | ||
|
||
- **`res.sendTyped(statusCode, data?)`**: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sendTyped
is still not the best name.
I'm not sure if patching the original res.send()
is acceptable here, but if it is, I suggest the following naming:
res.send()
– for static type checking (nowres.sendTyped()
).res.sendOriginal()
– for original method (nowres.send()
).res.sendValidated()
– without any changes.
Or use res.contract.send()
and res.contract.sendValidated()
.
@resure what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original express method should not be patched. Such an override can have a lot of side effects and is probably not worth it.
res.contract.send()
I'm not 100% sure if this is better than sendTyped, but maybe it could be a logical way to move all new helpers to new namespaces (req.contract., res.contract.). The only worry is that it might look pretty long that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imsitnikov @resure what you think about having semantic methods: req.ok(data)
, req.created(data)
, req.badRequest(data)
, etc, depending on what is defined in schema? They will have static validation.
And probably req.ok.validated(data)
for dynamic validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like additional pollution of the req object, and one method will be easier to maintain than many.
Overall, I'm okay with res.contract.send()
or res.sendTyped()
, but I'm not the decision maker here.
ExpressKit Validator
Validation framework for ExpressKit that provides:
Features
TODO
req.body
will be already validated in a handler