Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

RevisionStore: improve error handling in newRevisionsFromBatch

Browse source 
When for some reason we can't determine the title for a revision
in the batch, this should not trigger a fatal TypeError, but handled
gracefully, with helpful information included in the error message.

Bug: T205936
Change-Id: I0c7d2c1fee03d1c9208669a9b5ad66612494a47c
This commit is contained in:
daniel 2020-04-28 21:28:59 +02:00
parent 4496576b82
commit d351bb7e39
4 changed files with 59 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
includes/Revision/RevisionStore.php
View file

@ -1591,7 +1591,7 @@ class RevisionStore
foreach ( $rows as $row ) {
if ( isset( $rowsByRevId[$row->$revIdField] ) ) {
$result->warning(
'internalerror',
'internalerror_info',
"Duplicate rows in newRevisionsFromBatch, $revIdField {$row->$revIdField}"
);
}
@ -1671,12 +1671,20 @@ class RevisionStore
true,
array_map(
function ( $row )
use ( $queryFlags, $titlesByPageKey, $result, $newRevisionRecord ) {
use ( $queryFlags, $titlesByPageKey, $result, $newRevisionRecord, $revIdField ) {
try {
if ( !isset( $titlesByPageKey[$row->_page_key] ) ) {
$result->warning(
'internalerror_info',
"Couldn't find title for rev {$row->$revIdField} "
. "(page key {$row->_page_key})"
);
return null;
}
return $newRevisionRecord( $row, null, $queryFlags,
$titlesByPageKey[ $row->_page_key ] ?? null );
$titlesByPageKey[ $row->_page_key ] );
} catch ( MWException $e ) {
$result->warning( 'internalerror', $e->getMessage() );
$result->warning( 'internalerror_info', $e->getMessage() );
return null;
}
},
@ -1712,11 +1720,19 @@ class RevisionStore
) {
if ( !isset( $slotRowsByRevId[$row->$revIdField] ) ) {
$result->warning(
'internalerror',
'internalerror_info',
"Couldn't find slots for rev {$row->$revIdField}"
);
return null;
}
if ( !isset( $titlesByPageKey[$row->_page_key] ) ) {
$result->warning(
'internalerror_info',
"Couldn't find title for rev {$row->$revIdField} "
. "(page key {$row->_page_key})"
);
return null;
}
try {
return $newRevisionRecord(
$row,
@ -1725,14 +1741,14 @@ class RevisionStore
$row->$revIdField,
$slotRowsByRevId[$row->$revIdField],
$queryFlags,
$titlesByPageKey[$row->_page_key] ?? null
$titlesByPageKey[$row->_page_key]
)
),
$queryFlags,
$titlesByPageKey[$row->_page_key]
);
} catch ( MWException $e ) {
$result->warning( 'internalerror', $e->getMessage() );
$result->warning( 'internalerror_info', $e->getMessage() );
return null;
}
},
@ -1838,7 +1854,7 @@ class RevisionStore
$slotRow->blob_data = $slotContents[$slotRow->content_address];
} else {
$result->warning(
'internalerror',
'internalerror_info',
"Couldn't find blob data for rev {$slotRow->slot_revision_id}"
);
$slotRow->blob_data = null;

48
maintenance/findBadBlobs.php
View file

@ -264,10 +264,12 @@ class FindBadBlobs extends Maintenance {
$queryInfo['joins']
);
$result = $this->revisionStore->newRevisionsFromBatch( $rows, [ 'slots' => true ] );
if ( !$result->isOK() ) {
$this->fatalError( Status::wrap( $result )->getMessage( false, false, 'en' )->text() );
}
return $result->value;
$this->handleStatus( $result );
$records = array_filter( $result->value );
'@phan-var RevisionStoreRecord[] $records';
return $records;
}
/**
@ -292,10 +294,12 @@ class FindBadBlobs extends Maintenance {
$rows,
[ 'archive' => true, 'slots' => true ]
);
if ( !$result->isOK() ) {
$this->fatalError( Status::wrap( $result )->getMessage( false, false, 'en' )->text() );
}
return $result->value;
$this->handleStatus( $result );
$records = array_filter( $result->value );
'@phan-var RevisionArchiveRecord[] $records';
return $records;
}
/**
@ -371,11 +375,10 @@ class FindBadBlobs extends Maintenance {
$result = $this->revisionStore->newRevisionsFromBatch( $rows, [ 'slots' => true ] );
if ( !$result->isOK() ) {
$this->fatalError( Status::wrap( $result )->getMessage( false, false, 'en' )->text() );
}
$this->handleStatus( $result );
$revisions = $result->value;
$revisions = array_filter( $result->value );
'@phan-var RevisionArchiveRecord[] $revisions';
// if not all revisions were found, check the archive table.
if ( count( $revisions ) < count( $ids ) ) {
@ -398,14 +401,10 @@ class FindBadBlobs extends Maintenance {
[ 'slots' => true, 'archive' => true ]
);
if ( !$archiveResult->isOK() ) {
$this->fatalError(
Status::wrap( $archiveResult )->getMessage( false, false, 'en' )->text()
);
}
$this->handleStatus( $archiveResult );
// don't use array_merge, since it will re-index
$revisions = $revisions + $archiveResult->value;
$revisions = $revisions + array_filter( $archiveResult->value );
}
return $revisions;
@ -497,6 +496,19 @@ class FindBadBlobs extends Maintenance {
return $this->lbFactory->waitForReplication();
}
private function handleStatus( StatusValue $status ) {
if ( !$status->isOK() ) {
$this->fatalError(
Status::wrap( $status )->getMessage( false, false, 'en' )->text()
);
}
if ( !$status->isGood() ) {
$this->error(
"\t! " . Status::wrap( $status )->getMessage( false, false, 'en' )->text()
);
}
}
}
$maintClass = FindBadBlobs::class;

8
tests/phpunit/includes/Revision/McrRevisionStoreDbTest.php
View file

@ -205,7 +205,7 @@ class McrRevisionStoreDbTest extends RevisionStoreDbTestBase {
$contentAddress = $rev1->getRevisionRecord()->getSlot( SlotRecord::MAIN )->getAddress();
$blobStatus = StatusValue::newGood( [] );
$blobStatus->warning( 'internalerror', 'oops!' );
$blobStatus->warning( 'internalerror_info', 'oops!' );
$mockBlobStore = $this->createMock( BlobStore::class );
$mockBlobStore->method( 'getBlobBatch' )
@ -230,14 +230,14 @@ class McrRevisionStoreDbTest extends RevisionStoreDbTestBase {
$this->assertSame( [
[
'type' => 'warning',
'message' => 'internalerror',
'message' => 'internalerror_info',
'params' => [
"oops!"
]
],
[
'type' => 'warning',
'message' => 'internalerror',
'message' => 'internalerror_info',
'params' => [
"Couldn't find blob data for rev " . $rev1->getId()
]
@ -322,7 +322,7 @@ class McrRevisionStoreDbTest extends RevisionStoreDbTestBase {
$this->assertNull( $records[$invalidRow->rev_id] );
$this->assertSame( [ [
'type' => 'warning',
'message' => 'internalerror',
'message' => 'internalerror_info',
'params' => [
"Couldn't find slots for rev 100500"
]

2
tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
View file

@ -2398,7 +2398,7 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
->newRevisionsFromBatch( [ $this->revisionToRow( $rev1 ), $this->revisionToRow( $rev1 ) ] );
$this->assertFalse( $status->isGood() );
$this->assertTrue( $status->hasMessage( 'internalerror' ) );
$this->assertTrue( $status->hasMessage( 'internalerror_info' ) );
}
/**