Skip to content

Commit 26cbf94

Browse files
icbakerivanbuper
icbaker
authored and
ivanbuper
committed
DataSourceContractTest: Tighten assertions around 'not found' URIs
This change: 1. Updates `DataSourceContractTest` to allow multiple "not found" resources, and to include additional info (e.g. headers) on them. 2. Updates the contract test to assert that `DataSource.getUri()` returns the expected (non-null) value for "not found" resources between the failed `open()` call and a subsequent `close()` call. The `DataSource` is 'open' at this point (since it needs to be 'closed' later), so `getUri()` must return non-null. * This change also fixes some implementations to comply with this contract. It also renames some imprecisely named `opened` booleans that **don't** track whether the `DataSource` is open or not. 3. Updates the contract test assertions to enforce that `DataSource.getResponseHeaders()` returns any headers associated with the 'not found' resource. 4. Configures `HttpDataSourceTestEnv` to provide both 404 and "server not found" resources, with the former having expected headers associated with it. PiperOrigin-RevId: 689316121 (cherry picked from commit 4a406be)
1 parent 08e55d8 commit 26cbf94

File tree

12 files changed

+224
-90
lines changed

12 files changed

+224
-90
lines changed

‎RELEASENOTES.md

+8
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,14 @@ This release includes the following changes since the
2121
resolved URI (as documented). Where this is different to the requested
2222
URI, tests can indicate this using the new
2323
`DataSourceContractTest.TestResource.Builder.setResolvedUri()` method.
24+
* `DataSourceContractTest`: Assert that `DataSource.getUri()` and
25+
`getResponseHeaders()` return their 'open' value after a failed call to
26+
`open()` (due to a 'not found' resource) and before a subsequent
27+
`close()` call.
28+
* Overriding `DataSourceContractTest.getNotFoundResources()` allows
29+
test sub-classes to provide multiple 'not found' resources, and to
30+
provide any expected headers too. This allows to distinguish between
31+
HTTP 404 (with headers) and "server not found" (no headers).
2432
* Text:
2533
* Fix CEA-608 subtitles in H.264 MPEG-TS streams not being output (this
2634
was broken in `1.5.0-alpha01` by

‎libraries/datasource/src/androidTest/java/androidx/media3/datasource/DefaultHttpDataSourceContractTest.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,11 @@
1515
*/
1616
package androidx.media3.datasource;
1717

18-
import android.net.Uri;
1918
import androidx.media3.test.utils.DataSourceContractTest;
2019
import androidx.media3.test.utils.HttpDataSourceTestEnv;
2120
import androidx.test.ext.junit.runners.AndroidJUnit4;
2221
import com.google.common.collect.ImmutableList;
22+
import java.util.List;
2323
import org.junit.Rule;
2424
import org.junit.runner.RunWith;
2525

@@ -40,7 +40,7 @@ protected ImmutableList<TestResource> getTestResources() {
4040
}
4141

4242
@Override
43-
protected Uri getNotFoundUri() {
44-
return Uri.parse(httpDataSourceTestEnv.getNonexistentUrl());
43+
protected List<TestResource> getNotFoundResources() {
44+
return httpDataSourceTestEnv.getNotFoundResources();
4545
}
4646
}

‎libraries/datasource/src/androidTest/java/androidx/media3/datasource/HttpEngineDataSourceContractTest.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@
1515
*/
1616
package androidx.media3.datasource;
1717

18-
import android.net.Uri;
1918
import android.net.http.HttpEngine;
2019
import androidx.media3.test.utils.DataSourceContractTest;
2120
import androidx.media3.test.utils.HttpDataSourceTestEnv;
2221
import androidx.test.core.app.ApplicationProvider;
2322
import androidx.test.ext.junit.runners.AndroidJUnit4;
2423
import com.google.common.collect.ImmutableList;
24+
import java.util.List;
2525
import java.util.concurrent.ExecutorService;
2626
import java.util.concurrent.Executors;
2727
import org.junit.After;
@@ -53,7 +53,7 @@ protected ImmutableList<TestResource> getTestResources() {
5353
}
5454

5555
@Override
56-
protected Uri getNotFoundUri() {
57-
return Uri.parse(httpDataSourceTestEnv.getNonexistentUrl());
56+
protected List<TestResource> getNotFoundResources() {
57+
return httpDataSourceTestEnv.getNotFoundResources();
5858
}
5959
}

‎libraries/datasource/src/main/java/androidx/media3/datasource/DefaultHttpDataSource.java

+14-7
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ public DefaultHttpDataSource createDataSource() {
261261
@Nullable private DataSpec dataSpec;
262262
@Nullable private HttpURLConnection connection;
263263
@Nullable private InputStream inputStream;
264-
private boolean opened;
264+
private boolean transferStarted;
265265
private int responseCode;
266266
private long bytesToRead;
267267
private long bytesRead;
@@ -296,7 +296,13 @@ private DefaultHttpDataSource(
296296
@Override
297297
@Nullable
298298
public Uri getUri() {
299-
return connection == null ? null : Uri.parse(connection.getURL().toString());
299+
if (connection != null) {
300+
return Uri.parse(connection.getURL().toString());
301+
} else if (dataSpec != null) {
302+
return dataSpec.uri;
303+
} else {
304+
return null;
305+
}
300306
}
301307

302308
@UnstableApi
@@ -372,7 +378,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
372378
long documentSize =
373379
HttpUtil.getDocumentSize(connection.getHeaderField(HttpHeaders.CONTENT_RANGE));
374380
if (dataSpec.position == documentSize) {
375-
opened = true;
381+
transferStarted = true;
376382
transferStarted(dataSpec);
377383
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : 0;
378384
}
@@ -442,7 +448,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
442448
HttpDataSourceException.TYPE_OPEN);
443449
}
444450

445-
opened = true;
451+
transferStarted = true;
446452
transferStarted(dataSpec);
447453

448454
try {
@@ -493,10 +499,12 @@ public void close() throws HttpDataSourceException {
493499
} finally {
494500
inputStream = null;
495501
closeConnectionQuietly();
496-
if (opened) {
497-
opened = false;
502+
if (transferStarted) {
503+
transferStarted = false;
498504
transferEnded();
499505
}
506+
connection = null;
507+
dataSpec = null;
500508
}
501509
}
502510

@@ -787,7 +795,6 @@ private void closeConnectionQuietly() {
787795
} catch (Exception e) {
788796
Log.e(TAG, "Unexpected error while disconnecting", e);
789797
}
790-
connection = null;
791798
}
792799
}
793800

‎libraries/datasource/src/main/java/androidx/media3/datasource/HttpEngineDataSource.java

+15-12
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,6 @@
3838
import androidx.media3.common.util.ConditionVariable;
3939
import androidx.media3.common.util.UnstableApi;
4040
import androidx.media3.common.util.Util;
41-
import androidx.media3.datasource.HttpDataSource.CleartextNotPermittedException;
42-
import androidx.media3.datasource.HttpDataSource.HttpDataSourceException;
43-
import androidx.media3.datasource.HttpDataSource.InvalidResponseCodeException;
4441
import com.google.common.base.Ascii;
4542
import com.google.common.base.Predicate;
4643
import com.google.common.net.HttpHeaders;
@@ -341,7 +338,7 @@ public OpenException(
341338
private final boolean keepPostFor302Redirects;
342339

343340
// Accessed by the calling thread only.
344-
private boolean opened;
341+
private boolean transferStarted;
345342
private long bytesRemaining;
346343

347344
@Nullable private DataSpec currentDataSpec;
@@ -430,14 +427,20 @@ public Map<String, List<String>> getResponseHeaders() {
430427
@Override
431428
@Nullable
432429
public Uri getUri() {
433-
return responseInfo == null ? null : Uri.parse(responseInfo.getUrl());
430+
if (responseInfo != null) {
431+
return Uri.parse(responseInfo.getUrl());
432+
} else if (currentDataSpec != null) {
433+
return currentDataSpec.uri;
434+
} else {
435+
return null;
436+
}
434437
}
435438

436439
@UnstableApi
437440
@Override
438441
public long open(DataSpec dataSpec) throws HttpDataSourceException {
439442
Assertions.checkNotNull(dataSpec);
440-
Assertions.checkState(!opened);
443+
Assertions.checkState(!transferStarted);
441444

442445
operation.close();
443446
resetConnectTimeout();
@@ -499,7 +502,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
499502
long documentSize =
500503
HttpUtil.getDocumentSize(getFirstHeader(responseHeaders, HttpHeaders.CONTENT_RANGE));
501504
if (dataSpec.position == documentSize) {
502-
opened = true;
505+
transferStarted = true;
503506
transferStarted(dataSpec);
504507
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : 0;
505508
}
@@ -558,7 +561,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
558561
bytesRemaining = dataSpec.length;
559562
}
560563

561-
opened = true;
564+
transferStarted = true;
562565
transferStarted(dataSpec);
563566

564567
skipFully(bytesToSkip, dataSpec);
@@ -568,7 +571,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
568571
@UnstableApi
569572
@Override
570573
public int read(byte[] buffer, int offset, int length) throws HttpDataSourceException {
571-
Assertions.checkState(opened);
574+
Assertions.checkState(transferStarted);
572575

573576
if (length == 0) {
574577
return 0;
@@ -639,7 +642,7 @@ public int read(byte[] buffer, int offset, int length) throws HttpDataSourceExce
639642
*/
640643
@UnstableApi
641644
public int read(ByteBuffer buffer) throws HttpDataSourceException {
642-
Assertions.checkState(opened);
645+
Assertions.checkState(transferStarted);
643646

644647
if (!buffer.isDirect()) {
645648
throw new IllegalArgumentException("Passed buffer is not a direct ByteBuffer");
@@ -696,8 +699,8 @@ public synchronized void close() {
696699
responseInfo = null;
697700
exception = null;
698701
finished = false;
699-
if (opened) {
700-
opened = false;
702+
if (transferStarted) {
703+
transferStarted = false;
701704
transferEnded();
702705
}
703706
}

‎libraries/datasource_cronet/src/androidTest/java/androidx/media3/datasource/cronet/CronetDataSourceContractTest.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
*/
1616
package androidx.media3.datasource.cronet;
1717

18-
import android.net.Uri;
1918
import androidx.media3.datasource.DataSource;
2019
import androidx.media3.test.utils.DataSourceContractTest;
2120
import androidx.media3.test.utils.HttpDataSourceTestEnv;
@@ -66,7 +65,7 @@ protected ImmutableList<TestResource> getTestResources() {
6665
}
6766

6867
@Override
69-
protected Uri getNotFoundUri() {
70-
return Uri.parse(httpDataSourceTestEnv.getNonexistentUrl());
68+
protected List<TestResource> getNotFoundResources() {
69+
return httpDataSourceTestEnv.getNotFoundResources();
7170
}
7271
}

‎libraries/datasource_cronet/src/main/java/androidx/media3/datasource/cronet/CronetDataSource.java

+15-9
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ public OpenException(
463463
private final boolean keepPostFor302Redirects;
464464

465465
// Accessed by the calling thread only.
466-
private boolean opened;
466+
private boolean transferStarted;
467467
private long bytesRemaining;
468468

469469
// Written from the calling thread only. currentUrlRequest.start() calls ensure writes are visible
@@ -555,14 +555,20 @@ public Map<String, List<String>> getResponseHeaders() {
555555
@Override
556556
@Nullable
557557
public Uri getUri() {
558-
return responseInfo == null ? null : Uri.parse(responseInfo.getUrl());
558+
if (responseInfo != null) {
559+
return Uri.parse(responseInfo.getUrl());
560+
} else if (currentDataSpec != null) {
561+
return currentDataSpec.uri;
562+
} else {
563+
return null;
564+
}
559565
}
560566

561567
@UnstableApi
562568
@Override
563569
public long open(DataSpec dataSpec) throws HttpDataSourceException {
564570
Assertions.checkNotNull(dataSpec);
565-
Assertions.checkState(!opened);
571+
Assertions.checkState(!transferStarted);
566572

567573
operation.close();
568574
resetConnectTimeout();
@@ -624,7 +630,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
624630
long documentSize =
625631
HttpUtil.getDocumentSize(getFirstHeader(responseHeaders, HttpHeaders.CONTENT_RANGE));
626632
if (dataSpec.position == documentSize) {
627-
opened = true;
633+
transferStarted = true;
628634
transferStarted(dataSpec);
629635
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : 0;
630636
}
@@ -683,7 +689,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
683689
bytesRemaining = dataSpec.length;
684690
}
685691

686-
opened = true;
692+
transferStarted = true;
687693
transferStarted(dataSpec);
688694

689695
skipFully(bytesToSkip, dataSpec);
@@ -693,7 +699,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
693699
@UnstableApi
694700
@Override
695701
public int read(byte[] buffer, int offset, int length) throws HttpDataSourceException {
696-
Assertions.checkState(opened);
702+
Assertions.checkState(transferStarted);
697703

698704
if (length == 0) {
699705
return 0;
@@ -764,7 +770,7 @@ public int read(byte[] buffer, int offset, int length) throws HttpDataSourceExce
764770
*/
765771
@UnstableApi
766772
public int read(ByteBuffer buffer) throws HttpDataSourceException {
767-
Assertions.checkState(opened);
773+
Assertions.checkState(transferStarted);
768774

769775
if (!buffer.isDirect()) {
770776
throw new IllegalArgumentException("Passed buffer is not a direct ByteBuffer");
@@ -818,8 +824,8 @@ public synchronized void close() {
818824
responseInfo = null;
819825
exception = null;
820826
finished = false;
821-
if (opened) {
822-
opened = false;
827+
if (transferStarted) {
828+
transferStarted = false;
823829
transferEnded();
824830
}
825831
}

‎libraries/datasource_okhttp/src/main/java/androidx/media3/datasource/okhttp/OkHttpDataSource.java

+14-7
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ public OkHttpDataSource createDataSource() {
192192
@Nullable private DataSpec dataSpec;
193193
@Nullable private Response response;
194194
@Nullable private InputStream responseByteStream;
195-
private boolean opened;
195+
private boolean connectionEstablished;
196196
private long bytesToRead;
197197
private long bytesRead;
198198

@@ -215,7 +215,13 @@ private OkHttpDataSource(
215215
@Override
216216
@Nullable
217217
public Uri getUri() {
218-
return response == null ? null : Uri.parse(response.request().url().toString());
218+
if (response != null) {
219+
return Uri.parse(response.request().url().toString());
220+
} else if (dataSpec != null) {
221+
return dataSpec.uri;
222+
} else {
223+
return null;
224+
}
219225
}
220226

221227
@UnstableApi
@@ -281,7 +287,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
281287
long documentSize =
282288
HttpUtil.getDocumentSize(response.headers().get(HttpHeaders.CONTENT_RANGE));
283289
if (dataSpec.position == documentSize) {
284-
opened = true;
290+
connectionEstablished = true;
285291
transferStarted(dataSpec);
286292
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : 0;
287293
}
@@ -325,7 +331,7 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
325331
bytesToRead = contentLength != -1 ? (contentLength - bytesToSkip) : C.LENGTH_UNSET;
326332
}
327333

328-
opened = true;
334+
connectionEstablished = true;
329335
transferStarted(dataSpec);
330336

331337
try {
@@ -352,11 +358,13 @@ public int read(byte[] buffer, int offset, int length) throws HttpDataSourceExce
352358
@UnstableApi
353359
@Override
354360
public void close() {
355-
if (opened) {
356-
opened = false;
361+
if (connectionEstablished) {
362+
connectionEstablished = false;
357363
transferEnded();
358364
closeConnectionQuietly();
359365
}
366+
response = null;
367+
dataSpec = null;
360368
}
361369

362370
/** Establishes a connection. */
@@ -524,7 +532,6 @@ private int readInternal(byte[] buffer, int offset, int readLength) throws IOExc
524532
private void closeConnectionQuietly() {
525533
if (response != null) {
526534
Assertions.checkNotNull(response.body()).close();
527-
response = null;
528535
}
529536
responseByteStream = null;
530537
}

0 commit comments

Comments
 (0)