Page MenuHomePhabricator

AbuseFilter should have an edit API
Open, Stalled, MediumPublic

Description

AbuseFilter has a query API for getting filter information, but no way to modify filters. This would be useful, for example, for semi-automated replacement of deprecated variables.

Event Timeline

Tgr created this task.Jan 6 2019, 9:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 6 2019, 9:46 PM
MusikAnimal added a subscriber: MusikAnimal.EditedFeb 17 2019, 4:45 AM

Another use-case is https://en.wikipedia.org/wiki/Special:AbuseFilter/944. It targets articles that are linked to on the Main Page, so it needs to be updated manually everyday. It would be to awesome have a bot do this for us!

Huji awarded a token.Feb 17 2019, 4:55 PM
Huji added a subscriber: Huji.
This comment was removed by DannyS712.
Restricted Application added a project: Platform Engineering. · View Herald TranscriptAug 15 2019, 4:26 AM
DannyS712 triaged this task as Medium priority.
Restricted Application added a project: User-DannyS712. · View Herald TranscriptAug 26 2019, 8:49 AM

Preliminary API plan, which doesn't allow all of the functionality of the special page, but is the first step:
Parameters:

  • Filter: integer, required
  • Rules: text, defaults to current rules if not provided
  • Notes: text, defaults to current notes if not provided
  • Token: edit token, required

Constraints:

  • User must be able to modify filters, and if the filter is global/restricted must be able to modify global/restricted filters
  • At least one of rules or notes must be changed

To do in the future:

  • Add editing the consequences
  • Add editing the flags

@Huji @Daimona Platform Engineering Is it okay to be introducing an api module that wouldn't include everything that can be done on-wiki?

@Huji @Daimona Platform Engineering Is it okay to be introducing an api module that wouldn't include everything that can be done on-wiki?

IMHO, it depends. If it's a not-so-commonly-used functionality, and/or something which would be pretty difficult to implement, that can be OK. Although generally speaking, when introducing new code, we should always strive to keep it free of tech debt.

For this specific case, I believe editing the flags shouldn't be too hard, while editing the consequences could be tricky because the interface is pretty complicated.

At any rate, my main recommendation is to avoid duplicating any logic and refactor the existing code to be reusable, if necessary.

Huji added a comment.Aug 26 2019, 1:25 PM

I second Daimona's comment: we should try to avoid technical debt as much as possible. And that I think this could be a good reason for us to try to refactor code and make it even more modular, to reduce redundancy between future API code and web code.

Change 532674 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Mostly remove $wgUser

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

Change 532674 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Mostly remove $wgUser

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

I believe this will be mandatory for such an edit API. And there's also some coupling to remove between AbuseFilterViewEdit and AbuseFilter::saveFilter (which I'll try to resolve in a dependent patch). @DannyS712 I suggest you to rebase your patch on top of mine(s), before starting to work on that, so that it'll be easier for you to implement the module.

Change 532674 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Mostly remove $wgUser

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

I believe this will be mandatory for such an edit API. And there's also some coupling to remove between AbuseFilterViewEdit and AbuseFilter::saveFilter (which I'll try to resolve in a dependent patch). @DannyS712 I suggest you to rebase your patch on top of mine(s), before starting to work on that, so that it'll be easier for you to implement the module.

Thanks. I haven't started yet (I wanted to give a few days for the feedback I asked for in T213037#5437164) and was planning to make some wider-scale changes to avoid code duplication (introduce AbuseFilterEditor that is called from both AbuseFilterViewEdit and the API to handle checking the parameters, permissions, etc) so I'll wait until you're done with your current work. You note that there may be multiple patches - can you ping me when its safe to proceed?

! In T213037#5440654, @DannyS712 wrote:
Thanks. I haven't started yet (I wanted to give a few days for the feedback I asked for in T213037#5437164)

No hurry :)

and was planning to make some wider-scale changes to avoid code duplication (introduce AbuseFilterEditor that is called from both AbuseFilterViewEdit and the API to handle checking the parameters, permissions, etc)

Well, about that... There's also https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/480069/ which adds a brand new class representing single filters, which could help as well. Nevertheless, we do need some dedicated place to check permissions etc., since those methods cannot live in the AbuseFilter class forever.

so I'll wait until you're done with your current work. You note that there may be multiple patches - can you ping me when its safe to proceed?

Sure. I still have to think about what patches we'd need exactly before moving on, and how to organize patch dependencies. I'll definitely let you know, though you'll try to add this task number to any new related patch.

Change 532715 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove AbuseFilter::saveFilter dependency on AbuseFilterViewEdit

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

Change 532674 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Mostly remove $wgUser

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

So, my next step is finishing https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/480069/, then remove some debt in a follow-up. I believe your patch should be rebased either on r480069 itself, or its follow-up (which I'll hopefully write tomorrow).

Tgr added a comment.Aug 27 2019, 7:27 PM

One option would be to wait for the REST API work to become stable and then use that. Dealing with complicated options is a lot easier in REST.

Anomie added a subscriber: Anomie.Aug 27 2019, 7:37 PM

I second Daimona's comment: we should try to avoid technical debt as much as possible. And that I think this could be a good reason for us to try to refactor code and make it even more modular, to reduce redundancy between future API code and web code.

I'll third that. Even if an initial patch leaves some functionality out, the design should include all the functionality to try to avoid creating an interface that can't represent the additional functionality.

One option would be to wait for the REST API work to become stable and then use that. Dealing with complicated options is a lot easier in REST.

What's so complicated? The web UI form looks pretty straightforward to me. There's (relatively) a lot of options, but nothing much complicated about them.

Tgr added a comment.Aug 27 2019, 9:24 PM

What's so complicated? The web UI form looks pretty straightforward to me. There's (relatively) a lot of options, but nothing much complicated about them.

Mainly due to the throttling option, which takes a limit, a time and a group list. You could make those all separate parameters, I guess, so it's no so much complicated as ugly.

(In general, I think all new APIs should be REST APIs unless they involve batching (ie. action=query APIs) once the REST API is feature-complete and stable; it's just a paradigm that's much easier understood by developers, not to mention the caching implications. Not sure when that's planned, but I imagine within a few months? So maybe worth waiting.)

Mainly due to the throttling option, which takes a limit, a time and a group list. You could make those all separate parameters, I guess, so it's no so much complicated as ugly.

Doesn't seem complicated or ugly to me. Three values in three parameters is very straightforward.

(In general, I think all new APIs should be REST APIs unless they involve batching (ie. action=query APIs) once the REST API is feature-complete and stable; it's just a paradigm that's much easier understood by developers, not to mention the caching implications. Not sure when that's planned, but I imagine within a few months? So maybe worth waiting.)

I strongly disagree. We have a huge user base for the Action API and I see no reason not to continue to support them with new features.

Only the R in CRUD benefits from caching. Here we're talking about C, U, and D.

As I've said elsewhere, if you do your architecture right it should be pretty simple to support both in most cases if you want: all the backend logic in one place, and the web UI page, Action API module, and REST API handler all are mainly concerned with extracting parameters and formatting output.

Change 480069 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add a new class representing single filters

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

Change 532715 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Remove AbuseFilter::saveFilter dependency on AbuseFilterViewEdit

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

eprodromou added a subscriber: eprodromou.

Looks like @DannyS712 got the answer he needed so I'm untagging CPT.

DannyS712 removed DannyS712 as the assignee of this task.Sep 18 2020, 8:48 PM

Not currently working on this, maybe @Daimona would be willing to add it to #AbuseFilter (Overhaul-2020) ? (can't seem to get it to link)

Not currently working on this, maybe @Daimona would be willing to add it to #AbuseFilter (Overhaul-2020) ? (can't seem to get it to link)

I think this is going to be quite easy to implement after the architecture review part. Unsure if there are resources for the actual implementation of the module as part of the project.

Change 628494 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Avoid depending on ContextSource in saveFilter

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

Change 480069 abandoned by Daimona Eaytoy:
[mediawiki/extensions/AbuseFilter@master] Add a new class representing single filters

Reason:
This is not aligned with current plans. I5f33227887c035e301313bbe24d1c1fefb75bc6a is a new beginning in this direction.

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

Change 628494 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Reduce dependencies of AbuseFilter::saveFilter

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

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:02 PM