Page MenuHomePhabricator

[API] Inconsistencies in response of `list=wbsubscribers` API Query module
Closed, ResolvedPublic3 Estimated Story Points

Description

Problem:

Calling the action=query&list=wbsubscribers API endpoint on a Wikibase returns a response with two different keys that could potentially contain results: subscribers and wbsubscribers. In addition, the XML response format is not structured as expected.

Examples:

Calling https://www.wikidata.org/w/api.php?action=query&list=wbsubscribers&wblsentities=L1-S1 results in:

JSON
{
    "batchcomplete": "",
    "query": {
        "wbsubscribers": {},
        "subscribers": {
            "L1-S1": {
                "subscribers": [
                    {
                        "site": "wikidatawiki"
                    }
                ]
            }
        }
    }
}
XML
<?xml version="1.0"?>
<api batchcomplete="">
  <query>
    <wbsubscribers />
    <subscribers>
      <L1-S1>
        <subscribers>
          <subscriber site="wikidatawiki" />
        </subscribers>
      </L1-S1>
    </subscribers>
  </query>
</api>

Desired behavior:

The results are added to only one key (either subscribers OR wbsubscribers), and in the correct XML structure.

<?xml version="1.0"?>
<api batchcomplete="">
  <query>
    <subscribers>
      <entity id="L1-S1">
        <subscribers>
          <subscriber site="wikidatawiki" />
        </subscribers>
      </entity>
    </subscribers>
  </query>
</api>

Notes:

This behavior is essentially caused by a typo in the code that sets up the API module. Essentially, it sets up $this->getModuleName() with wbsubscribers:

ListSubscribers::formatResult()
$result->addIndexedTagName( [ 'query', $this->getModuleName() ], 'entity' );
$result->addArrayType( [ 'query', $this->getModuleName() ], 'kvp', 'id' );

But then adds values under subscribers, without the wb:

$fit = $result->addValue( [ 'query', 'subscribers' ], $row->cs_entity_id, $entry );
NOTE: This is a breaking change. Announcement is being prepared here.

Event Timeline

I just happened to notice this while looking through some APIs during work on m3api. I’m not really sure what to do with this task, to be honest – it’s an easy fix, but still an API breaking change, which I’m not sure is worth the effort. Is this module even used a lot? (As far as I can tell, we only have usage statistics for main API modules in Grafana, not submodules like query+wbsubscribers.)

Prio Notes

  • Affects end users / production
  • Does not affect development efforts
  • Does not affect onboarding efforts
  • Does not affect additional stakeholders

Task Triage Notes:

  • After discussion with @Lydia_Pintscher & @Manuel We decided that we do not have sufficient information regarding the scope of impact of this change, therefore, we will wait for usage statistics of the API endpoint.

Is this module even used a lot?

In the last 30 days ~45K requests were made to ?action=query&list=wbsubscribers, most of them likely by one Python script (see T329637).

ItamarWMDE renamed this task from list=wbsubscribers API uses output inconsistently to [SW] list=wbsubscribers API uses output inconsistently.Feb 15 2023, 8:59 AM
ItamarWMDE moved this task from Radar to [DOT] Prioritized on the wmde-wikidata-tech board.

Story Writing Notes:

  • Rephrase this to a problem statement / bug description format

I think this should be a relatively painless breaking API change for users: in the response they receive, they can check if the wbsubscribers output is nonempty, and use it in that case; if it’s empty, fall back to subscribers. Then they don’t need to care when exactly we deploy the change.

(We could even add the same output under both names for a while, but I’m not sure that makes anything better, actually. And it introduces the risk of making the API response too large.)

ItamarWMDE renamed this task from [SW] list=wbsubscribers API uses output inconsistently to Inconsistencies in response of `list=wbsubscribers` API Query module.Feb 21 2023, 9:10 AM
ItamarWMDE updated the task description. (Show Details)
ItamarWMDE renamed this task from Inconsistencies in response of `list=wbsubscribers` API Query module to [API] Inconsistencies in response of `list=wbsubscribers` API Query module.Mar 14 2023, 1:31 PM

Task Breakdown Notes

  • Use wbsubscribers as main key and update subscribers to wbsubscribers in the code

Potential Plan of Action

  • Update ListSubscribers::formatResult() accordingly to remove subscribers and use wbsubscribers as the main key
  • Only merge the patch after the announcement is sent

Change 910003 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/910003

Lucas_Werkmeister_WMDE added a subscriber: Michael.

Moving to Waiting under the assumption that @Michael’s +1 on the change is sufficient to merge this once it’s ready to be deployed.

Change 914298 had a related patch set uploaded (by Michael Große; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@wmf/1.41.0-wmf.7] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/914298

I think this should be a relatively painless breaking API change for users: in the response they receive, they can check if the wbsubscribers output is nonempty, and use it in that case; if it’s empty, fall back to subscribers. Then they don’t need to care when exactly we deploy the change.

@Michael pointed out that this doesn’t really comply with the notification policy of the stable interface policy: the breaking change needs to be testable on Test Wikidata for two weeks before it happens on Wikidata. So we need a config flag in Wikibase after all. (API users can still write their code in a way that’s compatible with either response, as outlined above, but we need to enable them to test their code against the new response before it’s deployed on Wikidata.)

Change 914298 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@wmf/1.41.0-wmf.7] Fix output path of list=wbsubscribers API

Reason:

the master change needs some more work (feature flag), we’ll make a new cherry pick once it’s merged

https://gerrit.wikimedia.org/r/914298

Change 914752 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[operations/mediawiki-config@master] Make wbsubscribers API output sensible on Test Wikidata

https://gerrit.wikimedia.org/r/914752

Change 910003 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/910003

Change 914298 restored by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@wmf/1.41.0-wmf.7] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/914298

Change 915384 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@wmf/1.41.0-wmf.6] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/915384

Change 914298 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.41.0-wmf.7] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/914298

Mentioned in SAL (#wikimedia-operations) [2023-05-04T11:36:08Z] <lucaswerkmeister-wmde@deploy1002> Started scap: Backport for [[gerrit:914298|Fix output path of list=wbsubscribers API (T300458)]]

Mentioned in SAL (#wikimedia-operations) [2023-05-04T11:38:09Z] <lucaswerkmeister-wmde@deploy1002> lucaswerkmeister-wmde and migr: Backport for [[gerrit:914298|Fix output path of list=wbsubscribers API (T300458)]] synced to the testservers: mwdebug2002.codfw.wmnet, mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-05-04T11:44:32Z] <lucaswerkmeister-wmde@deploy1002> Finished scap: Backport for [[gerrit:914298|Fix output path of list=wbsubscribers API (T300458)]] (duration: 08m 24s)

Change 915384 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.41.0-wmf.6] Fix output path of list=wbsubscribers API

https://gerrit.wikimedia.org/r/915384

Mentioned in SAL (#wikimedia-operations) [2023-05-04T12:03:15Z] <lucaswerkmeister-wmde@deploy1002> Started scap: Backport for [[gerrit:915384|Fix output path of list=wbsubscribers API (T300458)]]

Mentioned in SAL (#wikimedia-operations) [2023-05-04T12:04:44Z] <lucaswerkmeister-wmde@deploy1002> lucaswerkmeister-wmde: Backport for [[gerrit:915384|Fix output path of list=wbsubscribers API (T300458)]] synced to the testservers: mwdebug1002.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-05-04T12:10:58Z] <lucaswerkmeister-wmde@deploy1002> Finished scap: Backport for [[gerrit:915384|Fix output path of list=wbsubscribers API (T300458)]] (duration: 07m 43s)

Change 914752 merged by jenkins-bot:

[operations/mediawiki-config@master] Make wbsubscribers API output sensible on Test Wikidata

https://gerrit.wikimedia.org/r/914752

Mentioned in SAL (#wikimedia-operations) [2023-05-04T13:45:04Z] <lucaswerkmeister-wmde@deploy1002> Started scap: Backport for [[gerrit:914752|Make wbsubscribers API output sensible on Test Wikidata (T300458)]]

Mentioned in SAL (#wikimedia-operations) [2023-05-04T13:47:27Z] <lucaswerkmeister-wmde@deploy1002> lucaswerkmeister-wmde: Backport for [[gerrit:914752|Make wbsubscribers API output sensible on Test Wikidata (T300458)]] synced to the testservers: mwdebug2002.codfw.wmnet, mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-05-04T13:54:56Z] <lucaswerkmeister-wmde@deploy1002> Finished scap: Backport for [[gerrit:914752|Make wbsubscribers API output sensible on Test Wikidata (T300458)]] (duration: 09m 52s)

Moving back to Waiting; config change to change the flag on Wikidata (doesn’t exist yet) can be deployed on 18 May or later.