HDDS-12794. Improve Lifecycle rule validity checking#9593
Conversation
| * @throws OMException if the prefix is a trash path | ||
| */ | ||
| public static void validateTrashPrefix(String prefix) throws OMException { | ||
| if (org.apache.commons.lang3.StringUtils.isEmpty(prefix)) { |
There was a problem hiding this comment.
Can we import the StringUtils?
| String key = entry.getKey(); | ||
| String value = entry.getValue(); | ||
|
|
||
| if (key == null || key.isEmpty() || |
| MAX_TAG_KEY_LENGTH, OMException.ResultCodes.INVALID_REQUEST); | ||
| } | ||
|
|
||
| if (value != null && value.getBytes(StandardCharsets.UTF_8).length > MAX_TAG_VALUE_LENGTH) { |
There was a problem hiding this comment.
StringUtils.isEmpty(value)
| /** | ||
| * Validates the OmLifecycleRuleAndOperator. | ||
| * Ensures the following: | ||
| * - Either tags or prefix must be specified. |
There was a problem hiding this comment.
Can we keep these comments?
|
|
||
| /** | ||
| * Validates the lifecycle rule. | ||
| * - ID length should not exceed the allowed limit |
There was a problem hiding this comment.
Can we keep these comments?
There was a problem hiding this comment.
There are currently multiple validation rules in place.
I'm concerned that if comments aren't properly maintained in the future, errors may occur. Therefore, it might be advisable to directly remove the information obtained from the method code.
There was a problem hiding this comment.
I believe that AWS will not change it's API spec and behavior frequently, so the validation logic is likely being stable. Besides, any future patch of this feature, likely at least one of us will help with the review, so we can guarantee that comments can get updated together with code change.
Let's keep these comments. It's very useful when I want to check that what's valid, and what's invalid of a configuration, than going through the code, as there are a few edge cases I would totally not sure about the conclusion without referring to these comments.
| "Lifecycle rule prefix cannot be trash root"); | ||
|
|
||
| // 2. Prefix too long | ||
| String longPrefix = RandomStringUtils.randomAlphanumeric(1025); |
There was a problem hiding this comment.
We can use (OmLifecycleUtils#MAX_PREFIX_LENGTH + 1 ) here and for below 1024 , so it will be very straightforward.
There was a problem hiding this comment.
Yes, more readable, updated
|
Thanks @xichen01 , overall it looks good to me. Just a few minor comments. |
3 participants
What changes were proposed in this pull request?
Add additional validations for Lifecycle Rules based on AWS S3's Lifecycle features and Ozone's inherent characteristics:
Ref:
[1] https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
[2] https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-tagging.html
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12794
How was this patch tested?
unit test.