add prefix/bucket_region params to get_all_buckets() method

[mozawa]

add prefix/bucket_region params to get_all_buckets() method

• HS-61145 - [ S3 Compatibility ] prefix for ListBuckets
• HS-61142 - [ S3 Compatibility ] bucket-region for ListBuckets

[hysh]

PR Review: Add prefix/bucket_region params to get_all_buckets()

Summary

This PR adds support for prefix and bucket-region parameters to the get_all_buckets() method and refactors the query string construction to be more maintainable.

✅ Positives

1. Cleaner Code: The refactoring from repetitive if-else blocks to a dictionary-based approach with list comprehension is much more readable and maintainable.

2. Feature Addition: Properly adds the two new S3 ListBuckets API parameters (prefix and bucket-region) as referenced in the Jira tickets.

3. Backward Compatible: The changes maintain backward compatibility - all existing parameters remain optional with default None values.

4. Consistent Handling: The shared parameter special case (flag without value) is handled correctly by inserting it at the beginning of the query list.

⚠️ Potential Issues

1. URL Encoding Not Applied: The PR doesn't URL-encode parameter values. While this is consistent with the existing code for max-buckets and continuation-token, the prefix parameter could potentially contain special characters (spaces, slashes, etc.) that should be encoded.

   Looking at line 229 in boto/s3/bucket.py, there's precedent for using urllib.parse.quote() for user-provided values:

   query_args_l.append('%s=%s' % (rk, urllib.parse.quote(rv)))

   Recommendation: Consider encoding at least the prefix parameter:

   params = {
       'max-buckets': max_buckets,
       'continuation-token': continuation_token,
       'prefix': urllib.parse.quote(prefix) if prefix else None,
       'bucket-region': bucket_region,
   }

2. No Input Validation: There's no validation on the new parameters (e.g., checking if bucket_region is a valid region). However, this is consistent with the existing code style in this file, which appears to rely on the server to validate inputs.

📝 Minor Notes

• The f-string usage (f"{k}={v}") is fine and more modern than % formatting
• The logic correctly filters out None values with the if v is not None check
• The & joining logic is correct

Verdict

The PR is good to merge with the caveat that URL encoding should be considered as a follow-up enhancement for the prefix parameter if users encounter issues with special characters. The refactoring improves code quality significantly.

Suggested priority: Add URL encoding for prefix parameter before merging to prevent potential issues with bucket names containing special characters.

[mozawa]

⚠️ Potential Issues

1. URL Encoding Not Applied: The PR doesn't URL-encode parameter values. While this is consistent with the existing code for max-buckets and continuation-token, the prefix parameter could potentially contain special characters (spaces, slashes, etc.) that should be encoded.
   Looking at line 229 in boto/s3/bucket.py, there's precedent for using urllib.parse.quote() for user-provided values:

   query_args_l.append('%s=%s' % (rk, urllib.parse.quote(rv)))

   Recommendation: Consider encoding at least the prefix parameter:

   params = {
       'max-buckets': max_buckets,
       'continuation-token': continuation_token,
       'prefix': urllib.parse.quote(prefix) if prefix else None,
       'bucket-region': bucket_region,
   }

Suggested priority: Add URL encoding for prefix parameter before merging to prevent potential issues with bucket names containing special characters.

I'm ok to add URL encoding for prefix parameter but I don't think it's necessary as general purpose buckets name rules are restricted as follows.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html#general-purpose-bucket-names