Skip to content

OAK-12352: Include diff indexes in the index definition status printer #3078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chibulcuteanu wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

OAK-12352: Include diff indexes in the index definition status printer #3078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions ...c/main/java/org/apache/jackrabbit/oak/plugins/index/inventory/IndexDefinitionPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,15 @@

import org.apache.felix.inventory.Format;
import org.apache.felix.inventory.InventoryPrinter;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.commons.json.JsonObject;
import org.apache.jackrabbit.oak.commons.json.JsopBuilder;
import org.apache.jackrabbit.oak.json.Base64BlobSerializer;
import org.apache.jackrabbit.oak.json.JsonSerializer;
import org.apache.jackrabbit.oak.plugins.index.IndexPathService;
import org.apache.jackrabbit.oak.plugins.index.diff.DiffIndex;
import org.apache.jackrabbit.oak.plugins.index.diff.DiffIndexMerger;
import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry;
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.apache.jackrabbit.oak.spi.state.NodeStateUtils;
import org.apache.jackrabbit.oak.spi.state.NodeStore;
Expand Down Expand Up @@ -69,6 +74,17 @@ public void print(PrintWriter printWriter, Format format, boolean isZip) {
NodeState idxState = NodeStateUtils.getNode(root, indexPath);
createSerializer(json).serialize(idxState);
}
// The "diff" indexes (diff.index / diff.index.optimizer) are not oak:QueryIndexDefinition nodes, so they

@ChlineSaurus ChlineSaurus Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detail (what confused Claude): In oak-doc/src/site/markdown/query/indexing.md we write:

The diff index consists of:

/oak:index/diff.index
- type: disabled
- jcr:primaryType: oak:QueryIndexDefinition
+ diff.json (nt:file)

This contradicts the jcr:primaryType actually used and recommended in the customer facing doc, but if following the oak doc the primary type is oak:QueryIndexDefinition and therefore picked up in the index printer.

@ChlineSaurus ChlineSaurus Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if one uses jcr:primaryType: oak:QueryIndexDefinition, we now end up with a duplicate key /oak:index/diff.index, so maybe worth changing the doc.

@chibulcuteanu chibulcuteanu Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. @thomasmueller , I will also create a small PR to change the docs.

// are not returned by the IndexPathService and would otherwise be missing from the output. Add them
// explicitly, rendering their diff.json payload as inline JSON so the pending diff is readable.
for (String name : new String[] {DiffIndexMerger.DIFF_INDEX, DiffIndexMerger.DIFF_INDEX_OPTIMIZER}) {

@bhabegger bhabegger Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, there is a lot of logic for something that should be a printer

@bhabegger bhabegger Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I see this kind of "work around", it very often means there is a design flaw somewhere. It's likely not the issue of this PR, but something else (the complexity of this PR is a symptom).

Before /oak:index/ contained a single type of object oak:QueryIndexDefinition. Now it can contain 2 types of objects oak:QueryIndexDefinition and these diff.index or diff.index.optimizer for those 2 it's not the type that is important but their name.

Before, printing was a natural walk through the tree outputting what ever was on it's path.

Maybe we can make this "printing" a bit cleaner by splitting this method up:
loop on the indexPathService.getIndexPaths() -> call a auxilary "printIndexDefinitionNode" method for this path

Move the logic to determine which diffIndexPaths to the indexPathService (where we add a method .getDiffPaths())
loop on those results and internally call an auxilary "printDiffNode" method (which is basically serializeDiffIndex, see my comments there).

String diffPath = "/oak:index/" + name;
NodeState idxState = NodeStateUtils.getNode(root, diffPath);
if (idxState.exists()) {
json.key(diffPath);
serializeDiffIndex(json, idxState);
}
}
json.endObject();
printWriter.print(JsopBuilder.prettyPrint(json.toString()));
}
Expand All @@ -81,4 +97,44 @@ public void setFilter(String filter) {
private JsonSerializer createSerializer(JsopBuilder json) {
return new JsonSerializer(json, filter, new Base64BlobSerializer());
}

/**
* Serialize a diff index node, inlining its {@code diff.json} payload as JSON. All other file child nodes are
* rendered as base64 blobs, for backward compatibility.
*/
private void serializeDiffIndex(JsopBuilder json, NodeState idxState) {
json.object();

@bhabegger bhabegger Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like so much that we have walking logic in this class and them more walking logic in the serializer class.

The role of this class is to delegate to JsonSerializer (which consists in walking a NodeState and outputting in JSON what it finds). So it does the walking... we shouldn't be doing it here. Why are we doing it here ? Because we want a special processing of a certain parts : the inlining of the diff.json file object. Why don't we just update the serializer (eventually with a config) to add inlining of certain nodes and the configure the serializer with that option here ?

Here we would not need the 2 extra loops as this would be taken care of by the serializer.

JsonSerializer serializer = createSerializer(json);
// definition properties (mirror the default filter, which drops :childOrder)
for (PropertyState p : idxState.getProperties()) {
if (":childOrder".equals(p.getName())) {
continue;
}
json.key(p.getName());
serializer.serialize(p);
}
// non-hidden child nodes other than diff.json, rendered normally
for (ChildNodeEntry child : idxState.getChildNodeEntries()) {

@ChlineSaurus ChlineSaurus Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this running with the right order of child nodes? Or does it even matter here?

(I struggle a bit to understand where OAK requires correct node ordering and where not)

String childName = child.getName();
if (childName.startsWith(":") || "diff.json".equals(childName)) {
continue;
}
json.key(childName);
createSerializer(json).serialize(child.getNodeState());

@bhabegger bhabegger Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the serializer so stateful that we need to recreate it ?

}
// diff.json: inline the JSON payload instead of a base64 blob
NodeState content = idxState.getChildNode("diff.json").getChildNode("jcr:content");
String diff = content.exists() ? DiffIndex.tryReadString(content.getProperty("jcr:data")) : null;
if (diff != null) {
json.key("diff.json");
try {
// Parse and re-serialize so a malformed diff.json cannot corrupt the whole output.
JsonObject.fromJson(diff, true).toJson(json);
} catch (Exception e) {
// Not valid JSON - keep the endpoint well-formed by emitting the raw payload as a string.
json.value(diff);
}
}
json.endObject();
}
}
32 changes: 32 additions & 0 deletions ...st/java/org/apache/jackrabbit/oak/plugins/index/inventory/IndexDefinitionPrinterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.io.PrintWriter;
import java.io.StringWriter;
import java.nio.charset.StandardCharsets;
import java.util.List;

import org.apache.felix.inventory.Format;
Expand All @@ -35,6 +36,7 @@
import org.json.simple.JSONValue;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -88,6 +90,36 @@ public void binaryProps() throws Exception{
assertTrue(updater.getIndexPaths().contains("/a"));
}

@Test
public void diffIndexInlined() throws Exception {
NodeBuilder builder = store.getRoot().builder();
NodeBuilder diffIndex = builder.child("oak:index").child("diff.index");
diffIndex.setProperty("type", "lucene");
diffIndex.setProperty("async", "async");
String payload = "{\"damAssetLucene\":{\"indexRules\":{\"dam:Asset\":"
+ "{\"properties\":{\"foo\":{\"propertyIndex\":true}}}}}}";
diffIndex.child("diff.json").child("jcr:content")
.setProperty("jcr:data", new ArrayBasedBlob(payload.getBytes(StandardCharsets.UTF_8)));
store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);

// diff.index is not returned by the path service, yet it must still appear in the output
when(pathService.getIndexPaths()).thenReturn(List.of());

String json = getJSON();
JSONObject o = (JSONObject) JSONValue.parseWithException(json);

JSONObject diff = (JSONObject) o.get("/oak:index/diff.index");
assertNotNull("diff.index should be present in the output", diff);
assertEquals("lucene", diff.get("type"));

// diff.json must be inlined as a JSON object, not a base64 blob string
Object diffJson = diff.get("diff.json");
assertTrue("diff.json should be inlined as a JSON object", diffJson instanceof JSONObject);
JSONObject damAsset = (JSONObject) ((JSONObject) diffJson).get("damAssetLucene");
assertNotNull(damAsset);
assertNotNull(damAsset.get("indexRules"));
}

private String getJSON() {
StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
Expand Down
Loading