chore(ir): add specific error message when an invalid resource property is used by colifran · Pull Request #594 · cdklabs/cdk-from-cfn

colifran · 2024-03-05T17:55:40Z

In the event that an invalid property is specified for a specific resource type the value_type will be None causing the other arm of the match statement below to execute.

cdk-from-cfn/src/ir/resources/mod.rs

Lines 119 to 129 in 2c14390

    
           ResourceValue::Object(o) => { 
        
               let property_bag: Box<dyn PropertyBag> = match &self.value_type { 
        
                   Some(TypeReference::Named(name)) => { 
        
                       Box::new(self.schema.type_named(name).cloned().unwrap()) 
        
                   } 
        
                   Some(TypeReference::Map(item_type)) => Box::new(MapOf(item_type)), 
        
                   Some(TypeReference::Primitive(Primitive::Json)) => { 
        
                       Box::new(MapOf(&TypeReference::Primitive(Primitive::Json))) 
        
                   } 
        
                   other => unimplemented!("{other:?}"), 
        
               };

Due to the unimplemented! macro panicking in-place, a non-descriptive error message is displayed while using cdk migrate:

cdk migrate --from-path './CDKMigrateExampleTemplate.yml' --stack-name CustomStackName
...
 ❌  Migrate failed for `CustomStackName`: stack generation failed due to error 'unreachable'

stack generation failed due to error 'unreachable'

This PR adds a new explicit check for the case where property_type is None. If that case is true, then a more descriptive error message will be displayed explaining what resource property is not valid. For example,

cdk migrate --from-path './CDKMigrateExampleTemplate.yml' --stack-name CustomStackName
...
 ❌  Migrate failed for `CustomStackName`: stack generation failed due to error 'ReadEndpoint is not a valid resource property for resource RDSCluster of type AWS::RDS::DBCluster'

stack generation failed due to error 'ReadEndpoint is not a valid resource property for resource RDSCluster of type AWS::RDS::DBCluster'

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Francis <colifran@amazon.com>

codecov · 2024-03-05T23:10:34Z

Codecov Report

Merging #594 (15f20dc) into main (2c14390) will increase coverage by 0.2%.
The diff coverage is 94.1%.

Additional details and impacted files

Components	Coverage Δ
Parser	`76.2% <ø> (ø)`
Intermediate Representation	`80.4% <94.1%> (+0.9%)`	⬆️
Synthesizers	`87.3% <ø> (ø)`
Other	`35.0% <ø> (+0.7%)`	⬆️

@@           Coverage Diff           @@
##            main    #594     +/-   ##
=======================================
+ Coverage   81.0%   81.2%   +0.2%     
=======================================
  Files         27      27             
  Lines       5098    5110     +12     
  Branches    5098    5110     +12     
=======================================
+ Hits        4130    4148     +18     
+ Misses       766     761      -5     
+ Partials     202     201      -1

Files	Coverage Δ
src/ir/resources/mod.rs	`79.9% <94.1%> (+1.4%)`	⬆️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c14390...15f20dc. Read the comment docs.

TheRealAmazonKendra · 2024-03-05T23:48:02Z

Could we wordsmith that error a little more? Maybe some thing more like: ${stackName} could not be generated because <the rest of your error message is great>

TheRealAmazonKendra · 2024-03-05T23:50:17Z

Neverminddddd, that part of the error message is on the CLI side so approving this since it looks great!

### Reason for this change This change is a follow-up to a [PR](cdklabs/cdk-from-cfn#594) that improved the error message thrown by `cdk-from-cfn` when an invalid resource property was used in a CloudFormation template. This PR further improves the error message on the cli side. ### Description of changes Primarily, this PR is a verbiage change. The base error message now states that the `<stack-name> could not be generated because <error-message>`. The error message itself is checked against `unreachable` because any use of `panic!`, `unreachable!`, or `unimplemented!` will cause the `cdk-from-cfn` source code to panic in-place. In the resulting wasm binary, this produces a `RuntimeError` that has an error message of `unreachable`. I've improved this slightly by stating `template and/or language inputs caused the source code to panic`. If the error message is not `unreachable`, then the error message is taken as is with `TransmuteError:` replaced. Note that we should still continue to improve our error messages in `cdk-from-cfn` by by replacing `panic!`, `unreachable!`, and `unimplemented!` with more detailed error messages. ### Description of how you validated changes An existing unit test was changed based on the error message verbiage change. Additionally, a new unit test was added to validate that the expected error message would be thrown by the cli when an invalid resource property was used in a CloudFormation template. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

added improved error message for property_type None

9f870cd

Signed-off-by: Francis <colifran@amazon.com>

cdklabs-automation enabled auto-merge March 5, 2024 17:55

colifran changed the title ~~chore: add specific error message when an invalid property_type is used~~ chore: add specific error message when an invalid resource property is used Mar 5, 2024

Francis added 7 commits March 5, 2024 10:03

formatting

5f390ce

Signed-off-by: Francis <colifran@amazon.com>

formatting

b715e81

Signed-off-by: Francis <colifran@amazon.com>

small update to error message

d140299

Signed-off-by: Francis <colifran@amazon.com>

add resource name to error message

a6de07b

Signed-off-by: Francis <colifran@amazon.com>

unit test for invalid resource property

2049c26

Signed-off-by: Francis <colifran@amazon.com>

remove unnecessary import

c6358aa

Signed-off-by: Francis <colifran@amazon.com>

formatting

15f20dc

Signed-off-by: Francis <colifran@amazon.com>

colifran changed the title ~~chore: add specific error message when an invalid resource property is used~~ chore(ir): add specific error message when an invalid resource property is used Mar 5, 2024

TheRealAmazonKendra approved these changes Mar 5, 2024

View reviewed changes

cdklabs-automation added this pull request to the merge queue Mar 5, 2024

Merged via the queue into main with commit 4346c85 Mar 5, 2024

cdklabs-automation deleted the colifran/improve-error-message branch March 5, 2024 23:58

TheRealAmazonKendra mentioned this pull request Mar 6, 2024

cdk migrate: cdk migrate fails with "unreachable" error when template includes "AWS::RDS::DBCluster" that specifies the "ReadEndpoint" property aws/aws-cdk#29293

Open

colifran mentioned this pull request Mar 7, 2024

chore(cli): improve error message for cdk migrate aws/aws-cdk#29392

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ir): add specific error message when an invalid resource property is used#594

chore(ir): add specific error message when an invalid resource property is used#594
cdklabs-automation merged 8 commits intomainfrom
colifran/improve-error-message

colifran commented Mar 5, 2024 •

edited

Loading

Uh oh!

codecov bot commented Mar 5, 2024

Uh oh!

TheRealAmazonKendra commented Mar 5, 2024

Uh oh!

TheRealAmazonKendra commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	ResourceValue::Object(o) => {
	let property_bag: Box<dyn PropertyBag> = match &self.value_type {
	Some(TypeReference::Named(name)) => {
	Box::new(self.schema.type_named(name).cloned().unwrap())
	}
	Some(TypeReference::Map(item_type)) => Box::new(MapOf(item_type)),
	Some(TypeReference::Primitive(Primitive::Json)) => {
	Box::new(MapOf(&TypeReference::Primitive(Primitive::Json)))
	}
	other => unimplemented!("{other:?}"),
	};

Conversation

colifran commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 5, 2024

Codecov Report

Uh oh!

TheRealAmazonKendra commented Mar 5, 2024

Uh oh!

TheRealAmazonKendra commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

colifran commented Mar 5, 2024 •

edited

Loading