SOLR-2802: change min/max processors to fail hard if values are not comparable, also imrpvoe error handling/logging in FieldMutatingUpdateProcessor

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1242625 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -207,7 +207,7 @@ New Features
 * SOLR-1726: Added deep paging support to search (sort by score only) which should use less memory when paging deeply into results
  by keeping the priority queue small. (Manojkumar Rangasamy Kannadasan, gsingers)
 
-* SOLR-2802: New FieldMutatingUpdateProcessor and Factory to simlify the 
+* SOLR-2802: New FieldMutatingUpdateProcessor and Factory to simplify the 
   development of UpdateProcessors that modify field values of documents as 
   they are indexed. Also includes several useful new implementations:
       RemoveBlankFieldUpdateProcessorFactory

solr/core/src/java/org/apache/solr/update/processor/FieldMutatingUpdateProcessor.java

@@ -75,6 +75,12 @@ public abstract class FieldMutatingUpdateProcessor
    */
   protected abstract SolrInputField mutate(final SolrInputField src);
   
+  /**
+   * Calls <code>mutate</code> on any fields identified by the selector 
+   * before forwarding the command down the chain.  Any SolrExceptions 
+   * thrown by <code>mutate</code> will be logged with the Field name, 
+   * wrapped and re-thrown.
+   */
   @Override
   public void processAdd(AddUpdateCommand cmd) throws IOException {
     final SolrInputDocument doc = cmd.getSolrInputDocument();
@@ -88,7 +94,15 @@ public abstract class FieldMutatingUpdateProcessor
       if (! selector.shouldMutate(fname)) continue;
       
       final SolrInputField src = doc.get(fname);
-      final SolrInputField dest = mutate(src);
+
+      SolrInputField dest = null;
+      try { 
+        dest = mutate(src);
+      } catch (SolrException e) {
+        String msg = "Unable to mutate field '"+fname+"': "+e.getMessage();
+        SolrException.log(log, msg, e);
+        throw new SolrException(BAD_REQUEST, msg, e);
+      }
       if (null == dest) {
         doc.remove(fname);
       } else {

solr/core/src/java/org/apache/solr/update/processor/MaxFieldValueUpdateProcessorFactory.java

@@ -17,6 +17,10 @@
 
 package org.apache.solr.update.processor;
 
+import static org.apache.solr.common.SolrException.ErrorCode.*;
+
+import org.apache.solr.common.SolrException;
+
 import org.apache.solr.core.SolrCore;
 
 import java.util.Collections;
@@ -25,10 +29,9 @@ import java.util.Iterator;
 
 /**
  * An update processor that keeps only the the maximum value from any selected 
- * fields where multiple values are found.  Correct behavior assumes that all 
+ * fields where multiple values are found.  Correct behavior requires tha all 
  * of the values in the SolrInputFields being mutated are mutually comparable; 
- * If this is not the case, then the full list of all values found will be 
+ * If this is not the case, then a SolrException will br thrown. 
- * used as is.
  * <p>
  * By default, this processor matches no fields.
  * </p>
@@ -59,7 +62,9 @@ public final class MaxFieldValueUpdateProcessorFactory extends FieldValueSubsetU
       result = Collections.singletonList
         (Collections.max((Collection)values));
     } catch (ClassCastException e) {
-      /* NOOP */
+      throw new SolrException
+        (BAD_REQUEST, 
+         "Field values are not mutually comparable: " + e.getMessage(), e);
     }
     return result;
   }

solr/core/src/java/org/apache/solr/update/processor/MinFieldValueUpdateProcessorFactory.java

@@ -17,6 +17,10 @@
 
 package org.apache.solr.update.processor;
 
+import static org.apache.solr.common.SolrException.ErrorCode.*;
+
+import org.apache.solr.common.SolrException;
+
 import org.apache.solr.core.SolrCore;
 
 import java.util.Collections;
@@ -25,10 +29,9 @@ import java.util.Iterator;
 
 /**
  * An update processor that keeps only the the minimum value from any selected 
- * fields where multiple values are found.  Correct behavior assumes that all 
+ * fields where multiple values are found.   Correct behavior requires tha all 
  * of the values in the SolrInputFields being mutated are mutually comparable; 
- * If this is not the case, then the full list of all values found will be 
+ * If this is not the case, then a SolrException will br thrown. 
- * used as is.
  * <p>
  * By default, this processor matches no fields.
  * </p>
@@ -59,7 +62,9 @@ public final class MinFieldValueUpdateProcessorFactory extends FieldValueSubsetU
       result = Collections.singletonList
         (Collections.min((Collection)values));
     } catch (ClassCastException e) {
-      /* NOOP */
+      throw new SolrException
+        (BAD_REQUEST, 
+         "Field values are not mutually comparable: " + e.getMessage(), e);
     }
     return result;
   }

solr/core/src/test/org/apache/solr/update/processor/FieldMutatingUpdateProcessorTest.java

@@ -27,6 +27,7 @@ import java.io.IOException;
 
 import org.apache.solr.SolrTestCaseJ4;
 
+import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrInputDocument;
 import org.apache.solr.common.SolrInputField;
 import org.apache.solr.common.params.ModifiableSolrParams;
@@ -480,24 +481,24 @@ public class FieldMutatingUpdateProcessorTest extends SolrTestCaseJ4 {
     assertEquals(Arrays.asList("aaa", "bbb"), 
                  d.getFieldValues("yak_t"));
    
-    // uncomparable should not fail
+    // failure when un-comparable
 
+    SolrException error = null;
+    try {
+      ignoreException(".*Unable to mutate field.*");
       d = processAdd("min-value", 
                      doc(f("id", "1111"),
                          f("foo_s", "zzz", new Integer(42), "bbb"),
                          f("bar_s", "aaa"),
                          f("yak_t", "aaa", "bbb")));
-
+    } catch (SolrException e) {
-    assertNotNull(d);
+      error = e;
-
+    } finally {
-    assertEquals(Arrays.asList("zzz", new Integer(42), "bbb"),
+      resetExceptionIgnores();
-                 d.getFieldValues("foo_s"));
+    }
-    assertEquals(Arrays.asList("aaa"), 
+    assertNotNull("no error on un-comparable values", error);
-                 d.getFieldValues("bar_s"));
+    assertTrue("error doesn't mention field name",
-    assertEquals(Arrays.asList("aaa", "bbb"), 
+               0 <= error.getMessage().indexOf("foo_s"));
-                 d.getFieldValues("yak_t"));
-
-
   }
 
   public void testMaxValue() throws Exception {
@@ -521,25 +522,26 @@ public class FieldMutatingUpdateProcessorTest extends SolrTestCaseJ4 {
     assertEquals(Arrays.asList("aaa", "bbb"), 
                  d.getFieldValues("yak_t"));
    
-    // uncomparable should not fail
+    // failure when un-comparable
 
-    d = processAdd("max-value", 
+    SolrException error = null;
+    try {
+      ignoreException(".*Unable to mutate field.*");
+      d = processAdd("min-value", 
                      doc(f("id", "1111"),
                          f("foo_s", "zzz", new Integer(42), "bbb"),
                          f("bar_s", "aaa"),
                          f("yak_t", "aaa", "bbb")));
-
+    } catch (SolrException e) {
-    assertNotNull(d);
+      error = e;
-
+    } finally {
-    assertEquals(Arrays.asList("zzz", new Integer(42), "bbb"),
+      resetExceptionIgnores();
-                 d.getFieldValues("foo_s"));
-    assertEquals(Arrays.asList("aaa"), 
-                 d.getFieldValues("bar_s"));
-    assertEquals(Arrays.asList("aaa", "bbb"), 
-                 d.getFieldValues("yak_t"));
-
-
     }
+    assertNotNull("no error on un-comparable values", error);
+    assertTrue("error doesn't mention field name",
+               0 <= error.getMessage().indexOf("foo_s"));
+  }
+  
 
   public void testHtmlStrip() throws Exception {
     SolrInputDocument d = null;