diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 1ac18d932b0..2f9bc541379 100644 --- a/solr/CHANGES.txt +++ b/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 ---------------------- diff --git a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java index 0bfe12015a6..44cfbbfefca 100644 --- a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java +++ b/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 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); diff --git a/solr/core/src/java/org/apache/solr/update/processor/RunUpdateProcessorFactory.java b/solr/core/src/java/org/apache/solr/update/processor/RunUpdateProcessorFactory.java index 3b6546923b3..5852709d23f 100644 --- a/solr/core/src/java/org/apache/solr/update/processor/RunUpdateProcessorFactory.java +++ b/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; diff --git a/solr/core/src/test-files/solr/collection1/conf/solrconfig-tlog.xml b/solr/core/src/test-files/solr/collection1/conf/solrconfig-tlog.xml index 0a19824e56c..a4dc6e937cd 100644 --- a/solr/core/src/test-files/solr/collection1/conf/solrconfig-tlog.xml +++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig-tlog.xml @@ -22,6 +22,14 @@ ${solr.data.dir:} + + + + + + diff --git a/solr/core/src/test/org/apache/solr/update/TestAtomicUpdateErrorCases.java b/solr/core/src/test/org/apache/solr/update/TestAtomicUpdateErrorCases.java new file mode 100644 index 00000000000..5dcf8d21b82 --- /dev/null +++ b/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 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(); + } + } + +}