snoopdave commented on code in PR #225:
URL: https://github.com/apache/roller/pull/225#discussion_r4419569906
##########
app/src/main/java/org/apache/roller/weblogger/business/search/lucene/IndexOperation.java:
##########
@@ -152,10 +145,6 @@ protected Document getDocument(WeblogEntry data) {
doc.add(new TextField(FieldConstants.C_CONTENT, commentContent,
Field.Store.NO));
- // keyword
- doc.add(new StringField(FieldConstants.C_EMAIL, commentEmail,
- Field.Store.YES));
-
// keyword
doc.add(new StringField(FieldConstants.C_NAME, commentName,
Review Comment:
`c_name` has the same problem as `c_email`: stored, and read by nothing
outside this class. Can we drop it too?
##########
app/src/main/java/org/apache/roller/weblogger/business/search/lucene/SearchOperation.java:
##########
@@ -229,4 +231,71 @@ public void setLocale(String locale) {
this.locale = locale;
}
+
+ /**
+ * Query parser that accepts explicit field names only for the fields that
+ * searches are meant to cover. Other index fields hold data that is not
+ * shown on the weblog, so a query naming one is a parse error.
+ */
+ static class SearchFieldsQueryParser extends MultiFieldQueryParser {
Review Comment:
This works only if we override every method that builds a field query. A
Lucene upgrade that adds one would silently reopen the hole. Lucene 9.12 has
`Query.visit(QueryVisitor)`: parse normally, then walk the query and refuse any
field not in `SEARCH_FIELDS` (`consumeTerms` for terms, `acceptField` for
wildcard, prefix, regexp and range). `*:*` is a `MatchAllDocsQuery` with no
field, so no special case. About 15 lines instead of 7 overrides, and it covers
any query type.
##########
app/src/test/java/org/apache/roller/weblogger/business/search/IndexManagerTest.java:
##########
@@ -97,6 +100,13 @@ public void testBasicOperation() throws Exception {
WebloggerFactory.getWeblogger().getUrlStrategy());
assertEquals(1, result.getResults().size());
+ // index fields that are not search fields cannot be queried
+ String handlePrefix = testWeblog.getHandle().substring(0,
3).toLowerCase() + "*";
+ assertThrows(WebloggerException.class, () -> indexManager.search(
Review Comment:
`search` throws the same generic `WebloggerException` for any failure, so
this would also pass if search broke for another reason. Can we assert that a
valid query still returns results just before this one is refused?
##########
CHANGES.md:
##########
@@ -58,6 +58,11 @@
- **Startup fails if the XML-RPC parser cannot be configured.** Roller used to
log an error and continue. It now stops at startup, so check the log if a
custom XML parser is on the classpath.
+- **Search queries can name only the search fields.** A weblog search may
+ name entry content, title or comment text (for example `title:roller`). A
+ query that names another field is treated like a query that cannot be
+ parsed. Comment email addresses are no longer added to the search index;
+ rebuild the index after upgrading to remove those already there.
Review Comment:
Old indexes keep the emails until an admin reads this and rebuilds. Roller
already rebuilds at startup when the consistency marker says the index is
unclean. An index format version checked at startup could trigger one rebuild
after the upgrade, so the data leaves the disk without anyone having to act.
--
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]
