mirror of https://github.com/apache/lucene.git
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
This commit is contained in:
parent
96aaf543af
commit
d4fa1aae21
|
@ -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-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
|
Other Changes
|
||||||
---------------------
|
---------------------
|
||||||
|
|
||||||
|
|
|
@ -16,6 +16,24 @@
|
||||||
*/
|
*/
|
||||||
package org.apache.solr.update.processor;
|
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.commons.lang3.tuple.Pair;
|
||||||
import org.apache.lucene.index.IndexWriter;
|
import org.apache.lucene.index.IndexWriter;
|
||||||
import org.apache.lucene.util.BytesRef;
|
import org.apache.lucene.util.BytesRef;
|
||||||
|
@ -43,15 +61,6 @@ import org.apache.solr.util.RefCounted;
|
||||||
import org.slf4j.Logger;
|
import org.slf4j.Logger;
|
||||||
import org.slf4j.LoggerFactory;
|
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;
|
import static org.apache.solr.common.params.CommonParams.ID;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -120,13 +129,8 @@ public class AtomicUpdateDocumentMerger {
|
||||||
doAddDistinct(toDoc, sif, fieldVal);
|
doAddDistinct(toDoc, sif, fieldVal);
|
||||||
break;
|
break;
|
||||||
default:
|
default:
|
||||||
Object id = toDoc.containsKey(idField.getName())? toDoc.getFieldValue(idField.getName()):
|
throw new SolrException(ErrorCode.BAD_REQUEST,
|
||||||
fromDoc.getFieldValue(idField.getName());
|
"Error:" + getID(toDoc, schema) + " Unknown operation for the an atomic update: " + key);
|
||||||
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);
|
|
||||||
}
|
}
|
||||||
// validate that the field being modified is not the id field.
|
// validate that the field being modified is not the id field.
|
||||||
if (idField.getName().equals(sif.getName())) {
|
if (idField.getName().equals(sif.getName())) {
|
||||||
|
@ -143,6 +147,15 @@ public class AtomicUpdateDocumentMerger {
|
||||||
return toDoc;
|
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.
|
* 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),
|
* 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) {
|
protected void doInc(SolrInputDocument toDoc, SolrInputField sif, Object fieldVal) {
|
||||||
SolrInputField numericField = toDoc.get(sif.getName());
|
SolrInputField numericField = toDoc.get(sif.getName());
|
||||||
SchemaField sf = schema.getField(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) {
|
if (numericField != null || sf.getDefaultValue() != null) {
|
||||||
// TODO: fieldtype needs externalToObject?
|
// TODO: fieldtype needs externalToObject?
|
||||||
String oldValS = (numericField != null) ?
|
String oldValS = (numericField != null) ?
|
||||||
|
@ -478,19 +496,22 @@ public class AtomicUpdateDocumentMerger {
|
||||||
sf.getType().readableToIndexed(oldValS, term);
|
sf.getType().readableToIndexed(oldValS, term);
|
||||||
Object oldVal = sf.getType().toObject(sf, term.get());
|
Object oldVal = sf.getType().toObject(sf, term.get());
|
||||||
|
|
||||||
String fieldValS = fieldVal.toString();
|
// behavior similar to doAdd/doSet
|
||||||
Number result;
|
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) {
|
if (oldVal instanceof Long) {
|
||||||
result = ((Long) oldVal).longValue() + Long.parseLong(fieldValS);
|
result = ((Long) oldVal).longValue() + result.longValue();
|
||||||
} else if (oldVal instanceof Float) {
|
} else if (oldVal instanceof Float) {
|
||||||
result = ((Float) oldVal).floatValue() + Float.parseFloat(fieldValS);
|
result = ((Float) oldVal).floatValue() + result.floatValue();
|
||||||
} else if (oldVal instanceof Double) {
|
} else if (oldVal instanceof Double) {
|
||||||
result = ((Double) oldVal).doubleValue() + Double.parseDouble(fieldValS);
|
result = ((Double) oldVal).doubleValue() + result.doubleValue();
|
||||||
} else {
|
} else {
|
||||||
// int, short, byte
|
// 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 {
|
} else {
|
||||||
toDoc.setField(sif.getName(), fieldVal);
|
toDoc.setField(sif.getName(), fieldVal);
|
||||||
|
@ -553,7 +574,15 @@ public class AtomicUpdateDocumentMerger {
|
||||||
return val;
|
return val;
|
||||||
}
|
}
|
||||||
SchemaField sf = schema.getField(fieldName);
|
SchemaField sf = schema.getField(fieldName);
|
||||||
|
try {
|
||||||
return sf.getType().toNativeType(val);
|
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) {
|
private static boolean isChildDoc(Object obj) {
|
||||||
|
|
|
@ -47,6 +47,7 @@ import org.apache.solr.cloud.AbstractFullDistribZkTestBase;
|
||||||
import org.apache.solr.cloud.ZkShardTerms;
|
import org.apache.solr.cloud.ZkShardTerms;
|
||||||
import org.apache.solr.common.SolrDocument;
|
import org.apache.solr.common.SolrDocument;
|
||||||
import org.apache.solr.common.SolrDocumentList;
|
import org.apache.solr.common.SolrDocumentList;
|
||||||
|
import org.apache.solr.common.SolrException;
|
||||||
import org.apache.solr.common.SolrInputDocument;
|
import org.apache.solr.common.SolrInputDocument;
|
||||||
import org.apache.solr.common.cloud.ClusterState;
|
import org.apache.solr.common.cloud.ClusterState;
|
||||||
import org.apache.solr.common.cloud.DocCollection;
|
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.params.SolrParams;
|
||||||
import org.apache.solr.common.util.ExecutorUtil;
|
import org.apache.solr.common.util.ExecutorUtil;
|
||||||
import org.apache.solr.common.util.NamedList;
|
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.common.util.TimeSource;
|
||||||
import org.apache.solr.index.NoMergePolicyFactory;
|
import org.apache.solr.index.NoMergePolicyFactory;
|
||||||
import org.apache.solr.update.processor.DistributedUpdateProcessor;
|
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.RefCounted;
|
||||||
import org.apache.solr.util.TimeOut;
|
import org.apache.solr.util.TimeOut;
|
||||||
import org.apache.zookeeper.KeeperException;
|
import org.apache.zookeeper.KeeperException;
|
||||||
|
import org.hamcrest.MatcherAssert;
|
||||||
import org.junit.BeforeClass;
|
import org.junit.BeforeClass;
|
||||||
import org.junit.Test;
|
import org.junit.Test;
|
||||||
import org.slf4j.Logger;
|
import org.slf4j.Logger;
|
||||||
import org.slf4j.LoggerFactory;
|
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.
|
* 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);
|
assertTrue(currentVersion > version);
|
||||||
version = currentVersion;
|
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)
|
// RTG from tlog(s)
|
||||||
for (SolrClient client : clients) {
|
for (SolrClient client : clients) {
|
||||||
final String clientDebug = client.toString() + (LEADER.equals(client) ? " (leader)" : " (not leader)");
|
final String clientDebug = client.toString() + (LEADER.equals(client) ? " (leader)" : " (not leader)");
|
||||||
|
|
|
@ -18,6 +18,7 @@
|
||||||
|
|
||||||
package org.apache.solr.update;
|
package org.apache.solr.update;
|
||||||
|
|
||||||
|
import java.util.ArrayList;
|
||||||
import java.util.Arrays;
|
import java.util.Arrays;
|
||||||
import java.util.Collections;
|
import java.util.Collections;
|
||||||
import java.util.HashMap;
|
import java.util.HashMap;
|
||||||
|
@ -48,6 +49,7 @@ import org.apache.solr.schema.IndexSchema;
|
||||||
import org.apache.solr.schema.SchemaField;
|
import org.apache.solr.schema.SchemaField;
|
||||||
import org.apache.solr.update.processor.AtomicUpdateDocumentMerger;
|
import org.apache.solr.update.processor.AtomicUpdateDocumentMerger;
|
||||||
import org.apache.solr.util.RefCounted;
|
import org.apache.solr.util.RefCounted;
|
||||||
|
import org.hamcrest.MatcherAssert;
|
||||||
import org.junit.AfterClass;
|
import org.junit.AfterClass;
|
||||||
import org.junit.Before;
|
import org.junit.Before;
|
||||||
import org.junit.BeforeClass;
|
import org.junit.BeforeClass;
|
||||||
|
@ -121,6 +123,36 @@ public class TestInPlaceUpdatesStandalone extends SolrTestCaseJ4 {
|
||||||
assertU(commit("softCommit", "false"));
|
assertU(commit("softCommit", "false"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@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
|
@Test
|
||||||
public void testUpdatingDocValues() throws Exception {
|
public void testUpdatingDocValues() throws Exception {
|
||||||
long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), null);
|
long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), null);
|
||||||
|
|
|
@ -24,13 +24,17 @@ import java.util.List;
|
||||||
|
|
||||||
import com.google.common.collect.ImmutableMap;
|
import com.google.common.collect.ImmutableMap;
|
||||||
import org.apache.solr.SolrTestCaseJ4;
|
import org.apache.solr.SolrTestCaseJ4;
|
||||||
|
import org.apache.solr.common.SolrException;
|
||||||
import org.apache.solr.common.SolrInputDocument;
|
import org.apache.solr.common.SolrInputDocument;
|
||||||
import org.apache.solr.util.DateMathParser;
|
import org.apache.solr.util.DateMathParser;
|
||||||
|
import org.hamcrest.MatcherAssert;
|
||||||
import org.junit.Before;
|
import org.junit.Before;
|
||||||
import org.junit.BeforeClass;
|
import org.junit.BeforeClass;
|
||||||
import org.junit.Ignore;
|
import org.junit.Ignore;
|
||||||
import org.junit.Test;
|
import org.junit.Test;
|
||||||
|
|
||||||
|
import static org.hamcrest.core.StringContains.containsString;
|
||||||
|
|
||||||
public class AtomicUpdatesTest extends SolrTestCaseJ4 {
|
public class AtomicUpdatesTest extends SolrTestCaseJ4 {
|
||||||
|
|
||||||
@BeforeClass
|
@BeforeClass
|
||||||
|
@ -1420,6 +1424,15 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
|
||||||
assertQ(req("q", "id:123", "indent", "true"), "//result[@numFound = '1']");
|
assertQ(req("q", "id:123", "indent", "true"), "//result[@numFound = '1']");
|
||||||
assertQ(req("q", "id:12311", "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']");
|
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() {
|
public void testFieldsWithDefaultValuesWhenAtomicUpdatesAgainstTlog() {
|
||||||
|
|
Loading…
Reference in New Issue