Page MenuHomePhabricator

Split FilterRunner
Open, Needs TriagePublic

Description

During the overhaul, the FilterRunner class has become the core of AbuseFilter. While most of the code has been moved to standalone services, the interface is not clean enough.

Proposed organization using inheritance:

class BaseFilterRunner {
  public run() : Status {
    $result = $this->getResult();
    return $this->takeConsequences( $result );
  }
  protected getResult();
  abstract protected takeConsequences( $result ) : Status;
}
  • getResult() runs filters and returns information about filter matches, profiling, etc.
  • takeConsequences() gets the result and takes consequences.

The new classes will be:

class FilterRunner extends BaseFilterRunner {
  protected takeConsequences( $result ) : Status {
    // run Consequences, log matches and profiling data
  }
}

class EditStashAwareFilterRunner extends FilterRunner {
  protected EditStashCache $cache;
  protected getResult() {
    $result = $this->cache->seek();
    if ( $result === false ) {
      $result = parent::getResult();
    }
    return $result;
  }
}

class ForStashFilterRunner extends BaseFilterRunner {
  protected EditStashCache $cache;
  protected takeConsequences( $result ) : Status {
    $this->cache->save( $result );
    return Status::newGood();
  }
}

FilterRunnerFactory will provide newRunner, newForStashRunner, etc. methods.
Each hook will request the appropriate runner from the factory.
All of this gets its own namespace.

Event Timeline

I've also been having this idea for a while, although I still didn't think about a possible implementation, mainly because the stash-related code is still coupled with the non-stash code (r654181 should resolve this).

Overall, I agree with your proposal. A few remarks/ideas:

  • I'd like a narrower interface to account for runners that do not take consequences (e.g. ForStashFilterRunner). I guess we can just move takeConsequences() to a separate interface and have FilterRunner implement it;
  • As a consequence of the previous bullet, I'd suggest having just run() as an abstract method in the base interface
  • As a consequence of the previous 2 bullets, I think that BaseFilterRunner should be an abstract class if there remains some methods that we want to share amongst all children, and an interface otherwise
  • This is unrelated to the splitting itself, but I think FilterProfiler might be invoked as a Watcher, to reduce coupling (either make FilterProfiler implement watcher with some adjustments, or create a new watcher that would act as proxy for FilterProfiler)

@matej_suchanek Have you already written some code, or may I write and push a POC on gerrit?

Well, I had some dirty untested stuff before I created the task, but don't have time to rebase it now. Feel free to go ahead.

Well, I had some dirty untested stuff before I created the task, but don't have time to rebase it now. Feel free to go ahead.

Sure, no problem. Actually, no worry, I'd like to focus on another couple of things first (mainly tests).

Change 654181 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/AbuseFilter@master] Introduce EditStashCache

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

Change 662066 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/AbuseFilter@master] [POC] Split FilterRunner

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

Change 654181 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Introduce EditStashCache

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