You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "nfsantos (via GitHub)" <gi...@apache.org> on 2023/12/19 16:34:45 UTC
[PR] OAK-10590 - Indexing job downloads and creates FFS with full node store if includedPaths is specified as a string instead of array of strings [jackrabbit-oak]
nfsantos opened a new pull request, #1254:
URL: https://github.com/apache/jackrabbit-oak/pull/1254
The includedPaths property of an index definition should be an array of strings. But a common mistake made by users is to define it as a String when it has a single element. That is, instead of:
```
"includedPaths": [ "/a/b"] ,
```
it is defined as:
```
"includedPaths": "/a/b",
```
If `includedPaths` is defined as a String, the indexing job would ignore its value and default to download the full node store and creates an FFS containing everything (except hidden paths). This will slow down significantly the indexing job, as it will negate any benefits from using regex filtering. And even if regex filtering is not enabled or cannot be used, using `/` as includedPaths will also result in the FFS containing more nodes than it should, which will once again slow down the indexing job.
The same is true for `excludedPaths`, but in this case the default is empty, so it will ignore the value of this property and will not exclude anything. This may in fact lead to parts of the node store being indexed that should not be indexed.
This PR makes the logic that reads the `includedPaths` and `excludedPaths` properties more lenient, by treating Strings as one-element arrays and issuing a warning with a suggested fix.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10590 - If includedPaths and excludedPaths are specified as a String instead of array of String, interpret them as a one-element array of Strings [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos merged PR #1254:
URL: https://github.com/apache/jackrabbit-oak/pull/1254
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10590 - Indexing job downloads and creates FFS with full node store if includedPaths is specified as a string instead of array of strings [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1254:
URL: https://github.com/apache/jackrabbit-oak/pull/1254#discussion_r1432444245
##########
oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/filter/PathFilter.java:
##########
@@ -164,26 +167,36 @@ public String toString() {
'}';
}
- private static Iterable<String> getStrings(NodeBuilder builder, String propertyName,
- Collection<String> defaultVal) {
+ /**
+ * Gets the value of a property of expected type Strings. However, if the property is of type String, interpret
+ * it as a single-element list instead of returning the default value.
+ *
+ * @return the value of the property if the property is set and is of type Strings or String, otherwise the default value
+ */
+ private static Iterable<String> getStringsLenient(NodeBuilder builder, String propertyName, Collection<String> defaultVal) {
PropertyState property = builder.getProperty(propertyName);
if (property != null && property.getType() == Type.STRINGS) {
return property.getValue(Type.STRINGS);
+ } else if (property != null && property.getType() == Type.STRING) {
+ String value = property.getValue(Type.STRING);
+ LOG.warn("Property \"{}\"=\"{}\" has type String but it should be array of String. Proceeding by treating it as a " +
Review Comment:
I would probably not log a warning. Instead, I would document that this is also supported.
Reason: we already have many many entries that are defined as string (mostly "/dummy") instead of array.
##########
oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/filter/PathFilter.java:
##########
@@ -164,26 +167,36 @@ public String toString() {
'}';
}
- private static Iterable<String> getStrings(NodeBuilder builder, String propertyName,
- Collection<String> defaultVal) {
+ /**
+ * Gets the value of a property of expected type Strings. However, if the property is of type String, interpret
+ * it as a single-element list instead of returning the default value.
+ *
+ * @return the value of the property if the property is set and is of type Strings or String, otherwise the default value
+ */
+ private static Iterable<String> getStringsLenient(NodeBuilder builder, String propertyName, Collection<String> defaultVal) {
PropertyState property = builder.getProperty(propertyName);
if (property != null && property.getType() == Type.STRINGS) {
return property.getValue(Type.STRINGS);
+ } else if (property != null && property.getType() == Type.STRING) {
+ String value = property.getValue(Type.STRING);
+ LOG.warn("Property \"{}\"=\"{}\" has type String but it should be array of String. Proceeding by treating it as a " +
Review Comment:
I would make it public and add a unit test case for 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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org