Cfn error code exceptions do not use error messages from the cause exception, omitting service level error messages and request Ids #243

Activity

1. aygold92

   added

   bugSomething isn't workingSomething isn't working

   code quality

   good first issueGood for newcomersGood for newcomers

   on Apr 17, 2020
2. aygold92

   changed the title [-]Cfn error code exceptions do not take error messages[/-] [+]Cfn error code exceptions do not take error messages/are implemented incorrectly[/+] on Apr 17, 2020
3. aygold92 commented on Apr 17, 2020

   

   aygold92

   on Apr 17, 2020

   ContributorAuthor

   More actions

   After doing a bit of digging it looks like we actually should use exception messages

   cloudformation-cli-java-plugin/src/main/java/software/amazon/cloudformation/proxy/ExceptionMessages.java

   Line 17 in ddfbcbf

   final class ExceptionMessages {

   , but the constructors are pretty confusing and these messages never include AWS requestIds.
4. aygold92 commented on Apr 17, 2020

   

   aygold92

   on Apr 17, 2020

   ContributorAuthor

   More actions

   it also looks we use BaseHandlerException which includes the ErrorCode

   cloudformation-cli-java-plugin/src/main/java/software/amazon/cloudformation/exceptions/BaseHandlerException.java

   Line 20 in ddfbcbf

   @Getter
5. aygold92 commented on Apr 17, 2020

   

   aygold92

   on Apr 17, 2020

   Last edited by aygold92

   ContributorAuthor

   More actions

   it looks like really what the work is:

   1. improve exception messages from the BaseHandlerExceptions to include AWS error messages/requestIds
      Note: ResourceAlreadyExistsException actually extends CfnAlreadyExistsException and ResourceNotFoundException extends CfnNotFoundException, so again the issue is just we omit the service level exception message/requestId
      https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/exceptions/ResourceAlreadyExistsException.java
      https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/exceptions/ResourceNotFoundException.java
   2. add documentation that while it is required to return a progress event, handlers using the java plugin can throw a BaseHandlerException, which will be translated into a Progress Event by wrapper. This is the only acceptable exception to be thrown, otherwise they will see internal failure
   3. remove TerminalException https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/exceptions/TerminalException.java, which is not used or caught anywhere and is confusing
6. aygold92

   changed the title [-]Cfn error code exceptions do not take error messages/are implemented incorrectly[/-] [+]Cfn error code exceptions do not use error messages[/+] on Apr 17, 2020
7. aygold92

   changed the title [-]Cfn error code exceptions do not use error messages[/-] [+]Cfn error code exceptions do not use error messages from the cause, omitting service level error messages and request Ids[/+] on Apr 17, 2020
8. aygold92

   changed the title [-]Cfn error code exceptions do not use error messages from the cause, omitting service level error messages and request Ids[/-] [+]Cfn error code exceptions do not use error messages from the cause exception, omitting service level error messages and request Ids[/+] on Apr 17, 2020
9. aygold92

   added

   documentationI'll give you a guess what this is about.I'll give you a guess what this is about.

   and removed

   bugSomething isn't workingSomething isn't working

   good first issueGood for newcomersGood for newcomers

   on Apr 17, 2020
10. PatMyron commented on Aug 26, 2020

    

    PatMyron

    on Aug 26, 2020

    Contributor

    More actions

    Lastly, NotFound and AlreadyExists have duplicate exceptions, and I'm not sure what they are for:

    • ResourceNotFoundException and CfnNotFoundException
    • ResourceAlreadyExistsException and CfnAlreadyExistsException

    looks like these should be deprecated to avoid conflicts:

    cloudformation-cli-java-plugin/src/main/java/software/amazon/cloudformation/exceptions/ResourceNotFoundException.java

    Lines 18 to 21 in 1dd595a

    	* Uses for this exception class should delegate instead to
    	* CfnNotFoundException. Many APIs use exception classes titled
    	* ResourceNotFoundException, and if handler authors use this class, they'll
    	* often need to use the fully qualified exception name.

    cloudformation-cli-java-plugin/src/main/java/software/amazon/cloudformation/exceptions/ResourceAlreadyExistsException.java

    Lines 18 to 21 in 1dd595a

    	* Uses for this exception class should delegate instead to
    	* CfnAlreadyExistsException. Many APIs use exception classes titled
    	* ResourceAlreadyExistsException, and if handler authors use this class,
    	* they'll often need to use the fully qualified exception name.
11. wbingli commented on Sep 11, 2020

    

    wbingli

    on Sep 11, 2020

    More actions

    hmm, this issue should be fixed by #264
12. ammokhov commented on Sep 22, 2020

    

    ammokhov

    on Sep 22, 2020

    Contributor

    More actions

    closing this issue as has been fixed by #264
13. ammokhov

    closed this as completedon Sep 22, 2020