Skip to content

Conversation

@bodnia
Copy link
Contributor

@bodnia bodnia commented Apr 14, 2017

No description provided.

@bodnia bodnia requested review from buunguyen and ponelat April 14, 2017 14:12
Copy link
Contributor

@ponelat ponelat left a comment

Choose a reason for hiding this comment

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

one more test, please

test/execute.js Outdated
})
})

it('should not add content-type with no form-data or body param', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not? I see content-type lower down

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see a test, where no contentType is added. As one of our current issues, is that GET requests get a content-type: application/json. And I'd like to make sure that doesn't happen in swagger-js ( I'm pretty sure it doesn't, but still ).

})

it('should handle requestContentType', function () {
it('should not add content-type with no form-data or body param', function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ponelat this test is about not to set content type

Copy link
Contributor

Choose a reason for hiding this comment

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

ok... but this test includes a requestContentType. If that is present, it should always set the header.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the issue in swagger-api/swagger-ui#2909, there is likely a requestContentType being passing in from swagger-ui. And that shouldn't happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here https://github.com/swagger-api/swagger-js/pull/1016/files#diff-2932c1430f43e1d22b8cf8b98fa5d2a8R127 first we check if there is body or form and only after that - if there is content-type passed from UI

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that. But we shouldn't 😛 if there is a requestContentType we must use it. Its an override provided by the user ( or swagger-ui in our case ).

Copy link
Contributor

Choose a reason for hiding this comment

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

you're right @bodnia it does only make sense if there is a body. The user can override the content-type header in other ways, if need be IIRC.

@bodnia
Copy link
Contributor Author

bodnia commented Apr 15, 2017

Issue swagger-api/swagger-ui#2909

@bodnia
Copy link
Contributor Author

bodnia commented Apr 18, 2017

@ponelat merge?

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.

2 participants