Crash when field data is missing in formdata

[timsuchanek]

Hi, when constructing a formData object that I pass in intro rnfb.fetch('POST'..., and having the data attribute missing, the library crashes:

com.facebook.react.bridge.NoSuchKeyException: couldn't find key data in dynamic object
    at com.facebook.react.bridge.ReadableNativeMap.getString(Native Method)
    at com.RNFetchBlob.RNFetchBlobBody.countFormDataLength(RNFetchBlobBody.java:242)
    at com.RNFetchBlob.RNFetchBlobBody.writeFormData(RNFetchBlobBody.java:84)
    at com.RNFetchBlob.RNFetchBlobBody.writeTo(RNFetchBlobBody.java:70)
    at okhttp3.internal.http.HttpEngine$NetworkInterceptorChain.proceed(HttpEngine.java:704)
    at okhttp3.internal.http.HttpEngine.readResponse(HttpEngine.java:563)
    at okhttp3.RealCall.getResponse(RealCall.java:241)
    at okhttp3.RealCall$ApplicationInterceptorChain.proceed(RealCall.java:198)
    at com.RNFetchBlob.RNFetchBlobReq$1.intercept(RNFetchBlobReq.java:276)
    at okhttp3.RealCall$ApplicationInterceptorChain.proceed(RealCall.java:187)
    at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:160)
    at okhttp3.RealCall.access$100(RealCall.java:30)
    at okhttp3.RealCall$AsyncCall.execute(RealCall.java:127)
    at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
    at java.lang.Thread.run(Thread.java:818)

I know that is not the newest version of rnfb, but it still should happen in the newest as that code didn't change in the writeTo method of the RNFetchBlobBody.

So a simple check field.hasKey("data") and rejecting the promise when not would be enough, but where would you put that logic? You know better then me.

Another point, the source of my problem was, that rnfb.wrap(uri) didn't work properly, probably returned undefined, as I'm using it to get a file.
I don't know the specific uri yet that the user took, but the user used Android 6.0 and definitely could access the file via cameraroll.

I try to construct an uri that can reproduce the bug, but wanted you to know this bug already.

Thanks!

[timsuchanek]

Btw what I'm going right now to wrap the uri is the following:
(As I saw that the file:// or file:/ has to be removed)

const uri = imageUri.replace('file://', '').replace('file:/', '');  // remove the file:/ or file://
const wrappedUri = rnfb.wrap(uri);
let formData = [];

formData.push({
  name,
  filename,
  type: 'image/jpeg',
  data: wrappedUri
});


rnfb.fetch('POST', BASE_URL + '/file', {
  'Content-Type': 'multipart/form-data'
}, formData)
.then(res => {
  //...
})
.catch(e => {
  //...
});

What do you think, could using the single file upload instead of formData solve the problem?
Or is rnfb probably just not able to resolve the path of the file?

[wkh237]

@timsuchanek , thanks for reporting and detailed information !

About the first quest, I think we should check if the field has key data here and here, but I'm not sure if we should just make the data field an empty or rejecting the promise, from my point of view each would be nice, would you mind make a PR ? 😲

As for second question, rnfb.wrap() simply adding a prefix RNFetchBlob-file:// to the given uri, and this prefix let HTTP request builder knows that Okay, this is not a plain text data, it's an URI, it then remove the prefix and uses fs API read data from that URI.

Basically, our native fs API can handle the following types of URI :

• assets://
• asset-library:// (IOS CameraRoll)
• content:// (Android CameraRoll)
• /path-to-file

If you're wrapping an URI with prefix file:// that might not work properly, perhaps this is the problem ?

[timsuchanek]

@wkh237, Regarding quest 1:
I think so too, that you would have to do that in those places, but only if we would send an empty request. For me some kind of response would be nice in the app, so I know, that rnfb can't handle that path. Trying fs.stat before would be one workaround but personally I would prefer a promise rejection as probably no REST backend could make sense of that.
How do you see that? Would you see fs.stat as a best practice to find out if the path is correct or not?

Regarding quest 2:
So in my snippet I'm already removing file:// or only file:/.
Btw my two sources for files are react-native-camera and CameraRoll

The problem currently only occurs to my users on the Xperia Z3 Compact and the Galaxy S7.
That's less 5% of the users, for the others it works.

As I currently don't have access to those phones, I can't reproduce the bug.
But I will as I have to ;)

[wkh237]

I'm agree with you, perhaps simply show message in yellow box is pretty enough. Besides, I just realized there's a private class called FormField, so we can simply change the code this way

To prevent app crash 😄

As for the URI problem, after some investigation I found a StackOverflow thread, there're many comments under the +457 answer saying that, the way we're using to get the absolute path of a content:// URI does not working properly. I think I can try to find an alternative way to do this.

[timsuchanek]

Uh, that FormField solution sounds nice!

I found a pretty sophisticated path resolver here:
https://github.com/ivpusic/react-native-image-crop-picker/blob/master/android/src/main/java/com/reactnative/picker/RealPathUtil.java

I have been using it in my image resizer in production already, where it's working perfectly.
I use it in my chain as the following:

1. User chooses file
2. I resize it, resolving the path with the above script.
3. Try to upload it with rnfb.

So I think just unashamed stealing that code for react-native-fetch-blob and trying it for the path resolving could solve the problem :)

I will just try that!

Another question, just about the understanding: Why do you need the RNFetchBlob-file:// prefix?
Thanks 👍

Edit: So the method I have to replace is the normalizePath method?

[wkh237]

That sounds great !! You can just replace the normalizePath method. If that really solves the problem, I think I should send an email to thank the project owner 😄

Uh .. the reason why RNFetchBlob-file:// prefix is necessary ? I have no idea if that's the best solution, but it's simple. After all, someone may intended to send a request with body contains a URI, but it's less likely to send a request with body contains a string starts with RNFetchBlob-file://.

[timsuchanek]

So that means when I have an URI already, I don't need the RNFetchBlob-file://?

[wkh237]

For example, if the request body is a file path, it should be

RNFetchBlob-file:///path-to-the-file

more examples

RNFetchBlob-file://assets-library://path-to-asset-library-resource
RNFetchBlob-file://assets://asset-uri
RNFetchBlob-file://content://content-uri

[timsuchanek]

Ok, so only filtering file:// and file:/ before using wrap but not content:// seems the right approach?

[wkh237]

@timsuchanek , yes 👍

[wkh237]

@timsuchanek , I've published a beta version to npm, which contains the path resolver and form data fix. You may try this version and see if it works 😄

[timsuchanek]

Haha nice, yeah I implemented a version by myself, pretty same code.
I will let my users run it and report back here :)

[timsuchanek]

When compiling your new beta I get this:

/Users/timsuchanek/code/app/node_modules/react-native-fetch-blob/android/src/main/java/com/RNFetchBlob/RNFetchBlobReq.java:454: error: cannot find symbol
        resp.close();
            ^
  symbol:   method close()
  location: variable resp of type Response

Edit: I see that in master you have commented that line:
https://github.com/wkh237/react-native-fetch-blob/blob/master/src/android/src/main/java/com/RNFetchBlob/RNFetchBlobReq.java#L454
I just commented it and it works.
Is there any problem without that line?

[wkh237]

@timsuchanek , the error is due to the new added prelink script in 0.9.2-beta.2. It's for release the response data when request does not success. I think I'll fix this issue today and publish a new release.

EDIT

looks like RN 0.31 does not close the response body when request not success facebook/react-native#9478 which causes leaking.

[wkh237]

@timsuchanek , I've published 0.9.2-beta.4 on npm. I think this version is much more stable and tested. Please feel free install this version and leave any feedback, thank you ! 😄

[timsuchanek]

@wkh237 Ok, yeah the 0.9.2-beta.2 is now out in production and seems fine :)
Will upgrade to the beta.4 and report back :)
Btw one problem I already had:
In the cp method, if the dest is not valid, there is a nullpointerexception, you only catch IOException.
Just did a quick fix with catching all Exceptions ^^