imageCaptureProcessing maintain EXIF #2902
Conversation
|
Can one of the admins verify this patch? |
|
Thank you for the submission! Please ensure you meet the eligibility criteria for our program. 📝 Our team will review and process the payment if everything looks good. |
There was a problem hiding this comment.
Hey @melmathari. Your attempts looks great. Made some clarifying comments, but also wondering if it's possible to add unit tests for the new FileUtil methods you have added to copy the exif data. Thanks!
|
@damagatchi ok to test |
|
@shubham1g5 I've addressed the feedback. As for the unit tests, I'm not sure if I'll be able to complete them, i ran into several issues with testImplementation project(path: ':commcare-core', configuration: 'testsAsJar') |
There was a problem hiding this comment.
@melmathari It's fine to ignore the lint errors in the code you have not changed as the method refactoring will need to be replicated elsewhere in the calling code.
Curious what kind of errors are you facing here and if you have followed the instructions given here to run tests. |
|
@shubham1g5 Thanks for providing the doc. Unfortunately, I won't be able to commit to the unit testing for this build due to my current schedule. We have tested this on our local implementation though. |
|
@shubham1g5 Why did the developer bring a lint roller to the code review? Because we used SuppressLint to silence those pesky errors — nothing escapes our lint-taming prowess! |
There was a problem hiding this comment.
@melmathari Indeed lol. Although what I have been trying to say is don't worry about the lint check and let it fail. No roller needed 🤣
app/src/org/commcare/activities/components/ImageCaptureProcessing.java
Outdated
Show resolved
Hide resolved
melmathari
force-pushed
the
2689
branch
4 times, most recently
from
63bee37 to
e9f1985
Compare
Change requests change request SuppressLint SuppressLint imageCaptureProcessing.java
|
@melmathari We are seeing instrumentation test failures on this PR which looks related to media handling. 
You should be able to run these tests locally to look into the failures. |
|
@shubham1g5 Thanks, I will take some time to test this a bit more locally |
The file isn't an image (mimeType check) The image doesn't have EXIF data (ExifInterface handles this gracefully) There are any issues reading/writing EXIF data (caught and logged) The only failure that would propagate up is if the initial file copy fails, which is appropriate since that's the core operation we need to succeed.
Correct, this was the case. |
|
@shubham1g5 I had to update my branch from the latest changes from the master branch. |
|
Warning Rate limit exceeded@melmathari has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve modifications to two Java files: In Poem
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: 1
🧹 Outside diff range and nitpick comments (5)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java (3)
96-96: Enhance error message for EXIF-related failuresWhile the implementation correctly uses
copyFileWithExifData, the error message could be more specific when EXIF preservation fails.Consider updating the error message to be more descriptive:
- throw new IOException("Failed to rename " + originalImage.getAbsolutePath() + - " to " + rawImageFile.getAbsolutePath()); + throw new IOException("Failed to copy image with EXIF data from " + + originalImage.getAbsolutePath() + " to " + rawImageFile.getAbsolutePath());
Line range hint
1-24: Consider adding tests for EXIF preservationThe implementation looks solid, but given the critical nature of preserving GPS/location data, it would be beneficial to add tests verifying EXIF preservation, especially for:
- Basic image copying
- Image scaling with different dimensions
- Encrypted image handling
Would you like help creating test cases for these scenarios?
EXIF data is not preserved during encryption
The implementation of
EncryptionIO.encryptFile()directly streams the file content through encryption without any EXIF handling. WhileFileUtilhas robust EXIF preservation methods (copyFileWithExifData,scaleAndSaveImageWithExif), these are not used in the encryption flow. The encrypted file will contain the encrypted raw bytes of the image, making the EXIF data unreadable.To fix this:
- Extract and store EXIF data before encryption
- Encrypt the image
- Store the EXIF data alongside or within the encrypted file in a way that preserves it
🔗 Analysis chain
Line range hint
71-82: Verify EXIF preservation during encryptionWhen
HiddenPreferences.isMediaCaptureEncryptionEnabled()is true, we should ensure that EXIF data is properly preserved through the encryption process.Let's check the encryption implementation:
Let me gather more information about the encryption implementation to verify EXIF handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify EXIF handling in encryption flow # Look for EXIF-related handling in EncryptionIO rg -A 5 "class EncryptionIO" ast-grep --pattern 'encryptFile($source, $dest, $_) { $$$ }'Length of output: 725
Script:
#!/bin/bash # Get the full implementation of encryptFile and related methods rg -A 20 "public static void encryptFile" app/src/org/commcare/models/encryption/EncryptionIO.java # Look for any EXIF-related code in the codebase rg -i "exif" --type javaLength of output: 4661
app/src/org/commcare/utils/FileUtil.java (2)
881-926: Remove unnecessarythrows IOExceptiondeclarationThe method
copyExifDatadeclaresthrows IOExceptionbut catches and handlesIOExceptioninternally without rethrowing it. Consider removingthrows IOExceptionfrom the method signature to accurately reflect that the method does not propagate exceptions.Apply this diff to adjust the method signature:
-private static void copyExifData(String sourcePath, String destPath) throws IOException { +private static void copyExifData(String sourcePath, String destPath) {
870-938: Consider adding unit tests for EXIF data preservationThe new methods involve file operations and metadata handling. Adding unit tests for
copyFileWithExifData,copyExifData, andscaleAndSaveImageWithExifwill help ensure correctness and prevent regressions in EXIF data preservation functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java(2 hunks)app/src/org/commcare/utils/FileUtil.java(6 hunks)
🔇 Additional comments (2)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java (1)
59-60: LGTM! Verify scaled image EXIF preservation
The implementation correctly uses scaleAndSaveImageWithExif to preserve EXIF data during image scaling. The change is well-integrated with the existing error handling and control flow.
Let's verify the EXIF preservation in scaled images:
✅ Verification successful
EXIF preservation implementation is robust and properly handled
The implementation is well-structured with proper error handling and EXIF data preservation:
- Uses Android's
ExifInterfacefor EXIF operations - Properly preserves GPS data during image scaling
- Has appropriate error handling through IOException propagation
- Maintains a clean separation between scaling and EXIF preservation
- Includes validation of image type and MIME type before operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for EXIF-related error handling in FileUtil
# Look for proper error handling when dealing with EXIF data in the scaling method
ast-grep --pattern 'scaleAndSaveImageWithExif($src, $dest, $_) {
$$$
try {
$$$
} catch ($exif_error) {
$$$
}
}'
Length of output: 129
Script:
#!/bin/bash
# Let's first locate the FileUtil class to understand the implementation
fd FileUtil.java
# Then examine the implementation of scaleAndSaveImageWithExif
rg -A 10 "scaleAndSaveImageWithExif" --type java
Length of output: 2364
Script:
#!/bin/bash
# Let's examine the error handling in copyExifData method and scaleAndSaveImage
rg -B 2 -A 10 "copyExifData|scaleAndSaveImage\(" --type java app/src/org/commcare/utils/FileUtil.java
# Also check for any EXIF-related imports and utilities
rg "import.*Exif" --type java
Length of output: 2209
app/src/org/commcare/utils/FileUtil.java (1)
870-879: Add JavaDocs to the new public methods
The public methods copyFileWithExifData and scaleAndSaveImageWithExif lack JavaDocs. Adding documentation will improve code readability and help other developers understand their purpose and usage.
Also applies to: 928-938
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/src/org/commcare/utils/FileUtil.java (1)
902-947: Consider enhancing error handling and documentationWhile the implementation is solid, consider these improvements:
- The error handling could be more informative by including the specific EXIF tag that failed
- Consider adding documentation about which EXIF tags are preserved, perhaps as a class-level constant with comments
private static void copyExifData(String sourcePath, String destPath) { try { ExifInterface source = new ExifInterface(sourcePath); ExifInterface dest = new ExifInterface(destPath); for (String tag : tagsToPreserve) { String value = source.getAttribute(tag); if (value != null) { dest.setAttribute(tag, value); } } dest.saveAttributes(); } catch (IOException e) { - Logger.log(LogTypes.TYPE_WARNING_NETWORK, - "Failed to copy EXIF data from " + sourcePath + " to " + destPath + ": " + e.getMessage()); + Logger.log(LogTypes.TYPE_WARNING_NETWORK, + String.format("Failed to copy EXIF data from %s to %s: %s (Error: %s)", + sourcePath, destPath, e.getMessage(), e.getClass().getSimpleName())); } }app/src/org/commcare/activities/components/ImageCaptureProcessing.java (1)
151-154: LGTM: Raw copy now preserves EXIF dataThe implementation correctly preserves EXIF data when creating raw copies. Consider adding a test to verify EXIF preservation in the raw copies, especially for images with GPS data.
Consider implementing a utility class specifically for EXIF operations to centralize the EXIF handling logic and make it easier to maintain and test. This could include methods for:
- Validating EXIF data integrity
- Filtering sensitive EXIF data
- Handling EXIF extraction failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java(3 hunks)app/src/org/commcare/utils/FileUtil.java(8 hunks)
🔇 Additional comments (4)
app/src/org/commcare/utils/FileUtil.java (2)
629-630: LGTM: Appropriate logging for image scaling operations
The added logging statement provides clear visibility into image downsizing operations during form entry.
883-900: LGTM: Well-structured EXIF-aware file copy implementation
The method properly handles both regular files and image files with EXIF data, with appropriate MIME type checking.
app/src/org/commcare/activities/components/ImageCaptureProcessing.java (2)
127-135: LGTM: Improved error handling for EXIF operations
The error messages are specific and helpful for debugging EXIF-related issues. The separate handling of encrypted and unencrypted paths is appropriate.
59-60: LGTM: Image scaling now preserves EXIF data
The implementation correctly uses the new EXIF-aware scaling method while maintaining the existing dimension constraints.
Let's verify the scaleAndSaveImageWithExif implementation:
✅ Verification successful
LGTM: Image scaling implementation correctly preserves EXIF data
The implementation in FileUtil.java is robust and properly handles EXIF metadata preservation:
- Scales the image while maintaining dimension constraints
- Preserves important EXIF tags including GPS coordinates and orientation
- Handles both regular and encrypted image scenarios
- Includes proper error handling and logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the FileUtil implementation properly handles EXIF preservation
ast-grep --pattern 'scaleAndSaveImageWithExif($src, $dest, $_) {
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Let's try to find the FileUtil class and its implementation
rg -l "FileUtil" --type java
# Then look for any methods related to EXIF or image scaling
rg "scaleAndSaveImageWithExif|EXIF" --type java -A 5
Length of output: 14331
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java (1)
Line range hint
59-154: Consider adding comprehensive test coverageWhile the implementation successfully addresses EXIF preservation, it would benefit from comprehensive test coverage to verify:
- EXIF preservation during scaling
- EXIF preservation during encryption
- Error scenarios and edge cases
- Handling of various EXIF tags
Would you like me to help create a test plan or generate test cases for these scenarios?
app/src/org/commcare/utils/FileUtil.java (3)
10-10: Useandroidx.exifinterface.media.ExifInterfaceinstead of deprecatedandroid.media.ExifInterfaceThe
android.media.ExifInterfacehas been deprecated in API level 29. Consider usingandroidx.exifinterface.media.ExifInterfacefor better compatibility and future support.Apply this diff to update the import:
- import android.media.ExifInterface; + import androidx.exifinterface.media.ExifInterface;
897-898: Ensure compatibility of image formats withExifInterfaceThe
ExifInterfacesupports specific image formats (JPEG, PNG, WebP, HEIF, DNG). Before attempting to copy EXIF data, verify that the image format is supported to prevent potential exceptions.Apply this diff to check for supported formats:
if (mimeType != null && mimeType.startsWith("image/")) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N || isSupportedFormat(sourceFile)) { copyExifData(sourceFile.getAbsolutePath(), destFile.getAbsolutePath()); + } } + private static boolean isSupportedFormat(File file) { + String extension = getExtension(file.getAbsolutePath()).toLowerCase(); + return extension.equals("jpg") || extension.equals("jpeg") || + extension.equals("png") || extension.equals("webp") || + extension.equals("heif") || extension.equals("dng"); + }
629-630: Clarify log message contextThe log message in
scaleAndSaveImageprovides valuable information. Consider including the filename or additional context to help identify which image was downsized.Apply this diff to enhance the log message:
Logger.log(LogTypes.TYPE_FORM_ENTRY, - "An image captured during form entry was too large to be processed at its original size, " + + "Image " + originalImage.getName() + " was too large to be processed at its original size, " + "and had to be downsized");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java(3 hunks)app/src/org/commcare/utils/FileUtil.java(8 hunks)
🔇 Additional comments (6)
app/src/org/commcare/activities/components/ImageCaptureProcessing.java (3)
59-60: LGTM: Appropriate implementation of EXIF-aware scaling
The change to use scaleAndSaveImageWithExif ensures EXIF metadata preservation during image scaling operations.
127-128: LGTM: Comprehensive error handling
The error message provides clear context by including both source and destination paths, and properly chains the original exception.
70-125:
Security improvements needed for EXIF handling
While the EXIF preservation implementation is comprehensive, there are several security considerations to address:
- The unencrypted EXIF file deletion is not verified
- No validation of the encrypted files
- Potential exposure of sensitive metadata during the process
Consider implementing these improvements:
if (sourceExif != null) {
String exifPath = finalFilePath + ".exif";
ExifInterface destExif = new ExifInterface(exifPath);
// ... existing EXIF copying code ...
destExif.saveAttributes();
// Encrypt the EXIF data file
EncryptionIO.encryptFile(exifPath, exifPath + MediaWidget.AES_EXTENSION,
formEntryActivity.getSymetricKey());
- new File(exifPath).delete();
+ File unencryptedExif = new File(exifPath);
+ if (!unencryptedExif.delete()) {
+ Logger.log(LogTypes.TYPE_ERROR_STORAGE,
+ "Failed to delete unencrypted EXIF file: " + exifPath);
+ }
+
+ // Validate encrypted files
+ File encryptedExif = new File(exifPath + MediaWidget.AES_EXTENSION);
+ if (!encryptedExif.exists() || encryptedExif.length() == 0) {
+ throw new IOException("EXIF encryption failed or produced empty file");
+ }
+ if (!destFile.exists() || destFile.length() == 0) {
+ throw new IOException("Image encryption failed or produced empty file");
+ }
}Likely invalid or redundant comment.
app/src/org/commcare/utils/FileUtil.java (3)
68-73: Ensure JavaDoc comments are provided for all public methods
You've added JavaDoc comments to some methods, which is excellent for maintainability. Please ensure that all public methods in this class, including newly added ones like copyFileWithExifData and scaleAndSaveImageWithExif, have comprehensive JavaDoc comments detailing their purpose, parameters, return values, and exceptions thrown.
Also applies to: 93-99, 883-890, 950-959
902-948:
Declare thrown exceptions in copyExifData method signature
The copyExifData method can throw an IOException, but it is not declared in the method signature. Update the method to declare the exception, ensuring that callers are aware and can handle it appropriately.
Apply this diff to update the method signature:
- private static void copyExifData(String sourcePath, String destPath) {
+ private static void copyExifData(String sourcePath, String destPath) throws IOException {Additionally, remove the try-catch block inside the method to allow the exception to propagate:
- try {
// Existing code to copy EXIF data
- } catch (IOException e) {
- // Log but don't fail if EXIF copying fails
- Logger.log(LogTypes.TYPE_WARNING_NETWORK,
- String.format("Failed to copy EXIF data from %s to %s: %s (Error: %s)",
- sourcePath, destPath, e.getMessage(), e.getClass().getSimpleName()));
- }Likely invalid or redundant comment.
960-975:
Propagate exceptions to ensure EXIF data is preserved
In scaleAndSaveImageWithExif, if copyExifData fails due to an IOException, the method still returns true, which might indicate success to the caller even though EXIF data was not copied. Propagate the exception to ensure the caller is aware of the failure.
Apply this diff to propagate the exception:
} else {
// Copy EXIF data from source to scaled image
- copyExifData(sourceFile.getAbsolutePath(), destFile.getAbsolutePath());
+ try {
+ copyExifData(sourceFile.getAbsolutePath(), destFile.getAbsolutePath());
+ } catch (IOException e) {
+ throw new IOException("Failed to copy EXIF data after scaling image", e);
+ }
}Likely invalid or redundant comment.
app/src/org/commcare/activities/components/ImageCaptureProcessing.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/activities/components/ImageCaptureProcessing.java
Outdated
Show resolved
Hide resolved
| // Log but don't fail if EXIF copying fails | ||
| Logger.log(LogTypes.TYPE_WARNING_NETWORK, | ||
| String.format("Failed to copy EXIF data from %s to %s: %s (Error: %s)", | ||
| sourcePath, destPath, e.getMessage(), e.getClass().getSimpleName())); | ||
| } |
There was a problem hiding this comment.
Avoid swallowing exceptions without proper handling
Swallowing exceptions by only logging them can make debugging difficult and may hide critical issues. Consider rethrowing the exception or handling it in a way that alerts the caller to the failure.
As updated in the previous comment, rethrow the IOException after logging if necessary.
|
@coderabbitai full review |
|
I wanted to reach out with my appreciation for this contribution! Image capture is key to some of our implementers' critical workflows for everything from validating children's immunization histories to capturing photos of fields to support yield sizing. They'll appreciate this improved functionality. I wanted to reach out with a few small points to shepherd things through to wrap-up. 1 - Don't worry too much about the Coderabbit feedback (of course feel free to follow if you find it helpful, of course). We are just launching the system on this particular repo, and haven't fully calibrated the inputs, so it's not mandatory for processing. 2 - Given the complexity of the testing harness and the straightforwardness of the code I'm comfortable with the structure of your testing approach for #2689, but before closing out #2743 I think it's important to have an artifact image in the PR to validate that we got the right EXIF property as set by devices and respected by browsers. Does your testing device have an EXIF output with orientation data set which you can provide to confirm that the image appears in a browser with the original faithful orientation (would be nice to have before/after, but not necessary). 3 - Just a quick reminder from the program terms that before we merge, we'll need a CLA submission from your end. Thanks! |
| if (sizeIndex == -1) { | ||
| return false; | ||
| } | ||
| if (returnCursor.moveToFirst()) { | ||
| return returnCursor.getLong(sizeIndex) > FormUploadUtil.MAX_BYTES; | ||
| } else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
thanks for making this better.
| String[] tagsToPreserve = { | ||
| ExifInterface.TAG_GPS_LATITUDE, | ||
| ExifInterface.TAG_GPS_LATITUDE_REF, | ||
| ExifInterface.TAG_GPS_LONGITUDE, | ||
| ExifInterface.TAG_GPS_LONGITUDE_REF, | ||
| ExifInterface.TAG_GPS_TIMESTAMP, | ||
| ExifInterface.TAG_GPS_DATESTAMP, | ||
| ExifInterface.TAG_GPS_ALTITUDE, | ||
| ExifInterface.TAG_GPS_ALTITUDE_REF, | ||
| ExifInterface.TAG_GPS_AREA_INFORMATION, | ||
| ExifInterface.TAG_DATETIME, | ||
| ExifInterface.TAG_DATETIME_DIGITIZED, | ||
| ExifInterface.TAG_DATETIME_ORIGINAL, | ||
| ExifInterface.TAG_OFFSET_TIME, | ||
| ExifInterface.TAG_OFFSET_TIME_ORIGINAL, | ||
| ExifInterface.TAG_OFFSET_TIME_DIGITIZED, | ||
| ExifInterface.TAG_COPYRIGHT, | ||
| ExifInterface.TAG_IMAGE_DESCRIPTION, | ||
| ExifInterface.TAG_EXIF_VERSION, | ||
| ExifInterface.TAG_ORIENTATION | ||
| }; | ||
|
|
||
| for (String tag : tagsToPreserve) { | ||
| String value = sourceExif.getAttribute(tag); | ||
| if (value != null) { | ||
| destExif.setAttribute(tag, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
can we reuse code from copyExifData here ?
|
|
||
| // If we had EXIF data, store it in a companion file | ||
| if (sourceExif != null) { | ||
| String exifPath = finalFilePath + ".exif"; |
There was a problem hiding this comment.
I am a bit unsure how it's working. A few questions reg. encrypted images -
-
Curious why do we need to create a separate
.exiffile here to retain exif data ? -
Before uploading images to the server, we decrypt them first by using a EncryptedFileBody. Do we also need to upload the
.exiffile there to be able to retain exif data on server ? -
In your testing, have you been able to test the exif retention for final image on server with encyrption enabled ?
Images taken in CommCare were losing their EXIF metadata, including critical GPS/location data, during processing. This created headaches for users who needed that juicy metadata intact, especially for location-based verification.
Resolves issue #2689 and #2743
The Fix
We pimped the image capture and processing workflow to make EXIF data stick, no matter what. Here's what we did:
Injected EXIF-aware copying for raw image files.
Ensured EXIF data is kept intact during image scaling.
Made sure key EXIF tags stay in the game:
How We Pulled It Off
copyFileWithExifData()to keep EXIF data alive during file copying.scaleAndSaveImageWithExif()for maintaining EXIF while scaling images.ImageCaptureProcessingto use our new EXIF-hugging methods.ExifInterfaceAPI for bulletproof metadata handling.Test Drive
Basic EXIF Check
Scaled Image Test
Encrypted Image Test
Edge Cases
Tools to Verify
exiftoolto peek at the EXIF data.Additional Notes
/claim #2689
/claim #2743