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.

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

Include provided ARN string in Exception message. Enforce NPE. #3691

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

belugabehr
Copy link
Contributor

Motivation and Context

I recently had a complicated unit test that was failing to produce a valid ARN. However, the Exception message from parsing the ARN did not provide the value it was attempting to parse, so it was made more difficult to troubleshoot.

Modifications

  • Add 'arn' being parsed into the Exception message
  • Add explicit enforcement of null/empty/blank 'arn' string
  • Wrap a root IllegalArgumentException with the details of the failed parsing inside another IllegalArgumentException which contains the value of the ARN being parsed.

Testing

Added additional unit tests to cover these cases

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@belugabehr belugabehr requested a review from a team as a code owner January 13, 2023 17:06
@debora-ito debora-ito added the needs-review This issue or PR needs review from the team. label Jan 23, 2023
@L-Applin L-Applin self-requested a review March 7, 2023 14:50
L-Applin
L-Applin previously approved these changes Mar 8, 2023
Copy link
Contributor

@L-Applin L-Applin left a comment

Choose a reason for hiding this comment

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

Change looks good. Thanks for improving logging while preserving the same behaviour.

@L-Applin L-Applin dismissed their stale review March 14, 2023 14:26

There are test failures

@L-Applin
Copy link
Contributor

I noticed that tests in software.amazon.awssdk.services.s3.internal.resource.S3ArnConverterTest are failing due to exception message change:

  • parseArn_objectLambda_noName_colon
Expected: (an instance of java.lang.IllegalArgumentException and exception with message a string containing "resource must not be blank or empty")
--
but: exception with message a string containing "resource must not be blank or empty" message was "Malformed ARN: arn:aws:s3-object-lambda:us-west-2:123456789012:accesspoint:"
  • parseArn_objectLambda_noName_slash
Expected: (an instance of java.lang.IllegalArgumentException and exception with message a string containing "resource must not be blank or empty")
--
but: exception with message a string containing "resource must not be blank or empty" message was "Malformed ARN: arn:aws:s3-object-lambda:us-west-2:123456789012:accesspoint/"

@belugabehr please also update those tests to take your changes in account. Thanks!

@debora-ito
Copy link
Member

@belugabehr let us know if you want to keep working in this PR.

@debora-ito debora-ito added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. needs-review This issue or PR needs review from the team. and removed needs-review This issue or PR needs review from the team. labels Apr 19, 2023
@belugabehr
Copy link
Contributor Author

@debora-ito Other priorities keeping me from finishing - but my intent is to see it through. Thanks for the patience.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Apr 21, 2023
@belugabehr belugabehr force-pushed the arn-parse-exception-message branch from 8a4ce99 to c351026 Compare April 28, 2023 02:33
@belugabehr
Copy link
Contributor Author

@L-Applin Updated. Thanks.

@belugabehr
Copy link
Contributor Author

@L-Applin @debora-ito Any updates?

L-Applin
L-Applin previously approved these changes Sep 20, 2023
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@belugabehr belugabehr force-pushed the arn-parse-exception-message branch from 246e5f4 to a9194a2 Compare September 20, 2023 19:39
@belugabehr belugabehr force-pushed the arn-parse-exception-message branch from e0f6c16 to d998b70 Compare September 20, 2023 20:19
@belugabehr
Copy link
Contributor Author

Hey, sorry for all the SPAM on this PR. It's been so long since I've looked at this that I am falling into the same traps I fell into months ago.

I think the proposed solution is optimal. Users are told that their ARNs are not valid, and then they can dive into the cause to get the details of why.

The issue here is that there is another Exception that gets thrown from the Resource parsing code. The resource parsing code is only handled the resource string, not the entire ARN, so it cannot emit a proper exception with the ARN present.

@L-Applin L-Applin self-requested a review September 20, 2023 20:50
@L-Applin L-Applin dismissed their stale review September 20, 2023 20:50

need to address exception wrapping

Comment on lines -181 to 192
if (resource.isEmpty()) {
throw new IllegalArgumentException("Malformed ARN - no resource specified");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this on purpose as it is duplicative. The resource is further parsed (and validated) in the Resource parsing code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review This issue or PR needs review from the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants