fix #347: ServiceException retains causal errorInstanceId (#348)

fix #347: ServiceException retains causal `errorInstanceId`

Author: carterkozak

changelog/@unreleased/pr-348.v2.yml

@@ -0,0 +1,8 @@
+type: improvement
+improvement:
+  description: ServiceException retains the `errorInstanceId` from its cause, allowing RemoteExceptions
+    to be caught and used as the cause for as more specific ServiceException without sacrificing observability.
+    In most cases a single error identifier query is sufficient to discover the root cause of a failure
+    instead of individually requesting each segment back to the root.
+  links:
+  - https://github.com/palantir/conjure-java-runtime-api/pull/348

errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java

@@ -20,7 +20,9 @@
 import com.palantir.logsafe.SafeLoggable;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.Set;
 import java.util.UUID;
 import javax.annotation.Nullable;
 
@@ -30,7 +32,7 @@ public final class ServiceException extends RuntimeException implements SafeLogg
     private final ErrorType errorType;
     private final List<Arg<?>> args; // unmodifiable
 
-    private final String errorInstanceId = UUID.randomUUID().toString();
+    private final String errorInstanceId;
     private final String unsafeMessage;
     private final String noArgsMessage;
 
@@ -47,6 +49,7 @@ public ServiceException(ErrorType errorType, @Nullable Throwable cause, Arg<?>..
         // TODO(rfink): Memoize formatting?
         super(cause);
 
+        this.errorInstanceId = generateErrorInstanceId(cause);
         this.errorType = errorType;
         // Note that instantiators cannot mutate List<> args since it comes through copyToList in all code paths.
         this.args = copyToUnmodifiableList(args);
@@ -122,4 +125,29 @@ private static String renderUnsafeMessage(ErrorType errorType, Arg<?>... args) {
     private static String renderNoArgsMessage(ErrorType errorType) {
         return String.format("ServiceException: %s (%s)", errorType.code(), errorType.name());
     }
+
+    /**
+     * Finds the errorInstanceId of the most recent cause if present, otherwise generates a new random identifier.
+     * Note that this only searches {@link Throwable#getCause() causal exceptions}, not
+     * {@link Throwable#getSuppressed() suppressed causes}.
+     */
+    private static String generateErrorInstanceId(@Nullable Throwable cause) {
+        return generateErrorInstanceId(cause, Collections.newSetFromMap(new IdentityHashMap<>()));
+    }
+
+    private static String generateErrorInstanceId(
+            @Nullable Throwable cause,
+            // Guard against cause cycles, see Throwable.printStackTrace(PrintStreamOrWriter)
+            Set<Throwable> dejaVu) {
+        if (cause == null || !dejaVu.add(cause)) {
+            return UUID.randomUUID().toString();
+        }
+        if (cause instanceof ServiceException) {
+            return ((ServiceException) cause).getErrorInstanceId();
+        }
+        if (cause instanceof RemoteException) {
+            return ((RemoteException) cause).getError().errorInstanceId();
+        }
+        return generateErrorInstanceId(cause.getCause(), dejaVu);
+    }
 }

errors/src/test/java/com/palantir/conjure/java/api/errors/ServiceExceptionTest.java

@@ -21,6 +21,7 @@
 import com.palantir.logsafe.Arg;
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.UnsafeArg;
+import com.palantir.logsafe.exceptions.SafeRuntimeException;
 import java.util.UUID;
 import org.junit.jupiter.api.Test;
 
@@ -81,4 +82,35 @@ public void testErrorIdsAreUnique() {
 
         assertThat(errorId1).isNotEqualTo(errorId2);
     }
+
+    @Test
+    public void testErrorIdsAreInheritedFromServiceExceptions() {
+        ServiceException rootCause = new ServiceException(ERROR);
+        SafeRuntimeException intermediate = new SafeRuntimeException("Handled an exception", rootCause);
+        ServiceException parent = new ServiceException(ERROR, intermediate);
+        assertThat(parent.getErrorInstanceId()).isEqualTo(rootCause.getErrorInstanceId());
+    }
+
+    @Test
+    public void testErrorIdsAreInheritedFromRemoteExceptions() {
+        RemoteException rootCause = new RemoteException(new SerializableError.Builder()
+                .errorCode("errorCode")
+                .errorName("errorName")
+                .build(), 500);
+        SafeRuntimeException intermediate = new SafeRuntimeException("Handled an exception", rootCause);
+        ServiceException parent = new ServiceException(ERROR, intermediate);
+        assertThat(parent.getErrorInstanceId()).isEqualTo(rootCause.getError().errorInstanceId());
+    }
+
+    @Test
+    public void testCircularCause() {
+        RuntimeException first = new RuntimeException();
+        RuntimeException second = new RuntimeException(first);
+        // Yes, you can do this. In practice it's often more subtle when libraries attempt to piece together
+        // more helpful exception chains and encounter unexpected edge cases.
+        first.initCause(second);
+        // invoke getErrorInstanceId to ensure this is tested even if future developers
+        // optimize generation to occur lazily.
+        assertThat(new ServiceException(ERROR, second).getErrorInstanceId()).isNotNull();
+    }
 }