1044391 - Funsize should return correct MIME type

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Anhad Jai Singh (:ffledgling)]

Funsize does not return the correct MIME Type.
It should specify the correct type when returning a MAR






Footnote: Not the best bug to discover during a live demo.

Comment 1

[Harsimran Singh]

Hi i am interested in working on the bug but i am a little confused. It would be great if you could get me started.

Comment 2

[Harsimran Singh]

i think it`s api.py where the mimetype is sent in the response and it`s always 'application/json' and this is where the problem is. So accordingly the response needs to return the correct mimetype. If this is the correct approach then where can i find more information about the correct mimetype ?

Comment 3

[Ian Connolly [:ianconnolly]]

(In reply to harsimran from comment #2)
> i think it`s api.py where the mimetype is sent in the response and it`s
> always 'application/json' and this is where the problem is. So accordingly
> the response needs to return the correct mimetype. If this is the correct
> approach then where can i find more information about the correct mimetype ?

From https://github.com/ffledgling/Senbonzakura/blob/master/senbonzakura/utils/fetch.py we can see where we're getting the MAR files.

That TEST_URL doesn't work anymore (must've been moved/deleted), but we can go back up a couple of levels to find a fresh MAR to download manually.

Aaaand:

$ curl --head http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-04-03-02-08-mozilla-central/firefox-33.0a1.en-US.linux-i686.complete.mar
HTTP/1.1 200 OK
Server: Apache
X-Backend-Server: ftp2.dmz.scl3.mozilla.com
Cache-Control: max-age=3600
Content-Type: application/octet-stream
Date: Sat, 02 Aug 2014 18:11:49 GMT
Expires: Sat, 02 Aug 2014 19:11:49 GMT
Accept-Ranges: bytes
Access-Control-Allow-Origin: *
ETag: "483baa9-2ef5dc3-4fd5d0fa3745f"
Last-Modified: Fri, 04 Jul 2014 12:14:02 GMT
X-Cache-Info: caching
Content-Length: 49241539

so looks like application/octet-stream is what we're looking for :)

Comment 4

[Harsimran Singh]

So then according to me changing the mimetype to 'application/octet-stream' instead of 'application/json' in api.py and updating the TEST_URL in fetch.py should fix the problem ?

Comment 5

[Ian Connolly [:ianconnolly]]

I don't think editing the TEST_URL is required -- that's more a testing problem. 

I'll defer to :ffledgling on whether that's the only instance where you need to alter the MIME type. But as a rule, anywhere we're returning the MAR the MIME type of the response should be application/octet-stream.

Comment 6

[Anhad Jai Singh (:ffledgling)]

(In reply to harsimran from comment #4)
> So then according to me changing the mimetype to 'application/octet-stream'
> instead of 'application/json' in api.py and updating the TEST_URL in
> fetch.py should fix the problem ?

Mostly correct. The problem lies in the `get_partial` function in `api.py`.
The specific bit of code we need to fix is https://github.com/mozilla/build-funsize/blob/master/senbonzakura/frontend/api.py#L170-L177

Like Ian mentioned, the correct mime-type to return is indeed 'application/octet-stream'.
To do this, you'll need to use flask's Response object (see other examples of the response object in the same file).
Here's the official documentation for the response object: http://flask.pocoo.org/docs/api/#response-objects

Comment 7

[Harsimran Singh]

as suggested by :ffledgling and Ian i made the following change

replaced return cacheo.retrieve(identifier, 'partial') in api.py (https://github.com/mozilla/build-funsize/blob/master/senbonzakura/frontend/api.py#L177)

with     resp = (cacheo.retrieve(indetifier, 'partial'), status=200, mimetype='application/octet-stream')

so that proper mimetype is returned when a partial MAR is obtained which was probably not the case earlier due to which Firefox crashed. Is this correct ?

and regarding the other places where mimetype is concerned i think they are correct and need not be changed.

Comment 8

[Anhad Jai Singh (:ffledgling)]

I'm not entirely sure that's valid python syntax (assignment while tuple creation).
Have you tried making that change in the source and running the tests? Typically you will want to make a change and test if it works.
Also instead of returning a tuple and using flask's implicit conversion to a response object we'll want to use the flask Response object directly for readability sake.

Comment 9

[Harsimran Singh]

i apologize for making a mistake in copy paste,  actually i had replaced 
    
    return cacheo.retrieve(identifier, 'partial') in api.py (https://github.com/mozilla/build-funsize/blob/master/senbonzakura/frontend/api.py#L177)

with    resp = flask.Response(cacheo.retrieve(indetifier, 'partial'), status=200, mimetype='application/octet-stream')

Then i ran the tests...it worked fine. Here is the celery log (http://pastebin.com/CHDF2wdp) for the reference and i successfully obtained the partial MAR which is about 29.8 MB in size.

Comment 10

[Anhad Jai Singh (:ffledgling)]

Ah, I see. In that case your fix looks near perfect.
Now you'll need to attach a patch with your changes to this bug and request feedback and/or review.
Take care to *not* include the other changes you made (the *.ini files, the startup.sh script and so on), because they were specific to your machine.
Normally you'll want to attach the output for `git format-patch` if you're requesting for review.
If you need any help with git, feel free to ask in #releng or #introduction on IRC.

Once the patch is up, someone will review it and then if everything is alright, it'll get checked in.

Comment 11

[Harsimran Singh]

Attachment: MIME type error fix

Please provide feedback on the patch file.

Comment 12

[Anhad Jai Singh (:ffledgling)]

Comment on attachment 8467055 [details] [diff] [review]
MIME type error fix

Review of attachment 8467055 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this patch looks good, just a couple of nits with the whitespace and indentation that I've pointed out inline.
Generally it's a good idea to follow PEP8 when working with python.
Even if some of the code isn't PEP8 at the moment, let's try make sure we follow PEP8 wherever possible.
Once you make the changes, put up a patch again asking for a review, once reviewed we can check it in.
Good work!

::: senbonzakura/frontend/api.py
@@ +177,2 @@
>  
> +            resp = flask.Response(cacheo.retrieve(indetifier, 'partial'), status=200, mimetype='application/octet-stream')

We try to follow PEP8 when working with python in the Mozilla codebase. So in accordance with that let's break this line into two (break after `status=200,`) and align the new line's beginning with the character after the first open parenthesis.
See http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length for a more detailed example.

@@ +177,3 @@
>  
> +            resp = flask.Response(cacheo.retrieve(indetifier, 'partial'), status=200, mimetype='application/octet-stream')
> +            

I think you accidentally introduced empty white space here, let's get rid of that.

Comment 13

[Harsimran Singh]

Attachment: 0001-MIME-type-fix.patch

Here is the new patch with extra line removed and according to PEP8

Comment 14

[Anhad Jai Singh (:ffledgling)]

Comment on attachment 8467494 [details] [diff] [review]
0001-MIME-type-fix.patch

Review of attachment 8467494 [details] [diff] [review]:
-----------------------------------------------------------------

No major issues, a typo and trailing whitespace.
I've gone ahead and fixed them this time, but it'll be easier to catch these at your end if you run the tests under the /tests folder.

Something I forgot to mention, but you'll want to keep in mind when writing commit messages is that the recommended format is "Bug 123456 (or whatever bug number) - Bug title ; r=<reviewer's name>"

I've made the changes required and gone ahead and committed your fix.

It passes all tests here -- https://travis-ci.org/mozilla/build-funsize/builds/31687719

Congratulations, your changes are now in the official repository!

https://github.com/mozilla/build-funsize/commit/784d43326bb6dd2a66d69c0824ebb083f7d3dff9

::: senbonzakura/frontend/api.py
@@ +174,4 @@
>              # we can just throw it at the client just like that.
>  
>              # See -- http://stackoverflow.com/questions/7877282/how-to-send-image-generated-by-pil-to-browser
> +            resp = flask.Response(cacheo.retrieve(indetifier, 'partial'), 

Spelling error in "identifier".
There's a trailing space on this line.

Comment 15

[Anhad Jai Singh (:ffledgling)]

Let me or someone else know (in IRC or on another bug if you already have one in mind) if you're interested in working on more stuff.

Comment 16

[Harsimran Singh]

(In reply to Anhad Jai Singh (:ffledgling) from comment #14)

> No major issues, a typo and trailing whitespace.
> I've gone ahead and fixed them this time, but it'll be easier to catch these
> at your end if you run the tests under the /tests folder.

Sorry, next time i`ll be careful.
> 
> Something I forgot to mention, but you'll want to keep in mind when writing
> commit messages is that the recommended format is "Bug 123456 (or whatever
> bug number) - Bug title ; r=<reviewer's name>"
> 

i`ll keep these in mind next time onwards.

> I've made the changes required and gone ahead and committed your fix.
> 
> It passes all tests here --
> https://travis-ci.org/mozilla/build-funsize/builds/31687719
> 
> Congratulations, your changes are now in the official repository!
> 
yay!, my first bug.

> https://github.com/mozilla/build-funsize/commit/
> 784d43326bb6dd2a66d69c0824ebb083f7d3dff9


Thanks for being an awesome mentor.

Comment 17

[Harsimran Singh]

(In reply to Anhad Jai Singh (:ffledgling) from comment #15)
> Let me or someone else know (in IRC or on another bug if you already have
> one in mind) if you're interested in working on more stuff.

Yes i`ll let you know. btw i have Bug 1040887 - Funsize returns errors out and returns an HTTP 500 when requested for a non-existent partial in my mind at the moment.

Comment 18

[Pete Moore [:pmoore][:pete]]

Great to have you involved harsimran: many thanks for your contribution!

Comment 19

[Harsimran Singh]

(In reply to Pete Moore [:pete][:pmoore] from comment #18)
> Great to have you involved harsimran: many thanks for your contribution!

Pleasure is all mine !