[file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs #6438

camsim99 · 2024-03-29T19:53:20Z

Overview

Changes getDirectoryPath, openFile, and openFiles to return the directory path instead of the URI representing the directory or file.

Fixes flutter/flutter#141250.

Technical Details

Technically, getDirectoryPath returns the path of the selected directory directly, whereas openFile and openFiles actually copy the content that the URI points to into another file and returns the path of the file. This is a direct result of the fact that Android highly discourages folks from accessing files through direct paths, so it's extremely difficult to do reliably without this approach (can confirm by how much effort it took to get this PR out...).

This approach is also used in image_picker_android and this PR uses essentially the same logic.

I think we should move forward with this short term solution to unblock folks that need access to directories and files in Dart, but I strongly advise that we re-evaluate for the long term. I elaborate on this in this doc and welcome feedback!

Pre-launch Checklist

gmackall

Mostly lgtm, one comment on potential null pointer exceptions. I mostly skipped over the FileUtils.java and corresponding test because I'm assuming its copied over from image picker, let me know if thats not the case.

...id/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java

camsim99 · 2024-04-17T22:48:05Z

@gmackall to respond to your comment and clarify for any reviewers, the FileUtils.java that I added is different from that used in image_picker in the following ways:

I wrote the getPathFromUri method, so please review!
getPathFromCopyOfFileFromUri is the same logic used in image_picker except I removed image-specific assumptions (the only notable one being an assumption about the extension for files being .jpeg if it could not otherwise be found).

In short, a review on the whole file wouldn't hurt!

gmackall

Added some more comments on FileUtils.java!

...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

…java/dev/flutter/packages/file_selector_android/FileUtils.java Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>

reidbaker · 2024-04-18T19:27:04Z

...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

+      String uriDocumentId = DocumentsContract.getDocumentId(uri);
+      String documentStorageVolume = uriDocumentId.split(":")[0];
+
+      // Non-primary storage volumes come from SD cards, USB drives, etc. and are


Where are they handled? Also can you include a link to your source that "primary" is the correct string.

"primary" is the correct String because this constant is used as the root of the document ID. I verified in my own testing and looking through examples of getting the getting paths from URIs. There is no documentation about it probably because Android does not want folks accessing files this way.

I did not handle those cases because I honestly didn't think we needed to handle these cases in this plugin, but now that you mention it, there was never that restriction. I would prefer to leave this as a TODO and file an issue, though, because I would have to figure out how to test this myself and do some similar deducing since there's no documentation and wouldn't like to block this PR on that but let me know what you think.

I think that this entire comment is demonstrative of why we should prioritize finding a longer term solution that isn't reliant on file paths BTW :) it will be hard for us to find future-proof solutions and handle all cases with this approach.

You should link that constant in this code because that will be super hard to find later.

Also my understanding of this api is that a user is picking something and the host app has no control over what a user picks. Throwing an exception doesn't feel wrong but it seems odd to not give a caller a way to control or filter what users can see to the files that the caller can actually access.

You make a good point. In theory, the host app should not be tasked to control what kind of files the user picks and the plugin should just take care of it. So, the exception is meant to signal an issue with the plugin for users to report to us to handle. I don't love that solution but I do not feel like silently failing is much better since we won't find out as fast about the issues. Maybe we can catch the exception and send back a dummy value to the Flutter app that signifies an error was encountered?

Again, this issue is specific to this approach of using file paths because Android should handle all relevant file types in its storage framework.

Let me back up to a more simple statement. If a developer uses this plugin on android can their user select files that would cause us to throw this exception? My understanding of the codepaths is yes, that the file selector utility lets people pick things from anywhere on their device.

I get that a silent failure is not ok. What I am hearing in your answer is that the caller cannot avoid these errors but also they are outside of what you think the plugin can handle in this pr? Is that correct?

If a developer uses this plugin on android can their user select files that would cause us to throw this exception?

You're right -- theoretically yes, but we do not expect it. This is only the answer because I am not on the Android team with this sort of insight and no one else is besides them.

What I am hearing in your answer is that the caller cannot avoid these errors but also they are outside of what you think the plugin can handle in this pr? Is that correct?

Yes -- I argue that we spend more time on a long term solution. There's no further progress I can make on this PR to avoid those errors until they are encountered, unfortunately.

Ok those seem to be reasonable arguments. Merge away

reidbaker · 2024-04-18T19:29:00Z

...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

+  @Nullable
+  public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @NonNull Uri uri) {
+    try (InputStream inputStream = context.getContentResolver().openInputStream(uri)) {
+      String uuid = UUID.randomUUID().toString();


Testing random things can be hard consider having a way to set a seed that can be used in tests.

Changed this to a UUID based on the URI and updated FileSelectorAndroidTest to check for the expected path since we can now reproduce it in tests!

...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

gmackall

lgtm!

reidbaker

One philosophy question about how we should signal to callers what to do. But the code looks good.

reidbaker · 2024-04-29T16:06:59Z

...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

+      String uriDocumentId = DocumentsContract.getDocumentId(uri);
+      String documentStorageVolume = uriDocumentId.split(":")[0];
+
+      // Non-primary storage volumes come from SD cards, USB drives, etc. and are


You should link that constant in this code because that will be super hard to find later.

reidbaker · 2024-04-29T16:10:19Z

...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

+      String uriDocumentId = DocumentsContract.getDocumentId(uri);
+      String documentStorageVolume = uriDocumentId.split(":")[0];
+
+      // Non-primary storage volumes come from SD cards, USB drives, etc. and are


Also my understanding of this api is that a user is picking something and the host app has no control over what a user picks. Throwing an exception doesn't feel wrong but it seems odd to not give a caller a way to control or filter what users can see to the files that the caller can actually access.

…licensed code (#6626) Updates the `LICENSE` file to cover code used in https://github.com/flutter/packages/blob/main/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java, added in #6438.

…ile`, `openFiles` to return file/directory paths instead of URIs (flutter/packages#6438)

flutter/packages@87a7c51...cc47b06 2024-04-30 joonas.kerttula@codemate.com [google_maps_flutter_web] Add marker clustering support (flutter/packages#6187) 2024-04-30 joonas.kerttula@codemate.com [google_maps_flutter_android] Add marker clustering support (flutter/packages#6185) 2024-04-29 32538273+ValentinVignal@users.noreply.github.com [go_router] Don't log if `hierarchicalLoggingEnabled` is `true` (flutter/packages#6019) 2024-04-29 43054281+camsim99@users.noreply.github.com [file_selector_android] Update `LICENSE` file to include newly added licensed code (flutter/packages#6626) 2024-04-29 43054281+camsim99@users.noreply.github.com [file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs (flutter/packages#6438) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…nFiles` to return file/directory paths instead of URIs (flutter#6438) ## Overview Changes `getDirectoryPath`, `openFile`, and `openFiles` to return the directory path instead of the URI representing the directory or file. Fixes flutter/flutter#141250. ## Technical Details Technically, `getDirectoryPath` returns the path of the selected directory directly, whereas `openFile` and `openFiles` actually copy the content that the URI points to into another file and returns the path of the file. This is a direct result of the fact that Android **highly discourages** folks from accessing files through direct paths, so it's extremely difficult to do reliably without this approach (can confirm by how much effort it took to get this PR out...). This approach [is also used in `image_picker_android`](https://github.com/flutter/packages/blob/main/packages/image_picker/image_picker_android/android/src/main/java/io/flutter/plugins/imagepicker/FileUtils.java) and this PR uses essentially the same logic. I think we should move forward with this short term solution to unblock folks that need access to directories and files in Dart, but I strongly advise that we re-evaluate for the long term. I elaborate on this in [this doc](https://docs.google.com/document/d/12pJDtl0yubyc68UqKo2hQ7XJwv86_ixCjNemQ7ENCBQ/edit?usp=sharing) and welcome feedback!

…licensed code (flutter#6626) Updates the `LICENSE` file to cover code used in https://github.com/flutter/packages/blob/main/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java, added in flutter#6438.

Add fix

af73d13

github-actions bot added p: file_selector platform-android labels Mar 29, 2024

camsim99 changed the title ~~Add fix~~ [file_selector_android] Modifies methods to return file/directory paths instead of URI Mar 29, 2024

camsim99 changed the title ~~[file_selector_android] Modifies methods to return file/directory paths instead of URI~~ [file_selector_android] Modifies getDirectoryPath to return file/directory paths instead of URI Mar 29, 2024

camsim99 added 11 commits March 29, 2024 12:57

Add todo

d976090

Debugging

9a39660

Debugging

79f36fd

debugging

7b647fc

Add copy workaround

a9ced0d

Cleanup

9cb098c

Add todos, test file

3029580

Remove permissions

58e37d2

Adding tests

a870688

Adding tests

adfc0e7

Finish tests

27bf9a1

github-actions bot added the p: image_picker label Apr 17, 2024

camsim99 changed the title ~~[file_selector_android] Modifies getDirectoryPath to return file/directory paths instead of URI~~ [file_selector_android] Modifies getDirectoryPath, openFile, & openFiles to return file/directory paths instead of URI Apr 17, 2024

camsim99 changed the title ~~[file_selector_android] Modifies getDirectoryPath, openFile, & openFiles to return file/directory paths instead of URI~~ [file_selector_android] Modifies getDirectoryPath, openFile, openFiles to return file/directory paths instead of URI Apr 17, 2024

camsim99 changed the title ~~[file_selector_android] Modifies getDirectoryPath, openFile, openFiles to return file/directory paths instead of URI~~ [file_selector_android] Modifies getDirectoryPath, openFile, openFiles to return file/directory paths instead of URIs Apr 17, 2024

lints + formatting + version bump

6454e6a

github-actions bot removed the p: image_picker label Apr 17, 2024

camsim99 and others added 4 commits April 17, 2024 11:13

Merge branch 'main' into fs_dir

9d600bd

Bump roblectric

c454c09

Add doc

2874a19

Merge remote-tracking branch 'refs/remotes/origin/fs_dir' into fs_dir

d4983d4

camsim99 mentioned this pull request Apr 17, 2024

[file_selector] Result of getDirectoryPath is not usable flutter/flutter#141250

Closed

Fix weird typo

374d864

camsim99 marked this pull request as ready for review April 17, 2024 20:55

camsim99 requested a review from gmackall as a code owner April 17, 2024 20:55

camsim99 requested a review from a team April 17, 2024 20:56

gmackall reviewed Apr 17, 2024

View reviewed changes

...id/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java Show resolved Hide resolved

gmackall reviewed Apr 17, 2024

View reviewed changes

camsim99 and others added 2 commits April 18, 2024 12:19

Update packages/file_selector/file_selector_android/android/src/main/…

b4ce04c

…java/dev/flutter/packages/file_selector_android/FileUtils.java Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>

Update packages/file_selector/file_selector_android/android/src/main/…

2173f0d

…java/dev/flutter/packages/file_selector_android/FileUtils.java Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>

reidbaker reviewed Apr 18, 2024

View reviewed changes

camsim99 added 3 commits April 18, 2024 12:40

Address review

a027a99

Merge remote-tracking branch 'refs/remotes/origin/fs_dir' into fs_dir

614eca7

Address review pt 2

697bdfa

camsim99 requested review from reidbaker and gmackall April 19, 2024 19:11

gmackall approved these changes Apr 26, 2024

View reviewed changes

reidbaker approved these changes Apr 29, 2024

View reviewed changes

Add primary constant

ac6a883

camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 29, 2024

auto-submit bot merged commit 36c42bd into flutter:main Apr 29, 2024
78 checks passed

camsim99 mentioned this pull request Apr 29, 2024

[file_selector_android] Update LICENSE file to include newly added licensed code #6626

Merged

11 tasks

engine-flutter-autoroll mentioned this pull request Apr 30, 2024

Roll Packages from 87a7c51cc633 to cc47b06b438d (5 revisions) flutter/flutter#147580

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 30, 2024

36c42bd27 [file_selector_android] Modifies getDirectoryPath, `openF…

4e61dbc

…ile`, `openFiles` to return file/directory paths instead of URIs (flutter/packages#6438)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs #6438

[file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs #6438

camsim99 commented Mar 29, 2024 •

edited

gmackall left a comment

camsim99 commented Apr 17, 2024

gmackall left a comment

reidbaker Apr 18, 2024

camsim99 Apr 19, 2024

reidbaker Apr 29, 2024

reidbaker Apr 29, 2024

camsim99 Apr 29, 2024

reidbaker Apr 29, 2024

camsim99 Apr 29, 2024

reidbaker Apr 29, 2024

reidbaker Apr 18, 2024

camsim99 Apr 19, 2024

gmackall left a comment

reidbaker left a comment

reidbaker Apr 29, 2024

reidbaker Apr 29, 2024

[file_selector_android] Modifies getDirectoryPath, openFile, openFiles to return file/directory paths instead of URIs #6438

[file_selector_android] Modifies getDirectoryPath, openFile, openFiles to return file/directory paths instead of URIs #6438

Conversation

camsim99 commented Mar 29, 2024 • edited

Overview

Technical Details

Pre-launch Checklist

gmackall left a comment

Choose a reason for hiding this comment

camsim99 commented Apr 17, 2024

gmackall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmackall left a comment

Choose a reason for hiding this comment

reidbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs #6438

[file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs #6438

camsim99 commented Mar 29, 2024 •

edited