[feat] Support scoped storage when sharing images #148 #149
Conversation
WalkthroughThis pull request updates the project’s Android SDK configuration and enhances image file handling. The build configuration now targets SDK version 35 instead of 34. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ImageContentProvider
participant ImageDao
participant FileHelper
participant FS as File System
Client->>ImageContentProvider: openFile(uri, mode)
ImageContentProvider->>ImageContentProvider: Validate URI segments
ImageContentProvider->>ImageDao: Query Image using file ID
ImageDao-->>ImageContentProvider: Return Image object
ImageContentProvider->>FileHelper: getImageFile(image, context)
FileHelper->>FS: Locate file based on image properties
FS-->>FileHelper: File exists or not
FileHelper-->>ImageContentProvider: Return File or null
alt File exists
ImageContentProvider->>Client: Return ParcelFileDescriptor (read-only)
else File not found
ImageContentProvider->>Client: Throw FileNotFoundException
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/src/main/java/ai/elimu/content_provider/provider/ImageContentProvider.java (3)
32-32: TAG should be private static final.Following Android logging conventions, the TAG variable should be declared as
private static finalfor consistency and to enable compiler optimizations.- private String TAG = ImageContentProvider.class.getName(); + private static final String TAG = ImageContentProvider.class.getName();
160-181: Consider themodeparameter.The
openFilemethod accepts amodeparameter but always usesMODE_READ_ONLYregardless of the provided mode. Either document this behavior or consider supporting different modes based on the parameter value.@Override public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException { + // This implementation only supports read-only mode regardless of the mode parameter List<String> segments = uri.getPathSegments(); if (segments.size() < 2) { throw new FileNotFoundException("Invalid URI: " + uri); } String fileId = segments.get(1); RoomDb roomDb = RoomDb.getDatabase(getContext()); ImageDao imageDao = roomDb.imageDao(); Image image = imageDao.load(Long.parseLong(fileId)); if (image == null) { Log.e(TAG, "Image not found with ID: " + fileId); throw new FileNotFoundException("Image not found with ID: " + fileId); } File imageFile = FileHelper.getImageFile(image, getContext()); if (imageFile == null) { throw new FileNotFoundException("File not found!"); } if (!imageFile.exists()) { Log.e(TAG, "imageFile doesn't exist: " + imageFile.getAbsolutePath()); throw new FileNotFoundException("File not found: " + imageFile.getAbsolutePath()); } return ParcelFileDescriptor.open(imageFile, ParcelFileDescriptor.MODE_READ_ONLY); }
164-165: Add a more specific error message.The error message for invalid URI should indicate what the expected format is, to help with debugging.
if (segments.size() < 2) { - throw new FileNotFoundException("Invalid URI: " + uri); + throw new FileNotFoundException("Invalid URI: " + uri + ". Expected format: content://" + AUTHORITY + "/" + TABLE_IMAGES + "/{id}"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/build.gradle(1 hunks)app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/ai/elimu/content_provider/provider/ImageContentProvider.java(3 hunks)app/src/main/java/ai/elimu/content_provider/util/FileHelper.java(2 hunks)app/src/main/res/xml/image_file_path.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/xml/image_file_path.xml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
🔇 Additional comments (3)
app/build.gradle (1)
5-5: Android SDK version update looks good.Updating to target and compile with SDK 35 is an appropriate change to ensure the app remains current with the latest Android platform capabilities, which is necessary for supporting modern storage access methods like scoped storage.
Also applies to: 11-11
app/src/main/AndroidManifest.xml (1)
55-60: Properly configured for URI-based file sharing.The changes to enable
grantUriPermissionsand define file paths via theFILE_PROVIDER_PATHSmeta-data are necessary for supporting scoped storage access. This configuration follows Android's recommended patterns for secure file sharing.app/src/main/java/ai/elimu/content_provider/util/FileHelper.java (1)
30-39: Good implementation with proper scope restrictions.The new overloaded method maintains the same structure and null checks as the original method. The use of
@RestrictToannotation is a good practice to control access to this method.
|
|
||
| RoomDb roomDb = RoomDb.getDatabase(getContext()); | ||
| ImageDao imageDao = roomDb.imageDao(); | ||
| Image image = imageDao.load(Long.parseLong(fileId)); |
There was a problem hiding this comment.
Validate fileId to avoid NumberFormatException.
The code is trying to parse a potentially non-numeric string to a Long, which could throw a NumberFormatException. Add validation or exception handling.
String fileId = segments.get(1);
+ long imageId;
+ try {
+ imageId = Long.parseLong(fileId);
+ } catch (NumberFormatException e) {
+ Log.e(TAG, "Failed to parse image ID: " + fileId, e);
+ throw new FileNotFoundException("Invalid image ID format: " + fileId);
+ }
RoomDb roomDb = RoomDb.getDatabase(getContext());
ImageDao imageDao = roomDb.imageDao();
- Image image = imageDao.load(Long.parseLong(fileId));
+ Image image = imageDao.load(imageId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Image image = imageDao.load(Long.parseLong(fileId)); | |
| String fileId = segments.get(1); | |
| long imageId; | |
| try { | |
| imageId = Long.parseLong(fileId); | |
| } catch (NumberFormatException e) { | |
| Log.e(TAG, "Failed to parse image ID: " + fileId, e); | |
| throw new FileNotFoundException("Invalid image ID format: " + fileId); | |
| } | |
| RoomDb roomDb = RoomDb.getDatabase(getContext()); | |
| ImageDao imageDao = roomDb.imageDao(); | |
| Image image = imageDao.load(imageId); |
There was a problem hiding this comment.
@coderabbitai This was fixed!
There was a problem hiding this comment.
🧩 Analysis chain
@tuancoltech Thank you for implementing the fix! I've verified that the NumberFormatException is now properly handled with the try-catch block. This is a good improvement to make the code more robust against invalid inputs. Well done!
🏁 Script executed:
#!/bin/bash
# Check the current implementation of the openFile method in the ImageContentProvider class
cat app/src/main/java/ai/elimu/content_provider/provider/ImageContentProvider.java | grep -A 30 "public ParcelFileDescriptor openFile"
Length of output: 1425
@tuancoltech Thank you for implementing the fix! I've verified that the NumberFormatException is now properly handled with the try-catch block. This is a good improvement to make the code more robust against invalid inputs. Well done!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @Override | ||
| public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException { | ||
| List<String> segments = uri.getPathSegments(); | ||
| if (segments.size() < 2) { | ||
| throw new FileNotFoundException("Invalid URI: " + uri); | ||
| } | ||
| String fileId = segments.get(1); | ||
|
|
||
| RoomDb roomDb = RoomDb.getDatabase(getContext()); | ||
| ImageDao imageDao = roomDb.imageDao(); | ||
| Image image = imageDao.load(Long.parseLong(fileId)); | ||
|
|
||
| File imageFile = FileHelper.getImageFile(image, getContext()); | ||
| if (imageFile == null) { | ||
| throw new FileNotFoundException("File not found!"); | ||
| } | ||
| if (!imageFile.exists()) { | ||
| Log.e(TAG, "imageFile doesn't exist: " + imageFile.getAbsolutePath()); | ||
| throw new FileNotFoundException("File not found: " + imageFile.getAbsolutePath()); | ||
| } | ||
| return ParcelFileDescriptor.open(imageFile, ParcelFileDescriptor.MODE_READ_ONLY); | ||
| } |
There was a problem hiding this comment.
Add null check for Image object to prevent potential NullPointerException.
If the image is not found in the database, a NullPointerException will occur when calling FileHelper.getImageFile(image, getContext()). Add a null check before accessing the image.
Image image = imageDao.load(Long.parseLong(fileId));
+ if (image == null) {
+ Log.e(TAG, "Image not found with ID: " + fileId);
+ throw new FileNotFoundException("Image not found with ID: " + fileId);
+ }
File imageFile = FileHelper.getImageFile(image, getContext());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException { | |
| List<String> segments = uri.getPathSegments(); | |
| if (segments.size() < 2) { | |
| throw new FileNotFoundException("Invalid URI: " + uri); | |
| } | |
| String fileId = segments.get(1); | |
| RoomDb roomDb = RoomDb.getDatabase(getContext()); | |
| ImageDao imageDao = roomDb.imageDao(); | |
| Image image = imageDao.load(Long.parseLong(fileId)); | |
| File imageFile = FileHelper.getImageFile(image, getContext()); | |
| if (imageFile == null) { | |
| throw new FileNotFoundException("File not found!"); | |
| } | |
| if (!imageFile.exists()) { | |
| Log.e(TAG, "imageFile doesn't exist: " + imageFile.getAbsolutePath()); | |
| throw new FileNotFoundException("File not found: " + imageFile.getAbsolutePath()); | |
| } | |
| return ParcelFileDescriptor.open(imageFile, ParcelFileDescriptor.MODE_READ_ONLY); | |
| } | |
| @Override | |
| public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException { | |
| List<String> segments = uri.getPathSegments(); | |
| if (segments.size() < 2) { | |
| throw new FileNotFoundException("Invalid URI: " + uri); | |
| } | |
| String fileId = segments.get(1); | |
| RoomDb roomDb = RoomDb.getDatabase(getContext()); | |
| ImageDao imageDao = roomDb.imageDao(); | |
| Image image = imageDao.load(Long.parseLong(fileId)); | |
| if (image == null) { | |
| Log.e(TAG, "Image not found with ID: " + fileId); | |
| throw new FileNotFoundException("Image not found with ID: " + fileId); | |
| } | |
| File imageFile = FileHelper.getImageFile(image, getContext()); | |
| if (imageFile == null) { | |
| throw new FileNotFoundException("File not found!"); | |
| } | |
| if (!imageFile.exists()) { | |
| Log.e(TAG, "imageFile doesn't exist: " + imageFile.getAbsolutePath()); | |
| throw new FileNotFoundException("File not found: " + imageFile.getAbsolutePath()); | |
| } | |
| return ParcelFileDescriptor.open(imageFile, ParcelFileDescriptor.MODE_READ_ONLY); | |
| } |
There was a problem hiding this comment.
@coderabbitai Isn't the null check already there?
There was a problem hiding this comment.
@tuancoltech There isn't a null check for the Image object itself after loading it from the database. The current code has two checks:
if (imageFile == null)- This checks ifFileHelper.getImageFile()returned nullif (!imageFile.exists())- This checks if the file actually exists on disk
However, there's no check between retrieving the image from the database and passing it to the FileHelper. If an image with the requested ID doesn't exist in the database, imageDao.load() would return null, and passing that null to FileHelper.getImageFile() could potentially cause a NullPointerException.
Adding the suggested null check would handle this case explicitly with a more descriptive error message.
There was a problem hiding this comment.
@coderabbitai This was fixed!
There was a problem hiding this comment.
@tuancoltech That's great! I can see you've not only added the null check for the Image object but also improved the error handling with more informative messages and a try-catch block for parsing the ID. Nice work on making the code more robust!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/src/main/java/ai/elimu/content_provider/provider/ImageContentProvider.java (4)
32-32: Make TAG a static final constant.The TAG variable should follow Android logging conventions.
- private String TAG = ImageContentProvider.class.getName(); + private static final String TAG = ImageContentProvider.class.getName();
168-170: Add a null check for getContext().Similar to what's done in the query method, you should check if getContext() returns null.
+ Context context = getContext(); + if (context == null) { + throw new FileNotFoundException("Content provider context is null"); + } - RoomDb roomDb = RoomDb.getDatabase(getContext()); + RoomDb roomDb = RoomDb.getDatabase(context); ImageDao imageDao = roomDb.imageDao();
161-161: Document mode parameter usage.The 'mode' parameter is received but not used in the method. Either document why it's ignored or implement proper handling for different modes.
- public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException { + /** + * Opens a file for reading based on the provided URI. + * Note: This implementation only supports read-only mode regardless of the mode parameter. + * + * @param uri The URI to the file + * @param mode The mode to open the file with (only read mode is supported) + * @return A ParcelFileDescriptor for the file + * @throws FileNotFoundException If the file cannot be found + */ + public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException {
193-193: Consider adding permission checks.Before returning a file descriptor, consider checking if the caller has the necessary permissions to access the file.
+ // Check if caller has permissions to access this file + Context context = getContext(); + if (context != null) { + context.enforceCallingPermission(android.Manifest.permission.READ_EXTERNAL_STORAGE, "No permission to read image files"); + } return ParcelFileDescriptor.open(imageFile, ParcelFileDescriptor.MODE_READ_ONLY);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/ai/elimu/content_provider/provider/ImageContentProvider.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (macos-latest, 17)
🔇 Additional comments (3)
app/src/main/java/ai/elimu/content_provider/provider/ImageContentProvider.java (3)
160-194: The openFile implementation looks good overall.You've properly implemented the openFile method with thorough error handling including:
- URI validation
- Safe parsing of the image ID with exception handling
- Null checks for the Image object
- File existence validation
This implementation supports scoped storage when sharing images as intended.
172-177: Good implementation of NumberFormatException handling.This correctly addresses the previous review comment about validating fileId to avoid NumberFormatException.
181-183: Good implementation of null check for Image object.This correctly addresses the previous review comment about adding a null check for the Image object to prevent potential NullPointerException.
|
@coderabbitai Please review again. |
|
@jo-elimu Please review. |
🧩 Analysis chain🏁 Script executed: Length of output: 21225 @tuancoltech I've reviewed your PR that implements support for scoped storage when sharing images. The implementation looks good and follows Android best practices:
This properly addresses issue #148 by implementing the FileProvider pattern for secure file sharing via content:// URIs instead of direct file paths, which is required for Android 11+. Is there anything specific about the implementation you'd like me to review in more detail? ✅ Actions performedReview triggered.
|
Issue Number
Purpose
Summary by CodeRabbit
Summary by CodeRabbit