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

[Python] msrest does not enforce JSON Accept header #867

Closed
lmazuel opened this issue Mar 21, 2016 · 7 comments
Closed

[Python] msrest does not enforce JSON Accept header #867

lmazuel opened this issue Mar 21, 2016 · 7 comments
Labels

Comments

@lmazuel
Copy link
Member

lmazuel commented Mar 21, 2016

Hi,

msrest does no content negotiation currently and then does not fill the Accept header in any request.
Unfortunately, not all provider use JSON by default when the Accept header is not present (this leads to problem like this Azure/azure-sdk-for-python#553, solved by adding the right Accept header)

In addition, msrest cannot currently handle XML data. All requests have to be JSON.

I suggest to force Accept: application/json in all requests done using msrest. This can be made at several places:

# Construct headers
header_parameters = {}
header_parameters['Content-Type'] = 'application/json; charset=utf-8'

What do you think?

I don't mind adding directly in msrest core code, since as I said msrest cannot parse XML anyway.

I can do the PR once you we agreed on the result.

@annatisch

@annatisch
Copy link
Member

I would be keen to know how the other clients/languages support or enforce this.
The generated code will often set the Content-type header if the content-type is specified in the swagger spec so I'm not sure we should be simply overwriting it. Particularly for stream requests/responses.
Though perhaps we should set it where it doesn't already exist.

@devigned
Copy link
Member

I believe we select the first accept type from the swagger that corresponds to application/json.

@lmazuel
Copy link
Member Author

lmazuel commented Mar 21, 2016

@devigned

Are you saying that if the Swagger file has:

  "produces": [
    "application/json"
  ],

We should add an Accept for application/json, if not leave the server decides?

@lmazuel
Copy link
Member Author

lmazuel commented Mar 21, 2016

Side effect: do we have to refuse generate Python code if "produces" does not contains application/json (since anyway we can't do anything with something else notJSON)?

@annatisch
Copy link
Member

@lmazuel - I think it's okay to put this is msrest,
I can add:
session.headers['Accept'] = 'application/json'
at line 120 as you suggest.

@lmazuel
Copy link
Member Author

lmazuel commented Mar 22, 2016

Thank you @annatisch I will do a PR that way!

@annatisch
Copy link
Member

@amarzavery
This can be closed now :)

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

No branches or pull requests

4 participants