-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(openapi): throw OpenAPIValidatorMiddlewareError #446
Conversation
🦋 Changeset detectedLatest commit: a6151c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for openpayments-preview canceled.
|
@@ -0,0 +1,8 @@ | |||
--- | |||
'@interledger/openapi': major |
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.
I think it should be major since it changes how createValidatorMiddleware
is used.
But don't mind using minor
since its just Rafiki using it
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.
Major sounds right, if I'm understanding correctly. This will break consumers where NODE_ENV
is production
and a response will fail validation.
const validateResponse = | ||
process.env.NODE_ENV !== 'production' && | ||
spec.createResponseValidator(options) |
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.
I feel like directly using process.env.NODE_ENV
is not a fun time.
Will update Rafiki to have validationOptions.validateReponse: process.env.NODE_ENV !== 'production'
instead
} else if (isValidationError(err)) { | ||
ctx.throw(err.status ?? 500, err.errors[0]) | ||
} else { | ||
console.log('err=', err) |
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.
removing pesky console.log here
packages/openapi/src/index.ts
Outdated
status?: number | ||
errors: string[] | ||
errors: ReturnType<typeof errorTransformer>[] |
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 return type of errorTransformer
is
{
message: string
}
Might it make sense to define that and use it here and the errorTransformer
signature?
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.
Yup, I think I went with this since I didn't have a good name for the type 😆
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.
Notice that I didn't offer a name myself lol
Changes proposed in this pull request
Changes to the
createValidatorMiddleware
behaviour inpackages/openapi
:OpenAPIValidatorMiddlewareError
instead of directly using Koa'sctx.throw
function. This error class hasmessage
andstatus
properties that describe the exact validation error as well as the status code that should be thrown.validationOptions
argument that determines whether to validate just the request, the response or both. By default, both are validated, and the middleware no longer usesprocess.env.NODE_ENV
to determine whether to validate the response or not.Context
Fixes #445