Clean up error message handling

4 files changed, 132 insertions, 70 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/result/DefaultErrorHit.java b/container-search/src/main/java/com/yahoo/search/result/DefaultErrorHit.java
index 93574002d4f..16b07a86131 100644
--- a/container-search/src/main/java/com/yahoo/search/result/DefaultErrorHit.java
+++ b/container-search/src/main/java/com/yahoo/search/result/DefaultErrorHit.java
@@ -8,14 +8,15 @@ import java.util.List;
 import java.util.Set;
 
 /**
- * A hit which holds information on error conditions in a result.
- * En error hit maintains a main error - the main error of the result.
+ * A hit which holds a list of error conditions in a result.
  *
  * @author bratseth
  * @author Steinar Knutsen
  */
 public class DefaultErrorHit extends Hit implements ErrorHit, Cloneable {
 
+    // TODO: Check that nobody implements ErrorHit, rename this to ErrorHit, and make an empty, deprecated subclass DefaultErrorHit
+    
     /**
      * A list of unique error messages, where the first is considered the "main"
      * error. It should always contain at least one error.
@@ -23,16 +24,28 @@ public class DefaultErrorHit extends Hit implements ErrorHit, Cloneable {
     private List<ErrorMessage> errors = new ArrayList<>();
 
     /**
-     * Creates an error hit with a main error
+     * Creates an error hit with one error
      *
      * @param source the name of the source or backend of this hit
-     * @param error an initial main error to add to this hit, cannot be null
+     * @param error an initial error to add to this hit, cannot be null
      */
     public DefaultErrorHit(String source, ErrorMessage error) {
         super("error:" + source, new Relevance(Double.POSITIVE_INFINITY), source);
         addError(error);
     }
 
+    /**
+     * Creates an error hit with a list of errors
+     *
+     * @param source the name of the source or backend of this hit
+     * @param errors a list of errors for this to hold. The list will not be modified or retained.
+     */
+    public DefaultErrorHit(String source, List<ErrorMessage> errors) {
+        super("error:" + source, new Relevance(Double.POSITIVE_INFINITY), source);
+        for (ErrorMessage error : errors)
+            addError(error);
+    }
+
     public void setSource(String source) {
         super.setSource(source);
         for (Iterator<ErrorMessage> i = errorIterator(); i.hasNext();) {
@@ -47,10 +60,11 @@ public class DefaultErrorHit extends Hit implements ErrorHit, Cloneable {
     /**
      * Returns the main error of this result, never null.
      *
-     * @deprecated since 5.18, use {@link #errors()}
+     * @deprecated use {@link #errors()}
      */
     @Override
     @Deprecated
+    // TODO: Remove on Vespa 7
     public ErrorMessage getMainError() {
         return errors.get(0);
     }
@@ -63,14 +77,10 @@ public class DefaultErrorHit extends Hit implements ErrorHit, Cloneable {
         errors.add(error);
     }
 
-    /**
-     * Adds an error to this. This may change the main error
-     * and/or the list of detailed errors
-     */
+    /** Adds an error to this */
     public void addError(ErrorMessage error) {
-        if (error.getSource() == null) {
+        if (error.getSource() == null)
             error.setSource(getSource());
-        }
         removeAndAdd(error);
     }
 
@@ -97,6 +107,7 @@ public class DefaultErrorHit extends Hit implements ErrorHit, Cloneable {
         return s;
     }
 
+    @Override
     public String toString() {
         return "Error: " + errors.get(0).toString();
     }
diff --git a/container-search/src/main/java/com/yahoo/search/result/HitGroup.java b/container-search/src/main/java/com/yahoo/search/result/HitGroup.java
index 1469596d20e..8c7e05aee90 100644
--- a/container-search/src/main/java/com/yahoo/search/result/HitGroup.java
+++ b/container-search/src/main/java/com/yahoo/search/result/HitGroup.java
@@ -11,23 +11,26 @@ import com.yahoo.processing.response.DataList;
 import com.yahoo.processing.response.DefaultIncomingData;
 import com.yahoo.processing.response.IncomingData;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import static com.yahoo.collections.CollectionUtil.first;
 
 /**
  * <p>A group of ordered hits. Since hitGroup is itself a kind of Hit,
- * this can compose hierarchies of grouped hits.</p>
+ * this can compose hierarchies of grouped hits.
  *
  * <p>Group hits has a relevancy just as other hits - they can be ordered
  * between each other and in comparison to other hits.
  *
  * <p>Note that a group is by default a meta hit, but it can also contain its own content
- * in addition to subgroup content, in which case it should be set to non-meta.</p>
+ * in addition to subgroup content, in which case it should be set to non-meta.
  *
  * @author bratseth
  */
@@ -77,7 +80,7 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
      * A direct reference to the errors of this result, or null if there are no errors.
      * The error hit will also be listed in the set of this of this result
      */
-    private ErrorHit errorHit = null;
+    private DefaultErrorHit errorHit = null;
 
     private final ListenableFuture<DataList<Hit>> completedFuture;
 
@@ -223,12 +226,20 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
      */
     @Override
     public Hit add(Hit hit) {
-        if (hit.isMeta() && hit instanceof ErrorHit) {
-            boolean add = mergeErrors((ErrorHit) hit);
-            if (!add) return (Hit)errorHit;
+        System.out.println("Source of incoming hit: " + hit.getSource());
+        if (hit.isMeta() && hit instanceof DefaultErrorHit) {
+            if (errorHit != null) {
+                errorHit.addErrors((DefaultErrorHit)hit);
+                return errorHit; // don't add another error hit
+            }
+            else {
+                errorHit = merge(consumeAnyQueryErrors(), (DefaultErrorHit) hit);
+                hit = errorHit; // Add this hit below
+            }
         }
         handleNewHit(hit);
         hits.add(hit);
+        System.out.println("Source of hit added: " + hit.getSource());
         return hit;
     }
 
@@ -340,18 +351,19 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
     /**
      * Removes a hit from this group or any subgroup.
      *
-     * @param uri The uri of the hit to remove.
-     * @return The hit removed, or null if not found.
+     * @param uri the uri of the hit to remove.
+     * @return the hit removed, or null if not found.
      */
     public Hit remove(URI uri) {
         for (Iterator<Hit> it = hits.iterator(); it.hasNext(); ) {
             Hit hit = it.next();
+            System.out.println("Is " + uri + " equal to " + hit.getId() + "?");
             if (uri.equals(hit.getId())) {
                 it.remove();
                 handleRemovedHit(hit);
                 return hit;
             }
-            if (hit instanceof HitGroup) {
+            else if (hit instanceof HitGroup) {
                 Hit removed = ((HitGroup)hit).remove(uri);
                 if (removed != null) {
                     return removed;
@@ -391,7 +403,7 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
     public void addError(ErrorMessage error) {
         getError(); // update the list of errors
         if (errorHit == null)
-            add((Hit)createErrorHit(error));
+            add(new DefaultErrorHit(getSource(), error));
         else
             errorHit.addError(error);
     }
@@ -408,9 +420,11 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
      *
      * @return the error hit which was removed, or null if there were no errors
      */
-    public ErrorHit removeErrorHit() {
+    public DefaultErrorHit removeErrorHit() {
         updateHits(); // Consume and remove from the query producing this as well
-        ErrorHit removed = errorHit;
+        DefaultErrorHit removed = errorHit;
+        if (removed != null)
+            remove(removed.getId());
         errorHit = null;
         return removed;
     }
@@ -420,57 +434,53 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
      * or null if no searcher has produced an error AND the query doesn't contain an error
      */
     public ErrorMessage getError() {
-        // See updateHits if this method is changed
-        if (errorHit != null) {
-            return errorHit.errors().iterator().next();
-        }
-
-        if (getQuery() != null && getQuery().errors().size() != 0) {
-            updateHits();
-        } // Pull them over
-
-        if (errorHit == null) {
+        updateHits();
+        if (errorHit == null)
             return null;
-        }
-
-        return errorHit.errors().iterator().next();
+        else
+            return errorHit.errors().iterator().next();
     }
 
     /**
-     * Handles the addition of a new error hit, whether or not we already have one
+     * Combines two error hits to one. Any one argument may be null, in which case the other is returned.
      *
-     * @return true if this shouls also be added to the list of hits of this reslt
+     * @return true if this should also be added to the list of hits of this result
      */
-    private boolean mergeErrors(ErrorHit newHit) {
-        if (errorHit == null) {
-            errorHit = newHit;
-            return true;
-        } else {
-            errorHit.addErrors(newHit);
-            return false;
-        }
+    private DefaultErrorHit merge(DefaultErrorHit first, DefaultErrorHit second) {
+        if (first == null) return second;
+        if (second == null) return first;
+
+        String mergedSource = first.getSource()!=null ? first.getSource() : second.getSource();
+        List<ErrorMessage> mergedErrors = new ArrayList<>();
+        mergedErrors.addAll(first.errors());
+        mergedErrors.addAll(second.errors());
+        return new DefaultErrorHit(mergedSource, mergedErrors);
     }
 
     /**
      * Must be called before the list of hits, or anything dependent on the list of hits, is removed.
-     * Merges errors from the query if there is one set for this group
+     * Consumes errors from the query if there is one set for this group
      */
     private void updateHits() {
-        if (getQuery() == null) return;
-
-        if (getQuery().errors().size() == 0) return;
-
-        if (errorHit == null)
-            add((Hit)createErrorHit(toSearchError(getQuery().errors().get(0))));
-
-        // Add the rest of the errors
-        for (int i = 1; i < getQuery().errors().size(); i++)
-            errorHit.addError(toSearchError(getQuery().errors().get(i)));
-        getQuery().errors().clear();
+        DefaultErrorHit queryErrors = consumeAnyQueryErrors();
+        if (queryErrors != null)
+            add(queryErrors);
     }
 
-    protected ErrorHit createErrorHit(ErrorMessage errorMessage) {
-        return new DefaultErrorHit(getSource(), errorMessage);
+    /** 
+     * Consumes errors from the query and returns them in a new error hit
+     * 
+     * @return the error hit containing all query errors, or null if no query errors should be consumed
+     */
+    private DefaultErrorHit consumeAnyQueryErrors() {
+        if (errorHit != null) return null;
+        if (getQuery() == null) return null;
+        if (getQuery().errors().isEmpty()) return null;
+
+        // Move errors from the query into this
+        List<ErrorMessage> queryErrors = getQuery().errors().stream().map(this::toSearchError).collect(Collectors.toList());
+        getQuery().errors().clear(); // TODO: Remove this line (not promised, can be done at any time)
+        return new DefaultErrorHit(getSource(), queryErrors);
     }
 
     /** Compatibility */
@@ -478,7 +488,7 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
         if (error instanceof ErrorMessage) 
             return (ErrorMessage)error;
         else 
-            return new ErrorMessage(error.getCode(),error.getMessage(),error.getDetailedMessage(),error.getCause());
+            return new ErrorMessage(error.getCode(), error.getMessage(), error.getDetailedMessage(), error.getCause());
     }
 
     /**
@@ -720,14 +730,17 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
 
     // Filled is not kept in sync at removal
     private void handleRemovedHit(Hit hit) {
-        if (!hit.isAuxiliary()) {
+        if ( ! hit.isAuxiliary()) {
             concreteHitCount--;
-            if (!hit.isCached())
+            if ( ! hit.isCached())
                 notCachedCount--;
         }
         else if (hit instanceof HitGroup) {
             subgroupCount--;
         }
+        else if (hit instanceof DefaultErrorHit) {
+            errorHit = null;
+        }
 
         if (deletionBreaksOrdering) {
             hitsSorted = false;
@@ -812,8 +825,8 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable<
         }
         if (this.errorHit!=null) { // Find the cloned error and assign it
             for (Hit hit : hitGroupClone.asList()) {
-                if (hit instanceof ErrorHit)
-                    hitGroupClone.errorHit=(ErrorHit)hit;
+                if (hit instanceof DefaultErrorHit)
+                    hitGroupClone.errorHit=(DefaultErrorHit)hit;
             }
         }
 
diff --git a/container-search/src/main/java/com/yahoo/search/result/HitIterator.java b/container-search/src/main/java/com/yahoo/search/result/HitIterator.java
index 7436d57a135..3a7793cd6d5 100644
--- a/container-search/src/main/java/com/yahoo/search/result/HitIterator.java
+++ b/container-search/src/main/java/com/yahoo/search/result/HitIterator.java
@@ -11,7 +11,7 @@ import com.yahoo.search.Result;
 /**
  * An iterator for the list of hits in a result. This iterator supports the remove operation.
  *
- * @author  <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
+ * @author Steinar Knutsen
  */
 public class HitIterator implements Iterator<Hit> {
 
diff --git a/container-search/src/test/java/com/yahoo/search/result/test/HitGroupTestCase.java b/container-search/src/test/java/com/yahoo/search/result/test/HitGroupTestCase.java
index 61cc96479a5..b8f033444c8 100644
--- a/container-search/src/test/java/com/yahoo/search/result/test/HitGroupTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/result/test/HitGroupTestCase.java
@@ -2,6 +2,7 @@
 package com.yahoo.search.result.test;
 
 import com.yahoo.search.Query;
+import com.yahoo.search.result.DefaultErrorHit;
 import com.yahoo.search.result.ErrorHit;
 import com.yahoo.search.result.ErrorMessage;
 import com.yahoo.search.result.Hit;
@@ -9,6 +10,8 @@ import com.yahoo.search.result.HitGroup;
 import org.junit.Test;
 
 import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -32,7 +35,7 @@ public class HitGroupTestCase {
     }
     
     @Test
-    public void testErrors() {
+    public void testErrorsConsistencyUsingErrorOperations() {
         HitGroup hits = new HitGroup();
 
         Query query = new Query();
@@ -45,14 +48,44 @@ public class HitGroupTestCase {
         
         assertEquals(4, hits.getErrorHit().errors().size());
         assertEquals(0, query.errors().size());
+        assertEquals(Optional.of(hits.getErrorHit()), errorHitIn(hits.asList()));
         
-        ErrorHit errors = hits.removeErrorHit();
-        assertNotNull(errors);
-        assertEquals(4, errors.errors().size());
+        DefaultErrorHit removedErrors = hits.removeErrorHit();
+        assertNotNull(removedErrors);
+        assertEquals(4, removedErrors.errors().size());
+        assertNull(hits.get(removedErrors.getId().toString()));
+        assertFalse(errorHitIn(hits.asList()).isPresent());
+
         assertNull(hits.removeErrorHit());
     }
 
     @Test
+    public void testErrorsConsistencyUsingHitOperations() {
+        HitGroup hits = new HitGroup();
+
+        Query query = new Query();
+        query.errors().add(ErrorMessage.createIllegalQuery("test1"));
+        query.errors().add(ErrorMessage.createTimeout("test2"));
+        hits.setQuery(query);
+
+        DefaultErrorHit errors = new DefaultErrorHit("source", ErrorMessage.createForbidden("test3"));
+        errors.addError(ErrorMessage.createUnspecifiedError("test4"));
+        hits.add(errors);
+
+        assertEquals(4, hits.getErrorHit().errors().size());
+        assertEquals(0, query.errors().size());
+        assertEquals(Optional.of(hits.getErrorHit()), errorHitIn(hits.asList()));
+
+        DefaultErrorHit removedErrors = (DefaultErrorHit)hits.remove(errors.getId());
+        assertNotNull(removedErrors);
+        assertEquals(4, removedErrors.errors().size());
+        assertNull(hits.get(removedErrors.getId().toString()));
+        assertFalse(errorHitIn(hits.asList()).isPresent());
+
+        assertNull(hits.remove(errors.getId()));
+    }
+
+    @Test
     public void testRecursiveGet() {
         // Level 1
         HitGroup g1=new HitGroup();
@@ -221,5 +254,10 @@ public class HitGroupTestCase {
         assertFalse(hg.isFilled("anyclass"));
         assertTrue(hg.getFilled().isEmpty());
     }
+    
+    /** Returns the (first) error hit in the given list, or empty if none */
+    private Optional<ErrorHit> errorHitIn(List<Hit> hits) {
+        return hits.stream().filter(h -> h instanceof ErrorHit).map(ErrorHit.class::cast).findFirst();
+    }
 
 }