dantman
commented
7 years ago 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 anInputStream,size, and perhapsmimeTyperepresenting a file/document and pass that to whatever is handling data, instead of just the InputStream.- Though actually a
getInputStreamshould be provided instead of an InputStream instance to reduce the possibility of accidentally opening an input stream without closing it.
- Though actually a
- 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 FileInputStreamand usefile.length()to get the file size.
- Get an InputStream using
- 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 theOpenableColumns.SIZEcolumn. - If the
OpenableColumns.SIZEcolumn is null, treat it as unknown length.
- Use
- If a virtual file is provided by a document provider:
- Use
getContentResolver().openTypedAssetFileDescriptor(uri, mimeType)to get an asset file descriptor and usefd.createInputStream()to get an InputStream and try to get a size usingfd.getLength(). - If the size is
UNKNOWN_LENGTH, treat it as unknown length.
- Use
- 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
serhiiharbo
Traviskn
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_CONTENTto let the user select a file and then returns thecontent://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, andRNFetchBlobReq.javait looks like what RNFB does to handle a document provided by a document provider is hacky/inefficient.The
PathResolver. getRealPathFromURIis called throughnormalizePathby 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 usesopenInputStreamandgetCacheDirto 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:
Filefile 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 #358)I think it would be more reasonable to have a different method to
PathResolver.getRealPathFromURIthat instead returns an InputStream directly and doesn't use the path hacks. Methods likereadFileand 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.