HDDS-15616. ListBuckets API ignores MaxBuckets parameter and returns all buckets#10639
HDDS-15616. ListBuckets API ignores MaxBuckets parameter and returns all buckets#10639Gargi-jais11 wants to merge 1 commit into
Conversation
| final int maxBuckets = paginated | ||
| ? validateMaxBuckets(queryParams().getInt(QueryParams.MAX_BUCKETS, | ||
| S3Consts.MAX_BUCKETS_LIMIT)) | ||
| : Integer.MAX_VALUE; |
There was a problem hiding this comment.
can we not do this logic inside the method itself validateMaxBuckets, actually this isn't validate, it is like getMaxBuckets, because it does
return Math.min(maxBuckets, S3Consts.MAX_BUCKETS_LIMIT);
Maybe we can drop the ternary here and do all this logic inside the method
| clientStub.getObjectStore().createS3Bucket(bucketBaseName + i); | ||
| } | ||
|
|
||
| rootEndpoint.queryParamsForTest().setInt(QueryParams.MAX_BUCKETS, 2); |
There was a problem hiding this comment.
could the listWithMaxBuckets be used here?
| found.add(page.getBuckets().get(0).getName()); | ||
| continuationToken = page.getContinuationToken(); | ||
| } while (continuationToken != null | ||
| && !(found.contains(bucketA) && found.contains(bucketB))); |
There was a problem hiding this comment.
the second condition is unnecessary, pagination should naturally terminate when there are no elements remaining.
We can maybe assert that it returns exactly 2 elements which are bucketA & bucketB
| } while (continuationToken != null | ||
| && !(found.contains(bucketA) && found.contains(bucketB))); | ||
|
|
||
| assertThat(found).contains(bucketA, bucketB); |
There was a problem hiding this comment.
maybe assertThat(found).containsExactlyInAnyOrder(bucketA, bucketB);
| * when more buckets exist. | ||
| */ | ||
| @Test | ||
| public void testListBucketsPaginatedMaxBucketsOne() throws Exception { |
There was a problem hiding this comment.
is this a almost a copy from the V1 test, we should ideally refactor to make sure we are not having the same code both places.
unless that is some established practice for the particular case
What changes were proposed in this pull request?
S3 test test_list_buckets_paginated is failing today as ListBuckets pagination using MaxBuckets and ContinuationToken on GET / (ListAllMyBuckets) is not supported.
creates 2 buckets and calls list_buckets(MaxBuckets=1).
Expected: 1 bucket per page + ContinuationToken when more exist.
Ozone: Ignores MaxBuckets and returns both buckets → assert len == 1 fails with 2.
Simple example:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15616
How was this patch tested?
Added UT and IT.
Also tested manually.
Before Fix:
After Fix: