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

@@ -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`,

extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java

@@ -37,6 +37,7 @@ import java.net.SocketTimeoutException;
 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 @@ import org.chromium.net.UrlResponseInfo;
 /**
  * DataSource without intermediate buffer based on Cronet API set using UrlRequest.
  *
- * <p>This class's methods are organized in the sequence of expected calls.
+ * <p>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 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
         cronetEngine
             .newUrlRequestBuilder(dataSpec.uri.toString(), urlRequestCallback, executor)
             .allowDirectExecutor();
+
     // Set the headers.
-    boolean isContentTypeHeaderSet = false;
+    Map<String, String> requestHeaders = new HashMap<>();
     if (defaultRequestProperties != null) {
-      for (Entry<String, String> headerEntry : defaultRequestProperties.getSnapshot().entrySet()) {
-        String key = headerEntry.getKey();
-        isContentTypeHeaderSet = isContentTypeHeaderSet || CONTENT_TYPE.equals(key);
-        requestBuilder.addHeader(key, headerEntry.getValue());
-      }
+      requestHeaders.putAll(defaultRequestProperties.getSnapshot());
     }
-    Map<String, String> requestPropertiesSnapshot = requestProperties.getSnapshot();
-    for (Entry<String, String> headerEntry : requestPropertiesSnapshot.entrySet()) {
+    requestHeaders.putAll(requestProperties.getSnapshot());
+    requestHeaders.putAll(dataSpec.httpRequestHeaders);
+
+    for (Entry<String, String> 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)) {

extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java

@@ -60,6 +60,7 @@ import org.chromium.net.impl.UrlResponseInfoImpl;
 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 final class CronetDataSourceTest {
             TEST_READ_TIMEOUT_MS,
             /* resetTimeoutOnRedirects= */ true,
             Clock.DEFAULT,
-            /* defaultRequestProperties= */ null,
+            defaultRequestProperties,
             /* handleSetCookieRequests= */ false);
     dataSourceUnderTest.addTransferListener(mockTransferListener);
     when(mockCronetEngine.newUrlRequestBuilder(
@@ -189,18 +196,59 @@ public final class CronetDataSourceTest {
   }
 
   @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<String, String> 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<String, String> 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();
   }
 
@@ -242,6 +290,26 @@ public final class CronetDataSourceTest {
   }
 
   @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();
     when(mockNetworkException.getErrorCode())

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'
 }
 

extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java

@@ -34,6 +34,7 @@ import java.io.IOException;
 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.RequestBody;
 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}.
+ *
+ * <p>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 @@ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource {
     if (cacheControl != null) {
       builder.cacheControl(cacheControl);
     }
+
+    Map<String, String> headers = new HashMap<>();
     if (defaultRequestProperties != null) {
-      for (Map.Entry<String, String> property : defaultRequestProperties.getSnapshot().entrySet()) {
-        builder.header(property.getKey(), property.getValue());
-      }
+      headers.putAll(defaultRequestProperties.getSnapshot());
     }
-    for (Map.Entry<String, String> property : requestProperties.getSnapshot().entrySet()) {
-      builder.header(property.getKey(), property.getValue());
+
+    headers.putAll(requestProperties.getSnapshot());
+    headers.putAll(dataSpec.httpRequestHeaders);
+
+    for (Map.Entry<String, String> 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) {

extensions/okhttp/src/test/AndroidManifest.xml

+<?xml version="1.0" encoding="utf-8"?>
+<!--
+  ~ 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.
+  -->
+
+<manifest package="com.google.android.exoplayer2.ext.okhttp.test">
+    <uses-sdk/>
+</manifest>

extensions/okhttp/src/test/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSourceTest.java

+/*
+ * 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<String, String> 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);
+  }
+}

library/core/src/main/java/com/google/android/exoplayer2/upstream/HttpDataSource.java

@@ -326,6 +326,13 @@ public interface HttpDataSource extends DataSource {
 
   }
 
+  /**
+   * Opens the source to read the specified data.
+   *
+   * <p>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 interface HttpDataSource extends DataSource {
    * Sets the value of a request header. The value will be used for subsequent connections
    * established by the source.
    *
+   * <p>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.
    */

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           |