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

Fixes Swagger.js no setting jQuery.ajax contentType parameter #1337

Merged
merged 2 commits into from
Jan 2, 2018
Merged

Fixes Swagger.js no setting jQuery.ajax contentType parameter #1337

merged 2 commits into from
Jan 2, 2018

Conversation

noirbizarre
Copy link
Contributor

This PR fixes a nasty bug with Swagger.js/jQuery.

Context

Swagger.js only set the Content-Type HTTP header on request but does not give the contentType parameter to jQuery.ajax.

Consequence

jQuery consider the request body as url encoded and reencode content. As a side effect, all %20 characters are replaced by + signs.

Resolution

This PR adds a hackish Swagger.js request interceptor which set the jQuery.ajax contentType parameter to the same value than the Content-Type header.
This fixes many side-effects:

  • %20 are not replaced by + signs anymore (When URLs are saved, %20 are converted to plus signs #1126)
  • default success and error handlers are taken in account (side effect of operation call signature change)
  • any places where double encoding was occurring (I'm sure we missed some of them) are fixed too

@noirbizarre noirbizarre added this to the 1.2.6 milestone Dec 30, 2017
@noirbizarre noirbizarre requested a review from a team December 30, 2017 12:10
@noirbizarre noirbizarre changed the title Fixes Swagger.js no setting jQuery ajax contentType parameter (fix #1… Fixes Swagger.js no setting jQuery ajax contentType parameter Dec 30, 2017
@noirbizarre noirbizarre changed the title Fixes Swagger.js no setting jQuery ajax contentType parameter Fixes Swagger.js no setting jQuery.ajax contentType parameter Dec 30, 2017
@noirbizarre
Copy link
Contributor Author

noirbizarre commented Dec 30, 2017

For reference this is a combination of:

This swagger.js issue was inexistant (invisible and without side-effects) before jQuery 2.2.0

@taniki
Copy link
Contributor

taniki commented Dec 30, 2017

Can it be also related to https://swagger.io/docs/specification/2-0/describing-request-body/ ?

consume parameter does not seem to be set to application/json anywhere. Maybe this is not implicit and swagger doesn't provide proper content-type to the js part.

No dev env here. will get a try and review this on tuesday!

Copy link
Contributor

@pblayo pblayo left a comment

Choose a reason for hiding this comment

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

I took the liberty to change the interceptor's comment from

  • "swagge as" to
  • "swagger has"

Shouldn't an issue be opened in swagger.js itself ?
(with a quick search I didn't find any, except some related but not the same issues in swagger-ui like swagger-api/swagger-ui#2785)

@noirbizarre
Copy link
Contributor Author

The consume parameter properly set the Content-Type header, this is only the jQuery.ajax call which is broken.

Yes there might be something to fix in upstream swagger.js. Sadly, we are using Swagger.js 2.X and a fix on the 3.x won't be usable to us right now.

@noirbizarre noirbizarre merged commit 44185ee into opendatateam:master Jan 2, 2018
@noirbizarre noirbizarre deleted the gh-1126-spaces-in-fields branch January 2, 2018 10:41
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.

None yet

3 participants