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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
