hpoul / file_picker_writable

Flutter plugin to choose files which can be read, referenced and written back at a later time.
https://pub.dev/packages/file_picker_writable
MIT License
17 stars 13 forks source link

writeFileWithIdentifier method doesn't work properly on Android 10 #23

Closed yegor-pelykh closed 3 years ago

yegor-pelykh commented 3 years ago

Today I came across a problem like this. 1) I have an existing file of a certain size on the device 2) I try to overwrite it using the identifier (using writeFile() method) with content that is smaller than it was before

Current behavior: The file is not truncated to a smaller size. New content is written to the beginning of the file, and continues by the content that was previously in the file.

Correct behavior: The file should be completely overwritten and truncated if the new content is smaller.

I began to figure it out to find out what was the reason of this issue. And I found the reason.

In Kotlin implementation file there are such lines of code (writeFileWithIdentifier method):

withContext(Dispatchers.IO) {
  contentResolver.openOutputStream(fileUri, "w").use { output ->
    require(output != null)
    file.inputStream().use { input ->
      input.copyTo(output)
    }
  }
}

Method openOutputStream with mode "w" works successfully to rewrite the stream on android version less than 10. But in Android 10 it doesn't truncate the file. There should be a mode "wt" specified, instead of a mode "w".

I tried to replace "w" by "wt" locally, and it works.

There is an issue in Android issue tracker: https://issuetracker.google.com/issues/135714729?pli=1

Also, there is a similar issue in another repository with the same reason, as an example: https://github.com/itinance/react-native-fs/pull/837

This problem is very critical for me, since it does not allow overwriting files normally, could you think about how to fix the code in the part of specifying the stream opening mode? I would be very grateful. Thank you so much!

hpoul commented 3 years ago

could you think about how to fix the code

If I understand you correctly simply adding a t should do the job?

The only weird thing is that the documentation only lists: String: May be "w", "wa", "rw", or "rwt". This value cannot be null. - Maybe rwt would be more compatible? 🤔️ have you tried rwt as well?

yegor-pelykh commented 3 years ago

I found a great comment that can answer different questions

https://github.com/itinance/react-native-fs/pull/837#issuecomment-591214968

I haven't tried the mode rwt yet (I can try asap). But based on the last paragraph, it should work, but isn't necessary in this case IMHO.

I can change "wt" to "rwt". "rwt" also fixes the issue, and is explicitly mentioned in the android documentation. However, that may require adding READ_EXTERNAL_STORAGE permissions where it was previously not required. I was trying to avoid that.

Сonsidering this is a write-only method, I personally would not add the letter r to the mode string.

In android 10, the mode parsing method has changed:

I poked around the Android source code for different versions, and found that before Android 10, "w" and "wt" were explicitly being parsed exactly the same in ParcelFileDescriptor, resulting in a bitmask with both MODE_WRITE_ONLY and MODE_TRUNCATE to open the file. Then in Android 10, there was a change and 'w' and 't' are now parsed individually.

So, the correct set of supported modes in Android 10 is this:

This method linked out to ContentProvider.openFile which mention's ParcelFileDescriptor.parseMode(String) Its here that I found all of the modes listed:

mode String: The string representation of the file mode. Can be "r", "w", "wt", "wa", "rw" or "rwt".

Based on the same comment, the fact that the mode is not mentioned in the documentation for this method is just a flaw in the documentation for Android 10.

UPD: rwt is also working, I checked.

yegor-pelykh commented 3 years ago

So could you please replace w mode by wt?

yegor-pelykh commented 3 years ago

Any solution?

hpoul commented 3 years ago

feel free to submit a PR which uses wt for Android >= 10

yegor-pelykh commented 3 years ago

Created pull request

26

hpoul commented 3 years ago

honestly, I'm pretty confused.. I've now changed it to use wt on Android 10 and later. but keep w otherwise.

API documentation from api level 27:

for openOutputStream:

     * @param mode May be "w", "wa", "rw", or "rwt".

for openFileDescriptor:

     * If opening with the exclusive "r" or "w" modes, the returned
     * ParcelFileDescriptor could be a pipe or socket pair to enable streaming
     * of data. Opening with the "rw" mode implies a file on disk that supports
     * seeking. If possible, always use an exclusive mode to give the underlying
     * {@link ContentProvider} the most flexibility.

as you can see it does not say that wt is supported. But I didn't want to use rwt because maybe not all underlying ContentProvider implementations support seeking (?).

and it seems at least google drive won't support rwt? from https://github.com/itinance/react-native-fs/pull/837#issuecomment-776677509

We used "wt", after sometime users reported that they can no longer save the files. This specifically happen to files from Google Drive. The error we were getting is: java.io.FileNotFoundException: Unsupported mode: wt. The problem was fixed by switching to "rwt".

Also, I think that comment referencing "I poked around the Android source code" didn't quite grasp the problem. The method parseMode referenced by that comment is only used for file:// URIs. not for ContentProvider. As far as I know every ContentProvider simply gets the mode as a String and has to figure out on it's own what to do with it. So unless anyone has audited all ContentProvider implementations, I doubt anyone can say that randomly changing it from w to rwt will work. So imho the least breaking thing to do is use wt on Android 10+ where it is actually documented that wt is an option.

Feel free to test 2.0.1-dev.1 and let me know if this works.

yegor-pelykh commented 3 years ago

@hpoul I agree with you. I tested and it works. Thanks a lot!

Can you make a release with this change?

hpoul commented 3 years ago

@yegor-pelykh 👍️ thanks for testing.. it's now on pub as 2.0.1 - thanks.