Skip to content

Commit

Permalink
Addressing review comments:
Browse files Browse the repository at this point in the history
 - separate positive and negative unit tests
 - fix formatting
 - reuse s3 client for both positive and negative tests
  • Loading branch information
grishick committed Mar 7, 2022
1 parent 3da8bd3 commit 77a7101
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,14 @@ public class S3Destination extends BaseConnector implements Destination {
private static final Logger LOGGER = LoggerFactory.getLogger(S3Destination.class);
private final S3DestinationConfigFactory configFactory;

public S3Destination () {
public S3Destination() {
this.configFactory = new S3DestinationConfigFactory();
}

public S3Destination (final S3DestinationConfigFactory configFactory) {
public S3Destination(final S3DestinationConfigFactory configFactory) {
this.configFactory = configFactory;
}

public static void main(final String[] args) throws Exception {
new IntegrationRunner(new S3Destination()).run(args);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ public S3DestinationConfig(final String endpoint,
}

public S3DestinationConfig(final String endpoint,
final String bucketName,
final String bucketPath,
final String bucketRegion,
final String accessKeyId,
final String secretAccessKey,
final Integer partSize,
final S3FormatConfig formatConfig,
final AmazonS3 s3Client) {
final String bucketName,
final String bucketPath,
final String bucketRegion,
final String accessKeyId,
final String secretAccessKey,
final Integer partSize,
final S3FormatConfig formatConfig,
final AmazonS3 s3Client) {
this.endpoint = endpoint;
this.bucketName = bucketName;
this.bucketPath = bucketPath;
Expand All @@ -72,13 +72,13 @@ public S3DestinationConfig(final String endpoint,
}

public S3DestinationConfig(final String endpoint,
final String bucketName,
final String bucketPath,
final String bucketRegion,
final String accessKeyId,
final String secretAccessKey,
final Integer partSize,
final S3FormatConfig formatConfig) {
final String bucketName,
final String bucketPath,
final String bucketRegion,
final String accessKeyId,
final String secretAccessKey,
final Integer partSize,
final S3FormatConfig formatConfig) {
this(endpoint, bucketName, bucketPath, bucketRegion, accessKeyId, secretAccessKey, partSize, formatConfig, null);
}

Expand Down Expand Up @@ -141,7 +141,7 @@ public S3FormatConfig getFormatConfig() {
}

public AmazonS3 getS3Client() {
if(this.s3Client !=null) {
if (this.s3Client != null) {
return this.s3Client;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
/*
* Copyright (c) 2021 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.integrations.destination.s3;

import com.fasterxml.jackson.databind.JsonNode;

public class S3DestinationConfigFactory {

public S3DestinationConfig getS3DestinationConfig(final JsonNode config) {
return S3DestinationConfig.getS3DestinationConfig(config);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,16 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.when;

import com.amazonaws.services.s3.model.AmazonS3Exception;
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.model.AmazonS3Exception;
import com.amazonaws.services.s3.model.InitiateMultipartUploadRequest;
import com.amazonaws.services.s3.model.InitiateMultipartUploadResult;
import com.amazonaws.services.s3.model.ListObjectsRequest;
Expand All @@ -34,21 +33,16 @@
public class S3DestinationTest {

private AmazonS3 s3;
private AmazonS3 s3NoAccess;
private S3DestinationConfig config;

@BeforeEach
public void setup() {
// S3 client mock for successful operations
s3 = mock(AmazonS3.class);
final InitiateMultipartUploadResult uploadResult = mock(InitiateMultipartUploadResult.class);
final UploadPartResult uploadPartResult = mock(UploadPartResult.class);
when(s3.uploadPart(any(UploadPartRequest.class))).thenReturn(uploadPartResult);
when(s3.initiateMultipartUpload(any(InitiateMultipartUploadRequest.class))).thenReturn(uploadResult);

// S3 client mock that throws Access Denied exception for listObjects operation
s3NoAccess = mock(AmazonS3.class);
doThrow(new AmazonS3Exception("Access Denied")).when(s3NoAccess).listObjects(any(ListObjectsRequest.class));
config = new S3DestinationConfig(
"fake-endpoint",
"fake-bucket",
Expand All @@ -60,8 +54,12 @@ public void setup() {
}

@Test
public void checksS3NoListObjectPermission() {
final S3Destination destinationFail = new S3Destination(new S3DestinationConfigFactory () {
/**
* Test that check will fail if IAM user does not have listObjects permission
*/
public void checksS3WithoutListObjectPermission() {
final S3Destination destinationFail = new S3Destination(new S3DestinationConfigFactory() {

public S3DestinationConfig getS3DestinationConfig(final JsonNode config) {
return new S3DestinationConfig(
"fake-endpoint",
Expand All @@ -71,15 +69,23 @@ public S3DestinationConfig getS3DestinationConfig(final JsonNode config) {
"fake-accessKeyId",
"fake-secretAccessKey",
S3DestinationConfig.DEFAULT_PART_SIZE_MB,
null, s3NoAccess);
null, s3);
}

});
AirbyteConnectionStatus status = destinationFail.check(null);
doThrow(new AmazonS3Exception("Access Denied")).when(s3).listObjects(any(ListObjectsRequest.class));
final AirbyteConnectionStatus status = destinationFail.check(null);
assertEquals(Status.FAILED, status.getStatus(), "Connection check should have failed");
assertTrue(status.getMessage().indexOf("Access Denied") > 0, "Connection check returned wrong failure message");
}

@Test
/**
* Test that check will succeed when IAM user has all required permissions
*/
public void checksS3WithListObjectPermission() {
final S3Destination destinationSuccess = new S3Destination(new S3DestinationConfigFactory() {

// Test that check succeeds when IAM user has listObjects permission
final S3Destination destinationSuccess = new S3Destination(new S3DestinationConfigFactory () {
public S3DestinationConfig getS3DestinationConfig(final JsonNode config) {
return new S3DestinationConfig(
"fake-endpoint",
Expand All @@ -91,8 +97,9 @@ public S3DestinationConfig getS3DestinationConfig(final JsonNode config) {
S3DestinationConfig.DEFAULT_PART_SIZE_MB,
null, s3);
}

});
status = destinationSuccess.check(null);
final AirbyteConnectionStatus status = destinationSuccess.check(null);
assertEquals(Status.SUCCEEDED, status.getStatus(), "Connection check should have succeeded");
}

Expand Down

0 comments on commit 77a7101

Please sign in to comment.