SOLR-12167: Throw an exception, instead of just a warning, upon unknown atomic update

solr/CHANGES.txt

@@ -240,6 +240,9 @@ Improvements
 * SOLR-13337: Only request the minimum required number of terms from each shard when using terms.sort=index and none
   are discarded due to terms.min/maxcount (Morten Bøgeskov,Munendra S N via Mikhail Khludnev)
 
+* SOLR-12167: Throw an exception, instead of just a warning, when unknown atomic update operation is 
+  encountered (Munendra S N via Ishan Chattopadhyaya)
+
 Other Changes
 ----------------------
 

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

@@ -101,39 +101,36 @@ public class AtomicUpdateDocumentMerger {
         for (Entry<String,Object> entry : ((Map<String,Object>) val).entrySet()) {
           String key = entry.getKey();
           Object fieldVal = entry.getValue();
-          boolean updateField = false;
           switch (key) {
             case "add":
-              updateField = true;
               doAdd(toDoc, sif, fieldVal);
               break;
             case "set":
-              updateField = true;
               doSet(toDoc, sif, fieldVal);
               break;
             case "remove":
-              updateField = true;
               doRemove(toDoc, sif, fieldVal);
               break;
             case "removeregex":
-              updateField = true;
               doRemoveRegex(toDoc, sif, fieldVal);
               break;
             case "inc":
-              updateField = true;
               doInc(toDoc, sif, fieldVal);
               break;
             case "add-distinct":
-              updateField = true;
               doAddDistinct(toDoc, sif, fieldVal);
               break;
             default:
-              //Perhaps throw an error here instead?
+              Object id = toDoc.containsKey(idField.getName())? toDoc.getFieldValue(idField.getName()):
-              log.warn("Unknown operation for the an atomic update, operation ignored: " + key);
+                  fromDoc.getFieldValue(idField.getName());
-              break;
+              String err = "Unknown operation for the an atomic update, operation ignored: " + key;
+              if (id != null) {
+                err = err + " for id:" + id;
+              }
+              throw new SolrException(ErrorCode.BAD_REQUEST, err);
           }
           // validate that the field being modified is not the id field.
-          if (updateField && idField.getName().equals(sif.getName())) {
+          if (idField.getName().equals(sif.getName())) {
             throw new SolrException(ErrorCode.BAD_REQUEST, "Invalid update of id field: " + sif);
           }
 

solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java

@@ -1006,6 +1006,11 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
     assertQ(req("q", "cat:aaa", "indent", "true"), "//result[@numFound = '1']");
     assertQ(req("q", "cat:bbb", "indent", "true"), "//result[@numFound = '1']");
     assertQ(req("q", "cat:ccc", "indent", "true"), "//result[@numFound = '1']");
+
+    // update on id
+    doc = new SolrInputDocument();
+    doc.setField("id", ImmutableMap.of("set", "1001"));
+    assertFailedU(adoc(doc));
   }
 
   public void testAtomicUpdatesOnDateFields() throws Exception {
@@ -1190,7 +1195,7 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
     doc = new SolrInputDocument();
     doc.setField("id", "7");
     doc.setField("cat", ImmutableMap.of("whatever", "bbb"));
-    assertU(adoc(doc));
+    assertFailedU( adoc(doc));
     assertU(commit());
 
     assertQ(req("q", "cat:*", "indent", "true"), "//result[@numFound = '1']");
@@ -1198,6 +1203,45 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
     assertQ(req("q", "cat:bbb", "indent", "true"), "//result[@numFound = '0']");
     assertQ(req("q", "cat:ccc", "indent", "true"), "//result[@numFound = '1']");
 
+    // add a nested document;
+    doc = new SolrInputDocument();
+    doc.setField("id", "123");
+    doc.setField("cat", ImmutableMap.of("whatever", "ddd"));
+
+    SolrInputDocument childDoc = new SolrInputDocument();
+    childDoc.setField("id", "1231");
+    childDoc.setField("title", "title_nested");
+    doc.addChildDocument(childDoc);
+    assertFailedU(adoc(doc));
+    assertU(commit());
+
+    assertQ(req("q", "cat:*", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "cat:aaa", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "cat:ddd", "indent", "true"), "//result[@numFound = '0']");
+    assertQ(req("q", "title:title_nested", "indent", "true"), "//result[@numFound = '0']");
+    assertQ(req("q", "id:123", "indent", "true"), "//result[@numFound = '0']");
+    assertQ(req("q", "id:1231", "indent", "true"), "//result[@numFound = '0']");
+    assertQ(req("q", "cat:ccc", "indent", "true"), "//result[@numFound = '1']");
+
+    doc = new SolrInputDocument();
+    doc.setField("id", "123");
+    doc.setField("title", "title_parent");
+
+    childDoc = new SolrInputDocument();
+    childDoc.setField("id", "12311");
+    childDoc.setField("cat", "ddd");
+    childDoc.setField("title", "title_nested");
+    doc.addChildDocument(childDoc);
+    assertU(adoc(doc));
+    assertU(commit());
+
+    assertQ(req("q", "*:*", "indent", "true"), "//result[@numFound = '3']");
+    assertQ(req("q", "cat:aaa", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "cat:ddd", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "title:title_nested", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "id:123", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "id:12311", "indent", "true"), "//result[@numFound = '1']");
+    assertQ(req("q", "cat:ccc", "indent", "true"), "//result[@numFound = '1']");
   }
 
   public void testFieldsWithDefaultValuesWhenAtomicUpdatesAgainstTlog() {