Tighten DataSource contract test assertions

Assert that opening the DataSource at the end of the resource
results in only RESULT_END_OF_INPUT being read.

open() and read() are still permitted to throw, although this
permissiveness will be removed in subsequent commits.

PiperOrigin-RevId: 362905314

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

@@ -72,48 +72,61 @@ public final class ContentDataSource extends BaseDataSource {
       if (assetFileDescriptor == null) {
         throw new FileNotFoundException("Could not open file descriptor for: " + uri);
       }
+
+      long assetFileDescriptorLength = assetFileDescriptor.getLength();
       FileInputStream inputStream = new FileInputStream(assetFileDescriptor.getFileDescriptor());
       this.inputStream = inputStream;
 
-      long assetStartOffset = assetFileDescriptor.getStartOffset();
+      // We can't rely only on the "skipped < dataSpec.position" check below to detect whether the
-      long skipped = inputStream.skip(assetStartOffset + dataSpec.position) - assetStartOffset;
+      // position is beyond the end of the asset being read. This is because the file may contain
-      if (skipped != dataSpec.position) {
+      // multiple assets, and there's nothing to prevent InputStream.skip() from succeeding by
-        // We expect the skip to be satisfied in full. If it isn't then we're probably trying to
+      // skipping into the data of the next asset. Hence we also need to check against the asset
-        // skip beyond the end of the data.
+      // length explicitly, which is guaranteed to be set unless the asset extends to the end of the
+      // file.
+      if (assetFileDescriptorLength != AssetFileDescriptor.UNKNOWN_LENGTH
+          && dataSpec.position > assetFileDescriptorLength) {
         throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
       }
-      if (dataSpec.length != C.LENGTH_UNSET) {
+      long assetFileDescriptorOffset = assetFileDescriptor.getStartOffset();
-        bytesRemaining = dataSpec.length;
+      long skipped =
-      } else {
+          inputStream.skip(assetFileDescriptorOffset + dataSpec.position)
-        long assetFileDescriptorLength = assetFileDescriptor.getLength();
+              - assetFileDescriptorOffset;
-        if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
+      if (skipped != dataSpec.position) {
-          // The asset must extend to the end of the file. If FileInputStream.getChannel().size()
+        // We expect the skip to be satisfied in full. If it isn't then we're probably trying to
-          // returns 0 then the remaining length cannot be determined.
+        // read beyond the end of the last resource in the file.
-          FileChannel channel = inputStream.getChannel();
+        throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
-          long channelSize = channel.size();
+      }
-          if (channelSize == 0) {
+      if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
-            bytesRemaining = C.LENGTH_UNSET;
+        // The asset must extend to the end of the file. We can try and resolve the length with
-          } else {
+        // FileInputStream.getChannel().size().
-            bytesRemaining = channelSize - channel.position();
+        FileChannel channel = inputStream.getChannel();
-            if (bytesRemaining < 0) {
+        long channelSize = channel.size();
-              throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
+        if (channelSize == 0) {
-            }
+          bytesRemaining = C.LENGTH_UNSET;
-          }
         } else {
-          bytesRemaining = assetFileDescriptorLength - skipped;
+          bytesRemaining = channelSize - channel.position();
           if (bytesRemaining < 0) {
+            // The skip above was satisfied in full, but skipped beyond the end of the file.
             throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
           }
         }
+      } else {
+        bytesRemaining = assetFileDescriptorLength - skipped;
+        if (bytesRemaining < 0) {
+          throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
+        }
       }
     } catch (IOException e) {
       throw new ContentDataSourceException(e);
     }
 
+    if (dataSpec.length != C.LENGTH_UNSET) {
+      bytesRemaining =
+          bytesRemaining == C.LENGTH_UNSET ? dataSpec.length : min(bytesRemaining, dataSpec.length);
+    }
     opened = true;
     transferStarted(dataSpec);
-
+    return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining;
-    return bytesRemaining;
   }
 
   @Override

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

@@ -31,6 +31,7 @@ import java.io.EOFException;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.channels.FileChannel;
 
 /**
  * A {@link DataSource} for reading a raw resource inside the APK.
@@ -149,6 +150,7 @@ public final class RawResourceDataSource extends BaseDataSource {
     long assetFileDescriptorLength = assetFileDescriptor.getLength();
     FileInputStream inputStream = new FileInputStream(assetFileDescriptor.getFileDescriptor());
     this.inputStream = inputStream;
+
     try {
       // We can't rely only on the "skipped < dataSpec.position" check below to detect whether the
       // position is beyond the end of the resource being read. This is because the file will
@@ -160,31 +162,45 @@ public final class RawResourceDataSource extends BaseDataSource {
           && dataSpec.position > assetFileDescriptorLength) {
         throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
       }
-      inputStream.skip(assetFileDescriptor.getStartOffset());
+      long assetFileDescriptorOffset = assetFileDescriptor.getStartOffset();
-      long skipped = inputStream.skip(dataSpec.position);
+      long skipped =
-      if (skipped < dataSpec.position) {
+          inputStream.skip(assetFileDescriptorOffset + dataSpec.position)
+              - assetFileDescriptorOffset;
+      if (skipped != dataSpec.position) {
         // We expect the skip to be satisfied in full. If it isn't then we're probably trying to
         // read beyond the end of the last resource in the file.
         throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
       }
+      if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
+        // The asset must extend to the end of the file. We can try and resolve the length with
+        // FileInputStream.getChannel().size().
+        FileChannel channel = inputStream.getChannel();
+        if (channel.size() == 0) {
+          bytesRemaining = C.LENGTH_UNSET;
+        } else {
+          bytesRemaining = channel.size() - channel.position();
+          if (bytesRemaining < 0) {
+            // The skip above was satisfied in full, but skipped beyond the end of the file.
+            throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
+          }
+        }
+      } else {
+        bytesRemaining = assetFileDescriptorLength - skipped;
+        if (bytesRemaining < 0) {
+          throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
+        }
+      }
     } catch (IOException e) {
       throw new RawResourceDataSourceException(e);
     }
 
     if (dataSpec.length != C.LENGTH_UNSET) {
-      bytesRemaining = dataSpec.length;
-    } else {
-      // If the length is UNKNOWN_LENGTH then the asset extends to the end of the file.
       bytesRemaining =
-          assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH
+          bytesRemaining == C.LENGTH_UNSET ? dataSpec.length : min(bytesRemaining, dataSpec.length);
-              ? C.LENGTH_UNSET
-              : (assetFileDescriptorLength - dataSpec.position);
     }
-
     opened = true;
     transferStarted(dataSpec);
-
+    return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining;
-    return bytesRemaining;
   }
 
   @Override

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

@@ -388,8 +388,9 @@ public final class CacheDataSource implements DataSource {
 
   @Nullable private Uri actualUri;
   @Nullable private DataSpec requestDataSpec;
+  @Nullable private DataSpec currentDataSpec;
   @Nullable private DataSource currentDataSource;
-  private boolean currentDataSpecLengthUnset;
+  private long currentDataSourceBytesRead;
   private long readPosition;
   private long bytesRemaining;
   @Nullable private CacheSpan currentHoleSpan;
@@ -565,19 +566,27 @@ public final class CacheDataSource implements DataSource {
         notifyCacheIgnored(reason);
       }
 
-      if (dataSpec.length != C.LENGTH_UNSET || currentRequestIgnoresCache) {
+      if (currentRequestIgnoresCache) {
-        bytesRemaining = dataSpec.length;
+        bytesRemaining = C.LENGTH_UNSET;
       } else {
         bytesRemaining = ContentMetadata.getContentLength(cache.getContentMetadata(key));
         if (bytesRemaining != C.LENGTH_UNSET) {
           bytesRemaining -= dataSpec.position;
-          if (bytesRemaining <= 0) {
+          if (bytesRemaining < 0) {
             throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
           }
         }
       }
-      openNextSource(requestDataSpec, false);
+      if (dataSpec.length != C.LENGTH_UNSET) {
-      return bytesRemaining;
+        bytesRemaining =
+            bytesRemaining == C.LENGTH_UNSET
+                ? dataSpec.length
+                : min(bytesRemaining, dataSpec.length);
+      }
+      if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) {
+        openNextSource(requestDataSpec, false);
+      }
+      return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining;
     } catch (Throwable e) {
       handleBeforeThrow(e);
       throw e;
@@ -587,6 +596,7 @@ public final class CacheDataSource implements DataSource {
   @Override
   public int read(byte[] buffer, int offset, int readLength) throws IOException {
     DataSpec requestDataSpec = checkNotNull(this.requestDataSpec);
+    DataSpec currentDataSpec = checkNotNull(this.currentDataSpec);
     if (readLength == 0) {
       return 0;
     }
@@ -603,10 +613,16 @@ public final class CacheDataSource implements DataSource {
           totalCachedBytesRead += bytesRead;
         }
         readPosition += bytesRead;
+        currentDataSourceBytesRead += bytesRead;
         if (bytesRemaining != C.LENGTH_UNSET) {
           bytesRemaining -= bytesRead;
         }
-      } else if (currentDataSpecLengthUnset) {
+      } else if (isReadingFromUpstream()
+          && (currentDataSpec.length == C.LENGTH_UNSET
+              || currentDataSourceBytesRead < currentDataSpec.length)) {
+        // We've encountered RESULT_END_OF_INPUT from the upstream DataSource at a position not
+        // imposed by the current DataSpec. This must mean that we've reached the end of the
+        // resource.
         setNoBytesRemainingAndMaybeStoreLength(castNonNull(requestDataSpec.key));
       } else if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) {
         closeCurrentSource();
@@ -615,7 +631,16 @@ public final class CacheDataSource implements DataSource {
       }
       return bytesRead;
     } catch (IOException e) {
-      if (currentDataSpecLengthUnset && DataSourceException.isCausedByPositionOutOfRange(e)) {
+      // TODO: This is not correct, because position-out-of-range exceptions should only be thrown
+      // if the requested position is more than one byte beyond the end of the resource. Conversely,
+      // this code is assuming that a position-out-of-range exception indicates the requested
+      // position is exactly one byte beyond the end of the resource, which is not a case for which
+      // this type of exception should be thrown. This exception handling may be required for
+      // interop with current HttpDataSource implementations that do (incorrectly) throw a
+      // position-out-of-range exception at this position. It should be removed when the
+      // HttpDataSource implementations are fixed.
+      if (currentDataSpec.length == C.LENGTH_UNSET
+          && DataSourceException.isCausedByPositionOutOfRange(e)) {
         setNoBytesRemainingAndMaybeStoreLength(castNonNull(requestDataSpec.key));
         return C.RESULT_END_OF_INPUT;
       }
@@ -760,12 +785,13 @@ public final class CacheDataSource implements DataSource {
       currentHoleSpan = nextSpan;
     }
     currentDataSource = nextDataSource;
-    currentDataSpecLengthUnset = nextDataSpec.length == C.LENGTH_UNSET;
+    currentDataSpec = nextDataSpec;
+    currentDataSourceBytesRead = 0;
     long resolvedLength = nextDataSource.open(nextDataSpec);
 
     // Update bytesRemaining, actualUri and (if writing to cache) the cache metadata.
     ContentMetadataMutations mutations = new ContentMetadataMutations();
-    if (currentDataSpecLengthUnset && resolvedLength != C.LENGTH_UNSET) {
+    if (nextDataSpec.length == C.LENGTH_UNSET && resolvedLength != C.LENGTH_UNSET) {
       bytesRemaining = resolvedLength;
       ContentMetadataMutations.setContentLength(mutations, readPosition + bytesRemaining);
     }
@@ -816,8 +842,8 @@ public final class CacheDataSource implements DataSource {
     try {
       currentDataSource.close();
     } finally {
+      currentDataSpec = null;
       currentDataSource = null;
-      currentDataSpecLengthUnset = false;
       if (currentHoleSpan != null) {
         cache.releaseHoleSpan(currentHoleSpan);
         currentHoleSpan = null;

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

@@ -23,6 +23,7 @@ import com.google.android.exoplayer2.upstream.DataSpec;
 import com.google.android.exoplayer2.util.PriorityTaskManager;
 import com.google.android.exoplayer2.util.PriorityTaskManager.PriorityTooLowException;
 import com.google.android.exoplayer2.util.Util;
+import java.io.EOFException;
 import java.io.IOException;
 import java.io.InterruptedIOException;
 
@@ -142,6 +143,14 @@ public final class CacheWriter {
         nextPosition += readBlockToCache(nextPosition, nextRequestLength);
       }
     }
+
+    // TODO: Remove allowShortContent parameter, this code block, and return the number of bytes
+    // cached. The caller can then check whether fewer bytes were cached than were requested.
+    if (!allowShortContent
+        && dataSpec.length != C.LENGTH_UNSET
+        && nextPosition != dataSpec.position + dataSpec.length) {
+      throw new EOFException();
+    }
   }
 
   /**

library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java

@@ -154,7 +154,7 @@ public final class CacheDataSourceTest {
     try {
       cacheDataSource =
           createCacheDataSource(/* setReadException= */ false, /* unknownLength= */ false);
-      cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, defaultCacheKey));
+      cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, defaultCacheKey));
       fail();
     } catch (IOException e) {
       // Expected.
@@ -204,7 +204,7 @@ public final class CacheDataSourceTest {
       cacheDataSource =
           createCacheDataSource(
               /* setReadException= */ false, /* unknownLength= */ false, cacheKeyFactory);
-      cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, customCacheKey));
+      cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, customCacheKey));
       fail();
     } catch (IOException e) {
       // Expected.
@@ -257,7 +257,7 @@ public final class CacheDataSourceTest {
       cacheDataSource =
           createCacheDataSource(
               /* setReadException= */ false, /* unknownLength= */ false, cacheKeyFactory);
-      cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, customCacheKey));
+      cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, customCacheKey));
       fail();
     } catch (IOException e) {
       // Expected.

library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheWriterTest.java

@@ -27,16 +27,15 @@ import com.google.android.exoplayer2.testutil.FailOnCloseDataSink;
 import com.google.android.exoplayer2.testutil.FakeDataSet;
 import com.google.android.exoplayer2.testutil.FakeDataSource;
 import com.google.android.exoplayer2.testutil.TestUtil;
-import com.google.android.exoplayer2.upstream.DataSourceException;
 import com.google.android.exoplayer2.upstream.DataSpec;
 import com.google.android.exoplayer2.upstream.FileDataSource;
 import com.google.android.exoplayer2.util.Util;
+import java.io.EOFException;
 import java.io.File;
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.MockitoAnnotations;
@@ -175,7 +174,6 @@ public final class CacheWriterTest {
     assertCachedData(cache, fakeDataSet);
   }
 
-  @Ignore("Currently broken. See https://github.com/google/ExoPlayer/issues/7326.")
   @Test
   public void cacheLengthExceedsActualDataLength() throws Exception {
     FakeDataSet fakeDataSet = new FakeDataSet().setRandomData("test_data", 100);
@@ -206,18 +204,14 @@ public final class CacheWriterTest {
     Uri testUri = Uri.parse("test_data");
     DataSpec dataSpec = new DataSpec(testUri, /* position= */ 0, /* length= */ 1000);
 
-    IOException exception =
+    CacheWriter cacheWriter =
-        assertThrows(
+        new CacheWriter(
-            IOException.class,
+            new CacheDataSource(cache, dataSource),
-            () ->
+            dataSpec,
-                new CacheWriter(
+            /* allowShortContent= */ false,
-                        new CacheDataSource(cache, dataSource),
+            /* temporaryBuffer= */ null,
-                        dataSpec,
+            /* progressListener= */ null);
-                        /* allowShortContent= */ false,
+    assertThrows(EOFException.class, cacheWriter::cache);
-                        /* temporaryBuffer= */ null,
-                        /* progressListener= */ null)
-                    .cache());
-    assertThat(DataSourceException.isCausedByPositionOutOfRange(exception)).isTrue();
   }
 
   @Test

testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java

@@ -233,16 +233,22 @@ public abstract class DataSourceContractTest {
           new DataSpec.Builder().setUri(resource.getUri()).setPosition(resourceLength).build();
       try {
         long length = dataSource.open(dataSpec);
-        // TODO: For any cases excluded from the requirement that a position-out-of-range exception
+        // The DataSource.open() contract requires the returned length to equal the length in the
-        // is thrown, decide what the allowed behavior should be for the first read, and assert it.
+        // DataSpec if set. This is true even though the DataSource implementation may know that
-
+        // fewer bytes will be read in this case.
         if (length != C.LENGTH_UNSET) {
           assertThat(length).isEqualTo(0);
         }
+
+        try {
+          byte[] data =
+              unboundedReadsAreIndefinite() ? Util.EMPTY_BYTE_ARRAY : Util.readToEnd(dataSource);
+          assertThat(data).isEmpty();
+        } catch (IOException e) {
+          // TODO: Remove this catch and require that implementations do not throw.
+        }
       } catch (IOException e) {
-        // TODO: Decide whether to assert that a position-out-of-range exception must or must not be
+        // TODO: Remove this catch and require that implementations do not throw.
-        // thrown (with exclusions if necessary), rather than just asserting it must be a
-        // position-out-of-range exception *if* one is thrown at all.
         assertThat(DataSourceException.isCausedByPositionOutOfRange(e)).isTrue();
       } finally {
         dataSource.close();
@@ -270,17 +276,20 @@ public abstract class DataSourceContractTest {
               .build();
       try {
         long length = dataSource.open(dataSpec);
-        // TODO: For any cases excluded from the requirement that a position-out-of-range exception
-        // is thrown, decide what the allowed behavior should be for the first read, and assert it.
-
         // The DataSource.open() contract requires the returned length to equal the length in the
         // DataSpec if set. This is true even though the DataSource implementation may know that
         // fewer bytes will be read in this case.
         assertThat(length).isEqualTo(1);
+
+        try {
+          byte[] data =
+              unboundedReadsAreIndefinite() ? Util.EMPTY_BYTE_ARRAY : Util.readToEnd(dataSource);
+          assertThat(data).isEmpty();
+        } catch (IOException e) {
+          // TODO: Remove this catch and require that implementations do not throw.
+        }
       } catch (IOException e) {
-        // TODO: Decide whether to assert that a position-out-of-range exception must or must not be
+        // TODO: Remove this catch and require that implementations do not throw.
-        // thrown (with exclusions if necessary), rather than just asserting it must be a
-        // position-out-of-range exception *if* one is thrown at all.
         assertThat(DataSourceException.isCausedByPositionOutOfRange(e)).isTrue();
       } finally {
         dataSource.close();

testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeDataSource.java

@@ -117,7 +117,7 @@ public class FakeDataSource extends BaseDataSource {
       throw new IOException("Data is empty: " + dataSpec.uri);
     }
 
-    if (dataSpec.position >= totalLength) {
+    if (dataSpec.position > totalLength) {
       throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
     }
 

testutils/src/test/java/com/google/android/exoplayer2/testutil/FakeDataSourceTest.java

@@ -211,22 +211,6 @@ public final class FakeDataSourceTest {
     } finally {
       dataSource.close();
     }
-
-    // DataSpec out of bounds.
-    dataSource =
-        new FakeDataSource(
-            new FakeDataSet()
-                .newDefaultData()
-                .appendReadData(TestUtil.buildTestData(/* length= */ 10))
-                .endData());
-    try {
-      dataSource.open(new DataSpec(uri, /* position= */ 10, C.LENGTH_UNSET));
-      fail("IOException expected.");
-    } catch (IOException e) {
-      // Expected.
-    } finally {
-      dataSource.close();
-    }
   }
 
   private static void assertBuffer(byte[] expected) {