SOLR-4127: Added explicit error message if users attempt Atomic document updates with either updateLog or DistribUpdateProcessor

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1420297 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -312,6 +312,9 @@ Bug Fixes
   same way as on startup when collection.configName is not explicitly passed.
   (Po Rui, Mark Miller)
 
+* SOLR-4127: Added explicit error message if users attempt Atomic document 
+  updates with either updateLog or DistribUpdateProcessor. (hossman)
+
 Other Changes
 ----------------------
 

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

@@ -457,11 +457,22 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
   private boolean versionAdd(AddUpdateCommand cmd) throws IOException {
     BytesRef idBytes = cmd.getIndexedId();
 
-    if (vinfo == null || idBytes == null) {
+    if (idBytes == null) {
       super.processAdd(cmd);
       return false;
     }
 
+    if (vinfo == null) {
+      if (isAtomicUpdate(cmd)) {
+        throw new SolrException
+          (SolrException.ErrorCode.BAD_REQUEST,
+           "Atomic document updates are not supported unless <updateLog/> is configured");
+      } else {
+        super.processAdd(cmd);
+        return false;
+      }
+    }
+
     // This is only the hash for the bucket, and must be based only on the uniqueKey (i.e. do not use a pluggable hash here)
     int bucketHash = Hash.murmurhash3_x86_32(idBytes.bytes, idBytes.offset, idBytes.length, 0);
 
@@ -580,21 +591,26 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
     return false;
   }
 
+  /**
+   * Utility method that examines the SolrInputDocument in an AddUpdateCommand
+   * and returns true if the documents contains atomic update instructions.
+   */
+  public static boolean isAtomicUpdate(final AddUpdateCommand cmd) {
+    SolrInputDocument sdoc = cmd.getSolrInputDocument();
+    for (SolrInputField sif : sdoc.values()) {
+      if (sif.getValue() instanceof Map) {
+        return true;
+      }
+    }
+    return false;
+  }
 
   // TODO: may want to switch to using optimistic locking in the future for better concurrency
   // that's why this code is here... need to retry in a loop closely around/in versionAdd
   boolean getUpdatedDocument(AddUpdateCommand cmd, long versionOnUpdate) throws IOException {
+    if (!isAtomicUpdate(cmd)) return false;
+
     SolrInputDocument sdoc = cmd.getSolrInputDocument();
-    boolean update = false;
-    for (SolrInputField sif : sdoc.values()) {
-      if (sif.getValue() instanceof Map) {
-        update = true;
-        break;
-      }
-    }
-
-    if (!update) return false;
-
     BytesRef id = cmd.getIndexedId();
     SolrInputDocument oldDoc = RealTimeGetComponent.getInputDocument(cmd.getReq().getCore(), id);
 

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

@@ -18,7 +18,8 @@
 package org.apache.solr.update.processor;
 
 import java.io.IOException;
-
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.update.*;
@@ -58,6 +59,13 @@ class RunUpdateProcessor extends UpdateRequestProcessor
 
   @Override
   public void processAdd(AddUpdateCommand cmd) throws IOException {
+    
+    if (DistributedUpdateProcessor.isAtomicUpdate(cmd)) {
+      throw new SolrException
+        (SolrException.ErrorCode.BAD_REQUEST,
+         "RunUpdateProcessor has recieved an AddUpdateCommand containing a document that appears to still contain Atomic document update operations, most likely because DistributedUpdateProcessorFactory was explicitly disabled from this updateRequestProcessorChain");
+    }
+
     updateHandler.addDoc(cmd);
     super.processAdd(cmd);
     changesSinceCommit = true;

solr/core/src/test-files/solr/collection1/conf/solrconfig-tlog.xml

@@ -22,6 +22,14 @@
   <directoryFactory name="DirectoryFactory" class="${solr.directoryFactory:solr.RAMDirectoryFactory}"/>
   <dataDir>${solr.data.dir:}</dataDir>
 
+  <!-- an update processor the explicitly excludes distrib to test
+       clean errors when people attempt atomic updates w/o it
+  -->
+  <updateRequestProcessorChain name="nodistrib" >
+   <processor class="solr.NoOpDistributingUpdateProcessorFactory" />
+   <processor class="solr.RunUpdateProcessorFactory" />
+ </updateRequestProcessorChain>
+
   <requestHandler name="standard" class="solr.StandardRequestHandler">
   </requestHandler>
 

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

@@ -0,0 +1,99 @@
+/*
+ * 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;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.update.DirectUpdateHandler2;
+import org.apache.solr.common.SolrException;
+
+public class TestAtomicUpdateErrorCases extends SolrTestCaseJ4 {
+
+  public void testUpdateNoTLog() throws Exception {
+    try {
+      initCore("solrconfig.xml","schema15.xml");
+      
+      UpdateHandler uh = h.getCore().getUpdateHandler();
+      assertTrue("this test requires DirectUpdateHandler2",
+                 uh instanceof DirectUpdateHandler2);
+
+      assertNull("this test requires that the updateLog not be enabled, it " +
+                 "seems that someone modified the configs",
+                 ((DirectUpdateHandler2)uh).getUpdateLog());
+      
+      // creating docs should work fine
+      addAndGetVersion(sdoc("id", "1", "val_i", "42"), null);
+      assertU(commit());
+
+      try {
+        ignoreException("updateLog");
+
+        // updating docs should fail
+        addAndGetVersion(sdoc("id", "1", "val_i", map("inc",-666)), null);
+        
+        fail("didn't get error about needing updateLog");
+      } catch (SolrException ex) {
+        assertEquals(400, ex.code());
+        // if the message doesn't match our expectation, wrap & rethrow
+        if (ex.getMessage().indexOf("unless <updateLog/> is configured") < 0) {
+          throw new RuntimeException("exception message is not expected", ex);
+        }
+      } finally {
+        resetExceptionIgnores();
+      }
+
+    } finally {
+      deleteCore();
+    }
+  }
+
+  public void testUpdateNoDistribProcessor() throws Exception {
+    try {
+      initCore("solrconfig-tlog.xml","schema15.xml");
+      
+      assertNotNull("this test requires an update chain named 'nodistrib'",
+                    h.getCore().getUpdateProcessingChain("nodistrib")); 
+
+
+      // creating docs should work fine
+      addAndGetVersion(sdoc("id", "1", "val_i", "42"), 
+                       params("update.chain","nodistrib"));
+      assertU(commit());
+
+      try {
+        ignoreException("DistributedUpdateProcessorFactory");
+
+        // updating docs should fail
+        addAndGetVersion(sdoc("id", "1", "val_i", map("inc",-666)), 
+                         params("update.chain","nodistrib"));
+        
+        fail("didn't get error about needing DistributedUpdateProcessorFactory");
+      } catch (SolrException ex) {
+        assertEquals(400, ex.code());
+        // if the message doesn't match our expectation, wrap & rethrow
+        if (ex.getMessage().indexOf("DistributedUpdateProcessorFactory") < 0) {
+          throw new RuntimeException("exception message is not expected", ex);
+        }
+      } finally {
+        resetExceptionIgnores();
+      }
+
+    } finally {
+      deleteCore();
+    }
+  }
+
+}