Skip to content

Propagate error for ListAll #713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 16, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions firebase-storage/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
# Unreleased (17.1.0)
# 19.0.1
- [fixed] `listAll()` now propagates the error messages if the List operation
was denied by a Security Rule.

# 19.0.0
- [changed] Added missing nullability annotations for better Kotlin interop.
- [internal] Removed ``@PublicApi` annotations as they are no longer enforced
and have no semantic meaning.

# 18.1.0
- [feature] Added `StorageReference.list()` and `StorageReference.listAll()`,
which allows developers to list the files and folders under the given
StorageReference.
- [changed] Added validation to `StorageReference.getDownloadUrl()` and
`StorageReference.getMetadata()` to return an error if the reference is the
root of the bucket.
- [changed] Added missing nullability annotations for better Kotlin interop.
- [internal] Removed ``@PublicApi` annotations as they are no longer enforced
and have no semantic meaning.

# 17.0.0
- [internal] Updated the SDK initialization process and removed usages of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -626,15 +626,20 @@ public Task<ListResult> listAll() {
new Continuation<ListResult, Task<Void>>() {
@Override
public Task<Void> then(@NonNull Task<ListResult> currentPage) {
ListResult result = currentPage.getResult();
prefixes.addAll(result.getPrefixes());
items.addAll(result.getItems());

if (result.getPageToken() != null) {
Task<ListResult> nextPage = listHelper(/* maxResults= */ null, result.getPageToken());
nextPage.continueWithTask(executor, this);
if (currentPage.isSuccessful()) {
ListResult result = currentPage.getResult();
prefixes.addAll(result.getPrefixes());
items.addAll(result.getItems());

if (result.getPageToken() != null) {
Task<ListResult> nextPage =
listHelper(/* maxResults= */ null, result.getPageToken());
nextPage.continueWithTask(executor, this);
} else {
pendingResult.setResult(new ListResult(prefixes, items, /* pageToken= */ null));
}
} else {
pendingResult.setResult(new ListResult(prefixes, items, /* pageToken= */ null));
pendingResult.setException(currentPage.getException());
}

return Tasks.forResult(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,18 @@ public void listResultsWithMultiplePages() throws InterruptedException {
TestUtil.verifyTaskStateChanges("listMultiplePages", task.getResult().toString());
}

@Test
public void listFailure() throws InterruptedException {
MockConnectionFactory factory =
NetworkLayerMock.ensureNetworkMock("listSinglePageFailed", true);
Task<StringBuilder> task = TestCommandHelper.listFiles(/* pageSize= */ 10, /* pageCount= */ 1);

TestUtil.await(task);

factory.verifyOldMock();
TestUtil.verifyTaskStateChanges("listSinglePageFailed", task.getResult().toString());
}

@Test
public void listAll() throws InterruptedException {
MockConnectionFactory factory = NetworkLayerMock.ensureNetworkMock("listAll", true);
Expand All @@ -113,4 +125,15 @@ public void listAll() throws InterruptedException {
factory.verifyOldMock();
TestUtil.verifyTaskStateChanges("listAll", task.getResult().toString());
}

@Test
public void listAllWithFailure() throws InterruptedException {
MockConnectionFactory factory = NetworkLayerMock.ensureNetworkMock("listAllFailed", true);
Task<StringBuilder> task = TestCommandHelper.listAllFiles();

TestUtil.await(task);

factory.verifyOldMock();
TestUtil.verifyTaskStateChanges("listAllFailed", task.getResult().toString());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@

<new>
Url:https://firebasestorage.googleapis.com/v0/b/fooeylistAllFailed_network.txt.appspot.com/o?prefix=largeDirectory%2F&delimiter=%2F
setRequestMethod:GET
setRequestProperty:X-Firebase-Storage-Version,Android/[No Gmscore]
setRequestProperty:x-firebase-gmpid,fooey
setRequestProperty:Content-Length,0
setUseCaches:false
setDoInput:true
getResponseCode:403
getHeaderFields:
null:[HTTP/1.1 403 Forbidden]
Access-Control-Allow-Origin:[*]
Access-Control-Expose-Headers:[Content-Range, X-Firebase-Storage-XSRF]
Alt-Svc:[quic=":443"; ma=2592000; v="46,43,39"]
Cache-Control:[private, max-age=0]
Content-Length:[106]
Content-Type:[application/json; charset=UTF-8]
Date:[Fri, 16 Aug 2019 17:51:19 GMT]
Expires:[Fri, 16 Aug 2019 17:51:19 GMT]
Server:[UploadServer]
X-Android-Received-Millis:[1565977885114]
X-Android-Response-Source:[NETWORK 403]
X-Android-Selected-Protocol:[http/1.1]
X-Android-Sent-Millis:[1565977884943]
X-Content-Type-Options:[nosniff]
X-GUploader-UploadID:[AEnB2Up1Fu4ZwtUsQe7mXrzuQ_S2KZvRwUttfOYcrQsZm4kuJcUJFvMzTRxkmydO8ErBV1bI_Ku_B7xqFBp5r4bt-hEu4a2JBQ]
getErrorStream:{ "error": { "code": 403, "message": "Permission denied. Could not perform this operation" }}
disconnect:
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

listAll:
onComplete:Success=false
done.
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@

<new>
Url:https://firebasestorage.googleapis.com/v0/b/fooey.appspot.com/o?prefix=smallDirectory%2F&delimiter=%2F&maxResults=10
setRequestMethod:GET
setRequestProperty:X-Firebase-Storage-Version,Android/[No Gmscore]
setRequestProperty:x-firebase-gmpid,fooey
setRequestProperty:Content-Length,0
setUseCaches:false
setDoInput:true
getResponseCode:403
getHeaderFields:
null:[HTTP/1.1 403 Forbidden]
Access-Control-Allow-Origin:[*]
Access-Control-Expose-Headers:[Content-Range, X-Firebase-Storage-XSRF]
Alt-Svc:[quic=":443"; ma=2592000; v="46,43,39"]
Cache-Control:[private, max-age=0]
Content-Length:[106]
Content-Type:[application/json; charset=UTF-8]
Date:[Fri, 16 Aug 2019 17:51:19 GMT]
Expires:[Fri, 16 Aug 2019 17:51:19 GMT]
Server:[UploadServer]
X-Android-Received-Millis:[1565977885114]
X-Android-Response-Source:[NETWORK 403]
X-Android-Selected-Protocol:[http/1.1]
X-Android-Sent-Millis:[1565977884943]
X-Content-Type-Options:[nosniff]
X-GUploader-UploadID:[AEnB2Up1Fu4ZwtUsQe7mXrzuQ_S2KZvRwUttfOYcrQsZm4kuJcUJFvMzTRxkmydO8ErBV1bI_Ku_B7xqFBp5r4bt-hEu4a2JBQ]
getErrorStream:{ "error": { "code": 403, "message": "Permission denied. Could not perform this operation" }}
disconnect:
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

list:
onComplete:Success=false
done.
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,12 @@ public static Task<StringBuilder> listFiles(int pageSize, int pageCount) {
public void onComplete(@NonNull Task<ListResult> task) {
++pagesReceived[0];

ListResult listResult = task.getResult();

builder.append("\nlist:");
builder.append("\n onComplete:Success=").append(task.isSuccessful());

if (task.isSuccessful()) {
ListResult listResult = task.getResult();

builder.append("\n Received Prefixes:");
for (StorageReference prefix : listResult.getPrefixes()) {
builder.append("\n ").append(prefix.getPath());
Expand All @@ -257,12 +257,14 @@ public void onComplete(@NonNull Task<ListResult> task) {
builder.append("\n ").append(item.getPath());
}
builder.append("\n Page Token:").append(listResult.getPageToken());
}

if (pagesReceived[0] == pageCount) {
result.setResult(builder);
if (pagesReceived[0] == pageCount) {
result.setResult(builder);
} else {
reference.list(pageSize, listResult.getPageToken()).addOnCompleteListener(this);
}
} else {
reference.list(pageSize, listResult.getPageToken()).addOnCompleteListener(this);
result.setResult(builder);
}
}
});
Expand All @@ -280,12 +282,11 @@ public static Task<StringBuilder> listAllFiles() {
listFiles.addOnCompleteListener(
executor,
task -> {
ListResult listResult = task.getResult();

builder.append("\nlistAll:");
builder.append("\n onComplete:Success=").append(task.isSuccessful());

if (task.isSuccessful()) {
ListResult listResult = task.getResult();
builder.append("\n Received Prefixes:");
for (StorageReference prefix : listResult.getPrefixes()) {
builder.append("\n ").append(prefix.getPath());
Expand Down
4 changes: 3 additions & 1 deletion firebase-storage/test-app/test-app.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,6 @@ dependencies {
// androidTest integration tests.
// ==========================================================================
ext.packageName = "com.example.storage"
apply from: '../../gradle/googleServices.gradle'
apply from: '../../gradle/googleServices.gradle'

apply plugin: "com.google.gms.google-services"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to add this line for the Test App to start. I hope it doesn't beak CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to call initializeApp because it does break CI.