HDDS-14123. Refactor BucketEndpoint#Put method

[echonesis]

What changes were proposed in this pull request?

Refactored the BucketEndpoint#put method by extracting ACL handling logic into dedicated handler classes, improving code modularity and maintainability through the Strategy pattern.

Changes

• Created AclHandler class to handle bucket ACL PUT operations
• Added BucketOperationHandler interface for pluggable operation handlers
• Refactored BucketEndpoint#put() to delegate ACL operations to AclHandler

Testing

• Added TestAclHandler
• Updated existing TestBucketAcl tests

What is the link to the Apache JIRA

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

How was this patch tested?

GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/20752426565

[adoroszlai]

Thanks @echonesis for working on this. I would like to simplify the implementation, but first need to do some refactoring in S3G to make it easier. (#9519 done, #9522 PR ready, HDDS-14204 in progress, HDDS-14198, maybe more)

[adoroszlai]

I don't think we should introduce BucketEndpointContext. The context duplicates "random" functionality of BucketEndpoint. It would likely need to grow when extracting other PUT operations (tagging, etc.), GET operations. We will also want to extract handlers from ObjectEndpoint.

Handlers can be considered specialised "mini" endpoints. Thus they should simply extend EndpointBase to inherit all required functionality (configuration, headers, request context, audit logging, etc.).

[adoroszlai]

I think this is too complex.

1. Query params are already available in a map via ContainerRequestContext.
2. Handlers should take care of audit logging and metrics. The logic to decide value of s3GAction should not be in centralized in BucketEndpoint.

It is better to let handlers inspect query params (and possibly other inputs, like headers) to decide if they should handle the request or not.

Something like (in AclHandler):

  private boolean shouldHandle() {
    return hasQueryParam(ACL);
  }

  public Response handlePutRequest(String bucketName, InputStream body) ... {
    if (!shouldHandle()) {
      return null;
    }
    ... handle bucket ACL put

Then we can simply try all handlers until one of them does the work:

for (BucketOperationHandler handler : handlers) {
  Response response = handler.handlePutRequest(bucketName, body);
  if (response != null) {
    return response;
  }
}

... continue handling in `BucketEndpoint`

[echonesis]

Thanks @adoroszlai for the review.
I will keep up with those PRs and make this simpler.

[adoroszlai]

Thanks @echonesis for updating the patch (and sorry for the late review after holidays).

[adoroszlai]

Please rename to BucketAclHandler. Object ACL is not implemented yet, but is planned (HDDS-9631).

[adoroszlai]

nit: Please do not format method signature like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters.

I know it's the same as it was in BucketEndpoint, but we can improve formatting when moving code to a new file.

(Applies to few other similar methods.)

[adoroszlai]

BucketOperationHandler will eventually handle other operations (get, etc.), so this should be simply handlers.

[adoroszlai]

Some handlers may not implement PUT, so let's make this a default method that return null.

[echonesis]

Thanks for pointing it out!
I will update it in the next commit.

[adoroszlai]

nit: please import static for USER and DEFAULT, then this can be written in a single line.

(Also two other similar statements below.)

[adoroszlai]

We can spy(aclHandler), no need to create new instance.

[adoroszlai]

This can be added in setup() to reduce duplication, then unset only in testHandlePutRequestWithoutAclQueryParam.

[adoroszlai]

Can simplify a bit:

assertSucceeds(() -> aclHandler.handlePutRequest(BUCKET_NAME, null));

[adoroszlai]

testHandlePutRequestWithReadHeader, testHandlePutRequestWithWriteHeader, testHandlePutRequestWithReadAcpHeader, testHandlePutRequestWithWriteAcpHeader, testHandlePutRequestWithFullControlHeader can be combined into a single @ParameterizedTest.

[adoroszlai]

nit: {} is unnecessary (in all assertThrows calls).

[adoroszlai]

These can be simplified to:

assertErrorResponse(S3ErrorTable.NOT_IMPLEMENTED,
    () -> aclHandler.handlePutRequest(BUCKET_NAME, null));

[adoroszlai]

Thanks @echonesis for updating the patch.

[echonesis]

Thanks @adoroszlai for the review and merge.