From d4fa1aae213a4750d1b9834753ac400b93b58cd8 Mon Sep 17 00:00:00 2001 From: S N Munendra Date: Thu, 7 Jan 2021 20:44:48 +0530 Subject: [PATCH] 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 +++++++++++++------ .../update/TestInPlaceUpdatesDistrib.java | 18 ++++- .../update/TestInPlaceUpdatesStandalone.java | 32 ++++++++ .../update/processor/AtomicUpdatesTest.java | 13 +++ 5 files changed, 118 insertions(+), 26 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index adc0820d539..b4551c5998e 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 bdc4a72aaf8..1dd993afbe4 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 baf6320ced6..3eccdfaf83f 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 28679359553..91586ca68b3 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; @@ -121,6 +123,36 @@ public class TestInPlaceUpdatesStandalone extends SolrTestCaseJ4 { 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 public void testUpdatingDocValues() throws Exception { long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), 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 54e9f9d0e53..a0c1402e12b 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() {