ChlineSaurus commented on code in PR #3091:
URL: https://github.com/apache/jackrabbit-oak/pull/3091#discussion_r3892282378
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -199,8 +211,24 @@ protected boolean indexFacetProperty(Document doc, int
tag, PropertyState proper
} else if (tag == Type.STRING.tag()) {
String value = property.getValue(Type.STRING);
if (!value.isEmpty()) {
- doc.add(new SortedSetDocValuesFacetField(pname, value));
- fieldAdded = true;
+ if (FT_OAK_12372_DISABLE.get()) {
+ // Legacy mode
+ doc.add(new SortedSetDocValuesFacetField(pname,
value));
+ fieldAdded = true;
+ } else {
+ // Category path = pname + "/" + value --> cannot be
longer than the Lucene limit of 8191
+ int categoryPathLength = pname.length() +
value.length() + 1;
+ if (categoryPathLength >
FacetLabel.MAX_CATEGORY_PATH_LENGTH) {
+ if
(!LOG_SILENCER.silence(LOG_KEY_IGNORING_LONG_FACET_PROPERTY)) {
+ LOG.warn("[{}] Ignoring long facet property.
Property {} is too long (name + value length: {})"
Review Comment:
Just a thought: would it be worth to log the node path? (to make it easier
to identify where the issue occurred)?
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
Review Comment:
We don't protect the path for multi-values. I'm not sure, but it might be
worth capturing both in this PR and with this feature toggle. 🤔
Test that still fails with the same exception:
```
@Test
public void
facetPropertyMultiValuedExceedingMaxCategoryPathLengthStillThrows() throws
IOException {
LuceneIndexDefinitionBuilder builder = new
LuceneIndexDefinitionBuilder();
builder.indexRule("nt:base")
.property("foo")
.propertyIndex()
.facets();
LuceneIndexDefinition defn =
LuceneIndexDefinition.newLuceneBuilder(root, builder.build(), "/foo").build();
LuceneDocumentMaker docMaker = new LuceneDocumentMaker(null,
FacetsConfig::new, null, defn,
defn.getApplicableIndexingRule("nt:base"), "/x");
NodeBuilder test = EMPTY_NODE.builder();
// Multi-valued facet with a value well over Lucene's 8191
category-path limit
test.setProperty("foo", List.of("a".repeat(10000)), Type.STRINGS);
boolean originalFtValue =
LuceneDocumentMaker.FT_OAK_12372_DISABLE.get();
LuceneDocumentMaker.FT_OAK_12372_DISABLE.set(false); // fix enabled
(default) -- still not guarded here
docMaker.makeDocument(test.getNodeState());
}
```
--
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]
