SOLR-17274: allow JSON atomic updates to use multiple modifiers or a modifier like 'set' as a field name if child docs not enabled (#2451)
* fix two problems with json atomic update [SOLR-17274]
- this modifies `isChildDoc` to always return false if child docs are
not possible according to the schema, and restores part of the
logic of `parseObjectFieldValue` so that it can handle
multiple modifiers
- re-enables the ability to use multiple modifiers in a single
update, like `{"add": "foo", "remove": "bar"}`
- re-enables the ability to have a field with a name like
`set` or `remove` and be able to use updates like
`{"set": {"set": "foo"}`
- both of these are possible now but only if the schema
doesn't support child/nested docs; if the schema does
support them, then functionality is unchanged and either
will continue throwing an exception
---------
Co-authored-by: Calvin Smith <casmith@lucasfilm.com>
Co-authored-by: David Smiley <dsmiley@apache.org>diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index a98bc9b..d231a3d 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -113,6 +113,9 @@
* SOLR-14115: Add linkconfig, cluster, and updateacls as commands to SolrCLI. Allow bin/solr commands to have parity with zkcli.sh commands. (Eric Pugh)
+* SOLR-17274: Allow JSON atomic updates to use multiple modifiers or a modifier like 'set' as a field name
+ if child docs are not enabled. (Calvin Smith, David Smiley)
+
Optimizations
---------------------
* SOLR-17257: Both Minimize Cores and the Affinity replica placement strategies would over-gather
diff --git a/solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java b/solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java
index 82231d0..48e095a 100644
--- a/solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java
+++ b/solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java
@@ -29,7 +29,6 @@
import java.lang.invoke.MethodHandles;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
@@ -43,6 +42,7 @@
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.params.UpdateParams;
+import org.apache.solr.common.util.CollectionUtil;
import org.apache.solr.common.util.ContentStream;
import org.apache.solr.common.util.JsonRecordReader;
import org.apache.solr.common.util.StrUtils;
@@ -50,6 +50,7 @@
import org.apache.solr.handler.UpdateRequestHandler;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.schema.IndexSchema;
import org.apache.solr.schema.SchemaField;
import org.apache.solr.update.AddUpdateCommand;
import org.apache.solr.update.CommitUpdateCommand;
@@ -657,21 +658,31 @@
// Is this a child doc (true) or a partial update (false)
if (isChildDoc(extendedSolrDocument)) {
return extendedSolrDocument;
- } else { // partial update
- assert extendedSolrDocument.size() == 1;
- final SolrInputField pair = extendedSolrDocument.iterator().next();
- return Collections.singletonMap(pair.getName(), pair.getValue());
+ } else { // partial update: can include multiple modifiers (e.g. 'add', 'remove')
+ Map<String, Object> map = CollectionUtil.newLinkedHashMap(extendedSolrDocument.size());
+ for (SolrInputField inputField : extendedSolrDocument) {
+ map.put(inputField.getName(), inputField.getValue());
+ }
+ return map;
}
}
/** Is this a child doc (true) or a partial update (false)? */
private boolean isChildDoc(SolrInputDocument extendedFieldValue) {
- if (extendedFieldValue.size() != 1) {
+ IndexSchema schema = req.getSchema();
+ // If schema doesn't support child docs, return false immediately, which
+ // allows people to do atomic updates with multiple modifiers (like 'add'
+ // and 'remove' for a single doc) and to do single-modifier updates for a
+ // field with a name like 'set' that is defined in the schema, both of
+ // which would otherwise fail.
+ if (!schema.isUsableForChildDocs()) {
+ return false;
+ } else if (extendedFieldValue.size() != 1) {
return true;
}
// if the only key is a field in the schema, assume it's a child doc
final String fieldName = extendedFieldValue.iterator().next().getName();
- return req.getSchema().getFieldOrNull(fieldName) != null;
+ return schema.getFieldOrNull(fieldName) != null;
// otherwise, assume it's "set" or some other verb for a partial update.
// NOTE: it's fundamentally ambiguous with JSON; this is a best effort try.
}
diff --git a/solr/core/src/test-files/solr/collection1/conf/atomic-update-json-test.xml b/solr/core/src/test-files/solr/collection1/conf/atomic-update-json-test.xml
new file mode 100644
index 0000000..820c13e
--- /dev/null
+++ b/solr/core/src/test-files/solr/collection1/conf/atomic-update-json-test.xml
@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<schema name="id-version-and-multivalued-name-field" version="1.1">
+ <fieldType name="string" class="solr.StrField" sortMissingLast="true" omitNorms="true"/>
+ <field name="id" type="string" indexed="true" stored="true"/>
+ <field name="_version_" type="string" indexed="true" stored="true"/>
+ <field name="name" type="string" indexed="true" stored="true" multiValued="true"/>
+ <!-- two fields (one multivalued, one not) for verifying correct behavior when
+ an atomic update modifier is used as a field name. -->
+ <field name="set" type="string" indexed="true" stored="true"/>
+ <field name="add" type="string" indexed="true" stored="true" multiValued="true"/>
+ <!-- If the following is uncommented, then the schema will support
+ nested docs, and all but one of the tests in AtomicUpdatesJsonTest.java
+ will fail. -->
+ <!-- <field name="_root_" type="string" indexed="true" stored="true"/> -->
+ <uniqueKey>id</uniqueKey>
+</schema>
diff --git a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateJsonTest.java b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateJsonTest.java
new file mode 100644
index 0000000..699e01c
--- /dev/null
+++ b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateJsonTest.java
@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.update.processor;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrInputDocument;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+// Tests atomic updates using JSON loader, since the existing
+// tests all use XML format, and there have been some atomic update
+// issues that were specific to the JSON format.
+public class AtomicUpdateJsonTest extends SolrTestCaseJ4 {
+
+ @BeforeClass
+ public static void beforeTests() throws Exception {
+ System.setProperty("enable.update.log", "true");
+ initCore("solrconfig.xml", "atomic-update-json-test.xml");
+ }
+
+ @Before
+ public void before() {
+ assertU(delQ("*:*"));
+ assertU(commit());
+ }
+
+ @Test
+ public void testSchemaIsNotUsableForChildDocs() throws Exception {
+ // the schema we loaded shouldn't be usable for child docs,
+ // since we're testing JSON loader functionality that only
+ // works in that case and is ambiguous if nested docs are supported.
+ assertFalse(h.getCore().getLatestSchema().isUsableForChildDocs());
+ }
+
+ @Test
+ public void testAddOne() throws Exception {
+ SolrInputDocument doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", new String[] {"aaa"});
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '0']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", Map.of("add", "bbb"));
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '1']");
+ }
+
+ @Test
+ public void testRemoveOne() throws Exception {
+ SolrInputDocument doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", new String[] {"aaa", "bbb"});
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '1']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", Map.of("remove", "bbb"));
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '0']");
+ assertQ(req("q", "name:aaa"), "//result[@numFound = '1']");
+ }
+
+ @Test
+ public void testRemoveMultiple() throws Exception {
+ SolrInputDocument doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", new String[] {"aaa", "bbb", "ccc"});
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '1']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField(
+ "name", Map.of("add", new String[] {"ddd", "eee"}, "remove", new String[] {"aaa", "ccc"}));
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:aaa"), "//result[@numFound = '0']");
+ assertQ(req("q", "name:ccc"), "//result[@numFound = '0']");
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '1']");
+ assertQ(req("q", "name:ddd"), "//result[@numFound = '1']");
+ assertQ(req("q", "name:eee"), "//result[@numFound = '1']");
+ }
+
+ @Test
+ public void testAddAndRemove() throws Exception {
+ SolrInputDocument doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", new String[] {"aaa", "bbb", "ccc"});
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:aaa"), "//result[@numFound = '1']");
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '1']");
+ assertQ(req("q", "name:ccc"), "//result[@numFound = '1']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", Map.of("add", "ddd", "remove", "bbb"));
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "name:ddd"), "//result[@numFound = '1']");
+ assertQ(req("q", "name:bbb"), "//result[@numFound = '0']");
+ assertQ(req("q", "name:ccc"), "//result[@numFound = '1']");
+ assertQ(req("q", "name:aaa"), "//result[@numFound = '1']");
+ }
+
+ @Test
+ public void testAtomicUpdateModifierNameSingleValued() throws Exception {
+ // Testing atomic update with a single-valued field named 'set'
+ SolrInputDocument doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("set", "setval");
+ doc.setField("name", new String[] {"aaa"});
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "set:setval"), "//result[@numFound = '1']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("set", Map.of("set", "modval"));
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "set:modval"), "//result[@numFound = '1']");
+ assertQ(req("q", "set:setval"), "//result[@numFound = '0']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ Map<String, String> removeSetField = new HashMap<>();
+ removeSetField.put("set", null);
+ doc.setField("set", removeSetField);
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "set:modval"), "//result[@numFound = '0']");
+ assertQ(req("q", "name:aaa"), "//result[@numFound = '1']");
+ }
+
+ @Test
+ public void testAtomicUpdateModifierNameMultiValued() throws Exception {
+ // Testing atomic update with a multi-valued field named 'add'
+ SolrInputDocument doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField("name", new String[] {"aaa"});
+ doc.setField("add", new String[] {"aaa", "bbb", "ccc"});
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "add:bbb"), "//result[@numFound = '1']");
+
+ doc = new SolrInputDocument();
+ doc.setField("id", "1");
+ doc.setField(
+ "add", Map.of("add", new String[] {"ddd", "eee"}, "remove", new String[] {"bbb", "ccc"}));
+ updateJ(jsonAdd(doc), null);
+ assertU(commit());
+ assertQ(req("q", "add:ddd"), "//result[@numFound = '1']");
+ assertQ(req("q", "add:eee"), "//result[@numFound = '1']");
+ assertQ(req("q", "add:aaa"), "//result[@numFound = '1']");
+ assertQ(req("q", "add:bbb"), "//result[@numFound = '0']");
+ assertQ(req("q", "add:ccc"), "//result[@numFound = '0']");
+ }
+}
