Contract test for end-position-out-of-range case

Assert that an exception is not thrown from DataSource.open
if the DataSpec's start position is valid but its end position
extends beyond the end of the data.

HTTP based DataSource implementations have no good way of
knowing when this is the case, so it makes sense to make this
the required behaviour, rather than requiring an exception to
be thrown or allowing both.

There are also use cases where the caller may want to use the
end position as an upper bound, without knowing for sure how
long the content is. An example of this use case is wanting to
pre-cache the first N bytes of a stream. This implies that any
exception should be thrown after reading to the end of the
data, rather than preemptively in open.

Issue: #7326
PiperOrigin-RevId: 359063721
This commit is contained in:
olly 2021-02-23 17:27:15 +00:00 committed by marcbaechinger
parent e8b4986640
commit d8b5a1d7f4
10 changed files with 64 additions and 34 deletions

View File

@ -41,6 +41,10 @@ public final class DataSourceException extends IOException {
return false;
}
/**
* Indicates that the {@link DataSpec#position starting position} of the request was outside the
* bounds of the data.
*/
public static final int POSITION_OUT_OF_RANGE = 0;
/**
@ -56,5 +60,4 @@ public final class DataSourceException extends IOException {
public DataSourceException(int reason) {
this.reason = reason;
}
}

View File

@ -47,13 +47,13 @@ public final class ByteArrayDataSource extends BaseDataSource {
public long open(DataSpec dataSpec) throws IOException {
uri = dataSpec.uri;
transferInitializing(dataSpec);
readPosition = (int) dataSpec.position;
bytesRemaining = (int) ((dataSpec.length == C.LENGTH_UNSET)
? (data.length - dataSpec.position) : dataSpec.length);
if (bytesRemaining <= 0 || readPosition + bytesRemaining > data.length) {
throw new IOException("Unsatisfiable range: [" + readPosition + ", " + dataSpec.length
+ "], length: " + data.length);
if (dataSpec.position >= data.length) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
readPosition = (int) dataSpec.position;
bytesRemaining =
(int)
(dataSpec.length == C.LENGTH_UNSET ? data.length - dataSpec.position : dataSpec.length);
opened = true;
transferStarted(dataSpec);
return bytesRemaining;

View File

@ -69,7 +69,7 @@ public final class DataSchemeDataSource extends BaseDataSource {
}
endPosition =
dataSpec.length != C.LENGTH_UNSET ? (int) dataSpec.length + readPosition : data.length;
if (endPosition > data.length || readPosition > endPosition) {
if (readPosition >= endPosition) {
data = null;
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}

View File

@ -87,14 +87,6 @@ public final class ByteArrayDataSourceTest {
readTestData(TEST_DATA, TEST_DATA.length, 1, 1, 0, 1, true);
}
@Test
public void readWithInvalidLength() {
// Read more data than is available.
readTestData(TEST_DATA, 0, TEST_DATA.length + 1, 1, 0, 1, true);
// And with bound.
readTestData(TEST_DATA, 1, TEST_DATA.length, 1, 0, 1, true);
}
/**
* Tests reading from a {@link ByteArrayDataSource} with various parameters.
*

View File

@ -107,18 +107,6 @@ public final class DataSchemeDataSourceTest {
}
}
@Test
public void rangeExceedingResourceLengthRequest() throws IOException {
try {
// Try to open a range exceeding the resource's length.
schemeDataDataSource.open(
buildDataSpec(DATA_SCHEME_URI, /* position= */ 97, /* length= */ 11));
fail();
} catch (DataSourceException e) {
assertThat(e.reason).isEqualTo(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
@Test
public void incorrectScheme() {
try {

View File

@ -88,6 +88,11 @@ public class UdpDataSourceContractTest extends DataSourceContractTest {
@Override
public void dataSpecWithPositionAndLength_readExpectedRange() {}
@Test
@Ignore("UdpDataSource doesn't support DataSpec's position or length [internal: b/175856954]")
@Override
public void dataSpecWithEndPositionOutOfRange_readsToEnd() {}
/**
* Finds a free UDP port in the range of unreserved ports 50000-60000 that can be used from the
* test or throws an {@link IllegalStateException} if no port is available.

View File

@ -36,6 +36,7 @@ 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;
@ -174,6 +175,7 @@ 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);

View File

@ -212,6 +212,47 @@ public abstract class DataSourceContractTest {
}
}
@Test
public void dataSpecWithPositionEqualToLength_throwsPositionOutOfRangeException()
throws Exception {
// TODO: Implement this.
}
@Test
public void dataSpecWithEndPositionOutOfRange_readsToEnd() throws Exception {
ImmutableList<TestResource> resources = getTestResources();
Assertions.checkArgument(!resources.isEmpty(), "Must provide at least one test resource.");
for (int i = 0; i < resources.size(); i++) {
additionalFailureInfo.setInfo(getFailureLabel(resources, i));
TestResource resource = resources.get(i);
int resourceLength = resource.getExpectedBytes().length;
DataSource dataSource = createDataSource();
try {
long length =
dataSource.open(
new DataSpec.Builder()
.setUri(resource.getUri())
.setPosition(resourceLength - 1)
.setLength(2)
.build());
byte[] data = Util.readExactly(dataSource, /* length= */ 1);
// TODO: Decide what the allowed behavior should be for the next read, and assert it.
// The DataSpec.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(2);
byte[] expectedData =
Arrays.copyOfRange(resource.getExpectedBytes(), resourceLength - 1, resourceLength);
assertThat(data).isEqualTo(expectedData);
} finally {
dataSource.close();
}
additionalFailureInfo.setInfo(null);
}
}
/**
* {@link DataSpec#FLAG_ALLOW_GZIP} should either be ignored by {@link DataSource}
* implementations, or correctly handled (i.e. the data is decompressed before being returned from

View File

@ -117,11 +117,10 @@ public class FakeDataSource extends BaseDataSource {
throw new IOException("Data is empty: " + dataSpec.uri);
}
// If the source knows that the request is unsatisfiable then fail.
if (dataSpec.position >= totalLength || (dataSpec.length != C.LENGTH_UNSET
&& (dataSpec.position + dataSpec.length > totalLength))) {
if (dataSpec.position >= totalLength) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
// Scan through the segments, configuring them for the current read.
boolean findingCurrentSegmentIndex = true;
currentSegmentIndex = 0;

View File

@ -188,7 +188,7 @@ public final class FakeDataSourceTest {
}
@Test
public void testOpenSourceFailures() throws IOException {
public void testOpenSourceFailures() {
// Empty data.
FakeDataSource dataSource =
new FakeDataSource(new FakeDataSet().newData(uri.toString()).endData());
@ -217,10 +217,10 @@ public final class FakeDataSourceTest {
new FakeDataSource(
new FakeDataSet()
.newDefaultData()
.appendReadData(TestUtil.buildTestData(10))
.appendReadData(TestUtil.buildTestData(/* length= */ 10))
.endData());
try {
dataSource.open(new DataSpec(uri, 5, 10));
dataSource.open(new DataSpec(uri, /* position= */ 10, C.LENGTH_UNSET));
fail("IOException expected.");
} catch (IOException e) {
// Expected.