From 262e3d43b63083ad9f87f3f332827b2ae37a30ff Mon Sep 17 00:00:00 2001 From: jwilson Date: Wed, 1 Jan 2014 10:53:54 -0500 Subject: [PATCH] Don't let transports rewrite the request. Now that we're preparing Content-Length and Transfer-Encoding before passing a request to the HTTP engine, we have no strict need to rewrite requests in the transport. The nice upside of this change is that the transport becomes even less obvious when it's in place. --- .../java/com/squareup/okhttp/Request.java | 3 +- .../okhttp/internal/http/HttpEngine.java | 1 - .../okhttp/internal/http/HttpTransport.java | 4 -- .../okhttp/internal/http/SpdyTransport.java | 46 +++++++++++-------- .../okhttp/internal/http/Transport.java | 7 --- .../okhttp/internal/http/HeadersTest.java | 35 ++++++++++---- 6 files changed, 54 insertions(+), 42 deletions(-) diff --git a/okhttp/src/main/java/com/squareup/okhttp/Request.java b/okhttp/src/main/java/com/squareup/okhttp/Request.java index 6eb8d8393..b392196ad 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/Request.java +++ b/okhttp/src/main/java/com/squareup/okhttp/Request.java @@ -444,8 +444,9 @@ public final class Request { return this; } - public void removeHeader(String name) { + public Builder removeHeader(String name) { headers.removeAll(name); + return this; } public Builder setChunked() { diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java index 7120b299a..841e98a23 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java @@ -182,7 +182,6 @@ public class HttpEngine { } transport = (Transport) connection.newTransport(this); - request = transport.prepareRequest(request); // Create a request body if we don't have one already. We'll already have // one if we're retrying a failed POST. diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java index 1db10cc54..b7543102b 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java @@ -61,10 +61,6 @@ public final class HttpTransport implements Transport { this.socketIn = inputStream; } - public Request prepareRequest(Request request) { - return request; - } - @Override public OutputStream createRequestBody(Request request) throws IOException { long contentLength = request.getContentLength(); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java index 978764bd0..266512558 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java @@ -42,19 +42,6 @@ public final class SpdyTransport implements Transport { this.spdyConnection = spdyConnection; } - @Override public Request prepareRequest(Request request) { - Request.Builder builder = request.newBuilder() - .header(":method", request.method()) - .header(":scheme", request.url().getProtocol()) - .header(":path", RequestLine.requestPath(request.url())) - .header(":version", RequestLine.version(httpEngine.connection.getHttpMinorVersion())) - .header(":host", HttpEngine.hostHeader(request.url())); - - builder.removeHeader("Transfer-Encoding"); // SPDY doesn't use chunked encoding. - - return builder.build(); - } - @Override public OutputStream createRequestBody(Request request) throws IOException { // TODO: if bufferRequestBody is set, we must buffer the whole request writeRequestHeaders(request); @@ -67,8 +54,9 @@ public final class SpdyTransport implements Transport { httpEngine.writingRequestHeaders(); boolean hasRequestBody = httpEngine.hasRequestBody(); boolean hasResponseBody = true; + String version = RequestLine.version(httpEngine.connection.getHttpMinorVersion()); stream = spdyConnection.newStream( - writeNameValueBlock(request.getHeaders()), hasRequestBody, hasResponseBody); + writeNameValueBlock(request, version), hasRequestBody, hasResponseBody); stream.setReadTimeout(httpEngine.client.getReadTimeout()); } @@ -89,12 +77,23 @@ public final class SpdyTransport implements Transport { * Names are all lower case. No names are repeated. If any name has multiple * values, they are concatenated using "\0" as a delimiter. */ - public static List writeNameValueBlock(Headers headers) { + public static List writeNameValueBlock(Request request, String version) { + List result = new ArrayList(request.headerCount() + 10); + result.add(":method"); + result.add(request.method()); + result.add(":path"); + result.add(RequestLine.requestPath(request.url())); + result.add(":version"); + result.add(version); + result.add(":host"); + result.add(HttpEngine.hostHeader(request.url())); + result.add(":scheme"); + result.add(request.url().getProtocol()); + Set names = new LinkedHashSet(); - List result = new ArrayList(headers.length() * 2); - for (int i = 0; i < headers.length(); i++) { - String name = headers.getFieldName(i).toLowerCase(Locale.US); - String value = headers.getValue(i); + for (int i = 0; i < request.headerCount(); i++) { + String name = request.headerName(i).toLowerCase(Locale.US); + String value = request.headerValue(i); // Drop headers that are forbidden when layering HTTP over SPDY. if (name.equals("connection") @@ -105,6 +104,15 @@ public final class SpdyTransport implements Transport { continue; } + // They shouldn't be set, but if they are, drop them. We've already written them! + if (name.equals(":method") + || name.equals(":path") + || name.equals(":version") + || name.equals(":host") + || name.equals(":scheme")) { + continue; + } + // If we haven't seen this name before, add the pair to the end of the list... if (names.add(name)) { result.add(name); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java index cbe12fe35..59e986f1a 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java @@ -24,13 +24,6 @@ import java.io.OutputStream; import java.net.CacheRequest; interface Transport { - /** - * Returns a request equivalent to {@code request} but with transport-specific - * changes. For example, this may set a Transfer-Encoding header if it is - * required but not present for the current transport. - */ - Request prepareRequest(Request request); - /** * Returns an output stream where the request body can be written. The * returned stream will of one of two types: diff --git a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HeadersTest.java b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HeadersTest.java index 1dce00985..dc08b652a 100644 --- a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HeadersTest.java +++ b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HeadersTest.java @@ -53,13 +53,20 @@ public final class HeadersTest { } @Test public void toNameValueBlock() { - Headers.Builder builder = new Headers.Builder(); - builder.add("cache-control", "no-cache, no-store"); - builder.add("set-cookie", "Cookie1"); - builder.add("set-cookie", "Cookie2"); - builder.add(":status", "200 OK"); - List nameValueBlock = SpdyTransport.writeNameValueBlock(builder.build()); + Request request = new Request.Builder() + .url("http://square.com/") + .header("cache-control", "no-cache, no-store") + .addHeader("set-cookie", "Cookie1") + .addHeader("set-cookie", "Cookie2") + .header(":status", "200 OK") + .build(); + List nameValueBlock = SpdyTransport.writeNameValueBlock(request, "HTTP/1.1"); List expected = Arrays.asList( + ":method", "GET", + ":path", "/", + ":version", "HTTP/1.1", + ":host", "square.com", + ":scheme", "http", "cache-control", "no-cache, no-store", "set-cookie", "Cookie1\u0000Cookie2", ":status", "200 OK"); @@ -67,9 +74,17 @@ public final class HeadersTest { } @Test public void toNameValueBlockDropsForbiddenHeaders() { - Headers.Builder builder = new Headers.Builder(); - builder.add("Connection", "close"); - builder.add("Transfer-Encoding", "chunked"); - assertEquals(Arrays.asList(), SpdyTransport.writeNameValueBlock(builder.build())); + Request request = new Request.Builder() + .url("http://square.com/") + .header("Connection", "close") + .header("Transfer-Encoding", "chunked") + .build(); + List expected = Arrays.asList( + ":method", "GET", + ":path", "/", + ":version", "HTTP/1.1", + ":host", "square.com", + ":scheme", "http"); + assertEquals(expected, SpdyTransport.writeNameValueBlock(request, "HTTP/1.1")); } }