SOLR-12127: set op with null or empty list val should be atomic update

* Inplace update supports set and inc operation but when null or
  empty list is specified with set op, then it should always be treated
  as atomic update since this case is equivalent to removing field
  from the document

solr/CHANGES.txt

@@ -177,6 +177,9 @@ Bug Fixes
 
 * SOLR-13545: ContentStreamUpdateRequest refused to close file (Colvin Cowie, Mikhail Khludnev)
 
+* SOLR-12127: Updates containing 'set' operation with value null or empty list should be considered as Atomic Update
+  (Oliver Kuldmäe, Munendra S N, Ishan Chattopadhyaya)
+
 Other Changes
 ----------------------
 

solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java

@@ -16,8 +16,6 @@
  */
 package org.apache.solr.update.processor;
 
-import static org.apache.solr.common.params.CommonParams.ID;
-
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
@@ -59,6 +57,8 @@ import org.apache.solr.util.RefCounted;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import static org.apache.solr.common.params.CommonParams.ID;
+
 /**
  * @lucene.experimental
  */
@@ -194,10 +194,16 @@ public class AtomicUpdateDocumentMerger {
         return Collections.emptySet();
       }
       // else it's a atomic update map...
-      for (String op : ((Map<String, Object>)fieldValue).keySet()) {
+      Map<String, Object> fieldValueMap = (Map<String, Object>)fieldValue;
+      for (String op : fieldValueMap.keySet()) {
+        Object obj = fieldValueMap.get(op);
         if (!op.equals("set") && !op.equals("inc")) {
           // not a supported in-place update op
           return Collections.emptySet();
+        } else if (op.equals("set") && (obj == null || (obj instanceof Collection && ((Collection) obj).isEmpty()))) {
+          // when operation is set and value is either null or empty list
+          // treat the update as atomic instead of inplace
+          return Collections.emptySet();
         }
         // fail fast if child doc
         if(isChildDoc(((Map<String, Object>) fieldValue).get(op))) {

solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java

@@ -178,6 +178,8 @@ public class TestInPlaceUpdatesDistrib extends AbstractFullDistribZkTestBase {
     resetDelays();
     reorderedDBQsResurrectionTest();
     resetDelays();
+    setNullForDVEnabledField();
+    resetDelays();
     
     // AwaitsFix this test fails easily
     // reorderedDBQsUsingUpdatedValueFromADroppedUpdate();
@@ -219,6 +221,45 @@ public class TestInPlaceUpdatesDistrib extends AbstractFullDistribZkTestBase {
     assertEquals(2, NONLEADERS.size());
   }
 
+  private void setNullForDVEnabledField() throws Exception {
+    // to test set=null
+    // should this test be here? As set null would be an atomic update
+    clearIndex();
+    commit();
+
+    buildRandomIndex(0);
+    float inplace_updatable_float = 1;
+
+    // update doc, set
+    index("id", 0, "inplace_updatable_float", map("set", inplace_updatable_float));
+
+    LEADER.commit();
+    SolrDocument sdoc = LEADER.getById("0");  // RTG straight from the index
+    assertEquals(inplace_updatable_float, sdoc.get("inplace_updatable_float"));
+    assertEquals("title0", sdoc.get("title_s"));
+    long version0 = (long) sdoc.get("_version_");
+
+    for (SolrClient client : NONLEADERS) {
+      SolrDocument doc = client.getById(String.valueOf(0), params("distrib", "false"));
+      assertEquals(inplace_updatable_float, doc.get("inplace_updatable_float"));
+      assertEquals(version0, doc.get("_version_"));
+    }
+
+    index("id", 0, "inplace_updatable_float", map("set", null));
+    LEADER.commit();
+
+    sdoc = LEADER.getById("0");  // RTG straight from the index
+    assertNull(sdoc.get("inplace_updatable_float"));
+    assertEquals("title0", sdoc.get("title_s"));
+    long version1 = (long) sdoc.get("_version_");
+
+    for (SolrClient client : NONLEADERS) {
+      SolrDocument doc = client.getById(String.valueOf(0), params("distrib", "false"));
+      assertNull(doc.get("inplace_updatable_float"));
+      assertEquals(version1, doc.get("_version_"));
+    }
+  }
+
   final int NUM_RETRIES = 100, WAIT_TIME = 50;
 
   // The following should work: full update to doc 0, in-place update for doc 0, delete doc 0

solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java

@@ -335,6 +335,27 @@ public class TestInPlaceUpdatesStandalone extends SolrTestCaseJ4 {
 
   }
 
+  @Test
+  public void testUpdateWithValueNull() throws Exception {
+    long doc = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 42), null);
+    assertU(commit("softCommit", "false"));
+
+    assertQ(req("q", "*:*", "fq", "inplace_updatable_float:[* TO *]"), "//*[@numFound='1']");
+    // RTG before update
+    assertJQ(req("qt","/get", "id","1", "fl","id,inplace_updatable_float,title_s"),
+        "=={'doc':{'id':'1', 'inplace_updatable_float':" + 42.0 + ",'title_s':" + "first" + "}}");
+
+    // set the value to null
+    doc = addAndAssertVersion(doc, "id", "1", "inplace_updatable_float", map("set", null));
+    assertU(commit("softCommit", "false"));
+
+    // numProducts should be 0
+    assertQ(req("q", "*:*", "fq", "inplace_updatable_float:[* TO *]"), "//*[@numFound='0']");
+    // after update
+    assertJQ(req("qt","/get", "id","1", "fl","id,inplace_updatable_float,title_s"),
+        "=={'doc':{'id':'1','title_s':first}}");
+  }
+
   @Test
   public void testDVUpdatesWithDBQofUpdatedValue() throws Exception {
     long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", "0"), null);