lucene-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctarg...@apache.org
Subject [lucene-solr] 05/38: SOLR-10860: Return proper error code for bad input incase of inplace updates (#2121)
Date Fri, 15 Jan 2021 21:45:16 GMT
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 <munendrasn@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() {


Mime
View raw message