Fix ref handling in CB, AnyOf & examples (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
anthochristen
changed the title
|
@frantuma @gracekarina @walaniam Any help on this would be much appreciated :) |
|
Hi @frantuma @gracekarina @walaniam - I am the Product Manager for Java Frameworks at PayPal. This PR is currently blocking an enterprise-level initiative to upgrade to OAS 3.0. Below is the business case and impact if this PR is not merged in the coming weeks. Please let me know if there is a better channel to escalate this in to get better visibility. Thank you in advance - appreciate the partnership! The code generation tool is crucial for developers to accurately validate specification and implement REST APIs at PayPal. This PR fixes critical bug that impact virtually all OAS3 REST API specifications using examples, parameter, discriminator and callback features, making the codegen tool incompatible in its current state. Without this fix, developers are forced with cumbersome workarounds to choose between restructure their API specifications that hinder reusability or generate java model classes on the resolved specification resulting significant disruption to existing API implementation. This issue not only impacts individual teams but also prevents the API Platform team from effectively testing and ensuring the platform meets customer needs. Ultimately, Failing to merge this PR to address this critical issue could delay API deployments and hinder the overall adoption of the new platform. APIReferenceTemplate Repo: https://github.paypal.com/ApiSpecifications-R/APIReferenceSpecification/tree/develop/v1/schema/components |
frantuma
force-pushed
the
bug/fix-cb-and-discriminator-refs
branch
from
04d7ef2
to
e6b702a
Compare
|
@anthochristen eventually merged, thanks! |
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.