Re: [PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]

[via GitHub]

nfsantos merged PR #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]

[via GitHub]

steffenvan commented on PR #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878#issuecomment-2501282438

   Oh right my eyes must have deceived me. That one space character can be 
quite subtle, I can understand why someone might miss it 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]

[via GitHub]

nfsantos commented on PR #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878#issuecomment-2501268717

   > Should we add a test case where one of the paths start with a space? On 
another note, I thought we trimmed those string values?
   
   One of the tests is already testing paths that start with space. Let me know 
if you think we should add more tests.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]

[via GitHub]

nfsantos commented on code in PR #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878#discussion_r1858828301


##
oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/filter/PathFilter.java:
##
@@ -195,4 +197,12 @@ public boolean areAllDescendantsIncluded(String path) {
 }
 return false;
 }
+
+private static void checkPathsAreAbsolute(Iterable paths, String 
pathType) {
+for (String path : paths) {
+if (!PathUtils.isAbsolute(path)) {
+throw new IllegalStateException("Invalid path in " + pathType 
+ " paths list: " + path + ". Paths must be absolute.");

Review Comment:
   I agree it would be better, but the PathFilter constructor is already 
throwing `IllegalStateException` in a check it does in the constructor, so I 
prefer to keep it consistent and use the same exception.  



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]

[via GitHub]

steffenvan commented on PR #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878#issuecomment-2501180385

   Should we add a test case where one of the paths start with a space? On 
another note, I thought we trimmed those string values?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]

[via GitHub]

nit0906 commented on code in PR #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878#discussion_r1858760198


##
oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/filter/PathFilter.java:
##
@@ -195,4 +197,12 @@ public boolean areAllDescendantsIncluded(String path) {
 }
 return false;
 }
+
+private static void checkPathsAreAbsolute(Iterable paths, String 
pathType) {
+for (String path : paths) {
+if (!PathUtils.isAbsolute(path)) {
+throw new IllegalStateException("Invalid path in " + pathType 
+ " paths list: " + path + ". Paths must be absolute.");

Review Comment:
   would it be better to throw an IllegalArgumentException instead ? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org