404664 - SeaMonkey search results include Firefox-only addons

Status: VERIFIED FIXED

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

Description

[Basil Hashem [:baz]]

As reported by various SeaMonkey users. Performing certain searches result in Firefox only add-ons even when SeaMonkey is the "selected app".

1. Access https://addons.mozilla.org/en-US/seamonkey
2. Perform a search for "blog"
3. Review results and notice that "Google Site Indexer" is included even though it's not a SeaMonkey supported app

It should not show up.

Other search terms that yield Firefox results.

"parent" shows ProCon Latte
"sidebar" shows Moji
"ebay" shows Toolbar Buttons

Comment 1

[Fred Wenzel [:wenzel]]

The culprit here is an empty, deactivated version of this add-on which is marked as supporting all applications.

Obviously we should fix this, I suggest by only showing add-ons that have *available* (= public, if in public mode) versions for the current app.

Side note: A similar bug to this has been filed in the past, because the *current* version of an add-on did not support Seamonkey anymore, while an older one in the history did. This I consider expected behavior, while the behavior described in this bug here is actually a bug.

Comment 2

[Mike Shaver (:shaver emeritus)]

(In reply to comment #1)
> Side note: A similar bug to this has been filed in the past, because the
> *current* version of an add-on did not support Seamonkey anymore, while an
> older one in the history did. This I consider expected behavior, while the
> behavior described in this bug here is actually a bug.

I don't think that users will expect that behavior at all -- if there are people who expect "searching for SM add-ons" to return something other than "add-ons that I can install the latest version of in SM", I bet they fit in a thimble.  We recommend that users *not* install older versions of add-ons, because of possible security bugs fixed and so forth, so I don't think the search should dig back into history.

(Denormalizing the latest version's app-compat info to be keyed off of add-on id would probably simplify this a lot, and I suspect give us a perf win as well.)

Comment 3

[Fred Wenzel [:wenzel]]

(In reply to comment #2)
> (In reply to comment #1)
> > Side note: A similar bug to this has been filed in the past, because the
> > *current* version of an add-on did not support Seamonkey anymore, while an
> > older one in the history did. This I consider expected behavior, while the
> > behavior described in this bug here is actually a bug.
> 
> I don't think that users will expect that behavior at all -- if there are
> people who expect "searching for SM add-ons" to return something other than
> "add-ons that I can install the latest version of in SM", I bet they fit in a
> thimble.

Thanks for your comment, after thinking about it I agree with you now. Therefore let's redefine the scope of this bug to "only show add-ons whose latest version is compatible with the given app". If devs care for the users of that app, they should make their current version compatible, and if they don't, their add-on should not be found for that app on AMO.

Comment 4

[Robert Kaiser]

I fully agree with the last two comment, but shouldn't it be "only show add-ons that are activated and have a (public) version available and whose latest (public) version is compatible with the given app", with the brackets depending on public/sandbox mode?
Just to be really correct about what query we need to end up with and to surely close out the error of comment #1 ;-)

Comment 5

[Fred Wenzel [:wenzel]]

Yes, I was only mentioning the relevant changes to the query we have right now. The whole activated and available thing is already met with the query the way it is now.

Thanks anyway :)

Comment 6

[Basil Hashem [:baz]]

Requesting targeting v3.2

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Bug 407211 should be the reason: Update service needs tests to verify processing of AppOs

Clear the duplication if you have another opinion.

Comment 8

[Michael Morgan [:morgamic]]

This is totally unrelated.

Comment 9

[Fred Wenzel [:wenzel]]

No; I don't think the update services share any code with the search on the website; however the solution for this will likely be related (but not a duplicate of) bug 404846.

Comment 10

[Fred Wenzel [:wenzel]]

Hm. To only return addons whose *last* version is compatible with a particular app version, I'd have to do something along the lines of (half-pseudocode) "HAVING MAX(version.id) = version.id GROUP BY version.id WHERE app_version.id = FIREFOX".

HAVING, however, is entirely unoptimized by MySQL, so I have a feeling this would make search even tougher on the DB than it is now. This bug really depends on the search improvement (or is part of it), bug 400986.

A way around this would be a little denormalization, e.g. a table that just contains the last public version per add-on (or the last public AND sandbox versions). That would also reduce the footprint of pulling in versions for category listings, since only the last one is really needed for these lists anyway. Drawback would be maintaining this "materialized view". We'd have to make sure every update on a version invalidates/rebuilds the entries in this secondary table. Cake's afterSave() and afterDelete() callbacks may be the key tho this.

Comment 11

[Mike Shaver (:shaver emeritus)]

Yeah, HAVING is a performance deathtrap, as I understand it.  If the admin tools don't muck with the database directly, then maintaining the materialized views by hand is probably the right path, and could have a bunch of performance advantages.

Comment 12

[Robert Kaiser]

While you're talking of "materialized view" there, would a normal view not help here, or does it not improve performance in any useful way?

Comment 13

[Mike Shaver (:shaver emeritus)]

Non-materialized views are just a query convenience, indeed.

Comment 14

[Fred Wenzel [:wenzel]]

True; they just replace something like SELECT * FROM all_employees_who_have_dogs_and_more_than_two_children; by a bunch of joins, so they save you typos but not computation time -- much like C preprocessor macros, really.

Comment 15

[Fred Wenzel [:wenzel]]

(In reply to comment #11)
> Yeah, HAVING is a performance deathtrap, as I understand it.  If the admin
> tools don't muck with the database directly, then maintaining the materialized
> views by hand is probably the right path, and could have a bunch of performance
> advantages.

I've been thinking about this, but while I like the idea, this seems like a pretty invasive step at the base of the application, since I'd have to make a new table (obviously) and a new model and I'd have to investigate every use of "Version" in the code base by hand (at the moment, there's something > 300 (!)). Am I missing something here? So far I wasn't able to come up with a way to handle this transparently to the app.

On a positive aside, it seems like we could have MySQL 5 do the updates to the view table automatically on update by hooking a stored procedure up to a "trigger": http://pure.rednoize.com/2005/10/26/materialized-views-in-mysql/

Comment 16

[Fred Wenzel [:wenzel]]

Ah, I found a pretty feasible option: I can add two new columns to the addons table (last_version_public and ...sandbox) and have these updated (only for the affected addon) every time a change to the files table (sic! since it keeps the status information, not the version table) occurs. To keep me from changing the entire codebase and preventing major breakage, I'd add a function bindLastVersionOnly() to the addon model, allowing to opt-in to this much less resource intensive way of fetching the last version only.

morgamic: in a case where you have a public version 2.0 and a sandboxed 1.5, which one should be considered the "latest version" when you are in sandbox mode? The public one (in spite of you being in the sandbox) or the sandboxed one?

Comment 17

[Michael Morgan [:morgamic]]

(In reply to comment #16)
> morgamic: in a case where you have a public version 2.0 and a sandboxed 1.5,
> which one should be considered the "latest version" when you are in sandbox
> mode? The public one (in spite of you being in the sandbox) or the sandboxed
> one?

The update service delivers the latest version by versions.id that is public.  right now sandbox add-ons don't receive updates because it'd be a possible vector for updating people to bad stuff.  So only public add-ons, and the latest version by versions.id is the expected behavior for updates.

Comment 18

[Fred Wenzel [:wenzel]]

We need to postpone this and bug 404846 to after Milestone 3.2: The reasoning is that they sound easy to fix, but in order to do so efficiently, non-trivial database changes are needed, which we do not want to jeopardize the progress of the upcoming milestone.

Comment 19

[Fred Wenzel [:wenzel]]

Attachment: Restrict search results by app compatibility of last version

I replaced the join by a subquery, in order to only show results whose *last* version is compatible with the current application. This should take care of all the cases where a long time ago, a single version was marked compatible with SeaMonkey (or Tb, ...). In particular, it fixes the "blog" test case described in comment 0.

Comment 20

[Laura Thomson :laura]

Comment on attachment 321368 [details] [diff] [review]
Restrict search results by app compatibility of last version

WFM

Comment 21

[Fred Wenzel [:wenzel]]

r13311.

Comment 22

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

Verified FIXED using the testcases in comment 0; Moji still shows up post-patch (on preview) because 0.9.2 has support for SeaMonkey 1.0-1.1*:

https://preview.addons.mozilla.org/en-US/seamonkey/addons/versions/145

The rest don't on preview, but do on production.