adoroszlai commented on code in PR #9607:
URL: https://github.com/apache/ozone/pull/9607#discussion_r2672551625
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAclHandler.java:
##########
@@ -256,4 +256,75 @@ public void testAuditLoggingOnInvalidArgument() throws
Exception {
eq(S3GAction.PUT_ACL),
any(OS3Exception.class));
}
+
+ // ===== GET Request Tests =====
+
+ @Test
+ public void testHandleGetRequestWithAclQueryParam() throws Exception {
+ assertNotNull(aclHandler.handleGetRequest(BUCKET_NAME),
+ "Handler should handle request with ?acl param");
+ }
+
+ @Test
+ public void testHandleGetRequestWithoutAclQueryParam() throws Exception {
+ // Remove "acl" query parameter - handler should not handle request
+ aclHandler.queryParamsForTest().unset("acl");
+
+ assertNull(aclHandler.handleGetRequest(BUCKET_NAME),
+ "Handler should return null without ?acl param");
+ }
+
+ @Test
+ public void testHandleGetRequestSucceeds() throws Exception {
+ assertSucceeds(() -> aclHandler.handleGetRequest(BUCKET_NAME));
+ }
+
+ @Test
+ public void testHandleGetRequestBucketNotFound() {
+ assertThrows(OS3Exception.class,
+ () -> aclHandler.handleGetRequest("nonexistent-bucket"),
+ "Should throw OS3Exception for non-existent bucket");
+ }
+
+ @Test
+ public void testHandleGetRequestReturnsCorrectAclStructure() throws
Exception {
+ // First set some ACL
+ when(headers.getHeaderString(S3Acl.GRANT_READ))
+ .thenReturn("id=\"testuser\"");
+ aclHandler.handlePutRequest(BUCKET_NAME, null);
+
+ // Now get ACL
+ Response response = aclHandler.handleGetRequest(BUCKET_NAME);
+
+ assertNotNull(response);
+ S3BucketAcl result = (S3BucketAcl) response.getEntity();
Review Comment:
Please use `assertInstanceOf`.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java:
##########
@@ -79,6 +80,18 @@ public void clean() throws IOException {
}
}
+ /**
+ * Helper method to get ACL from bucket and validate response.
+ */
+ private S3BucketAcl getBucketAcl(String bucketName) throws Exception {
+ Response response = bucketEndpoint.get(bucketName);
+ assertEquals(HTTP_OK, response.getStatus(),
+ "GET ACL should return HTTP_OK");
+ assertTrue(response.getEntity() instanceof S3BucketAcl,
+ "Response entity should be S3BucketAcl");
+ return (S3BucketAcl) response.getEntity();
Review Comment:
Please use `assertInstanceOf`, which provides better failure message
including actual class, and also returns the object casted to the expected
class, so it can be returned directly.
```suggestion
return assertInstanceOf(S3BucketAcl.class, response.getEntity());
```
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAclHandler.java:
##########
@@ -256,4 +256,75 @@ public void testAuditLoggingOnInvalidArgument() throws
Exception {
eq(S3GAction.PUT_ACL),
any(OS3Exception.class));
}
+
+ // ===== GET Request Tests =====
+
+ @Test
+ public void testHandleGetRequestWithAclQueryParam() throws Exception {
+ assertNotNull(aclHandler.handleGetRequest(BUCKET_NAME),
+ "Handler should handle request with ?acl param");
Review Comment:
This is verified by `testHandleGetRequestSucceeds`, which also checks
response status, and `testHandleGetRequestReturnsCorrectAclStructure`, which
further checks response structure. So I don't think this test case is needed.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java:
##########
@@ -79,6 +80,18 @@ public void clean() throws IOException {
}
}
+ /**
+ * Helper method to get ACL from bucket and validate response.
+ */
+ private S3BucketAcl getBucketAcl(String bucketName) throws Exception {
+ Response response = bucketEndpoint.get(bucketName);
+ assertEquals(HTTP_OK, response.getStatus(),
+ "GET ACL should return HTTP_OK");
Review Comment:
nit: no need to add a description for each simple assertion
--
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]
