Fix Spotbugs issues with new Uluru checker (#63)

* Fix Spotbugs issues with AWS::Transfer::Agreement * Fix Spotbugs issues with AWS::Transfer::Certificate * Fix Spotbugs issues with AWS::Transfer::Connector * Fix Spotbugs issues with AWS::Transfer::Profile * Fix Spotbugs issues with AWS::Transfer::Workflow * Fix Spotbugs issues with AWS::Transfer::(Server|User) Also added coverage improvement tests for BaseHandlerStd classes. This change is necessary due to the fact that Uluru is adding Spotbugs and specifically a new Spotbugs plugin designed to catch problems in handler implementations. Signed-off-by: Alex Volanis <[email protected]>
aws-cloudformation · May 17, 2024 · 558258e · 558258e
1 parent 73f5849
commit 558258e
Show file tree

Hide file tree

Showing 78 changed files with 1,898 additions and 1,555 deletions.
diff --git a/aws-transfer-agreement/spotbugs-exclude.xml b/aws-transfer-agreement/spotbugs-exclude.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<FindBugsFilter>
+    <Match>
+        <Class name="software.amazon.transfer.agreement.HandlerWrapperExecutable"/>
+    </Match>
+    <Match>
+        <Class name="software.amazon.transfer.agreement.HandlerWrapper"/>
+    </Match>
+    <Match>
+        <Class name="software.amazon.transfer.agreement.ResourceModel"/>
+    </Match>
+    <Match>
+        <Bug code="RCN,EI,EI2"/>
+    </Match>
+</FindBugsFilter>
diff --git a/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/BaseHandlerStd.java b/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/BaseHandlerStd.java
@@ -0,0 +1,40 @@
+package software.amazon.transfer.agreement;
+
+import software.amazon.awssdk.services.transfer.TransferClient;
+import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy;
+import software.amazon.cloudformation.proxy.Logger;
+import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
+import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
+
+/**
+ * The purpose of this base class is to allow a simple calling pattern that
+ * makes testing Uluru handlers easier and avoids having to store context
+ * in class fields. The {@link MockableBaseHandler} interface is used to
+ * make isolation of the inner call such that in testing we guarantee that
+ * the caller will not pick the wrong method and avoid human error.
+ */
+public abstract class BaseHandlerStd extends BaseHandler<CallbackContext>
+        implements MockableBaseHandler<CallbackContext> {
+    @Override
+    public final ProgressEvent<ResourceModel, CallbackContext> handleRequest(
+            final AmazonWebServicesClientProxy proxy,
+            final ResourceHandlerRequest<ResourceModel> request,
+            final CallbackContext callbackContext,
+            final Logger logger) {
+        return handleRequest(
+                proxy,
+                request,
+                callbackContext != null ? callbackContext : new CallbackContext(),
+                proxy.newProxy(ClientBuilder::getClient),
+                logger);
+    }
+
+    @Override
+    public abstract ProgressEvent<ResourceModel, CallbackContext> handleRequest(
+            final AmazonWebServicesClientProxy proxy,
+            final ResourceHandlerRequest<ResourceModel> request,
+            final CallbackContext callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
+            final Logger logger);
+}
diff --git a/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/CreateHandler.java b/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/CreateHandler.java
@@ -21,31 +21,24 @@
 import software.amazon.cloudformation.proxy.Logger;
 import software.amazon.cloudformation.proxy.OperationStatus;
 import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
 import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
 
 import com.amazonaws.util.CollectionUtils;
 
 import lombok.NoArgsConstructor;
 
 @NoArgsConstructor
-public class CreateHandler extends BaseHandler<CallbackContext> {
-    private TransferClient client;
-
-    public CreateHandler(TransferClient client) {
-        this.client = client;
-    }
+public class CreateHandler extends BaseHandlerStd {
 
     @Override
     public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
             final AmazonWebServicesClientProxy proxy,
             final ResourceHandlerRequest<ResourceModel> request,
             final CallbackContext callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
             final Logger logger) {
 
-        if (this.client == null) {
-            this.client = ClientBuilder.getClient();
-        }
-
         final ResourceModel model = request.getDesiredResourceState();
 
         Map<String, String> allTags = new HashMap<>();
@@ -73,7 +66,7 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
                                         .collect(Collectors.toList()))
                 .build();
 
-        try {
+        try (TransferClient client = proxyClient.client()) {
             CreateAgreementResponse response =
                     proxy.injectCredentialsAndInvokeV2(createAgreementRequest, client::createAgreement);
             model.setAgreementId(response.agreementId());

diff --git a/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/DeleteHandler.java b/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/DeleteHandler.java
@@ -14,37 +14,29 @@
 import software.amazon.cloudformation.proxy.Logger;
 import software.amazon.cloudformation.proxy.OperationStatus;
 import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
 import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
 
 import lombok.NoArgsConstructor;
 
 @NoArgsConstructor
-public class DeleteHandler extends BaseHandler<CallbackContext> {
-    private TransferClient client;
-
-    public DeleteHandler(TransferClient client) {
-        this.client = client;
-    }
-
+public class DeleteHandler extends BaseHandlerStd {
     @Override
     public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
             final AmazonWebServicesClientProxy proxy,
             final ResourceHandlerRequest<ResourceModel> request,
             final CallbackContext callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
             final Logger logger) {
 
-        if (this.client == null) {
-            this.client = ClientBuilder.getClient();
-        }
-
         final ResourceModel model = request.getDesiredResourceState();
 
         DeleteAgreementRequest deleteAgreementRequest = DeleteAgreementRequest.builder()
                 .agreementId(model.getAgreementId())
                 .serverId(model.getServerId())
                 .build();
 
-        try {
+        try (TransferClient client = proxyClient.client()) {
             proxy.injectCredentialsAndInvokeV2(deleteAgreementRequest, client::deleteAgreement);
             logger.log(
                     String.format("%s %s deleted successfully", ResourceModel.TYPE_NAME, model.getPrimaryIdentifier()));

diff --git a/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/ListHandler.java b/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/ListHandler.java
@@ -16,29 +16,21 @@
 import software.amazon.cloudformation.proxy.Logger;
 import software.amazon.cloudformation.proxy.OperationStatus;
 import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
 import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
 
 import lombok.NoArgsConstructor;
 
 @NoArgsConstructor
-public class ListHandler extends BaseHandler<CallbackContext> {
-    private TransferClient client;
-
-    public ListHandler(TransferClient client) {
-        this.client = client;
-    }
-
+public class ListHandler extends BaseHandlerStd {
     @Override
     public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
             final AmazonWebServicesClientProxy proxy,
             final ResourceHandlerRequest<ResourceModel> request,
             final CallbackContext callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
             final Logger logger) {
 
-        if (this.client == null) {
-            this.client = ClientBuilder.getClient();
-        }
-
         final ResourceModel topModel = request.getDesiredResourceState();
         final List<ResourceModel> models = new ArrayList<>();
 
@@ -48,7 +40,7 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
                 .nextToken(request.getNextToken())
                 .build();
 
-        try {
+        try (TransferClient client = proxyClient.client()) {
             ListAgreementsResponse response =
                     proxy.injectCredentialsAndInvokeV2(listAgreementsRequest, client::listAgreements);
 

diff --git a/...nsfer-agreement/src/main/java/software/amazon/transfer/agreement/MockableBaseHandler.java b/...nsfer-agreement/src/main/java/software/amazon/transfer/agreement/MockableBaseHandler.java
@@ -0,0 +1,24 @@
+package software.amazon.transfer.agreement;
+
+import software.amazon.awssdk.services.transfer.TransferClient;
+import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy;
+import software.amazon.cloudformation.proxy.Logger;
+import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
+import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
+
+/**
+ * Interface exposing the only handler method that should be used when
+ * testing Uluru handlers. This provides a mechanism to feed the call chain
+ * a mock client as needed without complex static mocking of the ClientBuilder.
+ *
+ * @param <CallbackT>
+ */
+interface MockableBaseHandler<CallbackT> {
+    ProgressEvent<ResourceModel, CallbackT> handleRequest(
+            final AmazonWebServicesClientProxy proxy,
+            final ResourceHandlerRequest<ResourceModel> request,
+            final CallbackT callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
+            final Logger logger);
+}
diff --git a/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/ReadHandler.java b/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/ReadHandler.java
@@ -18,37 +18,29 @@
 import software.amazon.cloudformation.proxy.Logger;
 import software.amazon.cloudformation.proxy.OperationStatus;
 import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
 import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
 
 import com.amazonaws.util.CollectionUtils;
 
 import lombok.NoArgsConstructor;
 
 @NoArgsConstructor
-public class ReadHandler extends BaseHandler<CallbackContext> {
-    private TransferClient client;
-
-    public ReadHandler(TransferClient client) {
-        this.client = client;
-    }
-
+public class ReadHandler extends BaseHandlerStd {
     @Override
     public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
             final AmazonWebServicesClientProxy proxy,
             final ResourceHandlerRequest<ResourceModel> request,
             final CallbackContext callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
             final Logger logger) {
 
-        if (this.client == null) {
-            this.client = ClientBuilder.getClient();
-        }
-
         final ResourceModel model = request.getDesiredResourceState();
         DescribeAgreementRequest describeAgreementRequest = DescribeAgreementRequest.builder()
                 .agreementId(model.getAgreementId())
                 .serverId(model.getServerId())
                 .build();
-        try {
+        try (TransferClient client = proxyClient.client()) {
             DescribeAgreementResponse response =
                     proxy.injectCredentialsAndInvokeV2(describeAgreementRequest, client::describeAgreement);
             logger.log(String.format(

diff --git a/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/UpdateHandler.java b/aws-transfer-agreement/src/main/java/software/amazon/transfer/agreement/UpdateHandler.java
@@ -24,29 +24,21 @@
 import software.amazon.cloudformation.proxy.Logger;
 import software.amazon.cloudformation.proxy.OperationStatus;
 import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
 import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
 
 import lombok.NoArgsConstructor;
 
 @NoArgsConstructor
-public class UpdateHandler extends BaseHandler<CallbackContext> {
-    private TransferClient client;
-
-    public UpdateHandler(TransferClient client) {
-        this.client = client;
-    }
-
+public class UpdateHandler extends BaseHandlerStd {
     @Override
     public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
             final AmazonWebServicesClientProxy proxy,
             final ResourceHandlerRequest<ResourceModel> request,
             final CallbackContext callbackContext,
+            final ProxyClient<TransferClient> proxyClient,
             final Logger logger) {
 
-        if (this.client == null) {
-            this.client = ClientBuilder.getClient();
-        }
-
         final ResourceModel model = request.getDesiredResourceState();
         String arn = String.format(
                 "arn:%s:transfer:%s:%s:agreement/%s/%s",
@@ -82,7 +74,7 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
         Set<Tag> tagsToAdd = Sets.difference(new HashSet<>(desiredTags), new HashSet<>(previousTags));
         Set<Tag> tagsToRemove = Sets.difference(new HashSet<>(previousTags), new HashSet<>(desiredTags));
 
-        try {
+        try (TransferClient client = proxyClient.client()) {
             proxy.injectCredentialsAndInvokeV2(updateAgreementRequest, client::updateAgreement);
             logger.log(String.format("%s updated successfully", ResourceModel.TYPE_NAME));