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

GET requests with content-type header set result in errors #29

Closed
Dieterbe opened this issue Sep 18, 2019 · 3 comments · Fixed by #30
Closed

GET requests with content-type header set result in errors #29

Dieterbe opened this issue Sep 18, 2019 · 3 comments · Fixed by #30

Comments

@Dieterbe
Copy link
Contributor

hello,
a GET request has no body, yet the HTTP RFC allows to set content-type on GET requests
(https://tools.ietf.org/html/rfc7231#section-3.1.1.5)
in this case it simply does not apply.
We see this commonly with customers that switch requests between GET and POST, but leaving the content-type header specified. For GET requests, it should simply have no effect.

However, the binding middleware will, upon detection of a non-zero content-type header, switch to trying to use the json decoder on the body, even when there is no body such as in a GET request.
This results in errors such as grafana/metrictank#1465 :

http  'http://localhost:6060/render?target=stats.docker-env.response.200&from=-6s' 'Content-Type:application/json'
HTTP/1.1 422 Unprocessable Entity
Content-Encoding: gzip
Content-Length: 96
Content-Type: application/json; charset=utf-8
Date: Wed, 18 Sep 2019 14:29:00 GMT
Vary: Accept-Encoding

[
    {
        "classification": "RequiredError",
        "fieldNames": [
            "target"
        ],
        "message": "Required"
    }
]

the correct behavior would be to process the request based on its method (e.g. GET requests use GET parameters)

@unknwon
Copy link
Contributor

unknwon commented Sep 18, 2019

Should I tag a new release or it will be fine as it is?

@Dieterbe
Copy link
Contributor Author

hey, a new release would be helpful. thanks

@unknwon
Copy link
Contributor

unknwon commented Sep 19, 2019

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

Successfully merging a pull request may close this issue.

2 participants