From 260db03167419a7b94a8946f3388f1f47027cf66 Mon Sep 17 00:00:00 2001 From: christosts Date: Tue, 3 Sep 2019 11:48:08 +0100 Subject: [PATCH] Use DataSpec request params in HttpDataSource impls Include Dataspec.httpRequestHeaders in CronetDataSource, and OkHttpDataSource. Updated documentation of HttpDataSource.open() to suggest that it should set request headers (in decreasing priority) from (1) the passed DataSpec, (2) parameters set with setRequestProperty() and (3) default parameters set in the HttpDataSource.Factory. No mechanism has been put in place to enforce this. PiperOrigin-RevId: 266895574 --- RELEASENOTES.md | 4 + .../ext/cronet/CronetDataSource.java | 27 ++-- .../ext/cronet/CronetDataSourceTest.java | 82 ++++++++++- extensions/okhttp/build.gradle | 2 + .../ext/okhttp/OkHttpDataSource.java | 24 +++- .../okhttp/src/test/AndroidManifest.xml | 20 +++ .../ext/okhttp/OkHttpDataSourceTest.java | 128 ++++++++++++++++++ .../exoplayer2/upstream/HttpDataSource.java | 11 ++ .../upstream/DefaultHttpDataSourceTest.java | 6 +- 9 files changed, 276 insertions(+), 28 deletions(-) create mode 100644 extensions/okhttp/src/test/AndroidManifest.xml create mode 100644 extensions/okhttp/src/test/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSourceTest.java diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 03b3199e216..c90129e7ba1 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -2,6 +2,10 @@ ### dev-v2 (not yet released) ### +* Add `DataSpec.httpRequestHeaders` to set HTTP request headers when connecting + to an HTTP source. `DefaultHttpDataSource`, `CronetDataSource` and + `OkHttpDataSource` include headers set in the DataSpec when connecting to the + source. * Bypass sniffing in `ProgressiveMediaPeriod` in case a single extractor is provided ([#6325](https://github.com/google/ExoPlayer/issues/6325)). * Surface information provided by methods `isHardwareAccelerated`, diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 241b879b9a9..91bceb9aee6 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -37,6 +37,7 @@ import java.net.UnknownHostException; import java.nio.ByteBuffer; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -54,7 +55,9 @@ /** * DataSource without intermediate buffer based on Cronet API set using UrlRequest. * - *

This class's methods are organized in the sequence of expected calls. + *

Note: HTTP request headers will be set using all parameters passed via (in order of decreasing + * priority) the {@code dataSpec}, {@link #setRequestProperty} and the default parameters used to + * construct the instance. */ public class CronetDataSource extends BaseDataSource implements HttpDataSource { @@ -685,22 +688,22 @@ private UrlRequest.Builder buildRequestBuilder(DataSpec dataSpec) throws IOExcep cronetEngine .newUrlRequestBuilder(dataSpec.uri.toString(), urlRequestCallback, executor) .allowDirectExecutor(); + // Set the headers. - boolean isContentTypeHeaderSet = false; + Map requestHeaders = new HashMap<>(); if (defaultRequestProperties != null) { - for (Entry headerEntry : defaultRequestProperties.getSnapshot().entrySet()) { - String key = headerEntry.getKey(); - isContentTypeHeaderSet = isContentTypeHeaderSet || CONTENT_TYPE.equals(key); - requestBuilder.addHeader(key, headerEntry.getValue()); - } + requestHeaders.putAll(defaultRequestProperties.getSnapshot()); } - Map requestPropertiesSnapshot = requestProperties.getSnapshot(); - for (Entry headerEntry : requestPropertiesSnapshot.entrySet()) { + requestHeaders.putAll(requestProperties.getSnapshot()); + requestHeaders.putAll(dataSpec.httpRequestHeaders); + + for (Entry headerEntry : requestHeaders.entrySet()) { String key = headerEntry.getKey(); - isContentTypeHeaderSet = isContentTypeHeaderSet || CONTENT_TYPE.equals(key); - requestBuilder.addHeader(key, headerEntry.getValue()); + String value = headerEntry.getValue(); + requestBuilder.addHeader(key, value); } - if (dataSpec.httpBody != null && !isContentTypeHeaderSet) { + + if (dataSpec.httpBody != null && !requestHeaders.containsKey(CONTENT_TYPE)) { throw new IOException("HTTP request with non-empty body must set Content-Type"); } if (dataSpec.isFlagSet(DataSpec.FLAG_ALLOW_ICY_METADATA)) { diff --git a/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java b/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java index 2be369bad94..47f6fa7d2f6 100644 --- a/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java +++ b/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java @@ -60,6 +60,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -95,6 +96,12 @@ public final class CronetDataSourceTest { @Before public void setUp() { MockitoAnnotations.initMocks(this); + + HttpDataSource.RequestProperties defaultRequestProperties = + new HttpDataSource.RequestProperties(); + defaultRequestProperties.set("defaultHeader1", "defaultValue1"); + defaultRequestProperties.set("defaultHeader2", "defaultValue2"); + dataSourceUnderTest = new CronetDataSource( mockCronetEngine, @@ -103,7 +110,7 @@ public void setUp() { TEST_READ_TIMEOUT_MS, /* resetTimeoutOnRedirects= */ true, Clock.DEFAULT, - /* defaultRequestProperties= */ null, + defaultRequestProperties, /* handleSetCookieRequests= */ false); dataSourceUnderTest.addTransferListener(mockTransferListener); when(mockCronetEngine.newUrlRequestBuilder( @@ -189,18 +196,59 @@ public void testRequestStartCalled() throws HttpDataSourceException { } @Test - public void testRequestHeadersSet() throws HttpDataSourceException { + public void testRequestSetsRangeHeader() throws HttpDataSourceException { testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null); mockResponseStartSuccess(); - dataSourceUnderTest.setRequestProperty("firstHeader", "firstValue"); - dataSourceUnderTest.setRequestProperty("secondHeader", "secondValue"); - dataSourceUnderTest.open(testDataSpec); // The header value to add is current position to current position + length - 1. verify(mockUrlRequestBuilder).addHeader("Range", "bytes=1000-5999"); - verify(mockUrlRequestBuilder).addHeader("firstHeader", "firstValue"); - verify(mockUrlRequestBuilder).addHeader("secondHeader", "secondValue"); + } + + @Test + public void testRequestHeadersSet() throws HttpDataSourceException { + + Map headersSet = new HashMap<>(); + doAnswer( + (invocation) -> { + String key = invocation.getArgument(0); + String value = invocation.getArgument(1); + headersSet.put(key, value); + return null; + }) + .when(mockUrlRequestBuilder) + .addHeader(ArgumentMatchers.anyString(), ArgumentMatchers.anyString()); + + dataSourceUnderTest.setRequestProperty("defaultHeader2", "dataSourceOverridesDefault"); + dataSourceUnderTest.setRequestProperty("dataSourceHeader1", "dataSourceValue1"); + dataSourceUnderTest.setRequestProperty("dataSourceHeader2", "dataSourceValue2"); + + Map dataSpecRequestProperties = new HashMap<>(); + dataSpecRequestProperties.put("defaultHeader3", "dataSpecOverridesAll"); + dataSpecRequestProperties.put("dataSourceHeader2", "dataSpecOverridesDataSource"); + dataSpecRequestProperties.put("dataSpecHeader1", "dataSpecValue1"); + testDataSpec = + new DataSpec( + /* uri= */ Uri.parse(TEST_URL), + /* httpMethod= */ DataSpec.HTTP_METHOD_GET, + /* httpBody= */ null, + /* absoluteStreamPosition= */ 1000, + /* position= */ 1000, + /* length= */ 5000, + /* key= */ null, + /* flags= */ 0, + dataSpecRequestProperties); + mockResponseStartSuccess(); + + dataSourceUnderTest.open(testDataSpec); + + assertThat(headersSet.get("defaultHeader1")).isEqualTo("defaultValue1"); + assertThat(headersSet.get("defaultHeader2")).isEqualTo("dataSourceOverridesDefault"); + assertThat(headersSet.get("defaultHeader3")).isEqualTo("dataSpecOverridesAll"); + assertThat(headersSet.get("dataSourceHeader1")).isEqualTo("dataSourceValue1"); + assertThat(headersSet.get("dataSourceHeader2")).isEqualTo("dataSpecOverridesDataSource"); + assertThat(headersSet.get("dataSpecHeader1")).isEqualTo("dataSpecValue1"); + verify(mockUrlRequest).start(); } @@ -241,6 +289,26 @@ public void testRequestOpenFail() { } } + @Test + public void open_ifBodyIsSetWithoutContentTypeHeader_fails() { + testDataSpec = + new DataSpec( + /* uri= */ Uri.parse(TEST_URL), + /* postBody= */ new byte[1024], + /* absoluteStreamPosition= */ 200, + /* position= */ 200, + /* length= */ 1024, + /* key= */ "key", + /* flags= */ 0); + + try { + dataSourceUnderTest.open(testDataSpec); + fail(); + } catch (IOException expected) { + // Expected + } + } + @Test public void testRequestOpenFailDueToDnsFailure() { mockResponseStartFailure(); diff --git a/extensions/okhttp/build.gradle b/extensions/okhttp/build.gradle index b0d9fb0187e..7f638e3c236 100644 --- a/extensions/okhttp/build.gradle +++ b/extensions/okhttp/build.gradle @@ -35,6 +35,8 @@ dependencies { implementation project(modulePrefix + 'library-core') implementation 'androidx.annotation:annotation:' + androidxAnnotationVersion compileOnly 'org.checkerframework:checker-qual:' + checkerframeworkVersion + testImplementation project(modulePrefix + 'testutils') + testImplementation 'org.robolectric:robolectric:' + robolectricVersion api 'com.squareup.okhttp3:okhttp:3.12.1' } diff --git a/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java b/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java index 1a6a67b140e..2a8f6715770 100644 --- a/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java +++ b/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java @@ -34,6 +34,7 @@ import java.io.InputStream; import java.io.InterruptedIOException; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import okhttp3.CacheControl; @@ -45,7 +46,13 @@ import okhttp3.Response; import okhttp3.ResponseBody; -/** An {@link HttpDataSource} that delegates to Square's {@link Call.Factory}. */ +/** + * An {@link HttpDataSource} that delegates to Square's {@link Call.Factory}. + * + *

Note: HTTP request headers will be set using all parameters passed via (in order of decreasing + * priority) the {@code dataSpec}, {@link #setRequestProperty} and the default parameters used to + * construct the instance. + */ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource { static { @@ -328,14 +335,19 @@ private Request makeRequest(DataSpec dataSpec) throws HttpDataSourceException { if (cacheControl != null) { builder.cacheControl(cacheControl); } + + Map headers = new HashMap<>(); if (defaultRequestProperties != null) { - for (Map.Entry property : defaultRequestProperties.getSnapshot().entrySet()) { - builder.header(property.getKey(), property.getValue()); - } + headers.putAll(defaultRequestProperties.getSnapshot()); } - for (Map.Entry property : requestProperties.getSnapshot().entrySet()) { - builder.header(property.getKey(), property.getValue()); + + headers.putAll(requestProperties.getSnapshot()); + headers.putAll(dataSpec.httpRequestHeaders); + + for (Map.Entry header : headers.entrySet()) { + builder.header(header.getKey(), header.getValue()); } + if (!(position == 0 && length == C.LENGTH_UNSET)) { String rangeRequest = "bytes=" + position + "-"; if (length != C.LENGTH_UNSET) { diff --git a/extensions/okhttp/src/test/AndroidManifest.xml b/extensions/okhttp/src/test/AndroidManifest.xml new file mode 100644 index 00000000000..3efff43f6b9 --- /dev/null +++ b/extensions/okhttp/src/test/AndroidManifest.xml @@ -0,0 +1,20 @@ + + + + + + diff --git a/extensions/okhttp/src/test/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSourceTest.java b/extensions/okhttp/src/test/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSourceTest.java new file mode 100644 index 00000000000..dab62b06e88 --- /dev/null +++ b/extensions/okhttp/src/test/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSourceTest.java @@ -0,0 +1,128 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.android.exoplayer2.ext.okhttp; + +import static com.google.common.truth.Truth.assertThat; + +import android.net.Uri; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.exoplayer2.upstream.DataSpec; +import com.google.android.exoplayer2.upstream.HttpDataSource; +import java.util.HashMap; +import java.util.Map; +import okhttp3.Call; +import okhttp3.MediaType; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.Response; +import okhttp3.ResponseBody; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentMatchers; +import org.mockito.Mockito; + +/** Unit tests for {@link OkHttpDataSource}. */ +@RunWith(AndroidJUnit4.class) +public class OkHttpDataSourceTest { + + @Test + public void open_setsCorrectHeaders() throws HttpDataSource.HttpDataSourceException { + /* + * This test will set HTTP default request parameters (1) in the OkHttpDataSource, (2) via + * OkHttpDataSource.setRequestProperty() and (3) in the DataSpec instance according to the table + * below. Values wrapped in '*' are the ones that should be set in the connection request. + * + * +-----------------------+---+-----+-----+-----+-----+-----+ + * | | Header Key | + * +-----------------------+---+-----+-----+-----+-----+-----+ + * | Location | 0 | 1 | 2 | 3 | 4 | 5 | + * +-----------------------+---+-----+-----+-----+-----+-----+ + * | Default |*Y*| Y | Y | | | | + * | OkHttpDataSource | | *Y* | Y | Y | *Y* | | + * | DataSpec | | | *Y* | *Y* | | *Y* | + * +-----------------------+---+-----+-----+-----+-----+-----+ + */ + + String defaultValue = "Default"; + String okHttpDataSourceValue = "OkHttpDataSource"; + String dataSpecValue = "DataSpec"; + + // 1. Default properties on OkHttpDataSource + HttpDataSource.RequestProperties defaultRequestProperties = + new HttpDataSource.RequestProperties(); + defaultRequestProperties.set("0", defaultValue); + defaultRequestProperties.set("1", defaultValue); + defaultRequestProperties.set("2", defaultValue); + + Call.Factory mockCallFactory = Mockito.mock(Call.Factory.class); + OkHttpDataSource okHttpDataSource = + new OkHttpDataSource( + mockCallFactory, "testAgent", /* cacheControl= */ null, defaultRequestProperties); + + // 2. Additional properties set with setRequestProperty(). + okHttpDataSource.setRequestProperty("1", okHttpDataSourceValue); + okHttpDataSource.setRequestProperty("2", okHttpDataSourceValue); + okHttpDataSource.setRequestProperty("3", okHttpDataSourceValue); + okHttpDataSource.setRequestProperty("4", okHttpDataSourceValue); + + // 3. DataSpec properties + Map dataSpecRequestProperties = new HashMap<>(); + dataSpecRequestProperties.put("2", dataSpecValue); + dataSpecRequestProperties.put("3", dataSpecValue); + dataSpecRequestProperties.put("5", dataSpecValue); + + DataSpec dataSpec = + new DataSpec( + /* uri= */ Uri.parse("http://www.google.com"), + /* httpMethod= */ 1, + /* httpBody= */ null, + /* absoluteStreamPosition= */ 1000, + /* position= */ 1000, + /* length= */ 5000, + /* key= */ null, + /* flags= */ 0, + dataSpecRequestProperties); + + Mockito.doAnswer( + invocation -> { + Request request = invocation.getArgument(0); + assertThat(request.header("0")).isEqualTo(defaultValue); + assertThat(request.header("1")).isEqualTo(okHttpDataSourceValue); + assertThat(request.header("2")).isEqualTo(dataSpecValue); + assertThat(request.header("3")).isEqualTo(dataSpecValue); + assertThat(request.header("4")).isEqualTo(okHttpDataSourceValue); + assertThat(request.header("5")).isEqualTo(dataSpecValue); + + // return a Call whose .execute() will return a mock Response + Call returnValue = Mockito.mock(Call.class); + Mockito.doReturn( + new Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(200) + .message("OK") + .body(ResponseBody.create(MediaType.parse("text/plain"), "")) + .build()) + .when(returnValue) + .execute(); + return returnValue; + }) + .when(mockCallFactory) + .newCall(ArgumentMatchers.any()); + okHttpDataSource.open(dataSpec); + } +} diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/HttpDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/HttpDataSource.java index 4d3aca570eb..63cad8786bb 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/HttpDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/HttpDataSource.java @@ -326,6 +326,13 @@ public InvalidResponseCodeException( } + /** + * Opens the source to read the specified data. + * + *

Note: {@link HttpDataSource} implementations are advised to set request headers passed via + * (in order of decreasing priority) the {@code dataSpec}, {@link #setRequestProperty} and the + * default parameters set in the {@link Factory}. + */ @Override long open(DataSpec dataSpec) throws HttpDataSourceException; @@ -339,6 +346,10 @@ public InvalidResponseCodeException( * Sets the value of a request header. The value will be used for subsequent connections * established by the source. * + *

Note: If the same header is set as a default parameter in the {@link Factory}, then the + * header value set with this method should be preferred when connecting with the data source. See + * {@link #open}. + * * @param name The name of the header field. * @param value The value of the field. */ diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSourceTest.java index 7c2b63c941b..d724369d93f 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSourceTest.java @@ -39,9 +39,9 @@ public class DefaultHttpDataSourceTest { public void open_withSpecifiedRequestParameters_usesCorrectParameters() throws IOException { /* - * This test will set HTTP request parameters in the HttpDataSourceFactory (default), - * in the DefaultHttpDataSource instance and in the DataSpec instance according to the table - * below. Values wrapped in '*' are the ones that should be set in the connection request. + * This test will set HTTP default request parameters (1) in the DefaultHttpDataSource, (2) via + * DefaultHttpDataSource.setRequestProperty() and (3) in the DataSpec instance according to the + * table below. Values wrapped in '*' are the ones that should be set in the connection request. * * +-----------------------+---+-----+-----+-----+-----+-----+ * | | Header Key |