Skip to content

AWS: remove S3URI scheme restrictions #3656

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

Merged
jackye1995 merged 2 commits into from
Dec 3, 2021
Merged

AWS: remove S3URI scheme restrictions #3656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41ae5a6
AWS: remove S3URI scheme restrictions
Dec 2, 2021
5213b7d
fix style
Dec 3, 2021
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
21 changes: 7 additions & 14 deletions aws/src/main/java/org/apache/iceberg/aws/s3/S3URI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@

package org.apache.iceberg.aws.s3;

import java.util.Set;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;

/**
* This class represents a fully qualified location in S3 for input/output
Expand All @@ -38,18 +36,16 @@ class S3URI {
private static final String PATH_DELIM = "/";
private static final String QUERY_DELIM = "\\?";
private static final String FRAGMENT_DELIM = "#";
private static final Set<String> VALID_SCHEMES = ImmutableSet.of("https", "s3", "s3a", "s3n");

private final String location;
private final String bucket;
private final String key;

/**
* Creates a new S3URI based on the bucket and key parsed from the location as defined in:
* https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html#access-bucket-intro
*
* Supported access styles are Virtual Hosted addresses and s3://... URIs with additional
* 's3n' and 's3a' schemes supported for backwards compatibility.
* Creates a new S3URI in the form of scheme://bucket/key?query#fragment
* <p>
* The URI supports any valid URI schemes to be backwards compatible with s3a and s3n,
* and also allows users to use S3FileIO with other S3-compatible object storage services like GCS.
*
* @param location fully qualified URI
*/
Expand All @@ -58,14 +54,11 @@ class S3URI {

this.location = location;
String [] schemeSplit = location.split(SCHEME_DELIM, -1);
ValidationException.check(schemeSplit.length == 2, "Invalid S3 URI: %s", location);

String scheme = schemeSplit[0];
ValidationException.check(VALID_SCHEMES.contains(scheme.toLowerCase()), "Invalid scheme: %s", scheme);
ValidationException.check(schemeSplit.length == 2, "Invalid S3 URI, cannot determine scheme: %s", location);

String [] authoritySplit = schemeSplit[1].split(PATH_DELIM, 2);
ValidationException.check(authoritySplit.length == 2, "Invalid S3 URI: %s", location);
ValidationException.check(!authoritySplit[1].trim().isEmpty(), "Invalid S3 key: %s", location);
ValidationException.check(authoritySplit.length == 2, "Invalid S3 URI, cannot determine bucket: %s", location);
ValidationException.check(!authoritySplit[1].trim().isEmpty(), "Invalid S3 URI, path is empty: %s", location);
this.bucket = authoritySplit[0];

// Strip query and fragment if they exist
Expand Down
25 changes: 17 additions & 8 deletions aws/src/test/java/org/apache/iceberg/aws/s3/TestS3URI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@

package org.apache.iceberg.aws.s3;

import org.apache.iceberg.AssertHelpers;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThrows;

public class TestS3URI {

Expand All @@ -49,18 +49,27 @@ public void testEncodedString() {
}

@Test
public void testMissingKey() {
assertThrows(ValidationException.class, () -> new S3URI("https://bucket/"));
public void testEmptyPath() {
AssertHelpers.assertThrows("Should not allow missing object key",
ValidationException.class,
"Invalid S3 URI, path is empty",
() -> new S3URI("https://bucket/"));
}

@Test
public void testRelativePathing() {
assertThrows(ValidationException.class, () -> new S3URI("/path/to/file"));
public void testMissingScheme() {
AssertHelpers.assertThrows("Should not allow missing scheme",
ValidationException.class,
"Invalid S3 URI, cannot determine scheme",
() -> new S3URI("/path/to/file"));
}

@Test
public void testInvalidScheme() {
assertThrows(ValidationException.class, () -> new S3URI("http://bucket/"));
public void testMissingBucket() {
AssertHelpers.assertThrows("Should not allow missing bucket",
ValidationException.class,
"Invalid S3 URI, cannot determine bucket",
() -> new S3URI("https://bucket"));
}

@Test
Expand All @@ -75,7 +84,7 @@ public void testQueryAndFragment() {

@Test
public void testValidSchemes() {
for (String scheme : Lists.newArrayList("https", "s3", "s3a", "s3n")) {
for (String scheme : Lists.newArrayList("https", "s3", "s3a", "s3n", "gs")) {
S3URI uri = new S3URI(scheme + "://bucket/path/to/file");
assertEquals("bucket", uri.bucket());
assertEquals("path/to/file", uri.key());
Expand Down