bhabegger commented on code in PR #2751:
URL: https://github.com/apache/jackrabbit-oak/pull/2751#discussion_r2833675335
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexTest.java:
##########
@@ -303,5 +305,73 @@ private void storeDiff(NodeStore store, String timestamp,
String json) throws Co
}
}
}
+
+ // verify @lucene and @elasticsearch are cleared
+ @Test
+ public void cleanedAndNormalizedRemoveAtLucene() {
+ assertEquals("{\n"
+ + " \"test\": 4,\n"
+ + " \"test@abc\": 3\n"
+ + "}",
+ DiffIndexMerger.cleanedAndNormalized(JsonObject.fromJson(
+ "{\"test@lucene\":1, \"test@elasticsearch\": 2,
\"test@abc\": 3, \"test\": 4}", true)).toString());
+ }
+
+ // verify @lucene and @elasticsearch are cleared
Review Comment:
```suggestion
// verify namespace prefixes are cleared
```
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/diff/JsonNodeUpdaterTest.java:
##########
@@ -142,20 +143,25 @@ public void store() throws CommitFailedException,
IOException {
JsonNodeUpdater.addOrReplace(builder, ns, "/test", "nt:test",
json.toString());
ns.merge(builder, new EmptyHook(), CommitInfo.EMPTY);
assertEquals("{\n"
- + " \"test\" : {\n"
- + " \"number\" : 1,\n"
- + " \"double2\" : 1.0,\n"
- + " \"jcr:primaryType\" : \"nt:test\",\n"
- + " \":childOrder\" : [ \"child2\" ],\n"
- + " \"child2\" : {\n"
- + " \"y\" : 2,\n"
- + " \"jcr:primaryType\" : \"nt:test\",\n"
- + " \":childOrder\" : [ ]\n"
+ + " \"test\": {\n"
+ + " \"number\": 1,\n"
+ + " \"double2\": 1.0,\n"
+ + " \"jcr:primaryType\": \"nt:test\",\n"
+ + " \":childOrder\": [\"child2\"],\n"
+ + " \"child2\": {\n"
+ + " \"y\": 2,\n"
+ + " \"jcr:primaryType\": \"nt:test\",\n"
+ + " \":childOrder\": []\n"
+ " }\n"
+ " }\n"
+ "}", reformatJson(JsonUtils.nodeStateToJson(ns.getRoot(),
5)));
}
+ String reformatJson(String json) {
+ // replace \r\n with \n
+ return JsopBuilder.prettyPrint(json);
+ }
+
Review Comment:
Any particular reason it was moved up ? (Non blocker)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexMerger.java:
##########
@@ -669,33 +712,68 @@ private static void addPrimaryType(String path,
JsonObject json) {
/**
* Merge a JSON diff into a target index definition.
*
- * @param path the path
+ * @param indexName the index name (for logging)
+ * @param path the path (relative to the index)
* @param diff the diff (what to merge)
* @param target where to merge into
+ * @param isNew whether the target node is newly created (didn't exist
before)
*/
- private void mergeInto(String path, JsonObject diff, JsonObject target) {
+ private void mergeInto(String indexName, String path, JsonObject diff,
JsonObject target,
+ boolean isNewNode) {
+ String pathForLogging = path.isEmpty() ? "the root" : "relative path "
+ path;
for (String p : diff.getProperties().keySet()) {
if (path.isEmpty()) {
if ("jcr:primaryType".equals(p)) {
continue;
}
}
+ if (!isNewNode) {
+ // for existing nodes, we do a few more checks before the merge
+ if (path.isEmpty() &&
REJECTED_TOP_LEVEL_PROPS_FOR_EXISTING_INDEX.contains(p)
+ && !target.getProperties().containsKey(p)) {
+ // at the top level, some properties (eg. selectionPolicy)
are not allowed to be added
+ // to an existing index
+ logWarn("{}: Ignoring new top-level property {} at {} for
existing index", indexName, p, pathForLogging);
+ continue;
+ }
+ if (REJECTED_ADDING_TO_EXISTING_PROPERTY.contains(p) &&
!target.getProperties().containsKey(p)) {
+ // some properties are not allowed to be added if the node
already exists
+ logWarn("{}: Ignoring new property \"{}\" at {} for
existing child", indexName, p, pathForLogging);
+ continue;
Review Comment:
Hmm, what happens when the diff is updated and rerun ? Are these properties
cleaned from the diff ? Otherwise this warning might pop up often. No ?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexMerger.java:
##########
@@ -830,6 +922,39 @@ private void log(String format, Object... arguments) {
}
}
+ /**
+ * Log a warning message and store it in a size-limited queue.
+ * The queue keeps the oldest entries and is limited to 100 entries or 1
MB total size.
+ *
+ * @param format the log message format
+ * @param arguments the log message arguments
+ */
+ public void logWarn(String format, Object... arguments) {
Review Comment:
```suggestion
public void collectAndlogWarn(String format, Object... arguments) {
```
Maybe ? To make it explicit that we are not just logging in fact.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexMerger.java:
##########
@@ -669,33 +712,68 @@ private static void addPrimaryType(String path,
JsonObject json) {
/**
* Merge a JSON diff into a target index definition.
*
- * @param path the path
+ * @param indexName the index name (for logging)
+ * @param path the path (relative to the index)
* @param diff the diff (what to merge)
* @param target where to merge into
+ * @param isNew whether the target node is newly created (didn't exist
before)
*/
- private void mergeInto(String path, JsonObject diff, JsonObject target) {
+ private void mergeInto(String indexName, String path, JsonObject diff,
JsonObject target,
+ boolean isNewNode) {
+ String pathForLogging = path.isEmpty() ? "the root" : "relative path "
+ path;
for (String p : diff.getProperties().keySet()) {
if (path.isEmpty()) {
if ("jcr:primaryType".equals(p)) {
continue;
}
}
+ if (!isNewNode) {
+ // for existing nodes, we do a few more checks before the merge
+ if (path.isEmpty() &&
REJECTED_TOP_LEVEL_PROPS_FOR_EXISTING_INDEX.contains(p)
+ && !target.getProperties().containsKey(p)) {
+ // at the top level, some properties (eg. selectionPolicy)
are not allowed to be added
+ // to an existing index
+ logWarn("{}: Ignoring new top-level property {} at {} for
existing index", indexName, p, pathForLogging);
+ continue;
+ }
+ if (REJECTED_ADDING_TO_EXISTING_PROPERTY.contains(p) &&
!target.getProperties().containsKey(p)) {
+ // some properties are not allowed to be added if the node
already exists
+ logWarn("{}: Ignoring new property \"{}\" at {} for
existing child", indexName, p, pathForLogging);
+ continue;
+ }
+ }
if (target.getProperties().containsKey(p)) {
- // we do not currently allow to overwrite most existing
properties
- if (p.equals("boost")) {
- // allow overwriting the boost value
- LOG.info("Overwrite property {} value at {}", p, path);
+ // we do not currently allow to overwrite most existing
properties,
+ // except for:
+ if (!path.isEmpty() &&
ALLOW_CHANGING_IN_EXISTING_PROPERTY.contains(p)) {
+ // allow overwriting the (eg.) boost value
target.getProperties().put(p, diff.getProperties().get(p));
+ } else if (path.isEmpty() &&
MERGE_MULTI_VALUES_STRINGS.contains(p)) {
+ // merge includedPaths, queryPaths, and tags,
+ // such that it contains more entries
+ // (if the property is not set, we would make it more
restrictive,
+ // which is not allowed)
+ TreeSet<String> oldSet =
JsonNodeUpdater.getStringSet(target.getProperties().get(p));
+ TreeSet<String> newSet =
JsonNodeUpdater.getStringSet(diff.getProperties().get(p));
+ TreeSet<String> mergedSet = new TreeSet<String>(oldSet);
+ mergedSet.addAll(newSet);
Review Comment:
And how could the user remove of property ? If that was his intent ?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java:
##########
@@ -58,21 +59,21 @@ public class DiffIndex {
* @param indexDefinitions the /oak:index node
*/
public static void applyDiffIndexChanges(NodeStore store, NodeBuilder
indexDefinitions) {
- JsonObject diffs = collectDiffs(indexDefinitions);
- if (diffs == null) {
- // nothing todo
- return;
+ JsonObject diffs = collectDiffs(indexDefinitions, MERGER);
Review Comment:
What is the rationale behind passing the MERGER object ? It seams only used
internally.
--
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]
