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

Supporting GET method with a request body is an option? #81

Closed
zhiyanliu opened this issue Jul 6, 2017 · 3 comments · Fixed by #82
Closed

Supporting GET method with a request body is an option? #81

zhiyanliu opened this issue Jul 6, 2017 · 3 comments · Fixed by #82
Assignees

Comments

@zhiyanliu
Copy link
Contributor

Hello,

According to the logic at [0], the request body handling will be ignored for all the requests with GET method currently, however some servers are supporting this kind of API/request like Elasticsearch. I did some searches on this, HTTP/1.1 does not seem to explicitly forbid this (correct me if I miss anything), and it seems some people have the similar requirements as well. I consider this is kind of needs on unclear defined standard, but from the perspective of a generic RESTful client, do you think it's a cent we can get to open the limitation to allow downstream developer decides API design or usage, and resty supports that in anyway?

Thanks!

[0] https://github.com/go-resty/resty/blob/master/client.go#L855

@jeevatkm jeevatkm self-assigned this Jul 6, 2017
@jeevatkm
Copy link
Member

jeevatkm commented Jul 6, 2017

I understand your view point. There is a lot of discussions around this topic on SO HTTP GET with body .

When I designed the resty I took approach of commonly followed. That's why I didn't added this support.

Elasticsearch does supports the POST request in parallel with GET request for the queries.

I see only option/solution to this issue is -

By default keep it disabled, provide an option to enable GET with payload. So developer can take the call.

Do you have any suggestion?

@zhiyanliu
Copy link
Contributor Author

Hello jeevatkm,

Thanks for your quick response.

Your design approach does make sense. I agree adding an option to do that seems like a good way to go currently, and disable the behavior by default to keep backward-compatible.

Would you like to take a look on my patch on this idea?

Thanks!

@jeevatkm
Copy link
Member

jeevatkm commented Jul 7, 2017

Thank you for the PR, I have just added my review.

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

Successfully merging a pull request may close this issue.

2 participants