You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by mk...@apache.org on 2023/03/08 09:41:13 UTC
[solr] branch branch_9x updated: SOLR-16681: Throw exception when attempting to replace uniqueKey via fl in distributed request (#1384) (#1439)
This is an automated email from the ASF dual-hosted git repository.
mkhl pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new 491fcee5f9f SOLR-16681: Throw exception when attempting to replace uniqueKey via fl in distributed request (#1384) (#1439)
491fcee5f9f is described below
commit 491fcee5f9f683586ccdf6b87a1a603a46400b04
Author: Mikhail Khludnev <mk...@users.noreply.github.com>
AuthorDate: Wed Mar 8 12:41:06 2023 +0300
SOLR-16681: Throw exception when attempting to replace uniqueKey via fl in distributed request (#1384) (#1439)
* fl=newid->id seems doesn't work in cloud
---
solr/CHANGES.txt | 3 +
.../solr/handler/component/QueryComponent.java | 31 ++++++++--
.../solr/cloud/TestCloudPseudoReturnFields.java | 71 +++++++++++++++++++---
.../org/apache/solr/search/ReturnFieldsTest.java | 11 +++-
.../apache/solr/search/TestPseudoReturnFields.java | 34 ++++++++---
5 files changed, 124 insertions(+), 26 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 13252f5b62c..43e832cf8c4 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -308,6 +308,9 @@ Other Changes
* SOLR-16684: Keep solr's opennlp-tools version in sync with Lucene (janhoy)
+* SOLR-16681: Throw exception when attempting to replace uniqueKey via fl in distributed request,
+ e.g. fl=old_id:id,id:new_id (Mikhail Khludnev)
+
================== 9.1.1 ==================
Bug Fixes
diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
index eba85c48a4c..5efde4786ce 100644
--- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
+++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
@@ -1272,13 +1272,15 @@ public class QueryComponent extends SearchComponent {
if ((sreq.purpose & ShardRequest.PURPOSE_GET_FIELDS) != 0) {
boolean returnScores = (rb.getFieldFlags() & SolrIndexSearcher.GET_SCORES) != 0;
- String keyFieldName = rb.req.getSchema().getUniqueKeyField().getName();
+ final String uniqueKey = rb.req.getSchema().getUniqueKeyField().getName();
+ String keyFieldName = uniqueKey;
boolean removeKeyField = !rb.rsp.getReturnFields().wantsField(keyFieldName);
if (rb.rsp.getReturnFields().getFieldRenames().get(keyFieldName) != null) {
// if id was renamed we need to use the new name
keyFieldName = rb.rsp.getReturnFields().getFieldRenames().get(keyFieldName);
}
-
+ String lastKeyString = "<empty>";
+ Boolean shardDocFoundInResults = null;
for (ShardResponse srsp : sreq.responses) {
if (srsp.getException() != null) {
// Don't try to get the documents if there was an exception in the shard
@@ -1323,9 +1325,11 @@ public class QueryComponent extends SearchComponent {
(SolrDocumentList)
SolrResponseUtil.getSubsectionFromShardResponse(rb, srsp, "response", false));
for (SolrDocument doc : docs) {
- Object id = doc.getFieldValue(keyFieldName);
- ShardDoc sdoc = rb.resultIds.get(id.toString());
+ final Object id = doc.getFieldValue(keyFieldName);
+ lastKeyString = id.toString();
+ final ShardDoc sdoc = rb.resultIds.get(lastKeyString);
if (sdoc != null) {
+ shardDocFoundInResults = Boolean.TRUE;
if (returnScores) {
doc.setField("score", sdoc.score);
} else {
@@ -1338,9 +1342,28 @@ public class QueryComponent extends SearchComponent {
doc.removeFields(keyFieldName);
}
rb.getResponseDocs().set(sdoc.positionInResponse, doc);
+ } else {
+ if (shardDocFoundInResults == null) {
+ shardDocFoundInResults = Boolean.FALSE;
+ }
}
}
}
+ if (Objects.equals(shardDocFoundInResults, Boolean.FALSE) && !rb.resultIds.isEmpty()) {
+ String keyMsg =
+ !uniqueKey.equals(keyFieldName)
+ ? "(either " + uniqueKey + " or " + keyFieldName + ")"
+ : uniqueKey;
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "Unable to merge shard response. Perhaps uniqueKey "
+ + keyMsg
+ + " was erased by renaming via fl parameters."
+ + " Expecting:"
+ + rb.resultIds.keySet()
+ + ", a sample of keys received:"
+ + lastKeyString);
+ }
}
}
diff --git a/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java b/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java
index 885fa843764..c439cf9166c 100644
--- a/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java
+++ b/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java
@@ -23,6 +23,7 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.Random;
import org.apache.lucene.tests.util.TestUtil;
@@ -35,6 +36,7 @@ import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.client.solrj.response.schema.SchemaResponse.FieldResponse;
import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
@@ -43,6 +45,7 @@ import org.apache.solr.search.TestPseudoReturnFields;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
+import org.junit.Test;
/**
* @see TestPseudoReturnFields
@@ -91,27 +94,27 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
assertEquals(
0,
CLOUD_CLIENT
- .add(sdoc("id", "42", "val_i", "1", "ssto", "X", "subject", "aaa"))
+ .add(sdoc("id", "42", "newid", "420", "val_i", "1", "ssto", "X", "subject", "aaa"))
.getStatus());
assertEquals(
0,
CLOUD_CLIENT
- .add(sdoc("id", "43", "val_i", "9", "ssto", "X", "subject", "bbb"))
+ .add(sdoc("id", "43", "newid", "430", "val_i", "9", "ssto", "X", "subject", "bbb"))
.getStatus());
assertEquals(
0,
CLOUD_CLIENT
- .add(sdoc("id", "44", "val_i", "4", "ssto", "X", "subject", "aaa"))
+ .add(sdoc("id", "44", "newid", "440", "val_i", "4", "ssto", "X", "subject", "aaa"))
.getStatus());
assertEquals(
0,
CLOUD_CLIENT
- .add(sdoc("id", "45", "val_i", "6", "ssto", "X", "subject", "aaa"))
+ .add(sdoc("id", "45", "newid", "450", "val_i", "6", "ssto", "X", "subject", "aaa"))
.getStatus());
assertEquals(
0,
CLOUD_CLIENT
- .add(sdoc("id", "46", "val_i", "3", "ssto", "X", "subject", "ggg"))
+ .add(sdoc("id", "46", "newid", "460", "val_i", "3", "ssto", "X", "subject", "ggg"))
.getStatus());
assertEquals(0, CLOUD_CLIENT.commit().getStatus());
}
@@ -124,7 +127,18 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
assertEquals(
0,
CLOUD_CLIENT
- .add(sdoc("id", "99", "val_i", "1", "ssto", "X", "subject", "uncommitted"))
+ .add(
+ sdoc(
+ "id",
+ "99",
+ "newid",
+ "990",
+ "val_i",
+ "1",
+ "ssto",
+ "X",
+ "subject",
+ "uncommitted"))
.getStatus());
}
@@ -234,7 +248,7 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
SolrDocumentList docs = assertSearch(params("q", "*:*", "rows", "10", "fl", fl));
// shouldn't matter what doc we pick...
for (SolrDocument doc : docs) {
- assertEquals(fl + " => " + doc, 5, doc.size());
+ assertEquals(fl + " => " + doc, 5 + 1, doc.size());
assertTrue(fl + " => " + doc, doc.getFieldValue("id") instanceof String);
assertTrue(fl + " => " + doc, doc.getFieldValue("val_i") instanceof Integer);
assertTrue(fl + " => " + doc, doc.getFieldValue("subject") instanceof String);
@@ -245,12 +259,49 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
}
}
+ @Test
+ public void testCopyPk() throws Exception {
+ String fl = "oldid:id,newid";
+ SolrDocumentList docs = assertSearch(params("q", "*:*", "rows", "10", "fl", fl));
+ for (SolrDocument doc : docs) {
+ assertTrue(
+ fl + " => " + doc,
+ Arrays.asList("420", "430", "440", "450", "460")
+ .indexOf((String) doc.getFieldValue("newid"))
+ >= 0);
+ assertTrue(
+ fl + " => " + doc,
+ Arrays.asList("42", "43", "44", "45", "46").indexOf((String) doc.getFieldValue("oldid"))
+ >= 0);
+ }
+ }
+
+ public void testMovePk() throws Exception {
+ try {
+ String fl = "oldid:id,id:newid"; // "id:newid";//
+ SolrDocumentList docs = assertSearch(params("q", "*:*", "rows", "10", "fl", fl));
+ fail("attempting to move PK causes 400");
+ for (SolrDocument doc : docs) {
+ assertTrue(
+ fl + " => " + doc,
+ Arrays.asList("420", "430", "440", "450", "460")
+ .indexOf((String) doc.getFieldValue("id"))
+ >= 0);
+ }
+ } catch (SolrException sse) {
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, sse.code());
+ final String message = sse.getMessage().toLowerCase(Locale.ROOT);
+ assertTrue(message.contains("uniqueKey".toLowerCase(Locale.ROOT)));
+ assertTrue(message.contains("fl".toLowerCase(Locale.ROOT)));
+ }
+ }
+
public void testAllRealFieldsRTG() throws Exception {
// shouldn't matter if we use RTG (committed or otherwise)
for (String fl : TestPseudoReturnFields.ALL_REAL_FIELDS) {
for (int i : Arrays.asList(42, 43, 44, 45, 46, 99)) {
SolrDocument doc = getRandClient(random()).getById("" + i, params("fl", fl));
- assertEquals(fl + " => " + doc, 5, doc.size());
+ assertEquals(fl + " => " + doc, 5 + 1, doc.size());
assertTrue(fl + " => " + doc, doc.getFieldValue("id") instanceof String);
assertTrue(fl + " => " + doc, doc.getFieldValue("val_i") instanceof Integer);
assertTrue(fl + " => " + doc, doc.getFieldValue("subject") instanceof String);
@@ -283,7 +334,7 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
SolrDocumentList docs = assertSearch(params("q", "*:*", "rows", "10", "fl", fl));
// shouldn't matter what doc we pick...
for (SolrDocument doc : docs) {
- assertEquals(fl + " => " + doc, 6, doc.size());
+ assertEquals(fl + " => " + doc, 6 + 1, doc.size());
assertTrue(fl + " => " + doc, doc.getFieldValue("id") instanceof String);
assertTrue(fl + " => " + doc, doc.getFieldValue("score") instanceof Float);
assertTrue(fl + " => " + doc, doc.getFieldValue("val_i") instanceof Integer);
@@ -300,7 +351,7 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
for (String fl : TestPseudoReturnFields.SCORE_AND_REAL_FIELDS) {
for (int i : Arrays.asList(42, 43, 44, 45, 46, 99)) {
SolrDocument doc = getRandClient(random()).getById("" + i, params("fl", fl));
- assertEquals(fl + " => " + doc, 5, doc.size());
+ assertEquals(fl + " => " + doc, 5 + 1, doc.size());
assertTrue(fl + " => " + doc, doc.getFieldValue("id") instanceof String);
assertTrue(fl + " => " + doc, doc.getFieldValue("val_i") instanceof Integer);
assertTrue(fl + " => " + doc, doc.getFieldValue("subject") instanceof String);
diff --git a/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java b/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java
index 0db966e01f3..20cda788f33 100644
--- a/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java
+++ b/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java
@@ -45,9 +45,9 @@ public class ReturnFieldsTest extends SolrTestCaseJ4 {
System.setProperty("enable.update.log", "false"); // schema12 doesn't support _version_
initCore("solrconfig.xml", "schema12.xml");
String v = "how now brown cow";
- assertU(adoc("id", "1", "text", v, "text_np", v, "#foo_s", v));
+ assertU(adoc("id", "1", "new_id_s", "10", "text", v, "text_np", v, "#foo_s", v));
v = "now cow";
- assertU(adoc("id", "2", "text", v, "text_np", v));
+ assertU(adoc("id", "2", "new_id_s", "20", "text", v, "text_np", v));
assertU(commit());
}
@@ -91,6 +91,13 @@ public class ReturnFieldsTest extends SolrTestCaseJ4 {
"*//doc[1]/str[2][.='1'] ");
}
+ public void testMovePk() {
+ assertQ(
+ req("q", "id:1", "fl", "old_id:id,id:new_id_s"),
+ "//*[@numFound='1'] ",
+ "*//doc[1]/arr[@name='id']/str[.='10'] ");
+ }
+
@Test
public void testToString() {
for (Method m : SolrReturnFields.class.getMethods()) {
diff --git a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java
index c6c7c155a39..cd0836da9bc 100644
--- a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java
+++ b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java
@@ -45,11 +45,11 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
public static void beforeTests() throws Exception {
initCore("solrconfig-tlog.xml", "schema-pseudo-fields.xml");
- assertU(adoc("id", "42", "val_i", "1", "ssto", "X", "subject", "aaa"));
- assertU(adoc("id", "43", "val_i", "9", "ssto", "X", "subject", "bbb"));
- assertU(adoc("id", "44", "val_i", "4", "ssto", "X", "subject", "aaa"));
- assertU(adoc("id", "45", "val_i", "6", "ssto", "X", "subject", "aaa"));
- assertU(adoc("id", "46", "val_i", "3", "ssto", "X", "subject", "ggg"));
+ assertU(adoc("id", "42", "newid", "420", "val_i", "1", "ssto", "X", "subject", "aaa"));
+ assertU(adoc("id", "43", "newid", "430", "val_i", "9", "ssto", "X", "subject", "bbb"));
+ assertU(adoc("id", "44", "newid", "440", "val_i", "4", "ssto", "X", "subject", "aaa"));
+ assertU(adoc("id", "45", "newid", "450", "val_i", "6", "ssto", "X", "subject", "aaa"));
+ assertU(adoc("id", "46", "newid", "460", "val_i", "3", "ssto", "X", "subject", "ggg"));
assertU(commit());
}
@@ -58,7 +58,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
// uncommitted doc in transaction log at start of every test
// Even if an RTG causes ulog to re-open realtime searcher, next test method
// will get another copy of doc 99 in the ulog
- assertU(adoc("id", "99", "val_i", "1", "ssto", "X", "subject", "uncommitted"));
+ assertU(adoc("id", "99", "newid", "990", "val_i", "1", "ssto", "X", "subject", "uncommitted"));
}
public void testMultiValued() throws Exception {
@@ -110,6 +110,20 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
"/doc=={'val2_ss':10,'val_ss':1,'subject':'uncommitted'}");
}
+ public void testMovePk() {
+
+ for (String fl : new String[] {"oldid:id,id:newid,val_i,subject,ssto"}) {
+ assertQ(
+ "fl=" + fl + " ... all real fields",
+ req("q", "*:*", "rows", "1", "fl", fl),
+ "//result[@numFound='5']",
+ "//result/doc/str[@name='id'][.='420' or .='430' or .='440' or .='450' or .='460']",
+ "//result/doc/int[@name='val_i']",
+ "//result/doc/str[@name='ssto']",
+ "//result/doc/str[@name='subject']");
+ }
+ }
+
public void testAllRealFields() {
for (String fl : ALL_REAL_FIELDS) {
@@ -121,7 +135,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
"//result/doc/int[@name='val_i']",
"//result/doc/str[@name='ssto']",
"//result/doc/str[@name='subject']",
- "//result/doc[count(*)=5]");
+ "//result/doc[count(*)=6]");
}
}
@@ -137,7 +151,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
"//doc/int[@name='val_i']",
"//doc/str[@name='ssto']",
"//doc/str[@name='subject']",
- "//doc[count(*)=5]");
+ "//doc[count(*)=6]");
}
}
}
@@ -178,7 +192,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
"//result/doc/str[@name='ssto']",
"//result/doc/str[@name='subject']",
"//result/doc/float[@name='score']",
- "//result/doc[count(*)=6]");
+ "//result/doc[count(*)=7]");
}
}
@@ -195,7 +209,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
"//doc/int[@name='val_i']",
"//doc/str[@name='ssto']",
"//doc/str[@name='subject']",
- "//doc[count(*)=5]");
+ "//doc[count(*)=6]");
}
}
}