You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by "nfsantos (via GitHub)" <gi...@apache.org> on 2024/11/26 15:00:07 UTC
[PR] OAK-11285 - Check if paths used in included/excluded Paths are absolute and fail if they are not [jackrabbit-oak]
nfsantos opened a new pull request, #1878:
URL: https://github.com/apache/jackrabbit-oak/pull/1878
The `includedPaths` and `excludedPaths` in an index definition must be absolute. If they are not or if they start with a space, then the indexing job may fail or hang. This PR adds a check to the `PathFilter` constructor to check that all paths are absolute and fail immediately if they are not.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@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]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@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]
Posted by "nit0906 (via GitHub)" <gi...@apache.org>.
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<String> 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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@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]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@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]
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@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]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
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<String> 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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@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]
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org