Closed Bug 1402064 Opened 7 years ago Closed 6 years ago

Remove use of legacy AMO api in Add-ons manager

Categories

(Toolkit :: Add-ons Manager, enhancement, P3)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: eviljeff, Assigned: aswan)

References

Details

(Keywords: meta)

Attachments

(4 files)

Bug 1402064 AddonRepository spring cleaning
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1402064 Refactor compatibility overrides
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1402064 Switch to modern AMO metadata API
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug_1402064.png
82.28 KB, image/png
Details 
The legacy API described here https://developer.mozilla.org/en-US/Add-ons/AMO/Add-ons_manager_API is used in the Firefox Add-ons manager.  Its old, unmaintained, and deprecated so it should be changed to the new AMO api http://addons-server.readthedocs.io/en/latest/topics/api/addons.html

Or dropped entirely.
I'd like to go one step further and completely remove all dependencies on the AMO API from about:addons. There's little of value there at all.

Jorge, should we put together a PRD for this?
Keywords: meta
Priority: -- → P3
Summary: change use of legacy AMO api in Add-ons manager to current external API → Remove use of legacy AMO api in Add-ons manager to current external API
Supposing Firefox did this, what other sources use it, Thunderbird, Seamonkey, anyone else? If this is the case, you should get a deprecation plan in ASAP.
Flags: needinfo?(awilliamson)
Thunderbird and Seamonkey add-ons will be removed from AMO (along with all the other non-webextension add-ons) sometime after 59 so the legacy API can go at the same time.  Firefox using the legacy API is the blocker.
Flags: needinfo?(awilliamson)
I started a doc here: https://docs.google.com/document/d/1EXGTOkbO73CeqTKrjHKg15XhS97NxPpyh3Ws2K31w78/edit#. Will run it by some people and expand on it in the next week or two.
Summary: Remove use of legacy AMO api in Add-ons manager to current external API → Remove use of legacy AMO api in Add-ons manager
After reviewing the docs and plans with Jorge, I think we should just move from the XML to the JSON based API. After a quick scan of the data, it appears all the data is there:

https://addons.mozilla.org/api/v3/addons/addon/1865/ vs
https://services.addons.mozilla.org/en-US/firefox/api/1.5/addon/1865

The long term plan remains to remove this API from add-ons manager as we rewrite add-ons manager. 

But there's two things that make that a bit more complicated: 1) we'd need to change the UI to redirect to AMO for more information and that means checking if the add-on is available on AMO and 2) we'd like to keep contributions in there, approx. 20% of contributions to add-on developers come from about:addons. I'll file bugs on seperate tracker for this.

It's a bit of a pain to change to an API, knowing we'll rip it out later, sorry about that.
Assignee: nobody → aswan
status-firefox59: --- → affected
I just started to take a look at what's involved here and a couple of questions came up.

First, periodically we send some general browser performance data to AMO, see the query parameters that begin with t here:
https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/app/profile/firefox.js#48
This looks like something that predates telemetry which gives us all the same information and much more with actual tools to look at that data later.  So can I rip that bit out as part of this bug?

Second, the old API lets us read info about multiple addons with one request, eg:
https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/guid:uBlock0%40raymondhill.net,support%40lastpass.com

I tried doing the same with the new API but this URL gives a 404:
https://services.addons.mozilla.org/api/v3/addons/addon/uBlock0%40raymondhill.net,support%40lastpass.com/

Individual URLs like this work fine:
https://services.addons.mozilla.org/api/v3/addons/addon/uBlock0%40raymondhill.net/

Is there a way to fetch info about multiple addons at once in the new API or is that too non-REST-y?
Flags: needinfo?(awilliamson)
You can only get multiple results from the addon search endpoint.  Currently you can't search based on guid, though we could potentially add that as an option if it's needed - can you file an issue on mozilla/addons-server?

No idea about the browser performance data - it doesn't look like it's handled in the API code by AMO (could potentially be used/collected somewhere else in the pipeline before django gets the url though)
Flags: needinfo?(awilliamson)
(In reply to Andrew Williamson [:eviljeff] from comment #9)
> You can only get multiple results from the addon search endpoint.  Currently
> you can't search based on guid, though we could potentially add that as an
> option if it's needed - can you file an issue on mozilla/addons-server?

Done: https://github.com/mozilla/addons-server/issues/7119
Krupa, heads up that I want to try to get this done and landed next week, I'll do what testing I can of course but automated testing will involve mocking the AMO response so further integration testing with real live AMO would probably be good if your team can fit it in.

As for how to test it, this is the code that collects extended information about addons for display in about:addons (including the long description, screenshots, etc.) so we would want to verify that that information is still present for AMO hosted addons.
Flags: needinfo?(krupa.mozbugs)
So there are a few fields that were available in the old API that don't appear to be available in the new API.  I think these are all obsolete since they were related to purchasing addons or to in-browser display of search results:
- <learnmore>
- <payment_data>
- <suggested_amount> from <contribution_data>
- <totalDownloads>

The one field that is missing that we still use is the num property on <reviews>, we use this to put a number in the link to reviews in the addon detail page (e.g., **** _500 reviews_).  Unless somebody objects and has a counter-proposal, I'm going to just make that link have static text that says something like "See all reviews")
+1 on making it static text, or even just removing the Rating line in about:addons.
(talked about this on irc in more detail, but to summarise:)

- <learnmore>
`url` - I think it used to point to the developer profile page but we dropped that last year.

- <payment_data>
`contributions_url`

- <suggested_amount> from <contribution_data>
we stopped serving that in the old API after paypal contributions were dropped last year.  There isn't a replacement.

- <totalDownloads>
`weekly_downloads`

All the review data (count and more) is available as ratings.   e.g. ratings.count is the total count; ratings.text_count is the count of ratings that have a text review.
Chatted with Andrew W on IRC and I was making things too complicated, I assumed reviews and ratings were separate things but in fact every rating comes with a review and we have a count of total ratings, so we're good to go without any UI changes (those can wait for bug 1422378)
It looks like the old search API was able to return compatibility overrides for extensions that aren't actually hosted on AMO.  See e.g.:
https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/toolkit/mozapps/extensions/internal/AddonRepository.jsm#810-811

Is this data present in the new API?  If not, are we okay losing this capability when this bug lands?  If the answer to both questions is no then how/where should we be getting this information?
Flags: needinfo?(jorge)
I don't know if it's present in the new API, Andrew can probably find out. We should get that data, though. We can always use blocklisting as an alternative to compatibility overrides, but it's a more heavy-handed tool.

Longer term, we want to move compatibility overrides to the blocklist so we have fewer dependencies between AOM and AMO.
Flags: needinfo?(jorge) → needinfo?(awilliamson)
Compatibility overrides aren't exposed in the v3 api currently (and are poorly, if at all, documented for the v1.5 api).

If they do need to be exposed in the v3 API can an issue be filed detailing what is needed from the compat updates in terms of the API?  I'd heavily lean towards it being a separate API endpoint as the underlying models aren't linked, and it's possible to have compat without a matching Addon (in addition to the more common Addon without compat).
Flags: needinfo?(awilliamson)
(In reply to Andrew Williamson [:eviljeff] from comment #21)
> API work is done.  See
> http://addons-server.readthedocs.io/en/latest/topics/api/addons.html#compat-
> override

For reference, here is a sample of the XML returned by the old API:
https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/toolkit/mozapps/extensions/test/xpcshell/data/test_compatoverrides.xml

In that structure, every <version_range> tag had a "type" attribute, though "incompatible" was the only value that Firefox paid any attention to.  I don't see anything analogous in the docs you linked to, though the descriptions imply that all the ranges returned by the API are versions that should be blocked?  (as opposed to explicitly listing versions that should be enabled)

Also its a tiny nit but there's a stray "string" at the beginning of the lines that documents the "applications" array on readthedocs.

Finally, can you give me some IDs for which this API will return live data (either on production or staging) for the purpose of testing?  Thanks.
Flags: needinfo?(awilliamson)
I dropped the `type` attribute as, like you mentioned, Firefox only ever supported the value of `incompatible`.

(fixed the docs, thanks)

ping @jorgev / @theone for some GUIDs from the compat override models in admin
Flags: needinfo?(jorge)
Flags: needinfo?(awilliamson)
Flags: needinfo?(awagner)
fi@dictionaries.addons.mozilla.org
abhere2@moztw.org
firefox@mega.co.nz
feedly@devhd
Flags: needinfo?(jorge)
Flags: needinfo?(awagner)
The scenarios on the AMO side for testing the new search implementation have been covered in https://public.etherpad-mozilla.org/p/Bug1263313. Does this covered the implementation tracked in this issue? If not please elaborate more on https://bugzilla.mozilla.org/show_bug.cgi?id=1402064#c11 it was unclear what the scenarios is.
Flags: needinfo?(krupa.mozbugs) → needinfo?(aswan)
I just chatted with Krupa and she is going to write out some specific test cases, but a few details for testing:
The code in these patches uses the following prefs that can be pointed to the staging environment for testing:
extensions.getAddons.get.url
extensions.getAddons.compatOverides.url

We do metadata checks at install time and then a background check every 24 hours, but you can trigger a background check by opening the browser console and running:

Cu.import("resource://gre/modules/AddonManager.jsm", {}).AddonManagerPrivate.backgroundUpdateCheck();
Flags: needinfo?(aswan)
Depends on: 1437068
Depends on: 1438148
Depends on: 1438149
I've got the browser-side patches close to ready here but would ideally like to get these issues addressed before landing:
https://github.com/mozilla/addons-server/issues/7513
https://github.com/mozilla/addons-server/issues/7514
https://github.com/mozilla/addons-server/issues/7534

Those are are all marked as P3.  Stuart, can you let me know when you expect these to get fixed on AMO so we can coordinate getting the browser-side changes landed vs deferring stuff to follow-up bugs?  If it would be easier to go over this interactively, we can chat on IRC tomorrow...
Flags: needinfo?(scolville)
(In reply to Andrew Swan [:aswan] from comment #28)
> I've got the browser-side patches close to ready here but would ideally like
> to get these issues addressed before landing:
> https://github.com/mozilla/addons-server/issues/7513
> https://github.com/mozilla/addons-server/issues/7514
> https://github.com/mozilla/addons-server/issues/7534
> 
> Those are are all marked as P3.  Stuart, can you let me know when you expect
> these to get fixed on AMO so we can coordinate getting the browser-side
> changes landed vs deferring stuff to follow-up bugs?  If it would be easier
> to go over this interactively, we can chat on IRC tomorrow...

Looks like those issues have either been handled or are in progress whilst I was on PTO.
Flags: needinfo?(scolville)
Attachment #8952901 - Flags: review?(kmaglione+bmo)
Attachment #8952902 - Flags: review?(kmaglione+bmo)
Attachment #8952903 - Flags: review?(kmaglione+bmo)
Comment on attachment 8952901 [details]
Bug 1402064 AddonRepository spring cleaning

https://reviewboard.mozilla.org/r/222144/#review228732

::: toolkit/mozapps/extensions/content/extensions.js:2860
(Diff revision 1)
>        sizeRow.value = formatted;
>      } else {
>        sizeRow.value = null;
>      }
>  
> -    var downloadsRow = document.getElementById("detail-downloads");
> +    var canUpdate = !aIsRemote && hasPermission(aAddon, "upgrade") && aAddon.id != AddonManager.hotfixID;

Why `id != hotfixID`?

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:187
(Diff revision 1)
>    /**
>     * The developer comments for the add-on. This includes any information
>     * that may be helpful to end users that isn't necessarily applicable to
>     * the add-on description (e.g. known major bugs)
>     */
>    developerComments: null,

Can remove, I believe.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:241
(Diff revision 1)
>    /**
>     * The URL to visit in order to purchase the add-on
>     */
>    purchaseURL: null,
>  
>    /**
>     * The numerical cost of the add-on in some currency, for sorting purposes
>     * only
>     */
>    purchaseAmount: null,
>  
>    /**
>     * The display cost of the add-on, for display purposes only
>     */
>    purchaseDisplayAmount: null,

Can remove.
Attachment #8952901 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8952902 [details]
Bug 1402064 Refactor compatibility overrides

https://reviewboard.mozilla.org/r/222146/#review228736
Attachment #8952902 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8952903 [details]
Bug 1402064 Switch to modern AMO metadata API

https://reviewboard.mozilla.org/r/222148/#review228744

I think this all makes sense, but reviewboard does not make this easy to follow...

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:312
(Diff revision 1)
> -   * searchFailed - Called when an error occurred when performing a search.
> +   * @param  aSearchTerms
> +   *         Search terms used to search the repository
>     */
> -  _callback: null,
> +  getSearchURL(aSearchTerms) {
> +    let url = this._formatURLPref(PREF_GETADDONS_BROWSESEARCHRESULTS, {
> +      TERMS: encodeURIComponent(aSearchTerms)

`_formatURLPref` should really take care of the escapng itself...

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:321
(Diff revision 1)
>  
> -  // Maximum number of results to return
> -  _maxResults: null,
> +  /**
> +   * Whether caching is currently enabled
> +   */
> +  get cacheEnabled() {
> +    let preference = PREF_GETADDONS_CACHE_ENABLED;

This seems... super enterprisey.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:334
(Diff revision 1)
>    metadataAge() {
>      let now = Math.round(Date.now() / 1000);
> -
>      let lastUpdate = Services.prefs.getIntPref(PREF_METADATA_LASTUPDATE, 0);
> -
> +    return Math.max(0, now - lastUpdate);
> -    // Handle clock jumps
> -    if (now < lastUpdate) {
> -      return now;
> -    }
> -    return now - lastUpdate;

This should probably go in the spring cleaning commit.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:445
(Diff revision 1)
> +    let results = [];
>  
> -    let addonsToCache = await getAddonsToCache(ids);
> +    const fetchNextPage = (url) => {
> +      return new Promise((resolve, reject) => {
> +        let request = new ServiceRequest();
> +        request.mozBackgroundRequest = true;

`ServiceRequest` is supposed to do this on its own...

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:464
(Diff revision 1)
> -      return;
> +            return;
> -    }
> +          }
>  
> -    await new Promise((resolve, reject) =>
> -      this._beginGetAddons(addonsToCache, {
> -        searchSucceeded: aAddons => {
> +          try {
> +            let newResults = handler(response.results).filter(e => ids.includes(e.id));
> +            results = results.concat(...newResults);

Pretty sure this is not what you want... Assuming none of the `newResults` values is an array, this is just a much slower version of `results = results.concat(newResults)` or `results.push(...newResults)`. If any of them is an array, it's different, but probably wrong.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:517
(Diff revision 1)
> -  /**
> -   * Cancels the search in progress. If there is no search in progress this
> -   * does nothing.
> -   */
> -  cancelSearch() {
> -    this._searching = false;
> +    let overridesPromise = this._fetchPaged(aIDs, PREF_COMPAT_OVERRIDES,
> +                                            results => results.map(
> +                                              entry => this._parseCompatEntry(entry)));
> +    let addons = [], overrides = [];
> +    try {
> +      [addons, overrides] = await Promise.all([metadataPromise, overridesPromise]);

No need for a Promise.all here. You can just await them one at a time:

    try {
      let addons = await metadataPromise;
      let overrides = await overridesPromise;
      return {addons, overrides};
    } catch (e) {
      return {addons: [], overrides: []};
    }

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:598
(Diff revision 1)
> +  _parseAddon(aEntry) {
> +    let addon = new AddonSearchResult(aEntry.guid);
>  
> -            if (previewNode.getAttribute("primary") == 1)
> -              addon.screenshots.unshift(screenshot);
> -            else
> +    addon.name = aEntry.name;
> +    if (typeof aEntry.current_version == "object") {
> +      addon.version = aEntry.current_version.version;

Nit: `version = String(...)`

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:603
(Diff revision 1)
> -            else
> -              addon.screenshots.push(screenshot);
> -          }
> -          break;
> -        case "learnmore":
> -          addon.learnmoreURL = this._getTextContent(node);
> +      addon.version = aEntry.current_version.version;
> +      if (Array.isArray(aEntry.current_version.files)) {
> +        for (let file of aEntry.current_version.files) {
> +          if (file.platform == "all" || file.platform == Services.appinfo.OS.toLowerCase()) {
> +            addon.sourceURI = NetUtil.newURI(file.url);
> +            addon.size = file.size;

Nit: `= Number(file.size)`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3931
(Diff revision 1)
>        function notifyComplete() {
>          if (--pending == 0)
>            aCallback();
>        }
>  
>        for (let addon of aAddons) {
> -        AddonRepository.getCachedAddonByID(addon.id, aRepoAddon => {
> -          if (aRepoAddon) {
> +        AddonRepository.getCachedAddonByID(addon.id).then(aRepoAddon => {
> +          if (aRepoAddon || AddonRepository.getCompatibilityOverridesSync(addon.id)) {
>              logger.debug("updateAddonRepositoryData got info for " + addon.id);
>              addon._repositoryAddon = aRepoAddon;
>              this.updateAddonDisabledState(addon);
>            }
>  
>            notifyComplete();

:( We should really use some sort of Promise.all() thing here...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5757
(Diff revision 1)
> -["fullDescription", "developerComments", "eula", "supportURL",
> +["fullDescription", "developerComments", "supportURL",
>   "contributionURL", "averageRating", "reviewCount",
> - "reviewURL"].forEach(function(aProp) {
> + "reviewURL", "weeklyDownloads"].forEach(function(aProp) {

Should go in the cleanup commit, I think.

::: toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_paging.js:70
(Diff revision 1)
> +  Assert.equal(addons.length, MAX_ADDON + 1);
> +  for (let i = 0; i <= MAX_ADDON; i++) {
> +    Assert.equal(addons[i].name, name(i));
> +    Assert.equal(addons[i].id, id(i));
> +    Assert.equal(addons[i].description, summary(i));
> +    Assert.equal(addons[i].fullDescription, description(i));

Nit: No need for `Assert.`
Attachment #8952903 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8952901 [details]
Bug 1402064 AddonRepository spring cleaning

https://reviewboard.mozilla.org/r/222144/#review228732

> Why `id != hotfixID`?

Rebase mixup, fixed.
Comment on attachment 8952903 [details]
Bug 1402064 Switch to modern AMO metadata API

https://reviewboard.mozilla.org/r/222148/#review228744

> This seems... super enterprisey.

I think I might be missing your point here.  This is existing code that I just moved around, are you suggesting a particular change to it?
Comment on attachment 8952903 [details]
Bug 1402064 Switch to modern AMO metadata API

https://reviewboard.mozilla.org/r/222148/#review228744

> `ServiceRequest` is supposed to do this on its own...

Agreed, but filed bug 1441253 to do this separately.
Comment on attachment 8952903 [details]
Bug 1402064 Switch to modern AMO metadata API

https://reviewboard.mozilla.org/r/222148/#review228744

> I think I might be missing your point here.  This is existing code that I just moved around, are you suggesting a particular change to it?

`    return Services.prefs.getBoolPref(PREF_GETADDONS_CACHE_ENABLED, false);`
Comment on attachment 8952903 [details]
Bug 1402064 Switch to modern AMO metadata API

https://reviewboard.mozilla.org/r/222148/#review228744

> No need for a Promise.all here. You can just await them one at a time:
> 
>     try {
>       let addons = await metadataPromise;
>       let overrides = await overridesPromise;
>       return {addons, overrides};
>     } catch (e) {
>       return {addons: [], overrides: []};
>     }

I think we do need Promise.all(), if they both reject here then the second one is an uncaught rejection (which at the very least causes test failures)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e1284875ab292d4c3b307333b442598e48fc62d8 -d 80d2fbdf93c0: rebasing 449048:e1284875ab29 "Bug 1402064 AddonRepository spring cleaning r=kmag"
merging services/sync/tps/extensions/tps/resource/modules/addons.jsm
warning: conflicts while merging services/sync/tps/extensions/tps/resource/modules/addons.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ea0a25705f8
AddonRepository spring cleaning r=kmag
https://hg.mozilla.org/integration/autoland/rev/bd9ef7ab3790
Refactor compatibility overrides r=kmag
https://hg.mozilla.org/integration/autoland/rev/5dd1b3a4ea85
Switch to modern AMO metadata API r=kmag
Depends on: 1441574
Depends on: 1444063
Attached image Bug_1402064.png — 
This issue is verified as fixed on Firefox 60.0a1 (20180309003239) under Windows 7 64-bit and Mac OS X 10.13.2.

The API from Add-ons Manager, has been switched to: “https://services.addons.mozilla.org/api/v3/addons/”

Please see the attached screenshot.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Depends on: 1463201
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: