SOLR-17274: allow JSON atomic updates to use multiple modifiers or a modifier like 'set' as a field name if child docs not enabled

solr/CHANGES.txt

@@ -106,6 +106,9 @@ Improvements
 
 * 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

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,13 +42,15 @@
 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;
 import org.apache.solr.handler.RequestHandlerUtils;
 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 @@ private Object parseObjectFieldValue(int ev, String fieldName) throws IOExceptio
       // 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.
     }

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>

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']");
+  }
+}