Lock down CloudFormationResponse; default SdkHttpClient

- Include powertools-cloudformation in .github config
- Address mutatable ObjectMapper spotbugs finding
- JavaDoc cleanup

Author: Joe Wolf

.github/workflows/build.yml

@@ -5,6 +5,7 @@ on:
     branches:
       - master
     paths:
+      - 'powertools-cloudformation/**'
       - 'powertools-core/**'
       - 'powertools-logging/**'
       - 'powertools-sqs/**'
@@ -18,6 +19,7 @@ on:
     branches:
       - master
     paths:
+      - 'powertools-cloudformation/**'
       - 'powertools-core/**'
       - 'powertools-logging/**'
       - 'powertools-sqs/**'

.github/workflows/spotbugs.yml

@@ -5,6 +5,7 @@ on:
     branches:
       - master
     paths:
+      - 'powertools-cloudformation/**'
       - 'powertools-core/**'
       - 'powertools-logging/**'
       - 'powertools-sqs/**'

pom.xml

@@ -111,6 +111,11 @@
                 <artifactId>http-client-spi</artifactId>
                 <version>${aws.sdk.version}</version>
             </dependency>
+            <dependency>
+                <groupId>software.amazon.awssdk</groupId>
+                <artifactId>url-connection-client</artifactId>
+                <version>${aws.sdk.version}</version>
+            </dependency>
             <dependency>
                 <groupId>io.burt</groupId>
                 <artifactId>jmespath-jackson</artifactId>

powertools-cloudformation/pom.xml

@@ -46,6 +46,10 @@
             <groupId>software.amazon.awssdk</groupId>
             <artifactId>http-client-spi</artifactId>
         </dependency>
+        <dependency>
+            <groupId>software.amazon.awssdk</groupId>
+            <artifactId>url-connection-client</artifactId>
+        </dependency>
         <dependency>
             <groupId>com.amazonaws</groupId>
             <artifactId>aws-lambda-java-core</artifactId>

powertools-cloudformation/src/main/java/software/amazon/lambda/powertools/cloudformation/AbstractCustomResourceHandler.java

@@ -6,6 +6,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import software.amazon.awssdk.http.SdkHttpClient;
+import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -22,12 +23,19 @@ public abstract class AbstractCustomResourceHandler
 
     private final SdkHttpClient client;
 
+    /**
+     * Creates a new Handler that uses the default HTTP client for communicating with custom CloudFormation resources.
+     */
+    protected AbstractCustomResourceHandler() {
+        this.client = UrlConnectionHttpClient.create();
+    }
+
     /**
      * Creates a new Handler that uses the provided HTTP client for communicating with custom CloudFormation resources.
      *
      * @param client cannot be null
      */
-    public AbstractCustomResourceHandler(SdkHttpClient client) {
+    protected AbstractCustomResourceHandler(SdkHttpClient client) {
         this.client = Objects.requireNonNull(client, "SdkHttpClient cannot be null.");
     }
 
@@ -52,16 +60,15 @@ public final Response handleRequest(CloudFormationCustomResourceEvent event, Con
             LOG.debug("Preparing to send response {} to {}.", response, responseUrl);
             client.send(event, context, response);
         } catch (IOException ioe) {
-            LOG.error("Unable to send {} success to {}.", responseUrl, ioe);
+            LOG.error("Unable to send response {} to {}.", response, responseUrl, ioe);
             onSendFailure(event, context, response, ioe);
         } catch (CustomResourceResponseException rse) {
-            LOG.error("Unable to create/serialize Response. Sending empty failure to {}", responseUrl, rse);
-            // send a failure with a null response on account of response serialization issues
+            LOG.error("Unable to generate response. Sending empty failure to {}", responseUrl, rse);
             try {
                 client.send(event, context, Response.failed());
             } catch (Exception e) {
-                // unable to serialize response AND send an empty response
-                LOG.error("Unable to send empty failure to {}.", responseUrl, e);
+                // unable to generate response AND send the failure
+                LOG.error("Unable to send failure response to {}.", responseUrl, e);
                 onSendFailure(event, context, null, e);
             }
         }
@@ -92,7 +99,7 @@ private Response getResponse(CloudFormationCustomResourceEvent event, Context co
      *
      * @return a client for sending the response
      */
-    protected CloudFormationResponse buildResponseClient() {
+    CloudFormationResponse buildResponseClient() {
         return new CloudFormationResponse(client);
     }
 

powertools-cloudformation/src/main/java/software/amazon/lambda/powertools/cloudformation/CloudFormationResponse.java

@@ -30,7 +30,7 @@
  * <p>
  * This class is thread-safe provided the SdkHttpClient instance used is also thread-safe.
  */
-public class CloudFormationResponse {
+class CloudFormationResponse {
 
     /**
      * Internal representation of the payload to be sent to the event target URL. Retains all properties of the payload
@@ -120,10 +120,19 @@ ObjectNode toObjectNode(JsonNode dataNode) {
      *
      * @param client HTTP client to use for sending requests; cannot be null
      */
-    public CloudFormationResponse(SdkHttpClient client) {
+    CloudFormationResponse(SdkHttpClient client) {
         this.client = Objects.requireNonNull(client, "SdkHttpClient cannot be null");
     }
 
+    /**
+     * The underlying SdkHttpClient used by this class.
+     *
+     * @return a non-null client
+     */
+    SdkHttpClient getClient() {
+        return client;
+    }
+
     /**
      * Forwards a response containing a custom payload to the target resource specified by the event. The payload is
      * formed from the event and context data. Status is assumed to be SUCCESS.

powertools-cloudformation/src/main/java/software/amazon/lambda/powertools/cloudformation/Response.java

@@ -14,14 +14,14 @@
 public class Response {
 
     /**
-     * Indicates whether this response should be sent to the custom resource as a success or failure.
+     * Indicates whether a response is a success or failure.
      */
     public enum Status {
         SUCCESS, FAILED
     }
 
     /**
-     * For building Response instances that wrap a single, arbitrary value.
+     * For building Response instances.
      */
     public static class Builder {
         private Object value;
@@ -45,13 +45,15 @@ public Builder value(Object value) {
         }
 
         /**
-         * Configures a custom ObjectMapper for serializing the value object.
+         * Configures a custom ObjectMapper for serializing the value object. Creates a copy of the mapper provided;
+         * future mutations of the ObjectMapper made using the provided reference will not affect Response
+         * serialization.
          *
          * @param objectMapper if null, a default mapper will be used
          * @return a reference to this builder
          */
         public Builder objectMapper(ObjectMapper objectMapper) {
-            this.objectMapper = objectMapper;
+            this.objectMapper = objectMapper == null ? null : objectMapper.copy();
             return this;
         }
 
@@ -166,7 +168,7 @@ private Response(JsonNode jsonNode, Status status, String physicalResourceId, bo
     }
 
     /**
-     * Returns a JsonNode representation of the response.
+     * Returns a JsonNode representation of the Response.
      *
      * @return a non-null JsonNode representation
      */
@@ -175,7 +177,7 @@ JsonNode getJsonNode() {
     }
 
     /**
-     * The success/failed status of the response.
+     * The success/failed status of the Response.
      *
      * @return a non-null Status
      */
@@ -184,7 +186,7 @@ public Status getStatus() {
     }
 
     /**
-     * The physical resource ID of the custom resource. If null, the default resource ID will be used.
+     * The physical resource ID. If null, the default physical resource ID will be provided to the custom resource.
      *
      * @return a potentially null physical resource ID
      */
@@ -202,9 +204,9 @@ public boolean isNoEcho() {
     }
 
     /**
-     * The Response JSON.
+     * Includes all Response attributes, including its value in JSON format
      *
-     * @return a String in JSON format
+     * @return a full description of the Response
      */
     @Override
     public String toString() {

powertools-cloudformation/src/test/java/software/amazon/lambda/powertools/cloudformation/AbstractCustomResourceHandlerTest.java

@@ -24,17 +24,14 @@
 public class AbstractCustomResourceHandlerTest {
 
     /**
-     * Uses a mocked CloudFormationResponse to avoid sending actual HTTP requests.
+     * Bare-bones implementation that returns null for abstract methods.
      */
-    static class NoOpCustomResourceHandler extends AbstractCustomResourceHandler {
-
-        NoOpCustomResourceHandler() {
-            super(mock(SdkHttpClient.class));
+    static class NullCustomResourceHandler extends AbstractCustomResourceHandler {
+        NullCustomResourceHandler() {
         }
 
-        @Override
-        protected CloudFormationResponse buildResponseClient() {
-            return mock(CloudFormationResponse.class);
+        NullCustomResourceHandler(SdkHttpClient client) {
+            super(client);
         }
 
         @Override
@@ -53,6 +50,21 @@ protected Response delete(CloudFormationCustomResourceEvent event, Context conte
         }
     }
 
+    /**
+     * Uses a mocked CloudFormationResponse to avoid sending actual HTTP requests.
+     */
+    static class NoOpCustomResourceHandler extends NullCustomResourceHandler {
+
+        NoOpCustomResourceHandler() {
+            super(mock(SdkHttpClient.class));
+        }
+
+        @Override
+        protected CloudFormationResponse buildResponseClient() {
+            return mock(CloudFormationResponse.class);
+        }
+    }
+
     /**
      * Creates a handler that will expect the Response to be sent with an expected status. Will throw an AssertionError
      * if the method is sent with an unexpected status.
@@ -109,6 +121,27 @@ static CloudFormationCustomResourceEvent eventOfType(String requestType) {
         return event;
     }
 
+    @Test
+    void defaultAndCustomSdkHttpClients() {
+        AbstractCustomResourceHandler defaultClientHandler = new NullCustomResourceHandler();
+
+        SdkHttpClient defaultClient = defaultClientHandler.buildResponseClient().getClient();
+        assertThat(defaultClient).isNotNull();
+
+        String customClientName = "mockCustomClient";
+        SdkHttpClient customClientArg = mock(SdkHttpClient.class);
+        when(customClientArg.clientName()).thenReturn(customClientName);
+        AbstractCustomResourceHandler customClientHandler = new NullCustomResourceHandler(customClientArg);
+
+        SdkHttpClient customClient = customClientHandler.buildResponseClient().getClient();
+        assertThat(customClient).isNotNull();
+        assertThat(customClient.clientName())
+                .isEqualTo(customClientName);
+
+        assertThat(customClient.clientName())
+                .isNotEqualTo(defaultClient.clientName());
+    }
+
     @ParameterizedTest
     @CsvSource(value = {"Create,1,0,0", "Update,0,1,0", "Delete,0,0,1"}, delimiter = ',')
     void eventsDelegateToCorrectHandlerMethod(String eventType, int createCount, int updateCount, int deleteCount) {

powertools-cloudformation/src/test/java/software/amazon/lambda/powertools/cloudformation/ResponseTest.java

@@ -109,6 +109,20 @@ void jsonObjectValueWithDefaultObjectMapper() {
         assertThat(response.toString()).contains("JSON = " + expected);
     }
 
+    @Test
+    void jsonObjectValueWithNullObjectMapper() {
+        DummyBean value = new DummyBean("test");
+
+        Response response = Response.builder()
+                .objectMapper(null)
+                .value(value)
+                .build();
+
+        String expected = "{\"PropertyWithLongName\":\"test\"}";
+        assertThat(response.getJsonNode().toString()).isEqualTo(expected);
+        assertThat(response.toString()).contains("JSON = " + expected);
+    }
+
     @Test
     void jsonObjectValueWithCustomObjectMapper() {
         ObjectMapper customMapper = new ObjectMapper()
@@ -125,6 +139,25 @@ void jsonObjectValueWithCustomObjectMapper() {
         assertThat(response.toString()).contains("JSON = " + expected);
     }
 
+    @Test
+    void jsonObjectValueWithPostConfiguredObjectMapper() {
+        ObjectMapper customMapper = new ObjectMapper()
+                .setPropertyNamingStrategy(PropertyNamingStrategies.KEBAB_CASE);
+
+        DummyBean value = new DummyBean(10);
+        Response response = Response.builder()
+                .objectMapper(customMapper)
+                .value(value)
+                .build();
+
+        // changing the mapper config should not affect serialization
+        customMapper.setPropertyNamingStrategy(PropertyNamingStrategies.UPPER_CAMEL_CASE);
+
+        String expected = "{\"property-with-long-name\":10}";
+        assertThat(response.getJsonNode().toString()).isEqualTo(expected);
+        assertThat(response.toString()).contains("JSON = " + expected);
+    }
+
     @Test
     void successFactoryMethod() {
         Response response = Response.success();