932337 - [mozprofile] Downloaded add-on should get a meaningful filename (e.g. add-on id)

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Right now downloaded add-ons will end-up as files like '/tmp/tmpoN7Kba.xpi'. For any debugging purposes it might be helpful to make use of a proper name, like the add-on id.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

Simple change which will help us to diagnose issues.

Comment 2

[Jeff Hammel]

Comment on attachment 827032 [details] [diff] [review]
Patch v1

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

::: mozprofile/mozprofile/addons.py
@@ +237,5 @@
> +                os.rename(path, new_path)
> +                path = new_path
> +            except:
> +                pass
> +

TBH I wouldn't do this.  The bare except: pass scares me; som ething could go wrong in either the rename or addon_details and we could be in a bad state which IMHO would be worse than the current situation.  I'd much rather log more verbosely, e.g. 'Addon [e-mail] from http://k0s.org/foo.xpi at /tmp/abcdef' than actually renaming on disk since the DLed location is temporary anyway.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jeff Hammel [:jhammel] from comment #2)
> ::: mozprofile/mozprofile/addons.py
> @@ +237,5 @@
> > +                os.rename(path, new_path)
> > +                path = new_path
> > +            except:
> > +                pass
> > +
> 
> TBH I wouldn't do this.  The bare except: pass scares me; som ething could
> go wrong in either the rename or addon_details and we could be in a bad
> state which IMHO would be worse than the current situation.  I'd much rather

Why would we get a bad state? If something throws inside addon_details() we don't do any renaming of the file and completely skip the remaining lines in this try block. If something later goes wrong with renaming, the path will not change because we do only update its value if renaming was successful. That means whatever happens here and could break the renaming we are safe and simply discard the action.

> log more verbosely, e.g. 'Addon [e-mail] from
> http://k0s.org/foo.xpi at /tmp/abcdef' than actually renaming on disk since
> the DLed location is temporary anyway.

So where should this logging happen? In AddonManager by using mozlog? As of now we don't have any logging in mozprofile. Also at which level should it log? I assume DEBUG?

When I run the Python tests how could I specify the level of logging to actually see the output? info() destroys the output on the console.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 827032 [details] [diff] [review]
Patch v1

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

::: mozprofile/mozprofile/addons.py
@@ +234,5 @@
> +                details = self.addon_details(path)
> +                new_path = os.path.join(os.path.dirname(path),
> +                                        details['id'] + '.xpi')
> +                os.rename(path, new_path)
> +                path = new_path

I see your point, but I still agree with jhammel, we might accidentally write code in the future that depends on this new naming scheme. Why not just pass in the id with the 'prefix' parameter to mkstemp? Or use mkdtemp and create the file under there?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> I see your point, but I still agree with jhammel, we might accidentally
> write code in the future that depends on this new naming scheme. Why not
> just pass in the id with the 'prefix' parameter to mkstemp? Or use mkdtemp
> and create the file under there?

That will not work because we don't know the id at this point. The XPI file has to be downloaded first, and then we can use addon_details() to determine the included addon information. So there is no way around it to store into a temp file first. What about moving this code block out into its own method called download()? In that case we can have code in the future which simply calls that new method and gets the path to the final file returned.

Comment 6

[Andrew Halberstadt [:ahal]]

If you think it's usable in other places sure, if not I think might as well leave it here. You're right though, so in that case why not just get rid of the try: except? Feel free to r+ it with that change.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: patch v2

So this got a bit larger as expected, but as discussed on IRC we wanted to have a dedicated download method, which can be used by customers to download add-ons. Therefore I also made it a class method. Also I routed various kinds of possible exceptions through the new AddonDownloadError, and added tests for all possibilities. I hope it matches your expectation.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 828266 [details] [diff] [review]
patch v2

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

Looks good except for the unnecessary re-raising. r+ with that addressed.

::: mozprofile/mozprofile/addons.py
@@ +70,5 @@
> +            os.close(fd)
> +        except (urllib2.HTTPError, urllib2.URLError), e:
> +            raise AddonDownloadError('%s (%s)' % (e.reason, url))
> +        except Exception, e:
> +            raise AddonDownloadError(str(e))

Why bother catching and just re-raising? I'd rather just get rid of this try: except and let the original exceptions propagate. That way the original tracebacks are preserved which makes debugging less painful. If you really want to change the exception name for some reason, there is a way to do that and still keep the original traceback (see http://www.ianbicking.org/blog/2007/09/re-raising-exceptions.html) though I don't see the benefit of doing that.

::: mozprofile/tests/test_addons.py
@@ +77,5 @@
> +        self.assertEqual(os.listdir(self.tmpdir), [])
> +
> +        # Download from an invalid URL
> +        addon = server.get_url() + 'not_existent.xpi'
> +        self.assertRaises(mozprofile.addons.AddonDownloadError,

These would need to be updated

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> ::: mozprofile/mozprofile/addons.py
> @@ +70,5 @@
> > +            os.close(fd)
> > +        except (urllib2.HTTPError, urllib2.URLError), e:
> > +            raise AddonDownloadError('%s (%s)' % (e.reason, url))
> > +        except Exception, e:
> > +            raise AddonDownloadError(str(e))
> 
> Why bother catching and just re-raising? I'd rather just get rid of this
> try: except and let the original exceptions propagate. That way the original
> tracebacks are preserved which makes debugging less painful. If you really
> want to change the exception name for some reason, there is a way to do that
> and still keep the original traceback (see
> http://www.ianbicking.org/blog/2007/09/re-raising-exceptions.html) though I
> don't see the benefit of doing that.

My intent was to simplify the usage of this method by supplying a single exception type to except for. Without that it's very hard to find the right types. That way consumer code only has to care about the new AddonDownloadError, which gets raised if something goes wrong. Do you think that's not beneficial? I can remove it for sure. Otherwise I could work on it to preserve the traceback as given by the URL. Thanks btw. for it!

Comment 10

[Andrew Halberstadt [:ahal]]

So I see the point, but what if you want to handle the failure in different ways depending on how the download failed? In this case, consumers can just use a blanket except if they don't care (or can see what gets raised in the python docs). We could even just say what could get raised in the docstring if we really want.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This list in the docstring could be long then. So I wouldn't do it as of now. Ok, so lets get this try/except removed and we can still figure out later if something else would be helpful. Updated patch upcoming.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: patch v3

Patch with removed try/except and AddonDownloadError. Taking over r+.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on master:
https://github.com/mozilla/mozbase/commit/677bdedcdbaa7583d249cedf2ad82472d37f5766

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Follow-up bustage fix

Follow-up patch which adds the missing invalid.xpi file to the repository.