Skip to content

Commit 9797b89

Browse files
markjschreibermarkjschreiber
authored
fix: Improve error handling and visibility in S3DirectoryStream (#404)
* fix: Improve error handling and visibility in S3DirectoryStream
1 parent 58faa50 commit 9797b89

File tree

4 files changed

+108
-36
lines changed

4 files changed

+108
-36
lines changed

‎src/integrationTest/java/software/amazon/nio/spi/s3/FilesDirectoryStreamTest.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,16 @@
55

66
package software.amazon.nio.spi.s3;
77

8-
import org.junit.jupiter.api.DisplayName;
9-
import org.junit.jupiter.api.Nested;
10-
import org.junit.jupiter.api.Test;
8+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
9+
import static software.amazon.nio.spi.s3.Containers.localStackConnectionEndpoint;
1110

11+
import java.io.IOException;
1212
import java.net.URI;
1313
import java.nio.file.Files;
14-
import java.nio.file.NoSuchFileException;
1514
import java.nio.file.Paths;
16-
17-
import static org.assertj.core.api.Assertions.assertThatThrownBy;
18-
import static software.amazon.nio.spi.s3.Containers.localStackConnectionEndpoint;
15+
import org.junit.jupiter.api.DisplayName;
16+
import org.junit.jupiter.api.Nested;
17+
import org.junit.jupiter.api.Test;
1918

2019
@DisplayName("Files$newDirectoryStream()")
2120
public class FilesDirectoryStreamTest {
@@ -30,7 +29,7 @@ class DirectoryDoesNotExist {
3029
public void whenBucketNotFound() {
3130
final var path = Paths.get(URI.create(localStackConnectionEndpoint() + "/does-not-exist/some-directory"));
3231
assertThatThrownBy(() -> Files.newDirectoryStream(path, p -> true))
33-
.isInstanceOf(NoSuchFileException.class);
32+
.isInstanceOf(IOException.class);
3433
}
3534
}
3635

‎src/main/java/software/amazon/nio/spi/s3/S3DirectoryStream.java‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,10 @@
1414
import java.nio.file.Path;
1515
import java.util.Iterator;
1616
import org.checkerframework.checker.nullness.qual.NonNull;
17+
import org.reactivestreams.Publisher;
1718
import org.slf4j.Logger;
1819
import org.slf4j.LoggerFactory;
20+
import software.amazon.awssdk.core.exception.SdkException;
1921
import software.amazon.awssdk.services.s3.model.CommonPrefix;
2022
import software.amazon.awssdk.services.s3.model.S3Object;
2123
import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Publisher;
@@ -53,20 +55,24 @@ public void close() {
5355
* @param finalDirName the directory name that will be streamed.
5456
* @param listObjectsV2Publisher the publisher that returns objects and common prefixes that are iterated on.
5557
* @return an iterator for {@code Path}s constructed from the {@code ListObjectsV2Publisher}s responses.
58+
* @throws SdkException if there is an error with S3 access. This is an unchecked Exception
5659
*/
5760
private Iterator<Path> pathIteratorForPublisher(
5861
final DirectoryStream.Filter<? super Path> filter,
5962
final FileSystem fs, String finalDirName,
60-
final ListObjectsV2Publisher listObjectsV2Publisher) {
61-
final var prefixPublisher = listObjectsV2Publisher.commonPrefixes().map(CommonPrefix::prefix);
62-
final var keysPublisher = listObjectsV2Publisher.contents().map(S3Object::key);
63+
final ListObjectsV2Publisher listObjectsV2Publisher) throws SdkException {
64+
65+
final Publisher<String> prefixPublisher =
66+
listObjectsV2Publisher.commonPrefixes().map(CommonPrefix::prefix);
67+
final Publisher<String> keysPublisher =
68+
listObjectsV2Publisher.contents().map(S3Object::key);
6369

6470
return Flowable.concat(prefixPublisher, keysPublisher)
65-
.map(fs::getPath)
66-
.filter(path -> !isEqualToParent(finalDirName, path)) // including the parent will induce loops
67-
.filter(path -> tryAccept(filter, path))
68-
.blockingStream()
69-
.iterator();
71+
.map(fs::getPath)
72+
.filter(path -> !isEqualToParent(finalDirName, path)) // including the parent will induce loops
73+
.filter(path -> tryAccept(filter, path))
74+
.blockingIterable()
75+
.iterator();
7076
}
7177

7278

‎src/main/java/software/amazon/nio/spi/s3/S3FileSystemProvider.java‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
import java.util.Objects;
4747
import java.util.Set;
4848
import java.util.concurrent.CompletableFuture;
49-
import java.util.concurrent.CompletionException;
5049
import java.util.concurrent.ExecutionException;
5150
import java.util.concurrent.TimeUnit;
5251
import java.util.concurrent.TimeoutException;
@@ -68,6 +67,7 @@
6867
import software.amazon.awssdk.services.s3.model.NoSuchKeyException;
6968
import software.amazon.awssdk.services.s3.model.ObjectIdentifier;
7069
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
70+
import software.amazon.awssdk.services.s3.model.S3Exception;
7171
import software.amazon.awssdk.services.s3.model.S3Response;
7272
import software.amazon.awssdk.transfer.s3.S3TransferManager;
7373
import software.amazon.awssdk.transfer.s3.model.CompletedCopy;
@@ -252,12 +252,20 @@ public DirectoryStream<Path> newDirectoryStream(Path dir, DirectoryStream.Filter
252252

253253
try {
254254
return new S3DirectoryStream(s3Path.getFileSystem(), s3Path.bucketName(), dirName, filter);
255-
} catch (CompletionException e) {
256-
if (e.getCause() instanceof NoSuchBucketException) {
257-
throw new NoSuchFileException("Bucket '" + s3Path.bucketName() + "' not found", s3Path.toString(),
258-
e.getMessage());
255+
} catch (RuntimeException e) {
256+
if (e.getCause() instanceof ExecutionException) {
257+
var cause = (Exception) e.getCause().getCause();
258+
if (cause instanceof NoSuchBucketException) {
259+
throw new FileSystemNotFoundException("Bucket '" + s3Path.bucketName() + "' not found: NoSuchBucket");
260+
}
261+
if (cause instanceof S3Exception && ((S3Exception) cause).statusCode() == 403) {
262+
throw new AccessDeniedException("Access to bucket '" + s3Path.bucketName() + "' denied", s3Path.toString(),
263+
cause.getMessage());
264+
}
265+
throw new IOException(cause.getMessage(), cause);
259266
}
260-
throw e;
267+
268+
throw new IOException(e.getMessage(), e);
261269
}
262270
}
263271

@@ -283,8 +291,8 @@ public void createDirectory(Path dir, FileAttribute<?>... attrs) throws IOExcept
283291
var timeOut = TIMEOUT_TIME_LENGTH_1;
284292
final var unit = MINUTES;
285293

286-
try {
287-
s3Directory.getFileSystem().client().putObject(
294+
try (S3AsyncClient client = s3Directory.getFileSystem().client()) {
295+
client.putObject(
288296
PutObjectRequest.builder()
289297
.bucket(s3Directory.bucketName())
290298
.key(directoryKey)

‎src/test/java/software/amazon/nio/spi/s3/S3FileSystemProviderTest.java‎

Lines changed: 71 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,26 @@
99
import static org.assertj.core.api.Assertions.assertThat;
1010
import static org.assertj.core.api.Assertions.assertThatCode;
1111
import static org.assertj.core.api.Assertions.assertThatThrownBy;
12-
import static org.junit.jupiter.api.Assertions.*;
12+
import static org.junit.jupiter.api.Assertions.assertEquals;
13+
import static org.junit.jupiter.api.Assertions.assertFalse;
14+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
15+
import static org.junit.jupiter.api.Assertions.assertNotNull;
16+
import static org.junit.jupiter.api.Assertions.assertNotSame;
17+
import static org.junit.jupiter.api.Assertions.assertNull;
18+
import static org.junit.jupiter.api.Assertions.assertSame;
19+
import static org.junit.jupiter.api.Assertions.assertThrows;
20+
import static org.junit.jupiter.api.Assertions.assertTrue;
1321
import static org.mockito.ArgumentMatchers.any;
14-
import static org.mockito.Mockito.*;
22+
import static org.mockito.Mockito.lenient;
23+
import static org.mockito.Mockito.spy;
24+
import static org.mockito.Mockito.times;
25+
import static org.mockito.Mockito.verify;
26+
import static org.mockito.Mockito.when;
1527
import static software.amazon.nio.spi.s3.S3Matchers.anyConsumer;
1628

1729
import java.io.IOException;
1830
import java.net.URI;
31+
import java.nio.file.AccessDeniedException;
1932
import java.nio.file.AccessMode;
2033
import java.nio.file.DirectoryStream;
2134
import java.nio.file.FileAlreadyExistsException;
@@ -31,7 +44,7 @@
3144
import java.time.Instant;
3245
import java.util.Collections;
3346
import java.util.concurrent.CompletableFuture;
34-
import java.util.concurrent.atomic.AtomicInteger;
47+
import java.util.concurrent.ExecutionException;
3548
import java.util.function.Consumer;
3649
import java.util.stream.Collectors;
3750
import java.util.stream.Stream;
@@ -54,9 +67,11 @@
5467
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
5568
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request;
5669
import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
70+
import software.amazon.awssdk.services.s3.model.NoSuchBucketException;
5771
import software.amazon.awssdk.services.s3.model.ObjectIdentifier;
5872
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
5973
import software.amazon.awssdk.services.s3.model.PutObjectResponse;
74+
import software.amazon.awssdk.services.s3.model.S3Exception;
6075
import software.amazon.awssdk.services.s3.model.S3Object;
6176
import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Publisher;
6277

@@ -184,6 +199,56 @@ public void newDirectoryStream() throws IOException {
184199
assertThat(stream).hasSize(2);
185200
}
186201

202+
@Test
203+
public void newDirectoryStreamS3AccessDeniedException() {
204+
when(mockClient.listObjectsV2Paginator(anyConsumer())).thenThrow(
205+
// this is what is thrown by the paginator in the case that access is denied
206+
new RuntimeException(new ExecutionException(
207+
S3Exception.builder()
208+
.statusCode(403)
209+
.message("AccessDenied")
210+
.build()
211+
))
212+
);
213+
214+
assertThatThrownBy(() -> provider.newDirectoryStream(fs.getPath(pathUri+"/"), entry -> true))
215+
.isInstanceOf(AccessDeniedException.class)
216+
.hasMessage("Access to bucket 'foo' denied -> /baa/: AccessDenied");
217+
}
218+
219+
@Test
220+
public void newDirectoryStreamS3BucketNotFoundException() {
221+
when(mockClient.listObjectsV2Paginator(anyConsumer())).thenThrow(
222+
// this is what is thrown by the paginator in the case that a bucket doesn't exist
223+
new RuntimeException(new ExecutionException(
224+
NoSuchBucketException.builder()
225+
.statusCode(404)
226+
.message("NoSuchBucket")
227+
.build()
228+
))
229+
);
230+
231+
assertThatThrownBy(() -> provider.newDirectoryStream(fs.getPath(pathUri+"/"), entry -> true))
232+
.isInstanceOf(FileSystemNotFoundException.class)
233+
.hasMessage("Bucket 'foo' not found: NoSuchBucket");
234+
}
235+
236+
@Test
237+
public void newDirectoryStreamOtherExceptionsBecomeIOExceptions() {
238+
when(mockClient.listObjectsV2Paginator(anyConsumer())).thenThrow(
239+
new RuntimeException(new ExecutionException(
240+
S3Exception.builder()
241+
.statusCode(500)
242+
.message("software.amazon.awssdk.services.s3.model.S3Exception: InternalError")
243+
.build()
244+
))
245+
);
246+
247+
assertThatThrownBy(() -> provider.newDirectoryStream(fs.getPath(pathUri+"/"), entry -> true))
248+
.isInstanceOf(IOException.class)
249+
.hasMessage("software.amazon.awssdk.services.s3.model.S3Exception: InternalError");
250+
}
251+
187252
@Test
188253
public void newDirectoryStreamFiltersSelf() throws IOException {
189254
final var publisher = new ListObjectsV2Publisher(mockClient, ListObjectsV2Request.builder().build());
@@ -221,12 +286,6 @@ public void newDirectoryStreamFilters() throws IOException {
221286
}
222287
}
223288

224-
private int countDirStreamItems(DirectoryStream<Path> stream) {
225-
var count = new AtomicInteger(0);
226-
stream.iterator().forEachRemaining(item -> count.incrementAndGet());
227-
return count.get();
228-
}
229-
230289
@Test
231290
public void createDirectory() throws Exception {
232291
when(mockClient.putObject(any(PutObjectRequest.class), any(AsyncRequestBody.class))).thenReturn(CompletableFuture.supplyAsync(() ->
@@ -390,15 +449,15 @@ public void checkAccessWithoutException() throws Exception {
390449
}
391450

392451
@Test
393-
public void checkAccessWithExceptionHeadObject() throws Exception {
452+
public void checkAccessWithExceptionHeadObject() {
394453
when(mockClient.headObject(anyConsumer())).thenReturn(CompletableFuture.failedFuture(new IOException()));
395454

396455
var foo = fs.getPath("/foo");
397456
assertThrows(IOException.class, () -> provider.checkAccess(foo, AccessMode.READ));
398457
}
399458

400459
@Test
401-
public void checkAccessWithExceptionListObjectsV2() throws Exception {
460+
public void checkAccessWithExceptionListObjectsV2() {
402461
when(mockClient.listObjectsV2(anyConsumer())).thenReturn(CompletableFuture.failedFuture(new IOException()));
403462

404463
var foo = fs.getPath("/dir/");
@@ -419,7 +478,7 @@ public void getFileAttributeView() {
419478
var foo = fs.getPath("/foo");
420479
final var fileAttributeView = provider.getFileAttributeView(foo, BasicFileAttributeView.class);
421480
assertNotNull(fileAttributeView);
422-
assertTrue(fileAttributeView instanceof S3BasicFileAttributeView);
481+
assertInstanceOf(S3BasicFileAttributeView.class, fileAttributeView);
423482
}
424483

425484
@Test

0 commit comments

Comments
 (0)