Skip to content

Conversation

@owenconti
Copy link
Contributor

@owenconti owenconti commented Sep 10, 2017

Fixes #1129

Should also fix the following swagger-ui issues:

swagger-api/swagger-ui#2946
swagger-api/swagger-ui#3490
swagger-api/swagger-ui#3342
swagger-api/swagger-ui#3127
swagger-api/swagger-ui#3093

and possibly others...

Added a patch at the end of the allOf plugin to make sure the original values for the definition are merged back into the result of the allOf. This includes the $$ref value. For definitions that did not originally include $$ref values, we make sure the result of allOf does not have a $$ref value.

I wrote a test for the new functionality, but during test runs (any test, not just mine), the $$ref value that is set during the refs plugin, is not available on the patch object inside the allOf plugin.

Once we get that figured out, then we should be able to remove the skip from my new test.

Here's a screenshot showing the fix in action in the UI project:

Before:

screen shot 2017-09-09 at 1 40 55 pm

After:

screen shot 2017-09-10 at 9 49 11 am

Adds a patch to the end of the `allOf` function which attaches the $$ref value, found from the `patch` object
…. Added test for $$ref values, but left it as skipped because $$ref values aren't being attached during any test.
@owenconti owenconti requested a review from shockey September 10, 2017 15:49
…al $$ref value, do not have one after the `allOf` plugin runs
@webron webron added the P1 label Sep 11, 2017
@webron
Copy link
Contributor

webron commented Sep 11, 2017

@shockey this solves oh-so-many issues, would love to see it merged

@dballance
Copy link
Contributor

Great fix - just saw this while following up on one of the linked issues!

Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

LGTM! Added a test, for good measure.

I'll miss the $$ref being available, but IMO no data is better than unreliable data.

@shockey
Copy link
Contributor

shockey commented Sep 12, 2017

Okay, now it's ready 😄

Thanks @owenconti!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

allOf pulls in incorrect $ref value

4 participants