You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ds...@apache.org on 2019/06/20 16:20:54 UTC
[lucene-solr] branch branch_8x updated: SOLR-13523: Fix Atomic
Updates when _nest_path_ is declared. Change the most common test schema to
include this field so we better test our code paths.
This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/branch_8x by this push:
new 0d51dd0 SOLR-13523: Fix Atomic Updates when _nest_path_ is declared. Change the most common test schema to include this field so we better test our code paths.
0d51dd0 is described below
commit 0d51dd06eef381269b6dbca8845401bdbca82dc0
Author: David Smiley <ds...@apache.org>
AuthorDate: Thu Jun 20 11:59:22 2019 -0400
SOLR-13523: Fix Atomic Updates when _nest_path_ is declared.
Change the most common test schema to include this field so we better
test our code paths.
---
solr/CHANGES.txt | 6 +++-
.../processor/DistributedUpdateProcessor.java | 17 ++++++-----
.../solr/collection1/conf/schema-root.xml | 34 ++++++++++++++++++++++
.../test-files/solr/collection1/conf/schema.xml | 3 +-
.../transform/TestChildDocTransformer.java | 34 +++++++++++-----------
.../AtomicUpdateProcessorFactoryTest.java | 10 +++++--
.../solr/update/processor/AtomicUpdatesTest.java | 9 ------
.../UpdateRequestProcessorFactoryTest.java | 33 ++++++++++++++-------
8 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 016f1b6..073eb7b 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -163,7 +163,11 @@ Jetty 9.4.14.v20181114
Bug Fixes
----------------------
-SOLR-13510: Intermittent 401's for internode requests with basicauth enabled (Cao Manh Dat, Colvin Cowie)
+* SOLR-13510: Intermittent 401's for internode requests with basicauth enabled (Cao Manh Dat, Colvin Cowie)
+
+* SOLR-13523: In 8.1, Atomic Updates were broken (NPE) when the schema declared the new _nest_path_ field even if you
+ weren't using nested docs. In-place updates were not affected (worked).
+ Reminder: if you don't need nested docs then remove both _root_ and _nest_path_ ! (David Smiley)
================== 8.1.1 ==================
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 1b7c81b..11667c8 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
@@ -668,18 +668,19 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
// full (non-inplace) atomic update
SolrInputDocument sdoc = cmd.getSolrInputDocument();
- BytesRef id = cmd.getIndexedId();
- SolrInputDocument oldRootDocWithChildren = RealTimeGetComponent.getInputDocument(cmd.getReq().getCore(), id, RealTimeGetComponent.Resolution.ROOT_WITH_CHILDREN);
+ BytesRef idBytes = cmd.getIndexedId();
+ String idString = cmd.getPrintableId();
+ SolrInputDocument oldRootDocWithChildren = RealTimeGetComponent.getInputDocument(cmd.getReq().getCore(), idBytes, RealTimeGetComponent.Resolution.ROOT_WITH_CHILDREN);
if (oldRootDocWithChildren == null) {
if (versionOnUpdate > 0) {
// could just let the optimistic locking throw the error
- throw new SolrException(ErrorCode.CONFLICT, "Document not found for update. id=" + cmd.getPrintableId());
+ throw new SolrException(ErrorCode.CONFLICT, "Document not found for update. id=" + idString);
} else if (req.getParams().get(ShardParams._ROUTE_) != null) {
// the specified document could not be found in this shard
// and was explicitly routed using _route_
throw new SolrException(ErrorCode.BAD_REQUEST,
- "Could not find document " + idField.getName() + ":" + id +
+ "Could not find document id=" + idString +
", perhaps the wrong \"_route_\" param was supplied");
}
} else {
@@ -692,12 +693,12 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
// create a new doc by default if an old one wasn't found
mergedDoc = docMerger.merge(sdoc, new SolrInputDocument());
} else {
- if(req.getSchema().savesChildDocRelations() &&
- !sdoc.getField(idField.getName()).getFirstValue().toString()
- .equals((String) oldRootDocWithChildren.getFieldValue(IndexSchema.ROOT_FIELD_NAME))) {
+ String oldRootDocRootFieldVal = (String) oldRootDocWithChildren.getFieldValue(IndexSchema.ROOT_FIELD_NAME);
+ if(req.getSchema().savesChildDocRelations() && oldRootDocRootFieldVal != null &&
+ !idString.equals(oldRootDocRootFieldVal)) {
// this is an update where the updated doc is not the root document
SolrInputDocument sdocWithChildren = RealTimeGetComponent.getInputDocument(cmd.getReq().getCore(),
- id, RealTimeGetComponent.Resolution.DOC_WITH_CHILDREN);
+ idBytes, RealTimeGetComponent.Resolution.DOC_WITH_CHILDREN);
mergedDoc = docMerger.mergeChildDoc(sdoc, oldRootDocWithChildren, sdocWithChildren);
} else {
mergedDoc = docMerger.merge(sdoc, oldRootDocWithChildren);
diff --git a/solr/core/src/test-files/solr/collection1/conf/schema-root.xml b/solr/core/src/test-files/solr/collection1/conf/schema-root.xml
new file mode 100644
index 0000000..aef3c97
--- /dev/null
+++ b/solr/core/src/test-files/solr/collection1/conf/schema-root.xml
@@ -0,0 +1,34 @@
+<?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="root" version="1.6">
+ <field name="id" type="string" indexed="true" stored="true"/>
+
+ <!-- points to the root document of a block of nested documents -->
+ <field name="_root_" type="string" indexed="true" stored="false"/>
+
+ <field name="intDefault" type="int" indexed="true" stored="true" default="42" multiValued="false"/>
+ <field name="intDvoDefault" type="int" indexed="false" stored="false" multiValued="false"
+ useDocValuesAsStored="true" docValues="true" default="42" />
+
+ <dynamicField name="*" type="string" indexed="true" stored="true"/>
+
+ <fieldType name="string" class="solr.StrField"/>
+ <fieldType name="int" class="${solr.tests.IntegerFieldType}" docValues="${solr.tests.numeric.dv}" precisionStep="0" omitNorms="true" positionIncrementGap="0"/>
+
+ <uniqueKey>id</uniqueKey>
+</schema>
diff --git a/solr/core/src/test-files/solr/collection1/conf/schema.xml b/solr/core/src/test-files/solr/collection1/conf/schema.xml
index 5dd65bd..31fd9e5 100644
--- a/solr/core/src/test-files/solr/collection1/conf/schema.xml
+++ b/solr/core/src/test-files/solr/collection1/conf/schema.xml
@@ -507,7 +507,8 @@
<field name="id" type="string" indexed="true" stored="${solr.tests.id.stored:true}" multiValued="false" required="false" docValues="${solr.tests.id.docValues:false}"/>
- <field name="_root_" type="string" indexed="true" stored="true" multiValued="false" required="false"/>
+ <field name="_root_" type="string" indexed="true" stored="false"/>
+ <field name="_nest_path_" type="_nest_path_" /><fieldType name="_nest_path_" class="solr.NestPathField"/>
<field name="signatureField" type="string" indexed="true" stored="false"/>
<field name="uuid" type="uuid" stored="true"/>
diff --git a/solr/core/src/test/org/apache/solr/response/transform/TestChildDocTransformer.java b/solr/core/src/test/org/apache/solr/response/transform/TestChildDocTransformer.java
index 85b476a..bee3f22 100644
--- a/solr/core/src/test/org/apache/solr/response/transform/TestChildDocTransformer.java
+++ b/solr/core/src/test/org/apache/solr/response/transform/TestChildDocTransformer.java
@@ -36,7 +36,7 @@ public class TestChildDocTransformer extends SolrTestCaseJ4 {
@BeforeClass
public static void beforeClass() throws Exception {
- initCore("solrconfig.xml","schema.xml"); // *not* the "nest" schema version
+ initCore("solrconfig.xml","schema-root.xml"); // *not* the "nest" schema
}
@After
@@ -356,12 +356,12 @@ public class TestChildDocTransformer extends SolrTestCaseJ4 {
String[] tests = new String[] {
"/response/docs/[0]/id=='1'",
"/response/docs/[0]/_childDocuments_/[0]/id=='2'",
- "/response/docs/[0]/_childDocuments_/[0]/cat/[0]/=='childDocument'",
- "/response/docs/[0]/_childDocuments_/[0]/title/[0]/=='" + titleVals[0] + "'",
+ "/response/docs/[0]/_childDocuments_/[0]/cat=='childDocument'",
+ "/response/docs/[0]/_childDocuments_/[0]/title=='" + titleVals[0] + "'",
"/response/docs/[1]/id=='4'",
"/response/docs/[1]/_childDocuments_/[0]/id=='5'",
- "/response/docs/[1]/_childDocuments_/[0]/cat/[0]/=='childDocument'",
- "/response/docs/[1]/_childDocuments_/[0]/title/[0]/=='" + titleVals[1] + "'"
+ "/response/docs/[1]/_childDocuments_/[0]/cat=='childDocument'",
+ "/response/docs/[1]/_childDocuments_/[0]/title=='" + titleVals[1] + "'"
};
@@ -384,12 +384,12 @@ public class TestChildDocTransformer extends SolrTestCaseJ4 {
String[] tests = new String[] {
"/response/docs/[0]/id=='1'",
"/response/docs/[0]/children/docs/[0]/id=='2'",
- "/response/docs/[0]/children/docs/[0]/cat/[0]/=='childDocument'",
- "/response/docs/[0]/children/docs/[0]/title/[0]/=='" + titleVals[0] + "'",
+ "/response/docs/[0]/children/docs/[0]/cat=='childDocument'",
+ "/response/docs/[0]/children/docs/[0]/title=='" + titleVals[0] + "'",
"/response/docs/[1]/id=='4'",
"/response/docs/[1]/children/docs/[0]/id=='5'",
- "/response/docs/[1]/children/docs/[0]/cat/[0]/=='childDocument'",
- "/response/docs/[1]/children/docs/[0]/title/[0]/=='" + titleVals[1] + "'"
+ "/response/docs/[1]/children/docs/[0]/cat=='childDocument'",
+ "/response/docs/[1]/children/docs/[0]/title=='" + titleVals[1] + "'"
};
@@ -417,12 +417,12 @@ public class TestChildDocTransformer extends SolrTestCaseJ4 {
"//*[@numFound='2']",
"/response/result/doc[1]/str[@name='id']='1'" ,
"/response/result/doc[1]/doc[1]/str[@name='id']='2'" ,
- "/response/result/doc[1]/doc[1]/arr[@name='cat']/str[1]='childDocument'" ,
- "/response/result/doc[1]/doc[1]/arr[@name='title']/str[1]='" + titleVals[0] + "'" ,
+ "/response/result/doc[1]/doc[1]/str[@name='cat']='childDocument'" ,
+ "/response/result/doc[1]/doc[1]/str[@name='title']='" + titleVals[0] + "'" ,
"/response/result/doc[2]/str[@name='id']='4'" ,
"/response/result/doc[2]/doc[1]/str[@name='id']='5'",
- "/response/result/doc[2]/doc[1]/arr[@name='cat']/str[1]='childDocument'",
- "/response/result/doc[2]/doc[1]/arr[@name='title']/str[1]='" + titleVals[1] + "'"};
+ "/response/result/doc[2]/doc[1]/str[@name='cat']='childDocument'",
+ "/response/result/doc[2]/doc[1]/str[@name='title']='" + titleVals[1] + "'"};
assertQ(req("q", "*:*",
"sort", "id asc",
@@ -443,12 +443,12 @@ public class TestChildDocTransformer extends SolrTestCaseJ4 {
"//*[@numFound='2']",
"/response/result/doc[1]/str[@name='id']='1'" ,
"/response/result/doc[1]/result[@name='children'][@numFound=1]/doc[1]/str[@name='id']='2'" ,
- "/response/result/doc[1]/result[@name='children'][@numFound=1]/doc[1]/arr[@name='cat']/str[1]='childDocument'" ,
- "/response/result/doc[1]/result[@name='children'][@numFound=1]/doc[1]/arr[@name='title']/str[1]='" + titleVals[0] + "'" ,
+ "/response/result/doc[1]/result[@name='children'][@numFound=1]/doc[1]/str[@name='cat']='childDocument'" ,
+ "/response/result/doc[1]/result[@name='children'][@numFound=1]/doc[1]/str[@name='title']='" + titleVals[0] + "'" ,
"/response/result/doc[2]/str[@name='id']='4'" ,
"/response/result/doc[2]/result[@name='children'][@numFound=1]/doc[1]/str[@name='id']='5'",
- "/response/result/doc[2]/result[@name='children'][@numFound=1]/doc[1]/arr[@name='cat']/str[1]='childDocument'",
- "/response/result/doc[2]/result[@name='children'][@numFound=1]/doc[1]/arr[@name='title']/str[1]='" + titleVals[1] + "'"};
+ "/response/result/doc[2]/result[@name='children'][@numFound=1]/doc[1]/str[@name='cat']='childDocument'",
+ "/response/result/doc[2]/result[@name='children'][@numFound=1]/doc[1]/str[@name='title']='" + titleVals[1] + "'"};
assertQ(req(
"q", "*:*", "fq", "subject:\"parentDocument\" ",
diff --git a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateProcessorFactoryTest.java b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateProcessorFactoryTest.java
index 2a20846..1598bf4 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateProcessorFactoryTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateProcessorFactoryTest.java
@@ -27,6 +27,7 @@ import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.request.LocalSolrQueryRequest;
+import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.update.AddUpdateCommand;
import org.junit.BeforeClass;
@@ -231,9 +232,10 @@ public class AtomicUpdateProcessorFactoryTest extends SolrTestCaseJ4 {
cmd.solrDoc.addField("int_i", index);
try {
+ SolrQueryResponse rsp = new SolrQueryResponse();
factory.getInstance(cmd.getReq(), new SolrQueryResponse(),
- createDistributedUpdateProcessor(cmd.getReq(), new SolrQueryResponse(),
- new RunUpdateProcessor(cmd.getReq(), null))).processAdd(cmd);
+ createDistributedUpdateProcessor(cmd.getReq(), rsp,
+ createRunUpdateProcessor(cmd.getReq(), rsp, null))).processAdd(cmd);
} catch (IOException e) {
}
}
@@ -266,6 +268,10 @@ public class AtomicUpdateProcessorFactoryTest extends SolrTestCaseJ4 {
}
+ private UpdateRequestProcessor createRunUpdateProcessor(SolrQueryRequest req, SolrQueryResponse rsp, UpdateRequestProcessor next) {
+ return new RunUpdateProcessorFactory().getInstance(req, rsp, next);
+ }
+
private String generateRandomString() {
char[] chars = "abcdefghijklmnopqrstuvwxyz0123456789".toCharArray();
StringBuilder sb = new StringBuilder();
diff --git a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java
index 53798a1..6c66285 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java
@@ -1259,7 +1259,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=8"
);
// do atomic update
@@ -1274,7 +1273,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=8"
);
assertU(commit());
@@ -1288,7 +1286,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=8"
);
}
@@ -1311,7 +1308,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=7"
);
// do atomic update
assertU(adoc(sdoc("id", "7", fieldToUpdate, ImmutableMap.of("inc", -555))));
@@ -1325,7 +1321,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=7"
);
// diff doc where we check that we can overwrite the default value
@@ -1340,7 +1335,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=7"
);
// do atomic update
assertU(adoc(sdoc("id", "8", fieldToUpdate, ImmutableMap.of("inc", -555))));
@@ -1354,7 +1348,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=7"
);
assertU(commit());
@@ -1369,7 +1362,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=7"
);
assertQ(fieldToUpdate + ": doc8 post commit RTG"
, req("qt", "/get", "id", "8")
@@ -1381,7 +1373,6 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
, "//doc/long[@name='_version_']"
, "//doc/date[@name='timestamp']"
, "//doc/arr[@name='multiDefault']/str[.='muLti-Default']"
- , "count(//doc/*)=7"
);
}
diff --git a/solr/core/src/test/org/apache/solr/update/processor/UpdateRequestProcessorFactoryTest.java b/solr/core/src/test/org/apache/solr/update/processor/UpdateRequestProcessorFactoryTest.java
index 65f3eca..7d53b4f 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/UpdateRequestProcessorFactoryTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/UpdateRequestProcessorFactoryTest.java
@@ -114,16 +114,22 @@ public class UpdateRequestProcessorFactoryTest extends SolrTestCaseJ4 {
assertNotNull(name, chain);
// either explicitly, or because of injection
- assertEquals(name + " chain length: " + chain.toString(), EXPECTED_CHAIN_LENGTH,
+ assertEquals(name + " factory chain length: " + chain.toString(), EXPECTED_CHAIN_LENGTH,
chain.getProcessors().size());
// test a basic (non distrib) chain
proc = chain.createProcessor(req(), new SolrQueryResponse());
procs = procToList(proc);
- assertEquals(name + " procs size: " + procs.toString(),
- // -1 = NoOpDistributingUpdateProcessorFactory produces no processor
- EXPECTED_CHAIN_LENGTH - ("distrib-chain-noop".equals(name) ? 1 : 0),
- procs.size());
+
+ int expectedProcLen = EXPECTED_CHAIN_LENGTH;
+ if ("distrib-chain-noop".equals(name)) { // NoOpDistributingUpdateProcessorFactory produces no processor
+ expectedProcLen--;
+ }
+ if (procs.stream().anyMatch(p -> p.getClass().getSimpleName().equals("NestedUpdateProcessor"))) {
+ expectedProcLen++; // NestedUpdate sneaks in via RunUpdate's Factory.
+ }
+
+ assertEquals(name + " procs size: " + procs.toString(), expectedProcLen, procs.size());
// Custom comes first in all three of our chains
assertTrue(name + " first processor isn't a CustomUpdateRequestProcessor: " + procs.toString(),
@@ -159,12 +165,19 @@ public class UpdateRequestProcessorFactoryTest extends SolrTestCaseJ4 {
procs.get(procs.size()-1) instanceof RunUpdateProcessor );
// either 1 proc was droped in distrib mode, or 1 for the "implicit" chain
+
+ expectedProcLen = EXPECTED_CHAIN_LENGTH;
+ expectedProcLen--; // -1 = all chains lose CustomUpdateRequestProcessorFactory
+ if ("distrib-chain-explicit".equals(name) == false) {
+ // -1 = distrib-chain-noop: NoOpDistributingUpdateProcessorFactory produces no processor
+ // -1 = distrib-chain-implicit: does RemoveBlank before distrib
+ expectedProcLen--;
+ }
+ if (procs.stream().anyMatch(p -> p.getClass().getSimpleName().equals("NestedUpdateProcessor"))) {
+ expectedProcLen++; // NestedUpdate sneaks in via RunUpdate's Factory.
+ }
assertEquals(name + " (distrib) chain has wrong length: " + procs.toString(),
- // -1 = all chains lose CustomUpdateRequestProcessorFactory
- // -1 = distrib-chain-noop: NoOpDistributingUpdateProcessorFactory produces no processor
- // -1 = distrib-chain-implicit: does RemoveBlank before distrib
- EXPECTED_CHAIN_LENGTH - ( "distrib-chain-explicit".equals(name) ? 1 : 2),
- procs.size());
+ expectedProcLen, procs.size());
}
}