You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ct...@apache.org on 2021/01/15 21:45:16 UTC
[lucene-solr] 05/38: SOLR-10860: Return proper error code for bad
input incase of inplace updates (#2121)
This is an automated email from the ASF dual-hosted git repository.
ctargett pushed a commit to branch jira/solr-13105-toMerge
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit e91d0e95b509f47f3ca5b6422fc18208b1f9c636
Author: S N Munendra <mu...@apache.org>
AuthorDate: Thu Jan 7 20:44:48 2021 +0530
SOLR-10860: Return proper error code for bad input incase of inplace updates (#2121)
Return proper error code on invalid value with in-place update.
Handle invalid value for inc op with the in-place update, uses toNativeType to convert increment value instead of direct parsing. Also, return an error when inc operation is specified for the non-numeric field
---
solr/CHANGES.txt | 2 +
.../processor/AtomicUpdateDocumentMerger.java | 79 +++++++++++++++-------
.../solr/update/TestInPlaceUpdatesDistrib.java | 18 ++++-
.../solr/update/TestInPlaceUpdatesStandalone.java | 32 +++++++++
.../solr/update/processor/AtomicUpdatesTest.java | 13 ++++
5 files changed, 118 insertions(+), 26 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index adc0820..b4551c5 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -296,6 +296,8 @@ Bug Fixes
* SOLR-15047: Fix collapse parser behavior when collapsing on numeric fields to differentiate '0' group from null group (hossman)
+ * SOLR-10860: Return proper error code for bad input in case of inplace updates (Tomas Eduardo Fernandez Lobbe, Munendra S N)
+
Other Changes
---------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java b/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java
index bdc4a72..1dd993a 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java
@@ -16,6 +16,24 @@
*/
package org.apache.solr.update.processor;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Stream;
+
import org.apache.commons.lang3.tuple.Pair;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.util.BytesRef;
@@ -43,15 +61,6 @@ import org.apache.solr.util.RefCounted;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.io.IOException;
-import java.lang.invoke.MethodHandles;
-import java.util.*;
-import java.util.Map.Entry;
-import java.util.function.BiConsumer;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-import java.util.stream.Stream;
-
import static org.apache.solr.common.params.CommonParams.ID;
/**
@@ -120,13 +129,8 @@ public class AtomicUpdateDocumentMerger {
doAddDistinct(toDoc, sif, fieldVal);
break;
default:
- Object id = toDoc.containsKey(idField.getName())? toDoc.getFieldValue(idField.getName()):
- fromDoc.getFieldValue(idField.getName());
- String err = "Unknown operation for the an atomic update, operation ignored: " + key;
- if (id != null) {
- err = err + " for id:" + id;
- }
- throw new SolrException(ErrorCode.BAD_REQUEST, err);
+ throw new SolrException(ErrorCode.BAD_REQUEST,
+ "Error:" + getID(toDoc, schema) + " Unknown operation for the an atomic update: " + key);
}
// validate that the field being modified is not the id field.
if (idField.getName().equals(sif.getName())) {
@@ -143,6 +147,15 @@ public class AtomicUpdateDocumentMerger {
return toDoc;
}
+ private static String getID(SolrInputDocument doc, IndexSchema schema) {
+ String id = "";
+ SchemaField sf = schema.getUniqueKeyField();
+ if (sf != null) {
+ id = "[doc="+doc.getFieldValue( sf.getName() )+"] ";
+ }
+ return id;
+ }
+
/**
* Given a schema field, return whether or not such a field is supported for an in-place update.
* Note: If an update command has updates to only supported fields (and _version_ is also supported),
@@ -470,6 +483,11 @@ public class AtomicUpdateDocumentMerger {
protected void doInc(SolrInputDocument toDoc, SolrInputField sif, Object fieldVal) {
SolrInputField numericField = toDoc.get(sif.getName());
SchemaField sf = schema.getField(sif.getName());
+
+ if (sf.getType().getNumberType() == null) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "'inc' is not supported on non-numeric field " + sf.getName());
+ }
+
if (numericField != null || sf.getDefaultValue() != null) {
// TODO: fieldtype needs externalToObject?
String oldValS = (numericField != null) ?
@@ -478,20 +496,23 @@ public class AtomicUpdateDocumentMerger {
sf.getType().readableToIndexed(oldValS, term);
Object oldVal = sf.getType().toObject(sf, term.get());
- String fieldValS = fieldVal.toString();
- Number result;
+ // behavior similar to doAdd/doSet
+ Object resObj = getNativeFieldValue(sf.getName(), fieldVal);
+ if (!(resObj instanceof Number)) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "Invalid input '" + resObj + "' for field " + sf.getName());
+ }
+ Number result = (Number)resObj;
if (oldVal instanceof Long) {
- result = ((Long) oldVal).longValue() + Long.parseLong(fieldValS);
+ result = ((Long) oldVal).longValue() + result.longValue();
} else if (oldVal instanceof Float) {
- result = ((Float) oldVal).floatValue() + Float.parseFloat(fieldValS);
+ result = ((Float) oldVal).floatValue() + result.floatValue();
} else if (oldVal instanceof Double) {
- result = ((Double) oldVal).doubleValue() + Double.parseDouble(fieldValS);
+ result = ((Double) oldVal).doubleValue() + result.doubleValue();
} else {
// int, short, byte
- result = ((Integer) oldVal).intValue() + Integer.parseInt(fieldValS);
+ result = ((Integer) oldVal).intValue() + result.intValue();
}
-
- toDoc.setField(sif.getName(), result);
+ toDoc.setField(sif.getName(), result);
} else {
toDoc.setField(sif.getName(), fieldVal);
}
@@ -553,7 +574,15 @@ public class AtomicUpdateDocumentMerger {
return val;
}
SchemaField sf = schema.getField(fieldName);
- return sf.getType().toNativeType(val);
+ try {
+ return sf.getType().toNativeType(val);
+ } catch (SolrException ex) {
+ throw new SolrException(SolrException.ErrorCode.getErrorCode(ex.code()),
+ "Error converting field '" + sf.getName() + "'='" +val+"' to native type, msg=" + ex.getMessage(), ex);
+ } catch (Exception ex) {
+ throw new SolrException( SolrException.ErrorCode.BAD_REQUEST,
+ "Error converting field '" + sf.getName() + "'='" +val+"' to native type, msg=" + ex.getMessage(), ex);
+ }
}
private static boolean isChildDoc(Object obj) {
diff --git a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java
index baf6320..3eccdfa 100644
--- a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java
+++ b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java
@@ -47,6 +47,7 @@ import org.apache.solr.cloud.AbstractFullDistribZkTestBase;
import org.apache.solr.cloud.ZkShardTerms;
import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.DocCollection;
@@ -56,18 +57,21 @@ import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.ExecutorUtil;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.common.util.TimeSource;
import org.apache.solr.index.NoMergePolicyFactory;
import org.apache.solr.update.processor.DistributedUpdateProcessor;
-import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.util.RefCounted;
import org.apache.solr.util.TimeOut;
import org.apache.zookeeper.KeeperException;
+import org.hamcrest.MatcherAssert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.hamcrest.core.StringContains.containsString;
+
/**
* Tests the in-place updates (docValues updates) for a one shard, three replica cluster.
*/
@@ -682,6 +686,18 @@ public class TestInPlaceUpdatesDistrib extends AbstractFullDistribZkTestBase {
assertTrue(currentVersion > version);
version = currentVersion;
+ // set operation with invalid value for field
+ SolrException e = expectThrows(SolrException.class,
+ () -> addDocAndGetVersion( "id", 100, "inplace_updatable_float", map("set", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
+ // inc operation with invalid inc value
+ e = expectThrows(SolrException.class,
+ () -> addDocAndGetVersion( "id", 100, "inplace_updatable_int", map("inc", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
// RTG from tlog(s)
for (SolrClient client : clients) {
final String clientDebug = client.toString() + (LEADER.equals(client) ? " (leader)" : " (not leader)");
diff --git a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java
index 2867935..91586ca 100644
--- a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java
+++ b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java
@@ -18,6 +18,7 @@
package org.apache.solr.update;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
@@ -48,6 +49,7 @@ import org.apache.solr.schema.IndexSchema;
import org.apache.solr.schema.SchemaField;
import org.apache.solr.update.processor.AtomicUpdateDocumentMerger;
import org.apache.solr.util.RefCounted;
+import org.hamcrest.MatcherAssert;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
@@ -122,6 +124,36 @@ public class TestInPlaceUpdatesStandalone extends SolrTestCaseJ4 {
}
@Test
+ public void testUpdateBadRequest() throws Exception {
+ final long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), null);
+ assertU(commit());
+
+ // invalid value with set operation
+ SolrException e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float", map("set", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
+ // invalid value with inc operation
+ e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float", map("inc", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
+ // inc op with null value
+ e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float", map("inc", null)));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("Invalid input 'null' for field inplace_updatable_float"));
+
+ e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float",
+ map("inc", new ArrayList<>(Collections.singletonList(123)))));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("Invalid input '[123]' for field inplace_updatable_float"));
+ }
+
+ @Test
public void testUpdatingDocValues() throws Exception {
long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), null);
long version2 = addAndGetVersion(sdoc("id", "2", "title_s", "second", "inplace_updatable_float", 42), null);
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 54e9f9d..a0c1402 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
@@ -24,13 +24,17 @@ import java.util.List;
import com.google.common.collect.ImmutableMap;
import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.util.DateMathParser;
+import org.hamcrest.MatcherAssert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
+import static org.hamcrest.core.StringContains.containsString;
+
public class AtomicUpdatesTest extends SolrTestCaseJ4 {
@BeforeClass
@@ -1420,6 +1424,15 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
assertQ(req("q", "id:123", "indent", "true"), "//result[@numFound = '1']");
assertQ(req("q", "id:12311", "indent", "true"), "//result[@numFound = '1']");
assertQ(req("q", "cat:ccc", "indent", "true"), "//result[@numFound = '1']");
+
+ // inc op on non-numeric field
+ SolrInputDocument invalidDoc = new SolrInputDocument();
+ invalidDoc.setField("id", "7");
+ invalidDoc.setField("cat", ImmutableMap.of("inc", "bbb"));
+
+ SolrException e = expectThrows(SolrException.class, () -> assertU(adoc(invalidDoc)));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("'inc' is not supported on non-numeric field cat"));
}
public void testFieldsWithDefaultValuesWhenAtomicUpdatesAgainstTlog() {