Page MenuHomePhabricator

AbuseFilter list not sorted correctly when ordered by Status
Closed, ResolvedPublic

Description

Steps to reproduce:

  • Go to Special:AbuseFilter.
  • Select "Include deleted filters" and update search.
  • Click status in the table header to order it by status.

While all enabled filters are together, disabled and deleted ones are mixed.

(screenshot link)

obrazek.png (742×246 px, 8 KB)

Event Timeline

Confirmed. The reason is pretty simple to explain: the 'status' column puts together af_enabled and af_deleted, which are completely separated in the DB. The sorting is performed on af_enabled, which of course doesn't take into account af_deleted. The solution might be to add a condition to ORDER BY so that the two values are handled separately, but this requires overriding something from parent (TablePager/IndexPager). I'll give it a look.

Change 445932 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use af_deleted as secondary sorting for af_enabled

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

Change 445932 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use af_deleted as secondary sorting for af_enabled

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

Hm, apparently the results aren't mixed anymore. However, some results are missing -- specifically, if I click "next page" on the first page I go straight to the last page. The weird thing is, apparently it was already broken without the patches above.

Ah, this is actually pretty clear. Neither af_enabled nor af_deleted are unique indexes, but IndexPager::getIndexField warns:

Needless to say, it's really not a good idea to use a non-unique index for this! That won't page right.

And that's what's happening, in fact, the URL says "&offset=1". Perhaps we should add an extra sort key (af_id), but it might not be enough.

In the meantime, some work was done in T244492 but there is also T244579.

It looks like the problem in AbuseFilter has already been reported as T49314.

Tagging for investigation and perhaps implementation

Investigation: I applied the following override in AbuseFilterPager and analyzed the queries, but it didn't work and always sorted on af_id (which is default):

public function getIndexField() {
	return array_filter(
		[
			'af_id' => [ 'af_id' ],
			'af_enabled' => [ 'af_enabled', 'af_deleted', 'af_id' ],
			'af_timestamp' => [ 'af_timestamp' ],
			'af_hidden' => [ 'af_hidden', 'af_id' ],
			'af_group' => [ 'af_group', 'af_id' ],
			'af_hit_count' => [ 'af_hit_count', 'af_id' ],
			'af_public_comments' => [ 'af_public_comments' ],
		],
		[ $this, 'isFieldSortable' ],
		ARRAY_FILTER_USE_KEY
	);
}

The problem is incompatibility of core's TablePager and IndexPager. The following is from IndexPager::__construct:

$order = $this->mRequest->getVal( 'order' );
if ( is_array( $index ) && isset( $index[$order] ) ) {
	$this->mOrderType = $order;
	$this->mIndexField = $index[$order];
	$this->mExtraSortFields = isset( $extraSort[$order] )
		? (array)$extraSort[$order]
		: [];
}

And the following from TablePager::getStartBody:

// Make table header
foreach ( $fields as $field => $name ) {
	if ( strval( $name ) == '' ) {
		$s .= Html::rawElement( 'th', [], "\u{00A0}" ) . "\n";
	} elseif ( $this->isFieldSortable( $field ) ) {
		$query = [ 'sort' => $field, 'limit' => $this->mLimit ];

TablePager generates links with sort= but IndexPager uses order= for its paging. So TablePager needs to be updated first, or we need to use a hack to set order= before IndexPager's constructor.

Not actively working on this, but I we should continue working on this task later on as part of the overhaul.

Change 810412 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/AbuseFilter@master] Special:AbuseFilter: Include primary key for unique pagination

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

Change 810412 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Special:AbuseFilter: Include primary key for unique pagination

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