Fix properties removal for openapi format. #129
Conversation
Properly detect empty objects before parsing.
|
Hi @benoitbzl I tested your changes with this test: Do I understand correctly what you're trying to achieve? I'm assuming your intentions after reading one of the lately submitted issues at #128 If my assumptions are correct, could you please add the patch to your pull request and solve it? (I couldn't see the |
|
Hi @kalinchernev My PR is not about fixing #128. |
|
@benoitbzl ok, that's good to know. Could you please expand the test cases with your known issues? |
|
Sure. Sorry, I always forget about the test cases :-( I will update the commit. |
|
@benoitbzl thanks! |
There was a problem hiding this comment.
@benoitbzl thank you for expanding on the tests!
I checked out your branch, pulled latest changes from the upstream master and all looks ok.
However, I couldn't manage to validate the newly added example reference specification. So far, all other reference specifications in examples are showing how certain documentations are parsed to validated outputs. Could you please check the feedback added below from the swagger editor?
Could you please also get latest changes from the upstream master?
To be honest, I still don't quite see which properties are removed and the reason to rewrite the helper isEmptyObject
| // eslint-disable-next-line | ||
| Object.keys(obj).forEach(key => { | ||
| if (key in obj) return false; | ||
| return !Object.keys(obj).some(key => { |
There was a problem hiding this comment.
Feel free to rename the helper to negation in order to match the new logic.
To be honest, I don't quite see the optimisation of the rewrite.
There was a problem hiding this comment.
I didn't change the logic of the function. The previous one was not working as plan. It was always returning true.
There was a problem hiding this comment.
I don't mind refactoring in it, honestly.
Rather I'm trying to see which cases haven't been covered.
Previously, there was a similar discussion, here's the background #122 (comment)
In general, the aim of this helper is to say whether an object key contains a value which is an empty object, i.e. the whole property is empty.
| @@ -0,0 +1,91 @@ | |||
| { | |||
There was a problem hiding this comment.
I couldn't validate this spec. Using https://editor.swagger.io/ and pasting the openapi.json, which converts to
openapi: 3.0.0
info:
version: 1.0.0
title: Sample specification testing definitions
paths:
/api/v1/audits:
post:
operationId: addAudit
tags:
- Audits
description: Create a new audit
consumes:
- application/json
produces:
- application/json
parameters:
- in: body
name: Audit initialization data
description: The necessary initialization data to create an audit object
schema:
type: object
properties:
filter:
$ref: '#/definitions/Filter'
time:
$ref: '#/definitions/Time'
responses:
'200':
description: return the created audit
'400':
description: Invalid or missing parameters
'500':
description: Server error
definitions:
Filter:
description: Query filters
type: array
items:
type: object
properties:
key:
description: field name
required: true
type: string
value:
description: field value
required: true
type: {}
op:
description: comparaison operation
required: false
type: string
enum:
- =
Time:
description: Audit query time range
type: object
properties:
to:
description: time range end
type: number
from:
description: time range start
type: number
components: {}
tags: []Seems to be invalid following the feedback:
Schema error should NOT have additional properties
additionalProperty: definitions
Jump to line 0
Schema error at paths['/api/v1/audits'].post
should NOT have additional properties
additionalProperty: consumes, produces
Jump to line 7
Schema error at paths['/api/v1/audits'].post.parameters[0].in
should be equal to one of the allowed values
allowedValues: path, query, header, cookie
Jump to line 17
Schema error at paths['/api/v1/audits'].post.parameters[0]
should match exactly one schema in oneOf
Jump to line 17
I searched to find information about the definitions property, but couldn't find any. Could it be a feature in WIP or something unsupported yet?
There was a problem hiding this comment.
That's our bad. Indeed the generated openapi.json file shows error in the swagger editor. But when we render it with the latest swagger-ui component, it seemed to be correctly rendered without error...
Anyway, I will fix the example so that it is accepted by swagger editor and compliant with openapi 3.x spec.
| 'callback', | ||
| 'links', | ||
| 'petstore', | ||
| 'definitions', |
There was a problem hiding this comment.
Nice approach on expanding the test suit 👍
| // Imaginary API helper | ||
| module.exports = function test(app) { | ||
| /** | ||
| * @swagger |
There was a problem hiding this comment.
If this part of the spec is not related to an endpoint/resource, I'd hoist it before the app export. I.e. in the other examples globals are before the module.exports.
| @@ -55,16 +53,16 @@ function getSpecificationObject(options) { | |||
| specHelper.addDataToSwaggerObject(specification, swaggerJsDocComments); | |||
| } | |||
|
|
|||
| if (specification.openapi) { | |||
There was a problem hiding this comment.
Definitely makes sense to clean unnecessary properties before the parsing 👍
|
I will try to make the changes tomorrow. |
|
Hi @kalinchernev. I fixed our documentation to be openapi 3.0 compliant and the original issue does not show up anymore ; the existing swagger-jsdoc package works perfectly. So I think we can close that PR as it was not needed originally. |
|
@benoitbzl thanks for the follow up! I'm closing the pull request. |
|
The 'definitions' property (among others) is not supported at the root of the swagger.json file for openapi 3. Instead, one has to put common definitions into the section components/schemas. |
Properly detect empty objects before parsing.