Complies with rfc7231 about statuses 307 and 308

[yschimke]

Fix for #2874 #3111 and #4229

https://tools.ietf.org/html/rfc7231#page-58

6.4.7. 307 Temporary Redirect

The 307 (Temporary Redirect) status code indicates that the target
resource resides temporarily under a different URI and the user agent
MUST NOT change the request method if it performs an automatic
redirection to that URI. Since the redirection can change over time,
the client ought to continue using the original effective request URI
for future requests.

The user agent MAY
use the Location field value for automatic redirection.

https://tools.ietf.org/html/rfc7238

The user agent MAY use the Location field
value for automatic redirection.

Note: This status code is similar to 301 (Moved Permanently)
([RFC7231], Section 6.4.2), except that it does not allow changing
the request method from POST to GET.

[yschimke]

@swankjesse Any thoughts on how to implement the interceptor? The logic sits between interceptors, and networkInterceptors. So seems like we would need to tag requests in interceptors such that the RetryAndFollowupInterceptor doesn't retry for these?

Or put this as a field like client.retryOnConnectionFailure.

[swankjesse]

I was thinking something much more basic. Maybe the interceptor would rewrite 308s to a bogus code like 399 to get the previous behavior.

[yschimke]

That feels tough to explain if you see that code elsewhere like another interceptor.

[yschimke]

The more I think about this, the more I think we should just follow the correct logic and leave reverting to the old behaviour to users affected by this. But with heavy notification and a suggestion for how users could revert to the previous behaviour, e.g. implement your own interceptor that rewrites the response code.

Under what scenarios would mobile clients want the old behaviour? And shouldn't we encourage clients and servers move towards the correct defined behaviour? An API server is likely to only have okhttp as the only mobile client transport.

So definitely not a client flag. A new public API as a workaround interceptor feels like a bad public API also.

[swankjesse]

Oh I was thinking the interceptor would be a code sample you'd gave to copy-paste.

[yschimke]

Oh I was thinking the interceptor would be a code sample you'd gave to copy-paste.

Perfect

[yschimke]

I hope noone ever uses this

  class RevertInterceptor implements Interceptor {
    @NotNull @Override public Response intercept(@NotNull Chain chain) throws IOException {
      Response response = chain.proceed(chain.request());

      return remapResponse(response);
    }

    @NotNull private Response remapResponse(Response response) {
      if (response.request().method().equals("POST") && (response.code() == HTTP_TEMP_REDIRECT || response.code() == HTTP_PERM_REDIRECT)) {
        // special response code to indicate custom rules
        return response.newBuilder().code(response.code() + 10).build();
      }

      return response;
    }
  }

[yschimke]

@swankjesse Is this for 4.6?

[swankjesse]

nit: we don’t typically use @NotNull because everything is not null unless otherwise specified

[swankjesse]

oh this should probably be checking for not GET and not HEAD, otherwise PUT isn‘t consistent!