Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip validation on delete requests #318 #330

Merged
merged 2 commits into from
Mar 2, 2021
Merged

Skip validation on delete requests #318 #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e93292
Skip validation on delete requests #318
Nov 11, 2020
f9e3aed
Replace Set of to Immutable Set
anshikg Mar 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/main/java/software/amazon/cloudformation/AbstractWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,17 @@
import com.fasterxml.jackson.databind.exc.MismatchedInputException;
import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import java.io.InputStream;
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 +74,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 = ImmutableSet.of(Action.CREATE, Action.DELETE, Action.UPDATE);
private static final Set<Action> VALIDATING_ACTIONS = ImmutableSet.of(Action.CREATE, Action.UPDATE);

protected final Serializer serializer;
protected LoggerProxy loggerProxy;
Expand Down Expand Up @@ -245,7 +247,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 +274,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
Open in desktop
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
Open in desktop
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
Open in desktop
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