HDDS-13444. Unify isValidKeyPath implementation#9521
HDDS-13444. Unify isValidKeyPath implementation#9521ivandika3 merged 7 commits intoapache:masterfrom
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
|
@sreejasahithi thanks for the review! |
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @rich7420 , overall LGTM.
|
@sreejasahithi thanks for the review! |
|
Thanks @rich7420 for the patch and @echonesis @sreejasahithi for the reviews. |
5 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