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

Allow body in custom HTTP methods #3038

Closed
wants to merge 1 commit into from

Conversation

zmarkan
Copy link

@zmarkan zmarkan commented Dec 12, 2016

This should fix the issues expressed here: #229

@zmarkan zmarkan changed the title Allow to explicitly allow body permissions in custom HTTP methods Explicitly allow body in custom HTTP methods Dec 12, 2016
@swankjesse
Copy link
Collaborator

I don’t think I understand the utility of the boolean parameter. Shouldn’t we just relax our rules and permit the custom method?

@zmarkan
Copy link
Author

zmarkan commented Dec 13, 2016

Ideally, yes - that'd be more in line with the spec as I understand it, and that's how I've done it in a custom internal fork.

My thinking was that the boolean parameter would keep the current behaviour and error handling exactly the same, just in case anyone relied on this current behaviour (unlikely but not possible).

I'd be happy to update the PR to remove the permitsRequestBody method though.

@swankjesse
Copy link
Collaborator

Yeah, let's do that.

@zmarkan zmarkan changed the title Explicitly allow body in custom HTTP methods Allow body in custom HTTP methods Dec 13, 2016
@zmarkan
Copy link
Author

zmarkan commented Dec 13, 2016

I updated the PR, I believe this is now better conforming to the HTTP spec.

@zmarkan
Copy link
Author

zmarkan commented Dec 22, 2016

Any news on this?

@@ -54,7 +54,6 @@
import okhttp3.internal.Version;
import okhttp3.internal.http.HttpDate;
import okhttp3.internal.http.HttpHeaders;
import okhttp3.internal.http.HttpMethod;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind reverting the changes to OkHttpUrlConnection? This is likely to be backwards-incompatible

Copy link
Collaborator

Choose a reason for hiding this comment

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

(particularly the part where it looks like we now always send a content-type, even on GET requests)

public static boolean redirectsWithBody(String method) {
return method.equals("PROPFIND"); // (WebDAV) redirects should also maintain the request body
}

public static boolean redirectsToGet(String method) {
// All requests but PROPFIND should redirect to a GET request.
return !method.equals("PROPFIND");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change this?

@swankjesse
Copy link
Collaborator

I’m getting ready to cut OkHttp 3.6. Wanna revert your unnecessary changes?

@JeroenBer
Copy link

Please integrate this fix because I really need to use PROPFIND with OkHttp.

@EhsanMashhadi
Copy link

Any plan for this pull request? I need it.

EhsanMashhadi added a commit to Carrene/okhttp that referenced this pull request Dec 10, 2017
EhsanMashhadi added a commit to Carrene/okhttp that referenced this pull request Dec 10, 2017
@yschimke
Copy link
Collaborator

I think #3777 covers this

@yschimke yschimke closed this Jan 16, 2018
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.

5 participants