Fix NPE in Spring when handling exceptions during LOG_ERROR strategy (#…

…93)

Adds a null check prior to adding the exception to the segment.

aws-xray-recorder-sdk-spring/pom.xml

@@ -53,12 +53,19 @@
             <version>2.0.0.RELEASE</version>
             <scope>provided</scope>
         </dependency>
+        <!-- Test -->
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
             <version>4.12</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-all</artifactId>
+            <version>1.10.19</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

aws-xray-recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/BaseAbstractXRayInterceptor.java

@@ -57,7 +57,7 @@ protected Object processXRayTrace(ProceedingJoinPoint pjp) throws Throwable {
             }
             return XRayInterceptorUtils.conditionalProceed(pjp);
         } catch (Exception e) {
-            AWSXRay.getCurrentSegment().addException(e);
+            AWSXRay.getCurrentSegmentOptional().ifPresent(x -> x.addException(e));
             throw e;
         } finally {
             logger.trace("Ending Subsegment");

aws-xray-recorder-sdk-spring/src/test/java/com/amazonaws/xray/spring/aop/BaseAbstractXRayInterceptorTest.java

@@ -0,0 +1,90 @@
+package com.amazonaws.xray.spring.aop;
+
+import com.amazonaws.xray.AWSXRay;
+import com.amazonaws.xray.AWSXRayRecorder;
+import com.amazonaws.xray.entities.Segment;
+import org.aspectj.lang.ProceedingJoinPoint;
+import org.aspectj.lang.Signature;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import java.util.Optional;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class BaseAbstractXRayInterceptorTest {
+
+    class ImplementedXRayInterceptor extends BaseAbstractXRayInterceptor {
+        @Override
+        protected void xrayEnabledClasses() {}
+    }
+
+    private BaseAbstractXRayInterceptor xRayInterceptor = new ImplementedXRayInterceptor();
+
+    @Mock
+    private AWSXRayRecorder mockRecorder;
+
+    @Mock
+    private ProceedingJoinPoint mockPjp;
+
+    @Mock
+    private Signature mockSignature;
+
+    @Before
+    public void setup() {
+        AWSXRay.setGlobalRecorder(mockRecorder);
+
+        when(mockPjp.getArgs()).thenReturn(new Object[]{});
+        when(mockPjp.getSignature()).thenReturn(mockSignature);
+        when(mockSignature.getName()).thenReturn("testSpringName");
+    }
+
+    @Test
+    public void testNullSegmentOnProcessFailure() throws Throwable {
+        // Test to ensure that getCurrentSegment()/getCurrentSegmentOptional() don't throw NPEs when customers are using
+        // the Log context missing strategy during the processXRayTrace call.
+        Exception expectedException = new RuntimeException("An intended exception");
+        // Cause an intended exception to be thrown so that getCurrentSegmentOptional() is called.
+        when(mockRecorder.beginSubsegment(any())).thenThrow(expectedException);
+
+        when(mockRecorder.getCurrentSegmentOptional()).thenReturn(Optional.empty());
+        when(mockRecorder.getCurrentSegment()).thenReturn(null);
+
+        try {
+            xRayInterceptor.processXRayTrace(mockPjp);
+        } catch (Exception e){
+            // A null pointer exception here could potentially indicate that a call to getCurrentSegment() is returning null.
+            // i.e. there is another exception other than our intended exception that is thrown that's not supposed to be thrown.
+            assertNotEquals(NullPointerException.class, e.getClass());
+            assertEquals(expectedException.getMessage(), e.getMessage());
+        }
+    }
+
+    @Test
+    public void testSegmentOnProcessFailure() throws Throwable {
+        // Test to ensure that the exception is populated to the segment when an error occurs during the
+        // processXRayTrace call.
+        Exception expectedException = new RuntimeException("An intended exception");
+        // Cause an intended exception to be thrown so that getCurrentSegmentOptional() is called.
+        when(mockRecorder.beginSubsegment(any())).thenThrow(expectedException);
+
+        Segment mockSegment = mock(Segment.class);
+        when(mockRecorder.getCurrentSegmentOptional()).thenReturn(Optional.of(mockSegment));
+        when(mockRecorder.getCurrentSegment()).thenReturn(mockSegment);
+
+        try {
+            xRayInterceptor.processXRayTrace(mockPjp);
+        } catch (Exception e){
+            verify(mockSegment).addException(expectedException);
+        }
+    }
+}