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

Update spec to OpenAPI 3.0.1 #101

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Conversation

clarakosi
Copy link
Member

  • swagger: 2.0 has been replaced with openapi: 3.0.1
  • definitions was renamed to schemas and  securityDefinitions to securitySchemes and they all were moved inside components
  • servers replaces the basePath  keywords used in OpenAPI 2.0

Bug: T218218

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.529% when pulling 08368d0 on clarakosi:swagger into f974df2 on wikimedia:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.529% when pulling 08368d0 on clarakosi:swagger into f974df2 on wikimedia:master.

@coveralls
Copy link

coveralls commented Apr 17, 2019

Coverage Status

Coverage increased (+0.1%) to 91.513% when pulling 70505ab on clarakosi:swagger into f974df2 on wikimedia:master.

Copy link
Contributor

@d00rman d00rman left a comment

Choose a reason for hiding this comment

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

I think it would better if we kept compatibility with swagger v2 at least for now, it will make the transition much easier. Other minor comments in-lined.

lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
lib/router.js Outdated Show resolved Hide resolved
lib/router.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
lib/filters/validator.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Pchelolo
Copy link
Contributor

The validator was not only responsible for validating the parameters, it's also responcible for casting the incoming parameters to right types. I believe the logic there with _paramCoersionFun and _bodyCoersionFun was dependent on swagger 2, so it now doesn't work. This is why, for example, you needed to JSON.parse some stuff manually here https://github.com/wikimedia/restbase/pull/1120/files#diff-d91965930eda1e26c9b5db0d3041c4acR590

@Pchelolo
Copy link
Contributor

The tests for the validator should also be updated even more, some of the specs in the tests use outdated stuff, for example 'formData'

@Pchelolo
Copy link
Contributor

Let's, for now, remove node from travis.yaml - node 12 has been released recently and getting support for a new major node version is unrelated to this work. We should also file a ticket for node 12 support and do it separately. I've checked and the tests fail locally with node 12 even without this pull request, so it's clearly something else.

lib/utils.js Show resolved Hide resolved
@@ -43,44 +42,44 @@ const supportedValidators = ['maximum',
* @constructor
*/
class Validator {
constructor(parameters, definitions) {
constructor(parameters = [], requestBody = {}, schemas) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name 'requestBody' is a little confusing. It's the req body spec, not the body itself right? Can we make it less confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, here we assume that only 'schemas' out of components will be used in parameter definitions (like we do now in RESTBase) for example. Starting to use parameters components seem nice, we have a lot of shared parameters like title or accept between endpoitns, so abstracting that out would be neat. However, these 2 PRs are already enormous, so we can followup to do that in the next iteration. For now, would you mind adding a comment that that is not supported?

Copy link
Member Author

@clarakosi clarakosi Apr 30, 2019

Choose a reason for hiding this comment

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

IDK requestBody is what is used in the swagger docs and spec so it seems appropriate to follow that name.

I did make a note that parameters and requestBodies are not currently supported in components.

@Pchelolo
Copy link
Contributor

Almost there! Left a couple of comments, but overall LGTM

- `swagger: 2.0` has been replaced with `openapi: 3.0.1`
- `definitions` was renamed to `schemas` and 
  `securityDefinitions` to `securitySchemes` and they all were
  moved inside components
- servers replaces the basePath  keywords used in OpenAPI 2.0

Bug: T218218
@Pchelolo Pchelolo merged commit e5bd4da into wikimedia:master Apr 30, 2019
@clarakosi
Copy link
Member Author

Published as v0.12.0

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

4 participants