Bugfix/upload via share #1920

ashishkumar468 · 2018-10-09T15:56:37Z

Sometimes uploads fails or queued, then don't upload. #1878

Fixes #1878 Sometimes uploads fails or queued, then don't upload.

Description (required)

As the app was unable to validate the MIME type of the image shared via intent or from any source, the api to upload image used to fail because the backend was smart enough to validate the same
Fixes #{GitHub issue number and title}

{Describe the changes made and why they were made.}
Added a java library to validate the image MIME type and accordingly append the image extension.

Tests performed (required)

Tested on {27 & Google Pixel}, with {build variant, ProdDebug}.

Screenshots showing what changed (optional)

NA

* Added a java library to fetch the MIME type from input stream * Fetch MIME type using this and use the contribution tag only when this fails:

misaochan · 2018-10-09T16:33:32Z

Thanks for the quick PR! A couple of things:

We need to discuss the new library that you are adding as per https://github.com/commons-app/apps-android-commons/wiki/Code-style#new-libraries . Could you please explain why you think it is beneficial to add a library for this task, as opposed to writing the file extension checking code ourselves? Is it worth the risk of additional bugs and additional complexity, and is the library reputable enough that we can be reasonably confident that any bugs with the library itself will be solved fairly quickly in future updates?
Could you please describe in further detail the tests you did? Did you try uploading a .png via Share and succeeded?

ashishkumar468 · 2018-10-09T17:22:32Z

@misaochan
Why do we need to add a new library ? -> I actually implemented a number of ways to extract the MIME type. All of them had one issue that they relied on the data received from the intent (which as i have mentioned in the issue thread was coming wrong at-least for me in Google Pixel device and emulator ). Also, the native implementations in Android do not validate the MIME type. They instead trust the source and after trying out more than 3-4 ways (some of them include URLConnection class, URI ), all with no genuine results, I could conclude that this could not be handled natively. Finally SimpleMagic came to rescue. Please refer https://stackoverflow.com/questions/51438/getting-a-files-mime-type-in-java (the source). The library has 83 stars and for a project like this seems quite reputable to me. Also as we can see in the source code of a library, a number of parsers and their corresponding handlers and of-course the appropriate test cases on a number of file types is there. The code base is not that small which would take a lot of time to implement on our own. Considering a major production bug/crash (and being a great supporter of code-re usability), I went with the library, considering it the best solution as of now (and I may be wrong). I am open to suggestions for the same.

Tests-> I have manually tested by uploading both png and jpeg images (via Share as well) confirming the extensions from the file info, both of them went to success (on Google Pixel where I could produce the issue easily as mentioned in the issue thread)

misaochan · 2018-10-10T09:03:50Z

app/src/main/java/fr/free/nrw/commons/upload/UploadService.java

            File file1 = new File(contribution.getLocalUri().getPath());
            fileInputStream = new FileInputStream(file1);
+            tempFileInputStream = new FileInputStream(file1);
+            if(contentInfoUtil==null){


Please use consistent formatting. :) E.g. see the catch block below for our usual bracket formatting

misaochan · 2018-10-10T09:04:26Z

app/src/main/java/fr/free/nrw/commons/upload/UploadService.java

@@ -222,9 +240,17 @@ private void uploadContribution(Contribution contribution) {

        String filename = null;
        try {
+            //try to fetch the MIME type from contentInfo first and then use the tag to do it
+            //Note : the tag has not proven trustworthy in the past


Good documentation! 👍

misaochan · 2018-10-10T09:04:38Z

app/src/main/java/fr/free/nrw/commons/upload/UploadService.java

+            String mimeType;
+            if(contentInfo==null || contentInfo.getMimeType()==null){
+                mimeType=(String)contribution.getTag("mimeType");
+            }else{


Formatting please

misaochan · 2018-10-10T09:17:33Z

@ashishkumar468 Thanks for the explanation, in that case I agree that the new library is needed as more than just a convenience. :)

I tested this on a Nexus S emulator on API 27, and indeed .png uploads via Share and the app work for me, and no issues with other filetypes either. Great job! 👍

We just need you to make a few formatting changes, and we also need 1 more person (@neslihanturan , @maskaravivek ?) to approve the new library. Then I'll merge and release the hotfix with this.

codecov-io · 2018-10-10T10:12:57Z

Codecov Report

Merging #1920 into 2.8-release will decrease coverage by <.01%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##           2.8-release   #1920      +/-   ##
==============================================
- Coverage         3.73%   3.72%   -0.01%     
==============================================
  Files              190     190              
  Lines             9537    9555      +18     
  Branches           849     852       +3     
==============================================
  Hits               356     356              
- Misses            9156    9174      +18     
  Partials            25      25

Impacted Files	Coverage Δ
app/src/main/java/fr/free/nrw/commons/Utils.java	`27.39% <ø> (ø)`	⬆️
...java/fr/free/nrw/commons/upload/UploadService.java	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a25474...560d398. Read the comment docs.

maskaravivek · 2018-10-10T16:04:58Z

@ashishkumar468 The build is failing for me. I was basically trying to check the method count for this build.

Without this build, master has 5183 methods. If the library doesn't add too many methods then IMO its safe to add this library.

ashishkumar468 · 2018-10-10T19:14:17Z

@maskaravivek Not sure about the build failure, please try cleaning the project. And yeah regarding the method count, I have checked in both the apk's one without this library and one with the library, total method counts in both is ~17 k self and 25 k referenced hardly differing in 20-30 methods. Also, there is negligible change in apk size.

maskaravivek

Looks good to me. The library isn't adding a lot of extra methods to the application and looks to be useful.

misaochan · 2018-10-11T05:55:20Z

Thanks Ashish!

* Bug Fix commons-app#1878 * Added a java library to fetch the MIME type from input stream * Fetch MIME type using this and use the contribution tag only when this fails: * formatting changes, removed unused commented out line

* Bugfix/upload via share (#1920) * Bug Fix #1878 * Added a java library to fetch the MIME type from input stream * Fetch MIME type using this and use the contribution tag only when this fails: * formatting changes, removed unused commented out line * Versioning and changelog for v2.8.5 (#1923) * Versioning for v2.8.5 * Update changelog.md

ilgazer · 2018-10-13T12:26:41Z

I know that this PR is closed, but why was a jar file used instead of a gradle reference?

Bug Fix commons-app#1878

77b0fa7

* Added a java library to fetch the MIME type from input stream * Fetch MIME type using this and use the contribution tag only when this fails:

ashishkumar468 changed the base branch from master to 2.8-release October 9, 2018 16:14

ashishkumar468 requested review from neslihanturan and misaochan October 9, 2018 16:15

ashishkumar468 self-assigned this Oct 9, 2018

misaochan reviewed Oct 10, 2018

View reviewed changes

formatting changes, removed unused commented out line

560d398

ashishkumar468 requested a review from maskaravivek October 10, 2018 09:56

misaochan approved these changes Oct 10, 2018

View reviewed changes

maskaravivek approved these changes Oct 10, 2018

View reviewed changes

misaochan merged commit 525d5da into commons-app:2.8-release Oct 11, 2018

misaochan mentioned this pull request Oct 14, 2018

Allow multiple uploads from within the app #604

Closed

ashishkumar468 deleted the bugfix/upload_via_share branch October 17, 2018 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bugfix/upload via share #1920

Bugfix/upload via share #1920

Uh oh!

ashishkumar468 commented Oct 9, 2018

Uh oh!

misaochan commented Oct 9, 2018

Uh oh!

ashishkumar468 commented Oct 9, 2018 •

edited

Loading

Uh oh!

misaochan Oct 10, 2018

Uh oh!

misaochan Oct 10, 2018

Uh oh!

misaochan Oct 10, 2018

Uh oh!

misaochan commented Oct 10, 2018 •

edited

Loading

Uh oh!

codecov-io commented Oct 10, 2018

Uh oh!

maskaravivek commented Oct 10, 2018

Uh oh!

ashishkumar468 commented Oct 10, 2018

Uh oh!

maskaravivek left a comment

Uh oh!

misaochan commented Oct 11, 2018

Uh oh!

ilgazer commented Oct 13, 2018

Uh oh!

Uh oh!

Bugfix/upload via share #1920

Bugfix/upload via share #1920

Uh oh!

Conversation

ashishkumar468 commented Oct 9, 2018

Sometimes uploads fails or queued, then don't upload. #1878

Description (required)

Tests performed (required)

Screenshots showing what changed (optional)

Uh oh!

misaochan commented Oct 9, 2018

Uh oh!

ashishkumar468 commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

misaochan Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

misaochan Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

misaochan Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

misaochan commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 10, 2018

Codecov Report

Uh oh!

maskaravivek commented Oct 10, 2018

Uh oh!

ashishkumar468 commented Oct 10, 2018

Uh oh!

maskaravivek left a comment

Choose a reason for hiding this comment

Uh oh!

misaochan commented Oct 11, 2018

Uh oh!

ilgazer commented Oct 13, 2018

Uh oh!

Uh oh!

ashishkumar468 commented Oct 9, 2018 •

edited

Loading

misaochan commented Oct 10, 2018 •

edited

Loading