Skip to content

Conversation

wbingli
Copy link

@wbingli wbingli commented Jun 8, 2020

Issue #, if available: #273

Description of changes:

This targets to solve the problem #273, which the Java plugin will fail sadly if the resource model type mismatch.e.g. needs a String but giving a List. Customer will see Internal Failure for such issue, it's pretty bad experience to debug such error, which needs cloudformation service support.

The whole fix is not trying to refactor the way we are doing validation now, but try to solve this specified problem. Therefore, the change and risk is minimal but result is what we expected.

Why change branch coverage to 0.80
The build failed due to branch coverage. But this change covers the code change, it's due to the whole all branch increase but other classes has low branch coverage.

Testing

Tested with LogGroup with an invalid property:

The RetentionInDays should an number not a list.

  LogGroup:
    Type: 'AWS::Logs::LogGroup'
    Properties:
      RetentionInDays:
        - 1

Got expected validation message:


2020-06-08 01:37:42 | AWS::Logs::LogGroup | LogGroup |   | CREATE_FAILED | Model validation failed (#/RetentionInDays: #: only 0 subschema matches out of 2 #/RetentionInDays: expected type: Number, found: JSONArray #/RetentionInDays: failed validation constraint for keyword [enum]) |



By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@wbingli wbingli requested review from rjlohan and johnttompkins June 8, 2020 00:56
@wbingli wbingli requested a review from alicesun16 June 8, 2020 02:36
@wbingli wbingli requested a review from sungkkim June 8, 2020 16:24
@wbingli wbingli merged commit a6cae49 into aws-cloudformation:master Jun 8, 2020
request = this.serializer.deserialize(input, typeReference);
} catch (MismatchedInputException e) {
JSONObject resourceSchemaJSONObject = provideResourceSchemaJSONObject();
JSONObject rawModelObject = rawInput.getJSONObject("requestData").getJSONObject("resourceProperties");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to exceptions thrown here, if the JSON payload is completely malformed and doesn't contain the fields you're expecting?

Copy link
Author

@wbingli wbingli Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The JSON payload completely malformed only if it invoked not by CloudFormation, which this case is really unlikely, or someone debug with local sam.

  2. The above this.serializer.deserialize(input, typeReference) will throw different exception if the payload is malformed, it won't be catched by MismatchedInputException. We have unit test to cover the malformed input (0 bytes or invalid json), those passed as expected.

  3. The only case I can think that rawInput.getJSONObject("requestData").getJSONObject("resourceProperties"); is null or malformed requestData, the whole payload HandlerRequest has MismatchedInputException in other fields. However, I don't think it's case we need to worry about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants