chungen0126 commented on code in PR #10138:
URL: https://github.com/apache/ozone/pull/10138#discussion_r3180605310
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -371,6 +416,13 @@ public static Builder builderFromProtobuf(BucketArgs
bucketArgs) {
builder.setBucketEncryptionKey(
OMPBHelper.convert(bucketArgs.getBekInfo()));
}
+ if (bucketArgs.hasCorsConfiguration()) {
+ builder.setCorsConfiguration(CorsConfiguration.getFromProtobuf(
+ bucketArgs.getCorsConfiguration()));
+ }
+ if (bucketArgs.hasClearCorsConfiguration()) {
Review Comment:
I think we could use an `else if` here since these two conditions appear to
be mutually exclusive. This would make the logic a bit clearer and slightly
more efficient.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/CorsConfiguration.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.helpers;
+
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CORSConfiguration;
+
+/**
+ * S3 bucket CORS configuration.
+ */
+public final class CorsConfiguration {
+ private final ImmutableList<CorsRule> rules;
+
+ private CorsConfiguration(Builder builder) {
+ this.rules = ImmutableList.copyOf(builder.rules);
+ }
+
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ public List<CorsRule> getRules() {
+ return rules;
+ }
+
+ public CORSConfiguration getProtobuf() {
+ return CORSConfiguration.newBuilder()
+ .addAllCorsRule(rules.stream()
+ .map(CorsRule::getProtobuf)
+ .collect(Collectors.toList()))
+ .build();
+ }
+
+ public static CorsConfiguration getFromProtobuf(
+ CORSConfiguration proto) {
+ return newBuilder()
+ .setRules(proto.getCorsRuleList().stream()
+ .map(CorsRule::getFromProtobuf)
+ .collect(Collectors.toList()))
+ .build();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof CorsConfiguration)) {
+ return false;
+ }
+ CorsConfiguration that = (CorsConfiguration) obj;
+ return Objects.equals(rules, that.rules);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(rules);
+ }
+
+ @Override
+ public String toString() {
+ return "CorsConfiguration{"
+ + "rules=" + rules
+ + '}';
+ }
+
+ /**
+ * Builder for {@link CorsConfiguration}.
+ */
+ public static final class Builder {
+ private List<CorsRule> rules = ImmutableList.of();
Review Comment:
Could we consider using a mutable list (like `ArrayList`) for the rules
inside the builder? Since the builder is a temporary object, using a mutable
collection might be more efficient by avoiding multiple copies. For instance,
current operations like `addRule` trigger a new copy every time they are
called. We can then convert it to an ImmutableList in the final `build` method.
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -220,6 +222,61 @@ public void listBuckets() throws Exception {
assertEquals(S3Owner.DEFAULT_S3OWNER_ID, syncResponse.owner().id());
}
+ @Test
+ public void testBucketCORSOperations() {
+ final String bucketName = getBucketName();
+ s3Client.createBucket(b -> b.bucket(bucketName));
+
+ CORSRule rule = CORSRule.builder()
+ .id("sdk-v2-cors")
+ .allowedOrigins("https://example.com";)
+ .allowedMethods("GET", "HEAD")
+ .allowedHeaders("Authorization")
+ .exposeHeaders("ETag")
+ .maxAgeSeconds(3600)
+ .build();
+ CORSConfiguration configuration = CORSConfiguration.builder()
+ .corsRules(rule)
+ .build();
+
+ s3Client.putBucketCors(b -> b.bucket(bucketName)
+ .corsConfiguration(configuration));
+
+ List<CORSRule> resultRules =
+ s3Client.getBucketCors(b -> b.bucket(bucketName)).corsRules();
+ assertThat(resultRules).hasSize(1);
+ CORSRule resultRule = resultRules.get(0);
+ assertEquals("sdk-v2-cors", resultRule.id());
+
assertThat(resultRule.allowedOrigins()).containsExactly("https://example.com";);
+ assertThat(resultRule.allowedMethods()).containsExactly("GET", "HEAD");
+ assertThat(resultRule.allowedHeaders()).containsExactly("Authorization");
+ assertThat(resultRule.exposeHeaders()).containsExactly("ETag");
+ assertEquals(3600, resultRule.maxAgeSeconds());
+
+ s3Client.deleteBucketCors(b -> b.bucket(bucketName));
+
+ S3Exception exception = assertThrows(S3Exception.class,
+ () -> s3Client.getBucketCors(b -> b.bucket(bucketName)));
+ assertEquals(404, exception.statusCode());
+ assertEquals("NoSuchCORSConfiguration",
+ exception.awsErrorDetails().errorCode());
+ }
+
+ @Test
+ public void testDeleteBucketCORSWithoutConfiguration() {
+ final String bucketName = getBucketName();
+ s3Client.createBucket(b -> b.bucket(bucketName));
+
+ s3Client.deleteBucketCors(b -> b.bucket(bucketName));
+ s3Client.deleteBucketCors(b -> b.bucket(bucketName));
Review Comment:
Could you please add a brief comment before this line explaining the two
consecutive `deleteBucketCors` calls? It would help clarify why both are
necessary.
--
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]
