Possible Inefficiency/issues with uploading document provider provided documents

[dantman]

I've been reviewing the code to see if RNFB is viable for my use case, I've got a document picker on Android (some native code that uses Intent.ACTION_GET_CONTENT to let the user select a file and then returns the content:// URI provided by the document provider) and I need to upload that document to our API.

From what I see in PathResolver.java, RNFetchBlobBody.java, and RNFetchBlobReq.java it looks like what RNFB does to handle a document provided by a document provider is hacky/inefficient.

The PathResolver. getRealPathFromURI is called through normalizePath by a bunch of methods that just want to get an InputStream (readFile to read it, various parts of the upload code to upload the data from that stream). To deal with document providers it first skips the standard way of doing things on Android and hardcodes patterns of a few different document providers manipulating the Uris returned by document providers like external stores, file content, google photos, and media provider in order to return something closer to a Filesystem path. If it's not one of them then it uses openInputStream and getCacheDir to copy the document provided by the document provider to a temporary file (which may involve a network download that doesn't have its progress tracked and blocks the upload; I'm not certain but it almost looks like it might block one of the threads to do this; and may be padding the size of the file to an incorrect length) and it returns the path to that file.

The upload code calling that method then uses the path to open an InputStream to that file and uploads from the stream.

So in short:

• If your document is a file on external storage or somewhere else on the filesystem, the lib will use non-standard methods of getting a file path and interact with it using the File file API. Which I believe may possibly bypass standard permissions granted for something returned by an intent and require extra permissions that aren't actually necessary or possibly not work at all at some point (because it's an entirely non-standard way of getting an InputStream from a document provider Uri). (possibly related to the problem "...exposed beyond app through Intent.getData() " at Android N #358)
• If your document comes from any other document provider (say Drive, Dropbox, a custom photo app, ...) the lib will open an InputStream for it, download the whole file to local temporary storage (ending up duplicating data if the file is actually already stored on the filesystem but is just in the private storage of another app), blocking a thread while doing this and not reporting upload progress, open another InputStream for the temp file and upload that, and then leave the temp file for the system to clean up at its leisure instead of cleaning it up.

I think it would be more reasonable to have a different method to PathResolver.getRealPathFromURI that instead returns an InputStream directly and doesn't use the path hacks. Methods like readFile and the upload body code would call that directly instead of trying to get a path.

Then filesystem files would come from the document provider provided stream and if its from a custom or remote document provider that data would stream into the upload as it is being generated or downloaded and be directly reflected in the upload progress provided by RNFS.

[dantman]

One possible reason the library may be writing to a temporary file first is because the Content-Length needs to be defined, and file size might not be immediately available for custom providers, so the library just doesn't bother with passing around file lengths and just copies the file to the filesystem so it can get a file based input stream. However the way the library does this is wrong in multiple ways for every method you could get a file.

To get the Content-Length this library seems to use inputStream.available() to set the Content-Length. The point of copying to a File appears to be to try and get .available() to return the length of the file instead of just what can be read currently. Except .available() is NOT the actual file length, it's an estimate of how much can be read without blocking and should never be used this way.

https://developer.android.com/reference/java/io/InputStream.html#available()

Note that while some implementations of InputStream will return the total number of bytes in the stream, many will not. It is never correct to use the return value of this method to allocate a buffer intended to hold all data in this stream.

Then after copying the document to the temporary file, for multipart the RNFetchBlobBody copies the data from the temp file to another temp file it uses to store the multipart body, while building headers using inefficient string concatenation open to injection attacks/corruption instead of the multipart builder.

TL;DR this library is handling files on Android in discouraged and unreliable ways and using inefficient hacks to stop the library from breaking. I'm definitely going to have to overhaul File related portions of this library if it will be at all usable.

The proper way to handle files and documents:

• Never rely on inputStream.available() instead use a custom class to hold an InputStream, size, and perhaps mimeType representing a file/document and pass that to whatever is handling data, instead of just the InputStream.
  • Though actually a getInputStream should be provided instead of an InputStream instance to reduce the possibility of accidentally opening an input stream without closing it.
• Don't duplicate file handling between single file as body, multipart count length, and multipart output, make one function in charge of getting the input streams and sizes.
• Use OkHttp's multipart builder, from what I understand doing this correctly will allow you to get the contentLength of a multipart body without needing to write the whole thing to a temporary file.
• If a filesystem path is used:
  • Get an InputStream using new FileInputStream and use file.length() to get the file size.
• If an openable file is provided by a document provider:
  • Use getContentResolver().openInputStream(uri) get get an InputStream and try to get a size using the OpenableColumns.SIZE column.
  • If the OpenableColumns.SIZE column is null, treat it as unknown length.
• If a virtual file is provided by a document provider:
  • Use getContentResolver().openTypedAssetFileDescriptor(uri, mimeType) to get an asset file descriptor and use fd.createInputStream() to get an InputStream and try to get a size using fd.getLength().
  • If the size is UNKNOWN_LENGTH, treat it as unknown length.
• If a file is treated as unknown length write the InputStream to a temporary file so the file size is known. (The key is that this is done only when the length is unknown, so 90%+ of the time this doesn't actually happen)

[lll000111]

Just an aside, you may want to also fork and clone https://github.com/wkh237/react-native-fetch-blob-dev

Plus my still open PR for that repo: wkh237/react-native-fetch-blob-dev#8

Tests are in ./test/

The main test file is ./test-init.js - it's best to only enable one or two tests or it's too much all at once. The per-release test files each test a feature introduced with that release.

A patch should pass the relevant tests, or if not, the API change should be documented.

If you have trouble setting it up you may find something in the closed issues of that repo, there are one or two minor issues where the documentation is outdated. I had planned to patch the README too but since my PR still is not merged I forgot about it.

Overall I found it pretty easy to set this up and run the tests though. If you need help setting it up I'll probably be able to help — unlike with the "fetch" part of RNFB, since I will only touch the filesystem related code.

[serhiiharbo]

We got a crash on production related to this issue.

[lll000111]

@Aquile

I just use my own fork of the 0.10.9 tree. Not a big deal?

Anyway, the package owner does not seem to have the time, clearly more help is needed. You can ask him or just maintain your own fork and merge back without time pressure on yourself.

That the situation is unsatisfactory is a general problem with ALL open source packages. Github and Co. don't help much with a professionalization — which would require a major effort of monetization, e.g. a subscription fee for all npm packages and maintainers are paid by some agreed-upon key out of that budget. The "it's free!" only goes so far.

[lll000111]

@dantman If this is still relevant, the package has a new maintainer, see the top level README, so there possibly is a chance of getting something done.

[Traviskn]

We'd love to see any PR's over at the new repo location! https://github.com/joltup/react-native-fetch-blob