Add MIME type to PersistentBlobProvider

[unrulygnu]

Fixes #4536

See #4536 (comment) for long-winded details.

[unrulygnu]

Rebased on 3.5.1 (6a18824)

[moxie0]

I don't 100% understand the rationale behind this change yet, but at first look we don't use shared preferences for things like that. Shared prefs should be for actual preferences, or for managing state that there is exactly one of (registered or not, registered phone number, etc...) We don't want to use them to emulate a database.

[unrulygnu]

@moxie0 , given the name, "Shared Preferences," I would agree, so I was surprised by the following from Saving Key-Value Sets | Android Developers:

If you have a relatively small collection of key-values that you'd like to save, you should use the SharedPreferences APIs.

That's exactly what is needed here, so I went with it given that it's a recommendation from the platform team. I totally understand if you still don't want to use it for this purpose. Since submitting this PR, I've also seen that attachment content type is stored encrypted in the database, so that's another concern that needs to be addressed.

The rationale is this: without MIME type in PersistentBlobProvider, it gets completely lost when a persisted draft media file becomes an official attachment. For audio and video, this means that they don't get the right extension on save, among potential other issues.

Since PersistentBlobProvider, at this point, keeps only files on storage (and an in-memory cache while writing), there is no existing data structure to which to append the MIME type. Hence the quickie.

I can add a new database table for PersistentBlobProvider and update the PR if you'd like. Let me know which direction you'd like to see this go.

[moxie0]

@unrulygnu In this case I think it's a little too dynamic for shared prefs. If I had exactly 10 of something, with 10 consistent key names, I'd consider using shared prefs for that.

If what we're mostly concerned about is the file extension, why don't we encode the mime type into the file extension of the file on disk that PersistentBlobProvider manages?

[unrulygnu]

@moxie0

why don't we encode the mime type into the file extension of the file on disk that PersistentBlobProvider manages?

Thanks for the feedback, especially today, you're awesome! I will look into this. One last question: does the attachment's content type being encrypted in the database suggest that this data (and its relationship with the URI) should be secured? Or are we ok tagging the encrypted blob with its actual content type in plaintext?

Updating the PR now with a rebase. Will update within the next day or two with a better solution.

Thanks again!

[moxie0]

In the AttachmentDatabase? I think only the content is encrypted there, which is mostly our policy with local storage. I have no problem with tagging the encrypted blob with its content type.

[unrulygnu]

In the AttachmentDatabase?

No, DraftDatabase:

values.put(DRAFT_TYPE, masterCipher.encryptBody(draft.getType()));

I have no problem with tagging the encrypted blob with its content type.

Thanks again for the clarification. I'm on it.

[unrulygnu]

@moxie0 : updated the PR, removing the use of SharedPreferences, and instead using the file extension of the blob file to reference the blob's type.

Note that the use of FilenameFilter in PersistentBlobProvider#getFile also accommodates the transition which this PR introduces: previously all blobs were appended with a .jpg extension, after merge the extensions will be variable. Since the FilenameFilter matches the ID portion of the filename (ignoring the extension), any existing blobs from before this PR was merged will be returned normally.

Problem types in issue #4536 were audio and video. Tested draft saves, sending, saving attachments, and forwarding with several subtypes of each, as well as images. All worked well on a Nexus 5.

There's an unrelated regression I discovered in testing which I believe may have been introduced by bcd0895, where blob files are not being deleted when attachment send is complete. I will document this in a separate issue.

Please let me know what you think.

[moxie0]

alignment?

[unrulygnu]

Fixed and pushed. Sorry for the obvious mistake.

[unrulygnu]

Iterating through the list of files doesn't feel great. We should probably encode the type/extension into the URI itself.

I will get started on this tonight.

[unrulygnu]

@moxie0: Sorry for the delay. Persistent blob files of all types now save with just a generic .blob extension to avoid iterating through them. Full, specific MIME types are now stored/queried in persistent blob URI's.

Drafts with persistent blobs created before this change will use the previous URI format, as well as the previously hardcoded .jpg extension for the blob files. This has been accommodated in its own commit for specific review if needed.

Tested with video and audio shared from third party apps via ACTION_SEND, quick camera, and drafts created both before and after the commits in this PR.

Let me know what you think, and whether you need any further edits or squashing.

[moxie0]

I'd like to encode the mime type into the file extension, not the path. I also don't want us to have some legacy URI support around forever, so let's keep the path the same and just leave those be.

[unrulygnu]

@moxie0: I know your hands are probably full, and my recent intermittence is probably making things harder. Here's a summary of the commit/review history:

(unrulygnu) See #4536 (comment) for long-winded details. (excerpt: I'm submitting a PR with an implementation using SharedPreferences)

(moxie0) In this case I think it's a little too dynamic for shared prefs. why don't we encode the mime type into the file extension of the file on disk that PersistentBlobProvider manages?

(unrulygnu) updated the PR, removing the use of SharedPreferences, and instead using the file extension of the blob file to reference the blob's type.

(moxie0) Iterating through the list of files doesn't feel great. We should probably encode the type/extension into the URI itself.

(unrulygnu) Full, specific MIME types are now stored/queried in persistent blob URI's.

(moxie0) I'd like to encode the mime type into the file extension, not the path.

Then would you like me to revert the recent commits and go back to the previous solution, using the blobs' file extensions, despite your reservations about iterating through the file list? Note that the number of persistent blob files should be very few at any given time since they exist only while the media message is in draft state.

Or are you suggesting that we append a file extension to the numeric ID in the Content URI, breaking the Android Content URI convention? In my opinion, this feels less great than iterating through the persistent blob files.

While I was trying to keep the impact of this PR to a minimum -- especially considering the simplicity of PersistentBlobProvider in its current state -- going to the database is another option.

Having spent a lot of time on PersistentBlobProvider lately, I feel that storing the MIME type in the URI path feels the most elegant. The type stays with the URI in a clean one-to-one relationship, requiring no potentially janky querying/iterating elsewhere. And the code handling the legacy URI format (which is important because it prevents user data loss on update) can be easily reverted after ample time is allowed for user migration.

Please let me know how you'd like to proceed.

[moxie0]

Yes, I'd like the extension to be in the URI. I don't want to have a legacy URI format.

[unrulygnu]

@moxie0: Removed the MIME Type from the URI path, appending relevant file extension to the ID (last path segment) instead, as you directed.

As I mentioned in my previous comment, this approach breaks the Android Content URI convention, which may cause problems later, such as using ContentUris#parseId on what one assumes to be a valid Content URI, for instance.

Tested with media shared from third party apps, as well as quick camera. Attachments save with the correct extension after sending.

What do you think?

[unrulygnu]

Just rebased on current master (72064d8), no conflicts.

[moxie0]

no need for a constant if it's only used in one place, and is designed not to be accessible from anywhere else

[unrulygnu]

Reverted to string literal in 7e4eb1d.

[moxie0]

Hmm, breaking parseId doesn't seem great. Right now you have the file extension in the URI but not in the file. What if you did the opposite, and put the file extension in the file but not the URI?

[unrulygnu]

@moxie0:

What if you did the opposite, and put the file extension in the file but not the URI?

We did this:

#4689 (comment)

If what we're mostly concerned about is the file extension, why don't we encode the mime type into the file extension of the file on disk that PersistentBlobProvider manages?

#4689 (comment)

updated the PR, removing the use of SharedPreferences, and instead using the file extension of the blob file to reference the blob's type.

#4689 (diff)

Iterating through the list of files doesn't feel great. We should probably encode the type/extension into the URI itself.

At your direction, I've delivered several approaches on fixing #4536. Although the most obvious approach we haven't tried yet is storing in the database, my previous opinion still stands:

Having spent a lot of time on PersistentBlobProvider lately, I feel that storing the MIME type in the URI path feels the most elegant. The type stays with the URI in a clean one-to-one relationship, requiring no potentially janky querying/iterating elsewhere

Please let me know how you'd like to proceed.

[moxie0]

lol, ok sorry for the back and forth. let's try it your way then

[unrulygnu]

Updated, tested. Note again that the issue this PR fixes concerns video/audio shared from third party apps to Signal via Intent.ACTION_SEND. Attaching from within Signal is unaffected.

Also note that per your previous directive, existing drafts with persistent blobs will be lost when users update Signal with this PR applied, due to the change in persistent blob URI:

#4689 (comment)

I don't want to have a legacy URI format.

Please let me know if you want me to squash the commits or need anything else. Thanks!

[moxie0]

in 3.13.0