Skip to content

Commit

Permalink
Skip validation on delete requests aws-cloudformation#318
Browse files Browse the repository at this point in the history
  • Loading branch information
@anshikg
John Tompkins authored and anshikg committed Mar 2, 2021
1 parent 8d02fd1 commit 5e93292
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 24 deletions.
17 changes: 11 additions & 6 deletions src/main/java/software/amazon/cloudformation/AbstractWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@
import java.io.OutputStream;
import java.nio.charset.StandardCharsets;
import java.time.Instant;
import java.util.Arrays;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
Expand Down Expand Up @@ -73,7 +73,8 @@ public abstract class AbstractWrapper<ResourceT, CallbackT> {

public static final SdkHttpClient HTTP_CLIENT = ApacheHttpClient.builder().build();

private static final List<Action> MUTATING_ACTIONS = Arrays.asList(Action.CREATE, Action.DELETE, Action.UPDATE);
private static final Set<Action> MUTATING_ACTIONS = Set.of(Action.CREATE, Action.DELETE, Action.UPDATE);
private static final Set<Action> VALIDATING_ACTIONS = Set.of(Action.CREATE, Action.UPDATE);

protected final Serializer serializer;
protected LoggerProxy loggerProxy;
Expand Down Expand Up @@ -245,7 +246,8 @@ public void processRequest(final InputStream inputStream, final OutputStream out
throw new TerminalException("Invalid request object received");
}

if (MUTATING_ACTIONS.contains(request.getAction())) {
final boolean isMutatingAction = MUTATING_ACTIONS.contains(request.getAction());
if (isMutatingAction) {
if (request.getRequestData().getResourceProperties() == null) {
throw new TerminalException("Invalid resource properties object received");
}
Expand All @@ -271,13 +273,16 @@ public void processRequest(final InputStream inputStream, final OutputStream out

this.metricsPublisherProxy.publishInvocationMetric(Instant.now(), request.getAction());

// for CUD actions, validate incoming model - any error is a terminal failure on
// for create and update actions, validate incoming model - any error is a
// terminal failure on
// the invocation
// NOTE: we validate the raw pre-deserialized payload to account for lenient
// serialization.
// NOTE: Validation is not required on deletion as only the primary identifier
// is required to delete.
// Here, we want to surface ALL input validation errors to the caller.
boolean isMutatingAction = MUTATING_ACTIONS.contains(request.getAction());
if (isMutatingAction) {
final boolean shouldValidate = VALIDATING_ACTIONS.contains(request.getAction());
if (shouldValidate) {
// validate entire incoming payload, including extraneous fields which
// are stripped by the Serializer (due to FAIL_ON_UNKNOWN_PROPERTIES setting)
JSONObject rawModelObject = rawRequest.getJSONObject("requestData").getJSONObject("resourceProperties");
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/software/amazon/cloudformation/ExecutableWrapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ public void invokeHandler_CompleteSynchronously_returnsSuccess(final String requ
// verify initialiseRuntime was called and initialised dependencies
verifyInitialiseRuntime();

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator, times(1)).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/java/software/amazon/cloudformation/LambdaWrapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ public void invokeHandler_CompleteSynchronously_returnsSuccess(final String requ
// verify initialiseRuntime was called and initialised dependencies
verifyInitialiseRuntime();

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator, times(1)).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand Down
29 changes: 15 additions & 14 deletions src/test/java/software/amazon/cloudformation/WrapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ public void invokeHandler_nullResponse_returnsFailure(final String requestDataPa
verify(providerMetricsPublisher).publishInvocationMetric(any(Instant.class), eq(action));
verify(providerMetricsPublisher).publishDurationMetric(any(Instant.class), eq(action), anyLong());

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand Down Expand Up @@ -269,8 +269,8 @@ public void invokeHandler_handlerFailed_returnsFailure(final String requestDataP
// verify initialiseRuntime was called and initialised dependencies
verifyInitialiseRuntime();

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand Down Expand Up @@ -325,8 +325,8 @@ public void invokeHandler_CompleteSynchronously_returnsSuccess(final String requ
// verify initialiseRuntime was called and initialised dependencies
verifyInitialiseRuntime();

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand Down Expand Up @@ -400,11 +400,12 @@ public void invokeHandler_InProgress_returnsInProgress(final String requestDataP
verify(providerMetricsPublisher).publishInvocationMetric(any(Instant.class), eq(action));
verify(providerMetricsPublisher).publishDurationMetric(any(Instant.class), eq(action), anyLong());

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator).validateObject(any(JSONObject.class), any(JSONObject.class));
}

// verify output response
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
verifyHandlerResponse(out, ProgressEvent.<TestModel, TestContext>builder().status(OperationStatus.IN_PROGRESS)
.resourceModel(TestModel.builder().property1("abc").property2(123).build()).build());
} else {
Expand Down Expand Up @@ -465,8 +466,8 @@ public void reInvokeHandler_InProgress_returnsInProgress(final String requestDat
// validation failure metric should not be published
verifyNoMoreInteractions(providerMetricsPublisher);

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand All @@ -477,7 +478,7 @@ public void reInvokeHandler_InProgress_returnsInProgress(final String requestDat
}

@ParameterizedTest
@CsvSource({ "create.request.json,CREATE", "update.request.json,UPDATE", "delete.request.json,DELETE" })
@CsvSource({ "create.request.json,CREATE", "update.request.json,UPDATE" })
public void invokeHandler_SchemaValidationFailure(final String requestDataPath, final String actionAsString)
throws IOException {
final Action action = Action.valueOf(actionAsString);
Expand All @@ -499,8 +500,8 @@ public void invokeHandler_SchemaValidationFailure(final String requestDataPath,
// all metrics should be published, even for a single invocation
verify(providerMetricsPublisher, times(1)).publishInvocationMetric(any(Instant.class), eq(action));

// verify that model validation occurred for CREATE/UPDATE/DELETE
if (action == Action.CREATE || action == Action.UPDATE || action == Action.DELETE) {
// verify that model validation occurred for CREATE/UPDATE
if (action == Action.CREATE || action == Action.UPDATE) {
verify(validator).validateObject(any(JSONObject.class), any(JSONObject.class));
}

Expand Down

0 comments on commit 5e93292

Please sign in to comment.