HDDS-13081. Add S3 Object tagging tests #9457

0lai0 · 2025-12-07T13:42:49Z

What changes were proposed in this pull request?

Adding PutObjectTagging, GetObjectTagging, DeleteObjectTagging SDK integration tests to validate.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13081

Gargi-jais11

Thanks @0lai0 for the patch.
I have few more test case suggestions which could be added.

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java

Gargi-jais11 · 2025-12-08T04:58:07Z

More Edge Cases which can be tested:

testPutObjectTaggingExceedsLimit() — Test 11 tags and validate it throws error there is 10-tag limit.
testPutObjectTaggingReplacesExistingTags() — verifies replacement behavior (Initially put 2 tags then replace it with 1 tag and check the last replcaed tag is only applied)
testPutObjectTaggingInvalidConstraints - This can be a parameterised test like below:

private static Stream<Arguments> invalidTagConstraintsProvider() {
    return Stream.of(
        // Test 1: Tag key exceeds maximum length (128 bytes)
        Arguments.of(
            "Tag key exceeding 128 bytes should be rejected",
            Arrays.asList(Tag.builder().key("a".repeat(129)).value("value").build()),
            400,
            "exceeds the maximum length"
        ),
        
        // Test 2: Tag value exceeds maximum length (256 bytes)
        Arguments.of(
            "Tag value exceeding 256 bytes should be rejected",
            Arrays.asList(Tag.builder().key("valid-key").value("b".repeat(257)).build()),
            400,
            "exceeds the maximum length"
        ),
        // Test 3: Tag key with invalid characters (@, #, $, (, {, ....)
        Arguments.of(
            "Tag key with invalid characters should be rejected",
            Arrays.asList(Tag.builder().key("t$ag@#invalid").value("value").build()),
            400,
            "does not have a valid pattern"
        ), 
        // Test 4: Tag key starting with "aws:" prefix
        Arguments.of(
            "Tag key starting with 'aws:' prefix should be rejected",
            Arrays.asList(Tag.builder().key("aws:test").value("value").build()),
            400,
            "cannot start with \"aws:\"" 
        )
    );
    }
@ParameterizedTest
@MethodSource("invalidTagConstraintsProvider")
public void testPutObjectTaggingInvalidConstraints(
    String testCaseName,
    List<Tag> invalidTags,
    int expectedStatusCode,
    String expectedErrorMessage) {
    // your code
}

testPutAndGetObjectTaggingOnNonExistentObject() - Verify NoSuchKeyException occurs and 404 status code.
testDeleteObjectTaggingOnNonExistentObject() - Verify NoSuchKeyException occurs and 404 status code.

Tejaskriya

Thanks for working on this @0lai0 , I second the suggestion of @Gargi-jais11 , the tests added mostly look good, but they are mainly the positive cases. Can we add tests for the error cases?

Also, multiple related test cases could be put in one test method to avoid duplication.

Gargi-jais11 · 2025-12-09T12:58:59Z

Thanks for working on this @0lai0 , I second the suggestion of @Gargi-jais11 , the tests added mostly look good, but they are mainly the positive cases. Can we add tests for the error cases?

Also, multiple related test cases could be put in one test method to avoid duplication.

Yes correct @Tejaskriya above test cases can be added to check for error cases.

0lai0 · 2025-12-10T06:20:04Z

Thanks @Gargi-jais11 and @Tejaskriya review, I'll add error cases.

ivandika3

Thanks for adding the tests. Overall LGTM. Could you also add similar tests to the AbstractS3SDKV1Tests as well?

Please also help to check this failure in your latest test run https://github.com/0lai0/ozone/actions/runs/20160679799/job/57874354964
(org.apache.hadoop.ozone.s3.awssdk.v2.AbstractS3SDKV2Tests$S3BucketOwnershipVerificationConditionsTests$LinkBucketTests)

Gargi-jais11 · 2025-12-13T15:46:07Z

@0lai0 Thanks for updating the patch. It LGTM.

Could you also add similar tests to the AbstractS3SDKV1Tests as well?

Once this is done the patch will be good to go.

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java

0lai0 · 2025-12-15T08:36:37Z

Thanks @ivandika3, @Gargi-jais11 and @echonesis for review.I'll push a fix right away.

echonesis · 2025-12-15T08:41:38Z

Thanks @0lai0 , left some comments.

echonesis

Thanks @0lai0 for updating the patch, left some comments.

echonesis · 2025-12-17T08:55:15Z

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java


+  @Nested
+  @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+  class BucketOwnershipLinkBucketTests {


Maybe we could keep it consistent with the V2 class

Yes we should keep the name consistent with the V2 class as LinkBucketTests

Sure, I'll get on this.

echonesis · 2025-12-17T09:15:21Z

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java

 *
 */
 @TestMethodOrder(MethodOrderer.MethodName.class)
-public abstract class AbstractS3SDKV1Tests extends OzoneTestBase {


Removal of OzoneTestBase should be consistent between V1 and V2.

The V2 version still extends OzoneTestBase, so getTestName() returns a fixed test method name instead of a random string. This could cause naming conflicts in parallel test execution.

Consider removing OzoneTestBase from V2 as well and adding the same random-based getTestName() method.

I believe AbstractS3SDKV1Tests should extend OzoneTestBase .
OzoneTestBase.getTestName() returns the actual test method name (e.g., "testCreateBucket"), ensuring unique resource names per test

Without it, tests may interfere with each other and debugging becomes difficult

This is a standard pattern used across all Ozone test classes

The problem is test name may not be unique (e.g. for parameterized test). #9517 adds a method in OzoneTestBase to help with that, and changes these S3 tests to use uniqueObjectName() instead of just getTestName().

+1 for keeping OzoneTestBase.

@0lai0 HDDS-14197 is merged, now you can keep extends OzoneTestBase here and don't need to implement custom getTestName().

Thanks everyone for the discussion, and thanks @adoroszlai for the update on HDDS-14197. I'll keep OzoneTestBase and remove the custom getTestName() implementation.

Gargi-jais11

Left some comments below.

Gargi-jais11 · 2025-12-17T10:14:27Z

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java

+          .withExpectedBucketOwner("wrong-owner");
+      AmazonServiceException wrongOwner = assertThrows(AmazonServiceException.class,
+          () -> s3Client.getBucketAcl(wrongRequest));
+      assertEquals(403, wrongOwner.getStatusCode());


Use HttpURLConnection.HTTP_FORBIDDEN

Gargi-jais11 · 2025-12-17T10:14:47Z

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java

+          .withExpectedBucketOwner("wrong-owner");
+      AmazonServiceException wrongOwner = assertThrows(AmazonServiceException.class,
+          () -> s3Client.getBucketAcl(wrongRequest));
+      assertEquals(403, wrongOwner.getStatusCode());


use HttpURLConnection.HTTP_FORBIDDEN

Gargi-jais11 · 2025-12-17T10:17:59Z

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java

+            400
+        ),
+        Arguments.of(
+            Arrays.asList(Tag.builder().key("valid-key").value(repeatChar('b', 257)).build()),
+            400
+        ),
+        Arguments.of(
+            Arrays.asList(Tag.builder().key("t$ag@#invalid").value("value").build()),
+            400
+        ),
+        Arguments.of(
+            Arrays.asList(Tag.builder().key("aws:test").value("value").build()),
+            400


Use HTTP_BAD_REQUEST in this Parameterised test.

Gargi-jais11 · 2025-12-17T10:32:29Z

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java

+  private String getTestName() {
+    return RandomStringUtils.secure().nextAlphanumeric(8).toLowerCase(Locale.ROOT);
+  }
+


@0lai0 is there any specific purpose because of which you are creating getTestName explicity over here and not using OzoneTestBase#getTestName?

Thanks for point this out , I'll remove it.

Gargi-jais11 · 2025-12-17T10:44:17Z

@0lai0 After addressing the review comments please mark it as resolved.

ivandika3 · 2025-12-18T01:31:10Z

@0lai0 After addressing the review comments please mark it as resolved.

@Gargi-jais11 IMO the responsibility of resolving comments is in the reviewer since it signals that that the comments are already addressed to the reviewer's satisfaction.

0lai0 · 2025-12-19T09:31:53Z

Thanks for all the feedback. I'll review and make changes based on the comments.

adoroszlai · 2025-12-22T16:14:20Z

Thanks @0lai0 for updating the patch. There are some test failures:

Error:  Tests run: 31, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.045 s <<< FAILURE! -- in org.apache.hadoop.ozone.s3.awssdk.v1.AbstractS3SDKV1Tests$LinkBucketTests
Error:  org.apache.hadoop.ozone.s3.awssdk.v1.AbstractS3SDKV1Tests$LinkBucketTests.testDanglingBucket -- Time elapsed: 0.016 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <AccessDenied> but was: <Access Denied>
...
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
	at org.apache.hadoop.ozone.s3.awssdk.v1.AbstractS3SDKV1Tests$LinkBucketTests.testDanglingBucket(AbstractS3SDKV1Tests.java:1432)
...

Error:  org.apache.hadoop.ozone.s3.awssdk.v1.AbstractS3SDKV1Tests$LinkBucketTests.setBucketVerificationOnLinkBucket -- Time elapsed: 0.020 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <AccessDenied> but was: <Access Denied>
...
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
	at org.apache.hadoop.ozone.s3.awssdk.v1.AbstractS3SDKV1Tests$LinkBucketTests.setBucketVerificationOnLinkBucket(AbstractS3SDKV1Tests.java:1407)

https://github.com/0lai0/ozone/actions/runs/20396880835/job/58614335876#step:13:6020

0lai0 · 2025-12-29T09:49:48Z

Thanks @adoroszlai for review. I've pushed a commit to address test failures by AssertionFailedError.

HDDS-13081. Add S3 Object tagging tests

abf1c66

adoroszlai requested a review from ivandika3 December 7, 2025 15:31

adoroszlai added the test label Dec 7, 2025

Gargi-jais11 suggested changes Dec 8, 2025

View reviewed changes

...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java Outdated Show resolved Hide resolved

chungen0126 requested review from chungen0126 and peterxcli December 8, 2025 15:15

Merge branch 'apache:master' into HDDS-13081

f4fbf96

Tejaskriya reviewed Dec 9, 2025

View reviewed changes

0lai0 and others added 4 commits December 12, 2025 14:52

Merge branch 'apache:master' into HDDS-13081

96c1a4f

add error cases test

d127a98

fix checkstyle

a312048

checkstyle

010cfe7

ivandika3 reviewed Dec 13, 2025

View reviewed changes

echonesis reviewed Dec 15, 2025

View reviewed changes

Merge branch 'apache:master' into HDDS-13081

2ab4db1

0lai0 and others added 3 commits December 15, 2025 17:13

fix comment

746a680

Merge branch 'apache:master' into HDDS-13081

a8e87b3

fix error

0888e40

jojochuang requested review from Gargi-jais11, Tejaskriya, echonesis and ivandika3 December 17, 2025 06:23

echonesis reviewed Dec 17, 2025

View reviewed changes

Gargi-jais11 suggested changes Dec 17, 2025

View reviewed changes

0lai0 and others added 3 commits December 20, 2025 22:43

Merge branch 'master' into HDDS-13081

684b0a9

Merge branch 'apache:master' into HDDS-13081

3b99005

consistent LinkBucketTests, remove gettestName keep OzoneTestBase, HTTP

549bb53

0lai0 and others added 3 commits December 23, 2025 09:48

Merge branch 'apache:master' into HDDS-13081

a84c560

Merge branch 'apache:master' into HDDS-13081

0d835ea

fix bucket owner mismatch

18e9394

HDDS-13081. Add S3 Object tagging tests #9457

Are you sure you want to change the base?

HDDS-13081. Add S3 Object tagging tests #9457

Conversation

0lai0 commented Dec 7, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gargi-jais11 commented Dec 8, 2025

Uh oh!

Tejaskriya left a comment

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 commented Dec 9, 2025

Uh oh!

0lai0 commented Dec 10, 2025

Uh oh!

ivandika3 left a comment

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 commented Dec 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0lai0 commented Dec 15, 2025

Uh oh!

echonesis commented Dec 15, 2025

Uh oh!

echonesis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 commented Dec 17, 2025

Uh oh!

ivandika3 commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0lai0 commented Dec 19, 2025

Uh oh!

adoroszlai commented Dec 22, 2025

Uh oh!

0lai0 commented Dec 29, 2025

Uh oh!

Reviewers

Assignees

Labels

ivandika3 commented Dec 18, 2025 •

edited

Loading