Skip to content
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.

Sign up for GitHub

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

Destination Redshift: handle empty s3 bucket_path #18434

Merged
merged 11 commits into from
Oct 26, 2022
Merged

Destination Redshift: handle empty s3 bucket_path #18434

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fb4dbb7
make bucket_path required
edgao Oct 25, 2022
ddf3f16
check for nonempty path
edgao Oct 25, 2022
d1da006
not required
edgao Oct 25, 2022
6678145
Merge branch 'master' into edgao/dest_redshift_staging_spec
edgao Oct 25, 2022
9e0ba3b
handle null/empty path
edgao Oct 25, 2022
a90bd85
Merge branch 'master' into edgao/dest_redshift_staging_spec
edgao Oct 26, 2022
cfc3484
version bump + changelog
edgao Oct 26, 2022
b8b8c71
fix comment
edgao Oct 26, 2022
84e4539
auto-bump connector version
octavia-squidington-iii Oct 26, 2022
d566faa
Merge branch 'edgao/dest_redshift_staging_spec' of https://github.com…
octavia-squidington-iii Oct 26, 2022
87c0a33
format
edgao Oct 26, 2022
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
22 changes: 20 additions & 2 deletions ...bases/base-java-s3/src/main/java/io/airbyte/integrations/destination/s3/S3BaseChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.model.ListObjectsRequest;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import io.airbyte.integrations.destination.s3.util.StreamTransferManagerFactory;
import java.io.IOException;
import java.io.PrintWriter;
Expand Down Expand Up @@ -92,19 +93,36 @@ static void attemptS3WriteAndDelete(final S3StorageOperations storageOperations,
final S3DestinationConfig s3Config,
final String bucketPath,
final AmazonS3 s3) {
final var prefix = bucketPath.endsWith("/") ? bucketPath : bucketPath + "/";
final String prefix;
if (Strings.isNullOrEmpty(bucketPath)) {
prefix = "";
} else if (bucketPath.endsWith("/")) {
prefix = bucketPath;
} else {
prefix = bucketPath + "/";
}

final String outputTableName = prefix + "_airbyte_connection_test_" + UUID.randomUUID().toString().replaceAll("-", "");
attemptWriteAndDeleteS3Object(storageOperations, s3Config, outputTableName, s3);
}

/**
* Runs some permissions checks:
* 1. Check whether the bucket exists; create it if not
* 2. Check whether s3://bucketName://bucketPath/ exists; create it (with empty contents) if not
* 3. Attempt to create and delete s3://bucketName/outputTableName
* 4. Attempt to list all objects in the bucket
*/
private static void attemptWriteAndDeleteS3Object(final S3StorageOperations storageOperations,
final S3DestinationConfig s3Config,
final String outputTableName,
final AmazonS3 s3) {
final var s3Bucket = s3Config.getBucketName();
final var bucketPath = s3Config.getBucketPath();

storageOperations.createBucketObjectIfNotExists(bucketPath);
if (!Strings.isNullOrEmpty(bucketPath)) {
storageOperations.createBucketObjectIfNotExists(bucketPath);
}
s3.putObject(s3Bucket, outputTableName, "check-content");
testIAMUserHasListObjectPermission(s3, s3Bucket);
s3.deleteObject(s3Bucket, outputTableName);
Expand Down
5 changes: 5 additions & 0 deletions ...ase-java-s3/src/main/java/io/airbyte/integrations/destination/s3/S3StorageOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ public String getBucketObjectPath(final String namespace, final String streamNam
.replaceAll("/+", "/"));
}

/**
* Create a directory object at the specified location. Creates the bucket if necessary.
*
* @param objectPath The directory to create. Must be a nonempty string.
*/
@Override
public void createBucketObjectIfNotExists(final String objectPath) {
final String bucket = s3Config.getBucketName();
Expand Down
44 changes: 44 additions & 0 deletions ...s/base-java-s3/src/test/java/io/airbyte/integrations/destination/s3/S3BaseChecksTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.startsWith;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand All @@ -25,6 +26,8 @@ public class S3BaseChecksTest {
@BeforeEach
public void setup() {
s3Client = mock(AmazonS3.class);
when(s3Client.doesObjectExist(anyString(), eq(""))).thenThrow(new IllegalArgumentException("Object path must not be empty"));
when(s3Client.putObject(anyString(), eq(""), anyString())).thenThrow(new IllegalArgumentException("Object path must not be empty"));
}

@Test
Expand All @@ -49,4 +52,45 @@ public void attemptWriteAndDeleteS3Object_should_createSpecificFiles() {
verify(s3Client).deleteObject(eq("test_bucket"), startsWith("test/bucket/path/_airbyte_connection_test_"));
}

@Test
public void attemptWriteAndDeleteS3Object_should_skipDirectoryCreateIfRootPath() {
S3DestinationConfig config = new S3DestinationConfig(
null,
"test_bucket",
"",
null,
null,
null,
null,
s3Client);
S3StorageOperations operations = new S3StorageOperations(new S3NameTransformer(), s3Client, config);

S3BaseChecks.attemptS3WriteAndDelete(operations, config, "");

verify(s3Client, never()).putObject("test_bucket", "", "");
verify(s3Client).putObject(eq("test_bucket"), startsWith("_airbyte_connection_test_"), anyString());
verify(s3Client).listObjects(ArgumentMatchers.<ListObjectsRequest>argThat(request -> "test_bucket".equals(request.getBucketName())));
verify(s3Client).deleteObject(eq("test_bucket"), startsWith("_airbyte_connection_test_"));
}

@Test
public void attemptWriteAndDeleteS3Object_should_skipDirectoryCreateIfNullPath() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good overall, just noticed that there's a seemingly random _ in the method name and in the existing methods (so presume you're keeping with what's already existing). Minor nit but to keep test naming conventions, would it make more sense to have this consistent with camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol this is actually my preferred testcase naming convention - imo the testWhateverMethod style is harder to read and doesn't provide much info about what the testcase is intended to do

iirc there's a few of these scattered through the codebase (which is where I picked it up from). Not strongly opinionated on this but mildly prefer to spread this style further.

S3DestinationConfig config = new S3DestinationConfig(
null,
"test_bucket",
null,
null,
null,
null,
null,
s3Client);
S3StorageOperations operations = new S3StorageOperations(new S3NameTransformer(), s3Client, config);

S3BaseChecks.attemptS3WriteAndDelete(operations, config, null);

verify(s3Client, never()).putObject("test_bucket", "", "");
verify(s3Client).putObject(eq("test_bucket"), startsWith("_airbyte_connection_test_"), anyString());
verify(s3Client).listObjects(ArgumentMatchers.<ListObjectsRequest>argThat(request -> "test_bucket".equals(request.getBucketName())));
verify(s3Client).deleteObject(eq("test_bucket"), startsWith("_airbyte_connection_test_"));
}
}