adoroszlai commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3121932411
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -177,71 +164,242 @@ public Response get(
throw ex;
}
- // The valid encodingType Values is "url"
+ // Return empty response if no keys found
+ // This path is reached when FILE_NOT_FOUND exception is caught and handled
+ ListObjectResponse emptyResponse = new ListObjectResponse();
+ emptyResponse.setName(bucketName);
+ emptyResponse.setKeyCount(0);
+
+ // Log audit entry for empty response to align with previous behavior
+ long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+ perf.appendCount(0);
+ perf.appendOpLatencyNanos(opLatencyNs);
+ auditReadSuccess(s3GAction, perf);
+
+ return Response.ok(emptyResponse).build();
+ }
+
+ private int validateMaxKeys(int maxKeys) throws OS3Exception {
+ if (maxKeys < 0) {
+ throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be >= 0");
+ }
+
+ return Math.min(maxKeys, maxKeysLimit);
+ }
+
+ /**
+ * Context class to hold bucket listing parameters and state.
+ */
+ static class BucketListingContext {
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final int maxKeys;
+ private final String prefix;
+ private final String continueToken;
+ private final String startAfter;
+ private final String prevKey;
+ private final boolean shallow;
+ private final OzoneBucket bucket;
+ private final Iterator<? extends OzoneKey> ozoneKeyIterator;
+ private final ContinueToken decodedToken;
+
+ @SuppressWarnings("parameternumber")
+ BucketListingContext(String bucketName, String delimiter, String
encodingType,
+ String marker, int maxKeys, String prefix,
String continueToken,
+ String startAfter, String prevKey, boolean
shallow,
+ OzoneBucket bucket, Iterator<? extends
OzoneKey> ozoneKeyIterator,
+ ContinueToken decodedToken) {
+ this.bucketName = bucketName;
+ this.delimiter = delimiter;
+ this.encodingType = encodingType;
+ this.marker = marker;
+ this.maxKeys = maxKeys;
+ this.prefix = prefix;
+ this.continueToken = continueToken;
+ this.startAfter = startAfter;
+ this.prevKey = prevKey;
+ this.shallow = shallow;
+ this.bucket = bucket;
+ this.ozoneKeyIterator = ozoneKeyIterator;
+ this.decodedToken = decodedToken;
+ }
+
+ // Getters
+ public String getBucketName() {
+ return bucketName;
+ }
+
+ public String getDelimiter() {
+ return delimiter;
+ }
+
+ public String getEncodingType() {
+ return encodingType;
+ }
+
+ public String getMarker() {
+ return marker;
+ }
+
+ public int getMaxKeys() {
+ return maxKeys;
+ }
+
+ public String getPrefix() {
+ return prefix;
+ }
+
+ public String getContinueToken() {
+ return continueToken;
+ }
+
+ public String getStartAfter() {
+ return startAfter;
+ }
+
+ public String getPrevKey() {
+ return prevKey;
+ }
+
+ public boolean isShallow() {
+ return shallow;
+ }
+
+ public OzoneBucket getBucket() {
+ return bucket;
+ }
+
+ public Iterator<? extends OzoneKey> getOzoneKeyIterator() {
+ return ozoneKeyIterator;
+ }
+
+ public ContinueToken getDecodedToken() {
+ return decodedToken;
+ }
+ }
+
+ /**
+ * Validate and prepare parameters for bucket listing.
+ */
+ @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+ BucketListingContext validateAndPrepareParameters(
+ String bucketName, String delimiter, String encodingType, String marker,
+ int maxKeys, String prefix, String continueToken, String startAfter)
+ throws OS3Exception, IOException {
Review Comment:
Please move validation into `BucketListingContext`, which should perform it
on the values read from `queryParams`.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -177,71 +164,242 @@ public Response get(
throw ex;
}
- // The valid encodingType Values is "url"
+ // Return empty response if no keys found
+ // This path is reached when FILE_NOT_FOUND exception is caught and handled
+ ListObjectResponse emptyResponse = new ListObjectResponse();
+ emptyResponse.setName(bucketName);
+ emptyResponse.setKeyCount(0);
+
+ // Log audit entry for empty response to align with previous behavior
+ long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+ perf.appendCount(0);
+ perf.appendOpLatencyNanos(opLatencyNs);
+ auditReadSuccess(s3GAction, perf);
+
+ return Response.ok(emptyResponse).build();
+ }
+
+ private int validateMaxKeys(int maxKeys) throws OS3Exception {
+ if (maxKeys < 0) {
+ throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be >= 0");
+ }
+
+ return Math.min(maxKeys, maxKeysLimit);
+ }
+
+ /**
+ * Context class to hold bucket listing parameters and state.
+ */
+ static class BucketListingContext {
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final int maxKeys;
+ private final String prefix;
+ private final String continueToken;
+ private final String startAfter;
+ private final String prevKey;
+ private final boolean shallow;
+ private final OzoneBucket bucket;
+ private final Iterator<? extends OzoneKey> ozoneKeyIterator;
+ private final ContinueToken decodedToken;
+
+ @SuppressWarnings("parameternumber")
+ BucketListingContext(String bucketName, String delimiter, String
encodingType,
+ String marker, int maxKeys, String prefix,
String continueToken,
+ String startAfter, String prevKey, boolean
shallow,
+ OzoneBucket bucket, Iterator<? extends
OzoneKey> ozoneKeyIterator,
+ ContinueToken decodedToken) {
Review Comment:
Instead of passing too many parameters, `BucketListingContext` should
initialize itself directly from `queryParams`.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -177,71 +164,242 @@ public Response get(
throw ex;
}
- // The valid encodingType Values is "url"
+ // Return empty response if no keys found
+ // This path is reached when FILE_NOT_FOUND exception is caught and handled
+ ListObjectResponse emptyResponse = new ListObjectResponse();
+ emptyResponse.setName(bucketName);
+ emptyResponse.setKeyCount(0);
+
+ // Log audit entry for empty response to align with previous behavior
+ long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+ perf.appendCount(0);
+ perf.appendOpLatencyNanos(opLatencyNs);
+ auditReadSuccess(s3GAction, perf);
+
+ return Response.ok(emptyResponse).build();
+ }
+
+ private int validateMaxKeys(int maxKeys) throws OS3Exception {
+ if (maxKeys < 0) {
+ throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be >= 0");
+ }
+
+ return Math.min(maxKeys, maxKeysLimit);
+ }
+
+ /**
+ * Context class to hold bucket listing parameters and state.
+ */
+ static class BucketListingContext {
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final int maxKeys;
+ private final String prefix;
+ private final String continueToken;
+ private final String startAfter;
+ private final String prevKey;
+ private final boolean shallow;
+ private final OzoneBucket bucket;
+ private final Iterator<? extends OzoneKey> ozoneKeyIterator;
+ private final ContinueToken decodedToken;
+
+ @SuppressWarnings("parameternumber")
+ BucketListingContext(String bucketName, String delimiter, String
encodingType,
+ String marker, int maxKeys, String prefix,
String continueToken,
+ String startAfter, String prevKey, boolean
shallow,
+ OzoneBucket bucket, Iterator<? extends
OzoneKey> ozoneKeyIterator,
+ ContinueToken decodedToken) {
+ this.bucketName = bucketName;
+ this.delimiter = delimiter;
+ this.encodingType = encodingType;
+ this.marker = marker;
+ this.maxKeys = maxKeys;
+ this.prefix = prefix;
+ this.continueToken = continueToken;
+ this.startAfter = startAfter;
+ this.prevKey = prevKey;
+ this.shallow = shallow;
+ this.bucket = bucket;
+ this.ozoneKeyIterator = ozoneKeyIterator;
+ this.decodedToken = decodedToken;
+ }
+
+ // Getters
+ public String getBucketName() {
+ return bucketName;
+ }
+
+ public String getDelimiter() {
+ return delimiter;
+ }
+
+ public String getEncodingType() {
+ return encodingType;
+ }
+
+ public String getMarker() {
+ return marker;
+ }
+
+ public int getMaxKeys() {
+ return maxKeys;
+ }
+
+ public String getPrefix() {
+ return prefix;
+ }
+
+ public String getContinueToken() {
+ return continueToken;
+ }
+
+ public String getStartAfter() {
+ return startAfter;
+ }
+
+ public String getPrevKey() {
+ return prevKey;
+ }
+
+ public boolean isShallow() {
+ return shallow;
+ }
+
+ public OzoneBucket getBucket() {
+ return bucket;
+ }
+
+ public Iterator<? extends OzoneKey> getOzoneKeyIterator() {
+ return ozoneKeyIterator;
+ }
+
+ public ContinueToken getDecodedToken() {
+ return decodedToken;
+ }
+ }
+
+ /**
+ * Validate and prepare parameters for bucket listing.
+ */
+ @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+ BucketListingContext validateAndPrepareParameters(
+ String bucketName, String delimiter, String encodingType, String marker,
+ int maxKeys, String prefix, String continueToken, String startAfter)
+ throws OS3Exception, IOException {
+
+ // If you specify the encoding-type request parameter, should return
encoded key name values
+ // in the following response elements: Delimiter, Prefix, Key, and
StartAfter.
+ // For detail refer:
+ //
https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#AmazonS3-ListObjectsV2-response-EncodingType
+
+ // Validate encoding type
if (encodingType != null && !encodingType.equals(ENCODING_TYPE)) {
throw S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT, encodingType);
}
+ maxKeys = validateMaxKeys(maxKeys);
+
+ if (prefix == null) {
+ prefix = "";
+ }
+
+ // Assign marker to startAfter. for the compatibility of aws api v1
+ if (startAfter == null && marker != null) {
+ startAfter = marker;
+ }
+
+ ContinueToken decodedToken = ContinueToken.decodeFromString(continueToken);
+
+ // If continuation token and start after both are provided, then we
+ // ignore start After
+ String prevKey = continueToken != null ? decodedToken.getLastKey() :
startAfter;
+
+ // If shallow is true, only list immediate children
+ // delimited by OZONE_URI_DELIMITER
+ boolean shallow = listKeysShallowEnabled &&
OZONE_URI_DELIMITER.equals(delimiter);
+
+ OzoneBucket bucket = getBucket(bucketName);
+ S3Owner.verifyBucketOwnerCondition(getHeaders(), bucketName,
bucket.getOwner());
- // If you specify the encoding-type request parameter,should return
- // encoded key name values in the following response elements:
- // Delimiter, Prefix, Key, and StartAfter.
- //
- // For detail refer:
- // https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
- // #AmazonS3-ListObjectsV2-response-EncodingType
- //
+ Iterator<? extends OzoneKey> ozoneKeyIterator = bucket.listKeys(prefix,
prevKey, shallow);
+
+ return new BucketListingContext(bucketName, delimiter, encodingType,
marker,
+ maxKeys, prefix, continueToken, startAfter, prevKey, shallow,
+ bucket, ozoneKeyIterator, decodedToken);
+ }
+
+ /**
+ * Initialize ListObjectResponse object.
+ */
+ @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+ ListObjectResponse initializeListObjectResponse(
+ String bucketName, String delimiter, String encodingType, String marker,
+ int maxKeys, String prefix, String continueToken, String startAfter) {
Review Comment:
Please move the following methods into `BucketListing(Context)`, and use
member variables:
- `initializeListObjectResponse`
- `processKeyListing`
- `buildFinalResponse`
Also, please update the new unit test to exercise `BucketListing` only,
without involving `BucketEndpoint`. (`testGetDelegatesToAclHandler` can be
dropped.)
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -177,71 +164,242 @@ public Response get(
throw ex;
}
- // The valid encodingType Values is "url"
+ // Return empty response if no keys found
+ // This path is reached when FILE_NOT_FOUND exception is caught and handled
+ ListObjectResponse emptyResponse = new ListObjectResponse();
+ emptyResponse.setName(bucketName);
+ emptyResponse.setKeyCount(0);
+
+ // Log audit entry for empty response to align with previous behavior
+ long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+ perf.appendCount(0);
+ perf.appendOpLatencyNanos(opLatencyNs);
+ auditReadSuccess(s3GAction, perf);
+
+ return Response.ok(emptyResponse).build();
+ }
+
+ private int validateMaxKeys(int maxKeys) throws OS3Exception {
+ if (maxKeys < 0) {
+ throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be >= 0");
+ }
+
+ return Math.min(maxKeys, maxKeysLimit);
+ }
+
+ /**
+ * Context class to hold bucket listing parameters and state.
+ */
+ static class BucketListingContext {
Review Comment:
I'd prefer `BucketListing` as name, to not increase the number of "contexts"
in S3 Gateway.
I think it can also be a top-level class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
