Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed unnecessary scopes mutation #898

Merged
merged 1 commit into from
Nov 23, 2016
Merged

Conversation

frol
Copy link
Contributor

@frol frol commented Nov 13, 2016

The issue due to the mutated scopes popped up in Swagger-UI: swagger-api/swagger-ui#2483

I am not sure if this is a correct solution, but it works for me.

/cc @fehguy @janslow

The issue due to the mutated scopes popped up in Swagger-UI: swagger-api/swagger-ui#2483
@frol
Copy link
Contributor Author

frol commented Nov 14, 2016

This attempt seems to be wrong as the tests explicitly expect that vendorExtensions will be mounted. Thus, I think, the iteration over the list of scopes should be fixed in Swagger UI project.

I keep this PR open until someone from the maintainers makes a final decision.

@janslow
Copy link

janslow commented Nov 15, 2016

Overriding vendorExtensions on the scopes object could also cause issues if an API has a scope called vendorExtension. A safer property name for vendor extensions would be to prefix that name with x- (e.g., x-vendorExtensions) or use a symbol as the property name (which probably isn't ok due to compatibility with old JS engines).

Another option, which would solve this bug would be to use Object.defineProperty to make vendorExtensions non-enumerable.

Finally, is there a reason to aggregate all of the vendor extension properties into a single object, as opposed to just adding each of them to the main object?

@fehguy fehguy merged commit 1bc499e into swagger-api:master Nov 23, 2016
@fehguy fehguy modified the milestone: v2.1.24 Nov 23, 2016
@frol
Copy link
Contributor Author

frol commented Nov 23, 2016

Just for the record, this PR was merged and 5 hours later, it was reverted in d916f52

Also, beware that now it not only adds vendorExtensions, but also removes all scopes which happen to start with x- (IMHO, for no good reason at all).

@fehguy
Copy link
Contributor

fehguy commented Nov 23, 2016

The fix will be addressed in a Swagger-UI patch, and believe it or not, for good reason. The patch supplied here didn't make sense. See swagger-api/swagger-ui@30f0848

Pull master and enjoy

@frol
Copy link
Contributor Author

frol commented Nov 23, 2016

The patch supplied here didn't make sense.

I agree and I pointed this out in my second comment here. I didn't expect it would be merged (and I also stated that), so I was just surprised that it was merged to be reverted a few hours later. I wrote the last comment just so people don't get confused.

Thank you for the proper fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants