Fixes additionalProperties for v2 (swagger) specs #1414
Conversation
spacether
changed the title
spacether
changed the title
| @@ -819,6 +820,10 @@ public Model definition(ObjectNode node, String location, ParseResult result) { | |||
| JsonNode ap = node.get("additionalProperties"); | |||
| if (ap != null && ap.getNodeType().equals(JsonNodeType.OBJECT)) { | |||
| impl.setAdditionalProperties(Json.mapper().convertValue(ap, Property.class)); | |||
| } else if ("object".equalsIgnoreCase(type) && (ap == null || "true".equalsIgnoreCase((String) ap.asText()))) { | |||
There was a problem hiding this comment.
Should we set additionalProperties true for composed schemas also?
What does the master branch of swagger parser do?
|
@gracekarina can you help debug these failures? |
Never mind; I was able to fix them on my own. |
| assertNotNull(property); | ||
| assertTrue(property instanceof RefProperty); | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved + response mocked to get test to pass
There was a problem hiding this comment.
I have a question why the need to move the test from one class to another?
There was a problem hiding this comment.
It was easier to do it this way. All of our classes that we need to attach the fake request already exist in the file that I moved it in to. If I kept them where they were then I would need to add new import and setup lines for mocking the request for just those two tests in two separate files. We already have that set up code in the file that I moved them into.
Also, because these two tests need network resources, these are NetworkReferenceTests
|
|
||
| assertTrue(swagger.getDefinitions().get("Order") instanceof ModelImpl); | ||
| assertTrue(swagger.getDefinitions().get("Order").getProperties().size() == 6); | ||
| } |
There was a problem hiding this comment.
Moved + response mocked to get test to pass
| assertNotNull(schema); | ||
| assertNotNull(schema.getAdditionalProperties()); | ||
| assertEquals(schema.getAdditionalProperties().getType(), "ref"); | ||
| } |
There was a problem hiding this comment.
these are our additionalProperties tests
|
Converting to draft while I am making these changes: |
|
Making the TODO changes was too arduous so I stopped working on this. |
|
Closing this because there doesn't appear to be interest from the swagger-parser team in merging it and feedback in #1369 is that the v2 openapi spec definition of additionalProperties is not consistent with that the openapi spec authors intended. |
Fixes #1369
Brings additionalProperties values into compliance with the Openapi 2.0 specification
Fixes two CI tests which were failing. They were failing because their requests to get json files from a url were not mocked and the requested url resource was returning a http 302 status code without the spec content.
Here is what the code in the v1 branch has done before this PR:
If this fix is merged, the code will now do the following:
TODO, update to:
This will bring our results in line with the v3 spec parsing.
Future Thoughts
In the future it would be helpful if parsing of v2 and v3 specs resulted in:
Because that would comply with the Openapi v2 and v3 specifications. Before that though, we should aim for consistency of results when parsing v2 and v3 specs.
This fix is applied to the v1 branch, and should rev roll it to a new version, maybe 1.1.0 because this may be a breaking change for legacy usages of additionalProperties which did not handle true/false/and omitted correctly.
We can't rev roll to 2.X.X because that is used by the current master branch which handles v3 specs and uses this branch for v2 spec parsing before conversion to v3 specs.
@sebastien-rosset
@gracekarina
@hkosova