Skip to content

Catch NPE when creating URI #471

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

Merged
merged 1 commit into from
Aug 27, 2019
Merged

Catch NPE when creating URI #471

merged 1 commit into from
Aug 27, 2019

Conversation

pulkitsethi
Copy link
Contributor

This PR catches an NPE that is thrown when passing in a null url to MessageCreator.setMediaUrl().

A few reasons it makes sense to do this:

  1. MessageCreator.create() itself handles null values for any params:
  2. Checking for a null mediaUrl string messes up the flow using the API:
    Message message = Message.creator(toPN, fromPN, body)
            .setMediaUrl(mediaUrl)	
            .create();

turns into this:

    MessageCreator messageCreator = Message.creator(toPN, fromPN, body);

    if (!Strings.isNullOrEmpty(mediaUrl)) {
      messageCreator.setMediaUrl(mediaUrl);
    }

    Message message = messageCreator.create();
  1. We are already returning null if we throw a URISyntaxException.

} catch (URISyntaxException e) {

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@@ -18,7 +18,7 @@
public static URI uriFromString(final String url) {
try {
return new URI(url);
} catch (URISyntaxException e) {
} catch (URISyntaxException | NullPointerException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with this since we're already catching URISyntaxException, but I don't like that syntax exceptions are silently swallowed. But that's out-of-scope here.

@childish-sambino childish-sambino merged commit 02881bc into twilio:master Aug 27, 2019
@childish-sambino childish-sambino added status: code review request requesting a community code review or review from Twilio status: ready for deploy code ready to be released in next deploy type: community enhancement feature request not on Twilio's roadmap labels Aug 27, 2019
FalguniV pushed a commit to FalguniV/twilio-java that referenced this pull request Oct 13, 2020
FalguniV pushed a commit to FalguniV/twilio-java that referenced this pull request Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio status: ready for deploy code ready to be released in next deploy type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants