558237 - Version with public *and* unreviewed files

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Jeff Balogh (:jbalogh)]

The version selection machinery thinks 4.3.1 should be shown, but the install button finds the unreviewed file and shows the yellow button.

I saw this on a creatured add-on, which should never be unreviewed.

We should probably change the current_version selector to avoid versions with multiple file statuses.

mysql> select addon_id, versions.id, version, files.id, files.status, platform_id  from versions INNER JOIN files ON (files.version_id=versions.id)  where version_id=100232;
+----------+--------+---------+-------+--------+-------------+
| addon_id | id     | version | id    | status | platform_id |
+----------+--------+---------+-------+--------+-------------+
|     3895 | 100232 | 4.3.1   | 83601 |      4 |           5 |
|     3895 | 100232 | 4.3.1   | 83604 |      1 |           2 |
+----------+--------+---------+-------+--------+-------------+
2 rows in set (0.00 sec)

Comment 1

[Fred Wenzel [:wenzel]]

(In reply to comment #0)
> We should probably change the current_version selector to avoid versions with
> multiple file statuses.

I don't currently see how we can put this into an efficient query. What I could probably do is:

.exclude(status__in=[ s for s in statuses if s not in valid_statuses ])

... to exclude all versions that come into question but also have files with invalid statuses. That feels a little hackish though. What do you think?

Comment 2

[Jeff Balogh (:jbalogh)]

(In reply to comment #1)
> .exclude(status__in=[ s for s in statuses if s not in valid_statuses ])
> 
> ... to exclude all versions that come into question but also have files with
> invalid statuses. That feels a little hackish though. What do you think?

Feels hackish, but I thought it would be ok.  However, check out the query:

In [5]: Version.objects.filter(addon=3895, files__status=4).exclude(files__status__in=[1,2,3,5,6,7,8,9]).count()
Out[5]: 26

In [6]: connection.queries
Out[6]: 
[{'sql': u'SELECT COUNT(*) FROM `versions` INNER JOIN `files` ON (`versions`.`id` = `files`.`version_id`) WHERE (`files`.`status` = 4  AND `versions`.`addon_id` = 3895  AND NOT (`versions`.`id` IN (SELECT U1.`version_id` FROM `files` U1 WHERE U1.`status` IN (1, 2, 3, 5, 6, 7, 8, 9))))',
  'time': '0.002'}]

That inner select worries me.

Comment 3

[Fred Wenzel [:wenzel]]

(In reply to comment #2)
> That inner select worries me.

Oh. An even more compelling reason why we shouldn't do this. Another thing we could do in current_version is start at the first candidate (which is currently just returned), then check if all its files are valid (something like ``if version.files.exclude(status__in=validstatuses).exists: continue``), and move on to the next version if not. Until we run out of versions or find a valid one.

Downside: Potentially quite a handful of queries. Upside: For the vast majority of add-ons, the suspected "current version" is indeed the right one, so no expensive looping required.

Comment 4

[Jeff Balogh (:jbalogh)]

Looping makes me uncomfortable.  How about this?

from django.db import connection
from addons.models import Addon, AddonCategory, Feature
a = Addon.objects.get(id=3895)
q = a.versions.exclude(files__status__in=[1,2,3,5,6,7,8])
w = a.versions.filter(files__status__in=[4])
(q & w).count()
connection.queries[-1]

{'sql': u'SELECT COUNT(*) FROM `versions` INNER JOIN `files` T4 ON (`versions`.`id` = T4.`version_id`) WHERE (`versions`.`addon_id` = 3895  AND NOT (`versions`.`id` IN (SELECT U1.`version_id` FROM `files` U1 WHERE U1.`status` IN (1, 2, 3, 5, 6, 7, 8))) AND `versions`.`addon_id` = 3895  AND T4.`status` IN (4))',
 'time': '0.001'}

Comment 5

[Fred Wenzel [:wenzel]]

That didn't get rid of the subselect, did it. (Also, we don't need a count, but a select limit 1, but the rest of the SQL will stay the same either way). :(

Comment 6

[Jeff Balogh (:jbalogh)]

Can it be done without a subselect?  I was showing that we could make the subselect more efficient by limiting it to the current addon_id.

I'm just using count() so that no extra queries are run to pull in translations and such.

Comment 7

[Fred Wenzel [:wenzel]]

(In reply to comment #6)
> Can it be done without a subselect?

Off the top of my head:

SELECT * FROM versions AS v
INNER JOIN files as f ON (f.version_id = v.id AND f.status IN ($valid_statuses))
LEFT JOIN files as f_not ON (f.version_id = v.id AND f.status NOT IN ($valid_statuses))
WHERE f_not.id IS NULL;

That'll return all versions with valid status files, except those that also have files with an invalid status.

A subselect may perform better though. Your query in comment 4 does not actually seem to limit the subselect to this add-on's versions only, but I'll take a look.

Comment 8

[Fred Wenzel [:wenzel]]

FWIW, MySQL suggests optimizations for such queries here: http://dev.mysql.com/doc/refman/5.1/en/in-subquery-optimization.html

All of that will make us execute raw SQL there, but it seems if we want this done efficiently we can't exactly rely on Django in this case.

Comment 9

[Jeff Balogh (:jbalogh)]

(In reply to comment #8) 
> All of that will make us execute raw SQL there, but it seems if we want this
> done efficiently we can't exactly rely on Django in this case.

Sounds good to me.

(In reply to comment #7) 
> A subselect may perform better though. Your query in comment 4 does not
> actually seem to limit the subselect to this add-on's versions only, but I'll
> take a look.

Crap.  I saw the extra where and thought it was doing the right thing.  http://sqlformat.appspot.com/ shows that I'm wrong.

Comment 10

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 1

How about this?

Comment 11

[Jeff Balogh (:jbalogh)]

Comment on attachment 440435 [details] [diff] [review]
Patch, rev. 1

No test?  r-!

To get a fixture for this test, I would find the ids of all the files for versions 4.3.0 and 4.3.1 of addon 3895, then do this:

./manage.py dump_objects files.file xxx yyy zzz

Once you get that all sorted out, the test should check that we pull 4.3.0 instead of 4.3.1.

>diff --git a/apps/addons/models.py b/apps/addons/models.py
>index f64da32..fdc565b 100644
>--- a/apps/addons/models.py
>+++ b/apps/addons/models.py
>@@ -208,7 +208,25 @@ class Addon(amo.models.ModelBase):
>                 return self.versions.get()
>             else:
>                 status = amo.VALID_STATUSES
>-            return self.versions.filter(files__status__in=status)[0]
>+
>+            status_list = ','.join(map(str, status))
>+            cur_ver = self.versions.raw(
>+                """
>+                SELECT v.id FROM versions AS v

If you select v.* versions.raw will create a real Version object and you won't have to do the second query below.

>+                INNER JOIN files AS f ON (f.version_id = v.id)
>+                WHERE v.addon_id = %s
>+                AND f.status IN (%s)
>+                AND NOT EXISTS (
>+                    SELECT 1 FROM versions as v2
>+                    INNER JOIN files AS f2 ON (f2.version_id = v2.id)
>+                    WHERE v2.id = v.id
>+                    AND v.addon_id = %s

Isn't this already done in the outer query?

>+                    AND f.status NOT IN (%s))

Shouldn't that be f2?

>+                ORDER BY v.id DESC
>+                LIMIT 1
>+                """, params=[self.id, status_list, self.id, status_list])
>+            return Version.objects.get(id=cur_ver[0].id)
>+
>         except (IndexError, Version.DoesNotExist):

Will the raw query raise Version.DNE if there's no results?  Sounds like another test.

Comment 13

[Fred Wenzel [:wenzel]]

Thanks for the comments. I added a test and made the relevant part of the query a .extra() call on our query set. The possible exceptions (IndexError, DNE) haven't changed. The former is for our query here, the latter is for the .get() query for listed add-ons.

http://github.com/jbalogh/zamboni/commit/b33d887a3ebe6070914d79f09dbbbdf947384a63

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

How/where did this manifest itself, "the install
button finds the unreviewed file and shows the yellow button.
", from comment 0?

Comment 15

[Fred Wenzel [:wenzel]]

I tried to make this reproducible for you, but the Zamboni admin pages for individual add-ons throw an error 500 at the moment :-/ Jeff, should we make this one a [qa-] because it's such a rare event?

Comment 16

[Jeff Balogh (:jbalogh)]

I was going to point you towards https://preview.addons.mozilla.org/z/en-US/firefox/addon/3895/, but it looks like preview is too out of date for this to be useful.