You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2022/02/24 17:02:48 UTC
[GitHub] [jackrabbit-oak] blackfor opened a new pull request #504: OAK-9708 : ignore index tag property when logging warn
blackfor opened a new pull request #504:
URL: https://github.com/apache/jackrabbit-oak/pull/504
Ensure that the presence of the index tag (which creates a property restriction ':indexTag') or index name options does not trigger a spurious WARN in the logs.
--
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
[GitHub] [jackrabbit-oak] fabriziofortino commented on a change in pull request #504: OAK-9708 : ignore index tag property when logging warn
Posted by GitBox <gi...@apache.org>.
fabriziofortino commented on a change in pull request #504:
URL: https://github.com/apache/jackrabbit-oak/pull/504#discussion_r814556830
##########
File path: oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/FulltextIndexPlanner.java
##########
@@ -308,12 +308,12 @@ public static void setUseActualEntryCount(boolean useActualEntryCount) {
if (queryFilterPattern != null) {
if (ft != null && !queryFilterPattern.matcher(ft.toString()).find()) {
plan.addAdditionalMessage(Level.WARN, "Potentially improper use of index " + definition.getIndexPath() + " with queryFilterRegex "
- + queryFilterPattern + " to search for value " + ft.toString());
+ + queryFilterPattern + " to search for value '" + ft.toString() + "'");
}
for (PropertyRestriction pr : filter.getPropertyRestrictions()) {
- if (!queryFilterPattern.matcher(pr.toString()).find()) {
+ if (!pr.propertyName.startsWith(":") && !queryFilterPattern.matcher(pr.toString()).find()) {
plan.addAdditionalMessage(Level.WARN, "Potentially improper use of index " + definition.getIndexPath() + " with queryFilterRegex "
- + queryFilterPattern + " to search for value " + pr.toString());
+ + queryFilterPattern + " to search for value '" + pr.toString() + "'");
Review comment:
no need to call `.toString()`
```suggestion
+ queryFilterPattern + " to search for value '" + pr + "'");
```
##########
File path: oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexImproperUsageCommonTest.java
##########
@@ -148,12 +148,22 @@ public void queryFilterRegexRestrictionsWarn() throws Exception {
root.commit();
assertEventually(() -> {
- assertThat(explain("select [jcr:path] from [nt:base] where [propa] = \"oak\""), containsString(indexOptions.getIndexType() + ":" + indexName));
- // List appender should not have any warn logs as we are searching under right descendant as per path restrictions
+ assertTrue(explain("select [jcr:path] from [nt:base] where [propa] = \"oak\"").contains(indexOptions.getIndexType() + ":" + indexName));
+ // List appender should not have any warn logs as we are searching using a term matching regex
assertFalse(isWarnMessagePresent(listAppender ,String.format(QUERY_FILTER_WARN_MESSAGE, indexName, regex)));
Review comment:
this is not right. With the changes to `QUERY_FILTER_WARN_MESSAGE`, now `String.format` expects 3 arguments. I think `"oak"` should be added.
##########
File path: oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexImproperUsageCommonTest.java
##########
@@ -148,12 +148,22 @@ public void queryFilterRegexRestrictionsWarn() throws Exception {
root.commit();
assertEventually(() -> {
- assertThat(explain("select [jcr:path] from [nt:base] where [propa] = \"oak\""), containsString(indexOptions.getIndexType() + ":" + indexName));
- // List appender should not have any warn logs as we are searching under right descendant as per path restrictions
+ assertTrue(explain("select [jcr:path] from [nt:base] where [propa] = \"oak\"").contains(indexOptions.getIndexType() + ":" + indexName));
+ // List appender should not have any warn logs as we are searching using a term matching regex
assertFalse(isWarnMessagePresent(listAppender ,String.format(QUERY_FILTER_WARN_MESSAGE, indexName, regex)));
+
assertTrue(explain("select [jcr:path] from [nt:base] where [propa] = \"ack\"").contains(indexOptions.getIndexType() + ":" + indexName));
- // List appender now will have warn log as we are searching under root(/) but index definition have include path restriction.
- assertTrue(isWarnMessagePresent(listAppender, String.format(QUERY_FILTER_WARN_MESSAGE, indexName, regex)));
+ // List appender now still have warn log as property restriction does not match regex restriction.
+ assertTrue(isWarnMessagePresent(listAppender, String.format(QUERY_FILTER_WARN_MESSAGE, indexName, regex, "ack")));
+
+ assertTrue(explain("select [jcr:path] from [nt:base] where [:propa] = \"uck\" and [propb] = \"oak\"").contains(indexOptions.getIndexType() + ":" + indexName));
Review comment:
this test is failing
##########
File path: oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/FulltextIndexPlanner.java
##########
@@ -308,12 +308,12 @@ public static void setUseActualEntryCount(boolean useActualEntryCount) {
if (queryFilterPattern != null) {
if (ft != null && !queryFilterPattern.matcher(ft.toString()).find()) {
plan.addAdditionalMessage(Level.WARN, "Potentially improper use of index " + definition.getIndexPath() + " with queryFilterRegex "
- + queryFilterPattern + " to search for value " + ft.toString());
+ + queryFilterPattern + " to search for value '" + ft.toString() + "'");
Review comment:
no need to call `.toString()`
```suggestion
+ queryFilterPattern + " to search for value '" + ft + "'");
```
--
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
[GitHub] [jackrabbit-oak] fabriziofortino merged pull request #504: OAK-9708 : ignore index tag property when logging warn
Posted by GitBox <gi...@apache.org>.
fabriziofortino merged pull request #504:
URL: https://github.com/apache/jackrabbit-oak/pull/504
--
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