HDDS-13081. Add S3 Object tagging tests by 0lai0 · Pull Request #9457 · apache/ozone

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.

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.

ivandika3

Thanks for the update. Just noticed that this patch added LinkBucketTests in V1, but not the tagging tests. For completeness sake could you add the tagging tests in AbstractS3SDKV1Tests?

echonesis

Thanks @0lai0, left some comments.

echonesis · 2025-12-30T01:58:00Z

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

+        RequestBody.fromString("content"));
+
+    List<Tag> tags = new ArrayList<>();
+    for (int i = 1; i <= 11; i++) {


Could you please explain what 11 is?
We could use TAG_NUM_LIMIT + 1 to make it clear.

My apologies, I'll replace hardcoded values with constants

echonesis · 2025-12-30T02:00:12Z

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

+  private static Stream<Arguments> invalidTagConstraintsProvider() {
+    return Stream.of(
+        Arguments.of(
+            Arrays.asList(Tag.builder().key(repeatChar('a', 129)).value("value").build()),


We could use TAG_KEY_LENGTH_LIMIT + 1 instead of 129.

echonesis · 2025-12-30T02:00:51Z

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

+            HTTP_BAD_REQUEST
+        ),
+        Arguments.of(
+            Arrays.asList(Tag.builder().key("valid-key").value(repeatChar('b', 257)).build()),


We could use TAG_VALUE_LENGTH_LIMIT + 1 instead of 257.

Gargi-jais11

Thanks @0lai0 for updating the patch. LGTM!

ivandika3 · 2026-01-02T01:44:52Z

@0lai0 Thanks for the update. Overall LGTM.

Please kindly check the failing test https://github.com/0lai0/ozone/actions/runs/20588433897/job/59131635706

echonesis

Thanks @0lai0 for updating the patch.
LGTM.

chungen0126

Thanks @0lai0 for the patch.

chungen0126 · 2026-01-18T01:52:07Z

...r/src/main/java/org/apache/hadoop/ozone/om/request/s3/tagging/S3PutObjectTaggingRequest.java

+  private static final Pattern TAG_REGEX_PATTERN =
+      Pattern.compile("^([\\p{L}\\p{Z}\\p{N}_.:/=+\\-]*)$");
+
+  private static void validateTags(Map<String, String> tags) throws OMException {


Could you please explain why we need this here?

My mistake regarding the TAG_REGEX_PATTERN. S3 specs only enforce length limits, not character constraints.I'll remove the regex check and push the fix right away. Thanks @chungen0126 .

What I meant was, why do we need tag verification here? My understanding is that the issue is about adding integration tests. Since this is a unit test, I don't think we need to make any changes here.

Got it. I've removed this part since the PR focuses on integration tests. Thanks for pointing that out.

0lai0 · 2026-01-22T07:55:27Z

Thanks @chungen0126 for review.

jojochuang · 2026-01-24T04:56:53Z

Hi I see there's a lot of iterations on this test PR, which is great. To close the loop, since all comments are addressed I'd like to move it forward.

0lai0 · 2026-01-24T15:11:19Z

I notice some tests failed, I need to fix them. Thanks @jojochuang .

adoroszlai · 2026-01-30T11:09:42Z

I notice some tests failed, I need to fix them.

https://github.com/0lai0/ozone/actions/runs/21510299989/job/61976509731#step:13:5994

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

ivandika3 reviewed Dec 30, 2025

View reviewed changes

echonesis reviewed Dec 30, 2025

View reviewed changes

0lai0 and others added 2 commits December 30, 2025 11:38

Merge branch 'apache:master' into HDDS-13081

57149a6

replace hardcode and add vi tagging test

6c36a65

Gargi-jais11 approved these changes Dec 30, 2025

View reviewed changes

Merge branch 'apache:master' into HDDS-13081

a1ae180

echonesis approved these changes Jan 13, 2026

View reviewed changes

fix error

a3f4db7

chungen0126 reviewed Jan 18, 2026

View reviewed changes

0lai0 and others added 4 commits January 18, 2026 11:20

Merge branch 'apache:master' into HDDS-13081

b8d0104

remove character constraints

55caf51

Merge branch 'apache:master' into HDDS-13081

fab2d8c

remove S3putObjectTaggingRequest

43394fd

jojochuang requested a review from chungen0126 January 24, 2026 04:53

Merge branch 'apache:master' into HDDS-13081

66f4b09

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!

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!

ivandika3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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

ivandika3 commented Dec 18, 2025 •

edited

Loading

ivandika3 left a comment •

edited

Loading