Skip to content

Commit

Permalink
SOLR-16681: Throw exception when attempting to replace uniqueKey via …
Browse files Browse the repository at this point in the history 
…fl in distributed request (apache#1384)

* fl=newid->id seems doesn't work in cloud
  • Loading branch information
@mkhludnev
mkhludnev authored Mar 8, 2023
1 parent 90c4c50 commit 3baf180
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 26 deletions.
3 changes: 3 additions & 0 deletions solr/CHANGES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,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
Expand Down
31 changes: 27 additions & 4 deletions solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1272,13 +1272,15 @@ protected void returnFields(ResponseBuilder rb, ShardRequest sreq) {
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
Expand Down Expand Up @@ -1323,9 +1325,11 @@ protected void returnFields(ResponseBuilder rb, ShardRequest sreq) {
(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 {
Expand All @@ -1338,9 +1342,28 @@ protected void returnFields(ResponseBuilder rb, ShardRequest sreq) {
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);
}
}
}

Expand Down
71 changes: 61 additions & 10 deletions solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
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;
Expand All @@ -35,6 +36,7 @@
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;
Expand All @@ -43,6 +45,7 @@
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;

/**
* @see TestPseudoReturnFields
Expand Down Expand Up @@ -91,27 +94,27 @@ public static void createMiniSolrCloudCluster() throws Exception {
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());
}
Expand All @@ -124,7 +127,18 @@ public void addUncommittedDoc99() throws Exception {
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());
}

Expand Down Expand Up @@ -234,7 +248,7 @@ public void testAllRealFields() throws Exception {
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);
Expand All @@ -245,12 +259,49 @@ public void testAllRealFields() throws Exception {
}
}

@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);
Expand Down Expand Up @@ -283,7 +334,7 @@ public void testScoreAndAllRealFields() throws Exception {
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);
Expand All @@ -300,7 +351,7 @@ public void testScoreAndAllRealFieldsRTG() throws Exception {
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);
Expand Down
11 changes: 9 additions & 2 deletions solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ public static void beforeClass() throws Exception {
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());
}

Expand Down Expand Up @@ -91,6 +91,13 @@ public void testCopyRename() {
"*//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()) {
Expand Down
34 changes: 24 additions & 10 deletions solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand All @@ -58,7 +58,7 @@ public void addUncommittedDoc99() {
// 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 {
Expand Down Expand Up @@ -110,6 +110,20 @@ public void testMultiValuedRTG() throws Exception {
"/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) {
Expand All @@ -121,7 +135,7 @@ public void testAllRealFields() {
"//result/doc/int[@name='val_i']",
"//result/doc/str[@name='ssto']",
"//result/doc/str[@name='subject']",
"//result/doc[count(*)=5]");
"//result/doc[count(*)=6]");
}
}

Expand All @@ -137,7 +151,7 @@ public void testAllRealFieldsRTG() {
"//doc/int[@name='val_i']",
"//doc/str[@name='ssto']",
"//doc/str[@name='subject']",
"//doc[count(*)=5]");
"//doc[count(*)=6]");
}
}
}
Expand Down Expand Up @@ -178,7 +192,7 @@ public void testScoreAndAllRealFields() {
"//result/doc/str[@name='ssto']",
"//result/doc/str[@name='subject']",
"//result/doc/float[@name='score']",
"//result/doc[count(*)=6]");
"//result/doc[count(*)=7]");
}
}

Expand All @@ -195,7 +209,7 @@ public void testScoreAndAllRealFieldsRTG() {
"//doc/int[@name='val_i']",
"//doc/str[@name='ssto']",
"//doc/str[@name='subject']",
"//doc[count(*)=5]");
"//doc[count(*)=6]");
}
}
}
Expand Down

0 comments on commit 3baf180

Please sign in to comment.