-
Notifications
You must be signed in to change notification settings - Fork 59
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
QueryParams are parsed as separate params #49
Conversation
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.
Cool! Can we get a few tests in too?
src/generateSpec.ts
Outdated
schema, | ||
const paramSchema = getParamSchema(queriesMeta) as oa.ReferenceObject | ||
const paramSchemaName = _.last(_.split(paramSchema.$ref, '/')) || '' | ||
const schemas = validationMetadatasToSchemas({ |
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'm not a huge fan of calling validationMetadatasToSchemas
here and having class-validator
as a dependency.
Can we instead find the query object among the schemas passed in via the additionalProperties
argument?
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.
Sure, I committed the changes.
What do you think about this line?
const spec = getSpec(routes, additionalProperties.components?.schemas || {})
Both components and schemas are optional, so I needed to provide a default value to keep TS happy.
Gonna write some tests.
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.
@epiphone tests done
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.
That works well, I think.
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.
Nice work!
The only problem I found was that building the sample now fails in some sort of a type error:
app.ts:43:9 - error TS2345: Argument of type '"/docs"' is not assignable to parameter of type 'RequestHandlerParams'.
43 app.use('/docs', swaggerUiExpress.serve, swaggerUiExpress.setup(spec));
~~~~~~~
node_modules/@types/swagger-ui-express/node_modules/@types/express/index.d.ts:98:50 - error TS2694: Namespace '"/home/ap/koodi/routing-controllers-openapi/sample/01-basic/node_modules/@types/express-serve-static-core/index"' has no exported member 'Params'.
98 interface ErrorRequestHandler<P extends core.Params = core.ParamsDictionary, ResBody = any, ReqBody = any, ReqQuery = core.Query>
~~~~~~
...
I did a rm -rf node_modules && yarn install && yarn build
to trigger that.
Also please commit in the updated sample project yarn.lock
file.
There was a problem with the Some references: |
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.
Awesome, thanks!
Solves #39
QueryParams
object is split into separate params in Swagger UI. I even added a Swagger UI page and aQueryParams
decorator ongetAll
action to the example project in order to simply show new changes.Note:
class-validator-jsonschema
has become a direct dependency. Is it a problem?