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

[aws-xray-recorder-sdk-apache-http] Don't return a null target to avoid possible NPE #338

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

srprash
Copy link
Contributor

@srprash srprash commented Jun 16, 2022

Issue:
The Apache HTTP instrumentation is prone to throwing a NullPointerException by returning a null from the determineTarget method. Since a user is not expected to handle NPE, it will break the application.

Description of changes:
The method doesn't need to check for URL absoluteness and bail out early if it is not. The URIUtils.extractHost(requestUri) method does this check implicitly and we handle it's null return value by throwing a ClientProtocolException which a user will handle in their code.

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

@srprash srprash marked this pull request as ready for review June 20, 2022 16:31
@srprash srprash requested a review from a team as a code owner June 20, 2022 16:31
@srprash srprash requested a review from willarmiros June 20, 2022 16:31
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just want to confirm: The test case threw a NPE before the fix, and now just throws a ClientProtocolException? If so, then good to merge

@srprash
Copy link
Contributor Author

srprash commented Jun 20, 2022

Just want to confirm: The test case threw a NPE before the fix, and now just throws a ClientProtocolException?

Yes, it did. Running the same unit test without the fix:

TracedHttpClientTest > testExceptionOnRelativeUrl FAILED
    java.lang.AssertionError: unexpected exception type thrown; expected:<org.apache.http.client.ClientProtocolException> but was:<java.lang.NullPointerException>
        at org.junit.Assert.assertThrows(Assert.java:1020)
        at org.junit.Assert.assertThrows(Assert.java:981)
        at com.amazonaws.xray.proxies.apache.http.TracedHttpClientTest.testExceptionOnRelativeUrl(TracedHttpClientTest.java:124)
        Caused by:
        java.lang.NullPointerException
            at com.amazonaws.xray.proxies.apache.http.TracedHttpClient.execute(TracedHttpClient.java:180)
            at com.amazonaws.xray.proxies.apache.http.TracedHttpClientTest.lambda$testExceptionOnRelativeUrl$0(TracedHttpClientTest.java:125)
            at org.junit.Assert.assertThrows(Assert.java:1001)
            ... 2 more

@srprash srprash merged commit e3436c1 into aws:master Jun 20, 2022
@srprash srprash deleted the fix_npe branch June 20, 2022 17:58
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.

2 participants