HDDS-13444. Unify isValidKeyPath implementation #9521
Conversation
| if (allowLeadingSlash && i == 0) { | ||
| // Allow empty at start for absolute paths | ||
| continue; | ||
| } |
There was a problem hiding this comment.
It seems this section is not included in original OMClientRequest.isValidKeyPath().
Please describe more details in the commit message section.
| * Add reference counter to an object instance. | ||
| */ | ||
| class ReferenceCounted<T> { | ||
| public class ReferenceCounted<T> { |
There was a problem hiding this comment.
Could we keep it package-private?
There was a problem hiding this comment.
I change to public cuz ci test error. I'll try to figure out the solution to keep it private.
| * Whether the pathname is valid. Currently prohibits relative paths, | ||
| * names which contain a ":" or "//", or other non-canonical paths. | ||
| */ | ||
| public static boolean isValidName(String src) { |
There was a problem hiding this comment.
nit: use path instead of src to keep it consistent with isValidKeyPath
There was a problem hiding this comment.
sure, thanks for the reminder!
| } | ||
|
|
||
| @Test | ||
| public void testIsValidKeyPath() throws OMException { |
There was a problem hiding this comment.
We could add some tests like
assertEquals("a/b/", OzoneFSUtils.isValidKeyPath("a/b/", true));assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("/a/b", false));
adoroszlai
added
the
code-cleanup
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @rich7420 for working on this, left few comments
| public static String isValidKeyPath(String path, boolean throwOnEmpty) throws OMException { | ||
| if (path.isEmpty()) { | ||
| if (throwOnEmpty) { | ||
| throw new OMException("Invalid KeyPath, empty keyName" + path, |
There was a problem hiding this comment.
| throw new OMException("Invalid KeyPath, empty keyName" + path, | |
| throw new OMException("Invalid KeyPath, empty keyName " + path, |
or we could instead remove path from the message as it is empty and the message already says empty KeyName.
| } | ||
|
|
||
| @Test | ||
| public void testIsValidKeyPath() throws OMException { |
There was a problem hiding this comment.
you can add some more test cases for key paths likefile.txt, ./file
4 participants
What changes were proposed in this pull request?
This PR unified the duplicated isValidKeyPath validation logic between OzoneFSUtils and OMClientRequest by extracting the common path validation into a shared private method validateKeyPathComponents, and made OMClientRequest.isValidKeyPath delegate to OzoneFSUtils.isValidKeyPath to remove code duplication.
What is the link to the Apache JIRA
HDDS-13444
How was this patch tested?
https://github.com/rich7420/ozone/actions/runs/20323872107