Skip to content
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

Merged
merged 6 commits into from
Jun 15, 2020

Conversation

stefan-malcek
Copy link
Contributor

Solves #39

QueryParams object is split into separate params in Swagger UI. I even added a Swagger UI page and a QueryParams decorator on getAll 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?

Copy link
Owner

@epiphone epiphone left a 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?

schema,
const paramSchema = getParamSchema(queriesMeta) as oa.ReferenceObject
const paramSchemaName = _.last(_.split(paramSchema.$ref, '/')) || ''
const schemas = validationMetadatasToSchemas({
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epiphone tests done

Copy link
Owner

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.

Copy link
Owner

@epiphone epiphone left a 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.

@stefan-malcek
Copy link
Contributor Author

stefan-malcek commented Jun 13, 2020

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 @types/express-serve-static-core package. Setting it in package.json as a dev dependency should do it.

Some references:
DefinitelyTyped/DefinitelyTyped#40905
DefinitelyTyped/DefinitelyTyped#40138

Copy link
Owner

@epiphone epiphone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@epiphone epiphone merged commit a297f9b into epiphone:master Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants