Create micro image widget #1459
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request adds a new micro image control constant and introduces a class for handling Base64-encoded image data responses. The parser logic is updated to use a dedicated media type attribute for detecting image inputs and mapping them to the micro image control. In addition, serialization support for Base64 image data is implemented and the logging method is enhanced with a non-null annotation for its exception parameter. These modifications extend media handling and data serialization capabilities while keeping existing functionalities intact. Changes
Sequence Diagram(s)sequenceDiagram
participant P as XFormParser
participant E as Element
participant Q as QuestionDef
participant C as Constants
P->>E: Retrieve 'mediatype' attribute
alt Attribute equals "image/*"
P->>C: Use CONTROL_MICRO_IMAGE
P->>Q: Create a micro image QuestionDef
else
P->>Q: Create a default QuestionDef
end
sequenceDiagram
participant S as XFormAnswerDataSerializer
participant D as Base64ImageData
S->>D: Check if data is instance of Base64ImageData
alt Data is Base64ImageData
S->>D: Call serializeAnswerData(Base64ImageData)
D-->>S: Return concatenated string representation
else
S->>D: Proceed with default serialization
end
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: 2
🧹 Nitpick comments (3)
src/main/java/org/javarosa/core/model/data/Base64ImageData.java (1)
19-20: Consider adding null checks and validation for base64EncodedImage field.The field is not marked as final and could be null after deserialization. Consider adding validation in the constructor and after deserialization.
public class Base64ImageData implements IAnswerData { - private Pair<String, String> base64EncodedImage; + private Pair<String, String> base64EncodedImage; + + private void validateBase64Image() { + if (base64EncodedImage == null || base64EncodedImage.first == null || base64EncodedImage.second == null) { + throw new IllegalStateException("Base64 image data is invalid or null"); + } + }src/main/java/org/javarosa/core/model/Constants.java (1)
82-82: LGTM! Consider adding documentation.The constant follows the established pattern. Consider adding a documentation comment like other constants.
+ /** + * Micro image control type for handling Base64 encoded images. + */ public static final int CONTROL_MICRO_IMAGE = 14;src/main/java/org/javarosa/xform/util/XFormAnswerDataSerializer.java (1)
173-176: Consider adding string length validation.The serialization method should validate the length of the concatenated string to prevent potential issues with very large Base64 data.
public Object serializeAnswerData(Base64ImageData data) { Pair<String, String> base64ImageData = (Pair<String, String>) data.getValue(); + String result = base64ImageData.first + " " + base64ImageData.second; + if (result.length() > MAX_SERIALIZED_LENGTH) { + throw new IllegalStateException("Serialized Base64 image data exceeds maximum length"); + } - return base64ImageData.first + " " + base64ImageData.second; + return result; } + + private static final int MAX_SERIALIZED_LENGTH = 1024 * 1024; // 1MB limit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/javarosa/core/model/Constants.java(1 hunks)src/main/java/org/javarosa/core/model/data/Base64ImageData.java(1 hunks)src/main/java/org/javarosa/core/services/Logger.java(2 hunks)src/main/java/org/javarosa/xform/parse/XFormParser.java(4 hunks)src/main/java/org/javarosa/xform/util/XFormAnswerDataSerializer.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (6)
src/main/java/org/javarosa/core/services/Logger.java (1)
55-55: LGTM! Improved type safety with @NotNull annotation.The addition of @NotNull annotation helps prevent null pointer exceptions and improves the method contract.
src/main/java/org/javarosa/xform/util/XFormAnswerDataSerializer.java (1)
229-231: LGTM! Base64ImageData handling is properly integrated.The integration of Base64ImageData into the main serialization method is correct and follows the existing pattern.
src/main/java/org/javarosa/xform/parse/XFormParser.java (4)
90-90: LGTM! Constant declaration follows naming conventions.The constant
MEDIA_TYPE_ATTRis well-named and follows the existing pattern of constant declarations in the file.
889-902: LGTM! Method signature simplified and uses new constant.The changes improve code maintainability by:
- Removing unused
usedAttsparameter- Using the new
MEDIA_TYPE_ATTRconstant instead of hardcoded string
927-930: LGTM! Media type attribute properly tracked.The changes ensure that the media type attribute is properly tracked in
usedAtts, preventing false warnings about unused attributes.
904-914:Details
✅ Verification successful
Verify micro image control type handling.
The implementation looks correct, but let's verify that all input controls with "image/*" media type are properly converted to micro image controls.
🏁 Script executed:
#!/bin/bash # Search for input controls with image/* media type rg -A 5 'mediatype="image/\*"'Length of output: 1522
Micro Image Control Type Handling Verified
The recent changes in
src/main/java/org/javarosa/xform/parse/XFormParser.javacorrectly update input controls with the"image/*"media type by assigning them theCONTROL_MICRO_IMAGE. Additionally, the test XML files (e.g.,src/test/resources/xform_tests/test_upload_extension_1.xml,test_upload_extension_2.xml, andtest_upload_extension_3.xml) confirm that controls using the"image/*"attribute are present as expected.No further changes are required. Please ensure that all automated tests pass in your CI pipeline.
| @Override | ||
| public Base64ImageData cast(UncastData data) throws IllegalArgumentException { | ||
| String[] elements = DataUtil.splitOnSpaces(data.value); | ||
| if (elements.length != 2) { | ||
| throw new IllegalArgumentException("Two elements are expected, found "+ elements.length); | ||
| } | ||
| return new Base64ImageData(new Pair<>(elements[0], elements[1])); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation in cast method.
The cast method should validate both the filename and Base64 data format.
@Override
public Base64ImageData cast(UncastData data) throws IllegalArgumentException {
String[] elements = DataUtil.splitOnSpaces(data.value);
if (elements.length != 2) {
throw new IllegalArgumentException("Two elements are expected, found "+ elements.length);
}
+ if (!isValidFileName(elements[0])) {
+ throw new IllegalArgumentException("Invalid filename format");
+ }
+ if (!isBase64(elements[1])) {
+ throw new IllegalArgumentException("Invalid Base64 format");
+ }
return new Base64ImageData(new Pair<>(elements[0], elements[1]));
}
+
+ private boolean isValidFileName(String fileName) {
+ return fileName != null && !fileName.isEmpty() && !fileName.contains("/") && !fileName.contains("\\");
+ }Committable suggestion skipped: line range outside the PR's diff.
| @Override | ||
| public void setValue(Object o) { | ||
| if (o == null) { | ||
| throw new NullPointerException("Attempt to set an IAnswerData class to null."); | ||
| } | ||
| base64EncodedImage = (Pair<String, String>)o; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Base64 validation in setValue method.
The setValue method should validate that the second element of the pair is actually Base64 encoded data.
@Override
public void setValue(Object o) {
if (o == null) {
throw new NullPointerException("Attempt to set an IAnswerData class to null.");
}
+ if (!(o instanceof Pair)) {
+ throw new IllegalArgumentException("Expected Pair<String, String> but got " + o.getClass());
+ }
base64EncodedImage = (Pair<String, String>)o;
+ validateBase64Image();
+ if (!isBase64(base64EncodedImage.second)) {
+ throw new IllegalArgumentException("Image data is not Base64 encoded");
+ }
}
+
+ private boolean isBase64(String str) {
+ try {
+ java.util.Base64.getDecoder().decode(str);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
+1 on more argument validation here along with making sure none of the two objects in the pair are null
There was a problem hiding this comment.
@shubham1g5 I agree with adding null validation for both elements of the pair. Here's the complete implementation with all validations:
@Override
public void setValue(Object o) {
if (o == null) {
throw new NullPointerException("Attempt to set an IAnswerData class to null.");
}
if (!(o instanceof Pair)) {
throw new IllegalArgumentException("Expected Pair<String, String> but got " + o.getClass());
}
Pair<?, ?> pair = (Pair<?, ?>)o;
if (pair.first == null) {
throw new IllegalArgumentException("First element (filename) of the pair cannot be null");
}
if (pair.second == null) {
throw new IllegalArgumentException("Second element (image data) of the pair cannot be null");
}
base64EncodedImage = (Pair<String, String>)o;
if (!isBase64(base64EncodedImage.second)) {
throw new IllegalArgumentException("Image data is not Base64 encoded");
}
}
private boolean isBase64(String str) {
try {
java.util.Base64.getDecoder().decode(str);
return true;
} catch (IllegalArgumentException e) {
return false;
}
}
| @Override | ||
| public void setValue(Object o) { | ||
| if (o == null) { | ||
| throw new NullPointerException("Attempt to set an IAnswerData class to null."); | ||
| } | ||
| base64EncodedImage = (Pair<String, String>)o; | ||
| } |
There was a problem hiding this comment.
+1 on more argument validation here along with making sure none of the two objects in the pair are null
| String[] elements = DataUtil.splitOnSpaces(data.value); | ||
| if (elements.length != 2) { | ||
| throw new IllegalArgumentException("Two elements are expected, found "+ elements.length); | ||
| } |
There was a problem hiding this comment.
this validation needs to happen when we init/set Base64ImageData instead
| return lowerFilename.endsWith(".jpg") || lowerFilename.endsWith(".jpeg") || | ||
| lowerFilename.endsWith(".png") || lowerFilename.endsWith(".gif") || | ||
| lowerFilename.endsWith(".bmp") || lowerFilename.endsWith(".webp"); | ||
| } |
There was a problem hiding this comment.
where is this list coming from ? . Also better to store this list in some sort of collection for easy maintaineability.
| Pair<String, String> base64ImageData = (Pair<String, String>) data.getValue(); | ||
| return base64ImageData.first + " " + base64ImageData.second; |
There was a problem hiding this comment.
should just call toString() here instead ?
| Vector<String> usedAtts = new Vector<>(); | ||
| usedAtts.addElement("mediatype"); | ||
|
|
| @@ -904,7 +903,16 @@ protected QuestionDef parseUpload(IFormElement parent, Element e, int controlUpl | |||
| } | |||
|
|
|||
| protected QuestionDef parseControl(IFormElement parent, Element e, int controlType) { | |||
There was a problem hiding this comment.
this is an overloaded method for another parseControl method and should not contain any logic except calling the other parseControl method
| if (controlType == Constants.CONTROL_INPUT) { | ||
| String mediaType = e.getAttributeValue(null, MEDIA_TYPE_ATTR); | ||
| String appearance = e.getAttributeValue(null, APPEARANCE_ATTR); | ||
| if ("image/*".equals(mediaType) && MICRO_IMAGE_APPEARANCE_ATTR.equals(appearance)) { | ||
| question.setControlType(Constants.CONTROL_MICRO_IMAGE); | ||
| } | ||
| } |
There was a problem hiding this comment.
If we want to set a distinct controlType for micro image, it's best to do in a new top level method say parseInput (similar to parseUpload above)
Product Description
This PR adds a new data type, Base64ImageData, to handle data captured using the Micro Image question type and extends the Form parser to support this new question type.
commcare-android PR: dimagi/commcare-android#2954
Summary by CodeRabbit