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

[TIMOB-23459]: Null check in getFile() #8038

Merged
merged 1 commit into from Jun 22, 2016

Conversation

hieupham007
Copy link
Contributor

@@ -81,6 +81,10 @@ public boolean isExternalStoragePresent()
@Kroll.method
public FileProxy getFile(KrollInvocation invocation, Object[] parts)
{
//If directory doesn't exist, return
if (parts[0] == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if any of the parts are null? Does it cause an NPE or end up with a nonsense "null" string as a path segment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity: Maybe Arrays.asList(parts).subList(0, parts.length).contains(null) or a manual loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'd say we should log a warning or error to the user to let them know they passed in a null argument (maybe spit out the args received?) and we're returning null. Or throw a JS Error or something so they can pinpoint the bad call and know why they get back a null reference (or get the error thrown).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the file name is null, it won't make it to Java. I think the check should be sufficient

@ashcoding
Copy link
Contributor

Functionally tested this. Crash does not occur any more.
Will wait for comment by @sgtcoolguy to be addressed first before further actions.

@ashcoding
Copy link
Contributor

I'm going to merge and will have another PR to log out a warning.

@ashcoding ashcoding merged commit ec51a18 into tidev:master Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants