Fix various ref handling (fixes : #2086) #2087
Conversation
anthochristen
force-pushed
the
bug/fix-cb-and-discriminator-refs
branch
from
d04e41c
to
18400a0
Compare
anthochristen
changed the title
|
@gracekarina I see that you have worked on the files in which most of this changes are done for this issue (#2086). Would be great if you can take a look at these changes. |
|
Have a blanket IT type test for now, can add more in depth UTs once I get some comments on these changes. |
|
Thanks @frantuma for assigning this to yourself! |
|
P.S: These issues are not present in the npm |
There was a problem hiding this comment.
@anthochristen thanks a lot for your PR
added a couple of comments
| } else { | ||
| processSchema(s, file); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We would need to add processProperties(arraySchema.getItems().getProperties(), file); also here as a composed schema can also have properties
There was a problem hiding this comment.
Looks like this scenarios was handled in the latest snapshot version of reverted this change for arrays.
| @@ -246,9 +264,29 @@ private void processSchema(Schema property, String file) { | |||
| } | |||
| if (property instanceof ComposedSchema) { | |||
| ComposedSchema composed = (ComposedSchema) property; | |||
| final Map<String, String> refMap = Optional.ofNullable(property.getDiscriminator()) | |||
There was a problem hiding this comment.
IIANM the changes here, alongside the ones in SchemaProcessor and PathsProcessor, consider oneOf and anyOf as mutually exclusive, while they can coexist even if not so common. I would suggest to consider them both in the processing
There was a problem hiding this comment.
Updated as per suggestion, thanks!
| }if (composedSchema.getAnyOf() != null) { | ||
| for(Schema innerModel : composedSchema.getAnyOf()) { | ||
| } | ||
| if (composedSchema.getAnyOf() != null || composedSchema.getOneOf() != null) { |
There was a problem hiding this comment.
AS mentioned above:
IIANM the changes here consider oneOf and anyOf as mutually exclusive, while they can coexist even if not so common. I would suggest to consider them both in the processing
There was a problem hiding this comment.
Made the processing to be mutually inclusive.
Intuitively though, the definitions feel that they are mutually exclusive.
| @@ -157,8 +157,10 @@ public void processComposedSchema(ComposedSchema composedSchema) { | |||
| } | |||
| } | |||
| } | |||
| }if(composedSchema.getOneOf() != null){ | |||
| final List<Schema> schemas = composedSchema.getOneOf(); | |||
| } | |||
There was a problem hiding this comment.
AS mentioned above:
IIANM the changes here consider oneOf and anyOf as mutually exclusive, while they can coexist even if not so common. I would suggest to consider them both in the processing
anthochristen
force-pushed
the
bug/fix-cb-and-discriminator-refs
branch
from
076540b
to
0f34603
Compare
anthochristen
force-pushed
the
bug/fix-cb-and-discriminator-refs
branch
from
0f34603
to
a19e9e0
Compare
|
@frantuma I've changed the oneOf and anyOf to be inclusive, kindly help take a look when possible, thanks! |
anthochristen
force-pushed
the
bug/fix-cb-and-discriminator-refs
branch
from
a19e9e0
to
04d7ef2
Compare
This PR is to address the $ref handling issues in callbacks and discriminator mapping objects as detailed in #2086.
It does it with the following changes
anyOflike its done forOneOfinSchemaProcessorclass.ComplexSchemasprocessRefToExternalPathIteminExternalRefProcessorso that it can be used for $ref path resolution in PathItems used within callbacks.Each done in dedicated commits, although there might be some spill over.