Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed and added support for dataUri in form data #33548

Closed
wants to merge 0 commits into from
Closed

Fixed and added support for dataUri in form data #33548

wants to merge 0 commits into from

Conversation

hetanthakkar
Copy link
Contributor

@hetanthakkar hetanthakkar commented Apr 1, 2022 •
edited
Loading

Summary

Fixes #25790. The issue resulted because the getFileInputStream() method of RequestBodyUtil failed to return an inputStream of the given dataUri This happened because the openInputStream() method of context.getContentResolver() expects a content or a file in its parameter but instead received dataUri which resulted in FileNotFoundException.

issue

. Therefore it makes sense to provide a file path to the openInputStream method instead of dataUri.

Solution:

  1. Converted the dataUri to a bitmap and then stored the bitmap image in a file to get the file path. Code Reference
  2. This file path is passed to the openInputStream() method Code Referrence
  3. Once the FileInputStream is generated, I deleted the temporarily created file.

This issue has been unresolved for quite some time now, so resolving this PR would be greatly appreciated.

Changelog

[Android] [Added] - Support for dataUri in form data

Test Plan

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Apr 1, 2022
@hetanthakkar hetanthakkar changed the title Fixed inputStream exception to support DataUri in form data Fixed and added support for dataUri in form data Apr 1, 2022
@analysis-bot
Copy link

analysis-bot commented Apr 1, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: cf55fd5
Branch: main

@analysis-bot
Copy link

analysis-bot commented Apr 1, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,776,837 +0
android hermes armeabi-v7a 7,182,702 +0
android hermes x86 8,085,930 +0
android hermes x86_64 8,065,869 +0
android jsc arm64-v8a 9,649,905 +0
android jsc armeabi-v7a 8,424,158 +0
android jsc x86 9,599,182 +0
android jsc x86_64 10,196,540 +0

Base commit: cf55fd5
Branch: main

@react-native-bot react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Apr 3, 2022
@hetanthakkar
Copy link
Contributor Author

@kelset @cortinico @lunaleaps can you please review this pr?

ryancat
ryancat reviewed Apr 4, 2022
View reviewed changes
ReactAndroid/src/main/java/com/facebook/react/modules/network/RequestBodyUtil.java Outdated
Bitmap bitMap = BitmapFactory.decodeByteArray(decodedDataUrString, 0, decodedDataUrString.length);
ByteArrayOutputStream bytes = new ByteArrayOutputStream();
bitMap.compress(Bitmap.CompressFormat.JPEG, 100, bytes);
String path = MediaStore.Images.Media.insertImage(context.getContentResolver(), bitMap, "Title", null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to look up for more options, and datauri seems not supported out of the box. I agree to create a temp file and use the input stream like so is the correct solution. However, I suggest:

  1. Evaluate any impact on large image files
  2. Checkout the next method in this file, getDownloadFileInputStream, where it also created a temp file and return the inputstream. Can we somehow re-use the implementationt here?

Copy link
Contributor Author

@hetanthakkar hetanthakkar Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryancat Thanks for the review! I've now converted the dataUri to bitmap and then attached an inputStream to the compressed bitmap. This way we won't have to store the image temporarily. I think converting the dataUri to Bitmap is necessary as there is no method that lets us convert the dataUri to inputStream directly. And regarding large size images, converting them to bitmap is the only(efficient) step I can think of, as the conversion is handled by in-built java function.

@hetanthakkar
Copy link
Contributor Author

hetanthakkar commented Apr 5, 2022 •
edited
Loading

@ryancat The test which is failing, is related to my code changes? As I've made no changes in ios files.

@hetanthakkar hetanthakkar requested a review from ryancat April 8, 2022 17:31
@hetanthakkar
Copy link
Contributor Author

@cortinico Can you please review!?

cortinico
cortinico reviewed Apr 20, 2022
View reviewed changes
ReactAndroid/src/main/java/com/facebook/react/modules/network/RequestBodyUtil.java Outdated
Bitmap bitMap = BitmapFactory.decodeByteArray(decodedDataUrString, 0, decodedDataUrString.length);
ByteArrayOutputStream bytes = new ByteArrayOutputStream();
bitMap.compress(Bitmap.CompressFormat.JPEG, 100, bytes);
ByteArrayOutputStream baos = new ByteArrayOutputStream();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cortinico yes that variable is unused and i forgot to remove that before commiting.

Copy link
Contributor Author

@hetanthakkar hetanthakkar Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cortinico I'm really sorry for inconvenience but i was trying to push my code changes but i messed up and ended up closing this PR(which tried to re-open but I cant, maybe maintainer can reopen)🤦😕. So I've opened a new PR for the same . I need to work on my GitHub skills

@hetanthakkar hetanthakkar mentioned this pull request Apr 20, 2022
Closed
facebook-github-bot pushed a commit that referenced this pull request May 10, 2022
@hetanthakkar @facebook-github-bot
Fixed and added support for dataUri in form data (#33675)
c663c0e 
Summary:
Continuation of #33548
 Fixes #25790. The issue resulted because the [getFileInputStream() method of RequestBodyUtil failed to return an inputStream of the given dataUri ](https://github.com/facebook/react-native/blob/16397e0d3c0dd3374ddb642599725ca41f092a8a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java#L427) This happened because the openInputStream() method of [context.getContentResolver() expects a content or a file](https://developer.android.com/reference/android/content/ContentResolver#openInputStream(android.net.Uri)) in its parameter but instead received dataUri which resulted in FileNotFoundException.

![issue](https://user-images.githubusercontent.com/38756320/161345967-fd79d3e2-54a8-4a0e-8a6b-189ce9883a78.jpeg)

**Solution**:

I've now converted the dataUri to bitmap and then attached an inputStream to the compressed bitmap. This way we won't have to store the image temporarily. I think converting the dataUri to Bitmap is necessary as there is no method that lets us convert the dataUri to inputStream directly. And regarding large size images, converting them to bitmap is the only(efficient) step I can think of, as the conversion is handled by in-built java function.

This issue has been unresolved for quite some time now, so resolving this PR would be greatly appreciated.
## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Added] - Support for dataUri in form data

Pull Request resolved: #33675

Reviewed By: christophpurrer, mdvacca

Differential Revision: D36205586

Pulled By: cortinico

fbshipit-source-id: bfc83efcec0b2fcb1df42e4bf1d43c966de8f40e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cortinico cortinico cortinico left review comments

@ryancat ryancat Awaiting requested review from ryancat

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: React Native Team Attention Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android] FormData does not support data URIs
7 participants
@hetanthakkar @analysis-bot @ryancat @cortinico @facebook-github-bot @kelset @react-native-bot