Gergő Tisza has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/387185 )
Change subject: Fix deleted row handling
......................................................................
Fix deleted row handling
Add a number of missing checks against performing operations on
deleted lists, or deleted list entries, or entries of deleted lists.
The old logic tried to avoid the extra SELECT needed for those
checks by cramming all the check logic into the INSERT/UPDATE/DELETE
conditions and only doing explicit checks when the write failed.
This was unwieldy and the performance gain was unlikely to be
significant, so it is now replaced with explicit checks.
Bug: T177853
Change-Id: If8db3dee951be7038bb104eaf3a7c9f58b5f1723
---
M src/ReadingListRepository.php
M tests/src/ReadingListRepositoryTest.php
2 files changed, 201 insertions(+), 130 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/ReadingLists
refs/changes/85/387185/1
diff --git a/src/ReadingListRepository.php b/src/ReadingListRepository.php
index c23eb9c..a294d42 100644
--- a/src/ReadingListRepository.php
+++ b/src/ReadingListRepository.php
@@ -11,6 +11,7 @@
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
use Wikimedia\Rdbms\IDatabase;
+use Wikimedia\Rdbms\IResultWrapper;
use Wikimedia\Rdbms\LBFactory;
/**
@@ -132,6 +133,7 @@
/**
* Check whether reading lists have been set up for the given user (ie.
setupForUser() was
* called with $userId and teardownForUser() was not called with the
same id afterwards).
+ * Optionally also lock it.
* @param int $flags IDBAccessObject flags
* @throws ReadingListRepositoryException
* @return bool
@@ -249,6 +251,7 @@
$id, $name = null, $description = null, $color = null, $image =
null, $icon = null
) {
$this->assertUser();
+ $this->selectValidList( $id, self::READ_LOCKING );
$data = array_filter( [
'rl_name' => $name,
@@ -264,29 +267,9 @@
$this->dbw->update(
'reading_list',
$data,
- [
- 'rl_id' => $id,
- 'rl_user_id' => $this->userId,
- ],
- __METHOD__
+ [ 'rl_id' => $id ]
);
- if ( $this->dbw->affectedRows() ) {
- return;
- }
-
- // failed; see what went wrong so we can return a useful error
message
- /** @var ReadingListRow $row */
- $row = $this->dbw->selectRow(
- 'reading_list',
- [ 'rl_user_id' ],
- [ 'rl_id' => $id ],
- __METHOD__
- );
- if ( !$row ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list', [ $id ] );
- } elseif ( $row->rl_user_id != $this->userId ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list', [ $id ] );
- } else {
+ if ( !$this->dbw->affectedRows() ) {
throw new LogicException( 'updateList failed for
unknown reason' );
}
}
@@ -299,6 +282,10 @@
*/
public function deleteList( $id ) {
$this->assertUser();
+ $row = $this->selectValidList( $id, self::READ_LOCKING );
+ if ( $row->rl_is_default ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-cannot-delete-default-list' );
+ }
$this->dbw->update(
'reading_list',
@@ -306,39 +293,16 @@
'rl_deleted' => 1,
'rl_date_updated' => $this->dbw->timestamp(),
],
- [
- 'rl_id' => $id,
- 'rl_user_id' => $this->userId,
- // cannot delete the default list
- 'rl_is_default' => 0,
- ],
- __METHOD__
+ [ 'rl_id' => $id ]
);
- if ( $this->dbw->affectedRows() ) {
- $this->logger->info( 'Deleted list {list} for user
{user}', [
- 'list' => $id,
- 'user' => $this->userId,
- ] );
- return;
- }
-
- // failed; see what went wrong so we can return a useful error
message
- /** @var ReadingListRow $row */
- $row = $this->dbw->selectRow(
- 'reading_list',
- [ 'rl_user_id', 'rl_is_default' ],
- [ 'rl_id' => $id ],
- __METHOD__
- );
- if ( !$row ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list', [ $id ] );
- } elseif ( $row->rl_user_id != $this->userId ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list', [ $id ] );
- } elseif ( $row->rl_is_default ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-cannot-delete-default-list' );
- } else {
+ if ( !$this->dbw->affectedRows() ) {
throw new LogicException( 'deleteList failed for
unknown reason' );
}
+
+ $this->logger->info( 'Deleted list {list} for user {user}', [
+ 'list' => $id,
+ 'user' => $this->userId,
+ ] );
}
// list entry CRUD
@@ -353,23 +317,7 @@
*/
public function addListEntry( $id, $project, $title ) {
$this->assertUser();
-
- // verify that the list exists and we have access to it
- /** @var ReadingListRow $row */
- $row = $this->dbw->selectRow(
- 'reading_list',
- 'rl_user_id',
- [
- 'rl_id' => $id,
- ],
- __METHOD__,
- [ 'FOR UPDATE' ]
- );
- if ( !$row ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list', [ $id ] );
- } elseif ( $row->rl_user_id != $this->userId ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list', [ $id ] );
- }
+ $this->selectValidList( $id, self::READ_LOCKING );
// due to the combination of soft deletion + unique constraint
on
// rle_rl_id + rle_project + rle_title, recreation needs
special handling
@@ -414,6 +362,10 @@
} else {
throw new ReadingListRepositoryException(
'readinglists-db-error-duplicate-page' );
}
+ if ( !$this->dbw->affectedRows() ) {
+ throw new LogicException( 'addListEntry failed for
unknown reason' );
+ }
+
$insertId = $row ? (int)$row->rle_id : $this->dbw->insertId();
$this->logger->info( 'Added entry {entry} for user {user}', [
'entry' => $insertId,
@@ -493,41 +445,44 @@
public function deleteListEntry( $id ) {
$this->assertUser();
+ /** @var ReadingListRow|ReadingListEntryRow $row */
+ $row = $this->dbw->selectRow(
+ [ 'reading_list', 'reading_list_entry' ],
+ [ 'rl_id', 'rl_user_id', 'rl_deleted', 'rle_deleted' ],
+ [
+ 'rle_id' => $id,
+ 'rl_id = rle_rl_id',
+ ],
+ __METHOD__,
+ [ 'FOR UPDATE' ]
+ );
+ if ( !$row ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list-entry', [ $id ] );
+ } elseif ( $row->rl_user_id != $this->userId ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list-entry', [ $id ] );
+ } elseif ( $row->rl_deleted ) {
+ throw new ReadingListRepositoryException(
+ 'readinglists-db-error-list-deleted', [
$row->rl_id ] );
+ } elseif ( $row->rle_deleted ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-list-entry-deleted', [ $id ] );
+ }
+
$this->dbw->update(
'reading_list_entry',
[
'rle_deleted' => 1,
'rle_date_updated' => $this->dbw->timestamp(),
],
- [
- 'rle_id' => $id,
- 'rle_user_id' => $this->userId,
- ],
- __METHOD__
+ [ 'rle_id' => $id ]
);
- if ( $this->dbw->affectedRows() ) {
- $this->logger->info( 'Deleted entry {entry} for user
{user}', [
- 'entry' => $id,
- 'user' => $this->userId,
- ] );
- return;
- }
-
- // failed; see what went wrong so we can return a useful error
message
- /** @var ReadingListEntryRow $row */
- $row = $this->dbw->selectRow(
- 'reading_list_entry',
- [ 'rle_user_id' ],
- [ 'rle_id' => $id ],
- __METHOD__
- );
- if ( !$row ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list-entry', [ $id ] );
- } elseif ( $row->rle_user_id != $this->userId ) {
- throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list-entry', [ $id ] );
- } else {
+ if ( !$this->dbw->affectedRows() ) {
throw new LogicException( 'deleteListEntry failed for
unknown reason' );
}
+
+ $this->logger->info( 'Deleted entry {entry} for user {user}', [
+ 'entry' => $id,
+ 'user' => $this->userId,
+ ] );
}
// sorting
@@ -635,13 +590,13 @@
*/
public function getListEntryOrder( $id ) {
$this->assertUser();
+ $this->selectValidList( $id );
$ids = $this->dbr->selectFieldValues(
[ 'reading_list_entry', 'reading_list_entry_sortkey' ],
'rle_id',
[
'rle_rl_id' => $id,
- 'rle_user_id' => $this->userId,
'rle_deleted' => 0,
],
__METHOD__,
@@ -652,25 +607,6 @@
'reading_list_entry_sortkey' => [ 'LEFT JOIN',
'rle_id = rles_rle_id' ],
]
);
-
- if ( !$ids ) {
- /** @var ReadingListRow $row */
- $row = $this->dbr->selectRow(
- 'reading_list',
- [ 'rl_user_id', 'rl_deleted' ],
- [ 'rl_id' => $id ]
- );
- if ( !$row ) {
- throw new ReadingListRepositoryException(
- 'readinglists-db-error-no-such-list', [
$row->rl_id ] );
- } elseif ( $row->rl_user_id != $this->userId ) {
- throw new ReadingListRepositoryException(
- 'readinglists-db-error-not-own-list', [
$row->rl_id ] );
- } elseif ( $row->rl_deleted ) {
- throw new ReadingListRepositoryException(
- 'readinglists-db-error-list-deleted', [
$row->rl_id ] );
- }
- }
return array_map( 'intval', $ids );
}
@@ -684,16 +620,10 @@
*/
public function setListEntryOrder( $id, array $order ) {
$this->assertUser();
+ $this->selectValidList( $id, self::READ_LOCKING );
if ( !$order ) {
throw new ReadingListRepositoryException(
'readinglists-db-error-empty-order' );
}
- $this->dbw->select(
- 'reading_list',
- '1',
- [ 'rl_id' => $id ],
- __METHOD__,
- [ 'FOR UPDATE' ]
- );
// Make sure the list entries exist and the user owns them.
$res = $this->dbw->select(
@@ -1017,4 +947,34 @@
}
}
+ /**
+ * Get list data, and optionally lock the list.
+ * List must exist, belong to the current user and not be deleted.
+ * @param int $id List id
+ * @param int $flags IDBAccessObject flags
+ * @return ReadingListRow
+ * @throws ReadingListRepositoryException
+ */
+ private function selectValidList( $id, $flags = 0 ) {
+ $this->assertUser();
+ list( $index, $options ) = DBAccessObjectUtils::getDBOptions(
$flags );
+ $db = ( $index === DB_MASTER ) ? $this->dbw : $this->dbr;
+ /** @var ReadingListRow $row */
+ $row = $db->selectRow(
+ 'reading_list',
+ array_merge( $this->getListFields(), [ 'rl_user_id' ] ),
+ [ 'rl_id' => $id ],
+ __METHOD__,
+ $options
+ );
+ if ( !$row ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list', [ $id ] );
+ } elseif ( $row->rl_user_id != $this->userId ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list', [ $id ] );
+ } elseif ( $row->rl_deleted ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-list-deleted', [ $id ] );
+ }
+ return $row;
+ }
+
}
diff --git a/tests/src/ReadingListRepositoryTest.php
b/tests/src/ReadingListRepositoryTest.php
index dfb3071..6747f63 100644
--- a/tests/src/ReadingListRepositoryTest.php
+++ b/tests/src/ReadingListRepositoryTest.php
@@ -290,7 +290,7 @@
public function testUpdateList() {
$repository = new ReadingListRepository( 1, $this->db,
$this->db, $this->lbFactory );
$repository->setupForUser();
- list( $listId ) = $this->addLists( 1, [
+ list( $listId, $deletedListId ) = $this->addLists( 1, [
[
'rl_name' => 'foo',
'rl_description' => 'xxx',
@@ -301,6 +301,17 @@
'rl_icon' => 'ICON',
'rl_deleted' => '0',
'rls_index' => 1,
+ ],
+ [
+ 'rl_name' => 'bar',
+ 'rl_description' => 'yyy',
+ 'rl_date_created' => '20100101000000',
+ 'rl_date_updated' => '20120101000000',
+ 'rl_color' => 'blue',
+ 'rl_image' => 'image',
+ 'rl_icon' => 'ICON',
+ 'rl_deleted' => '1',
+ 'rls_index' => 2,
],
] );
@@ -335,16 +346,26 @@
$repository->updateList( $listId, 'foo' );
}
);
+ $this->assertFailsWith( 'readinglists-db-error-list-deleted',
+ function () use ( $repository, $deletedListId ) {
+ $repository->updateList( $deletedListId, 'bar'
);
+ }
+ );
}
public function testDeleteList() {
$repository = new ReadingListRepository( 1, $this->db,
$this->db, $this->lbFactory );
$repository->setupForUser();
- list( $listId ) = $this->addLists( 1, [
+ list( $listId, $deletedListId ) = $this->addLists( 1, [
[
'rl_name' => 'foo',
'rl_deleted' => '0',
'rls_index' => 1,
+ ],
+ [
+ 'rl_name' => 'bar',
+ 'rl_deleted' => '1',
+ 'rls_index' => 2,
],
] );
@@ -365,6 +386,11 @@
$repository->deleteList( $listId );
}
);
+ $this->assertFailsWith( 'readinglists-db-error-list-deleted',
+ function () use ( $repository, $deletedListId ) {
+ $repository->deleteList( $deletedListId );
+ }
+ );
$this->assertFailsWith(
'readinglists-db-error-cannot-delete-default-list',
function () use ( $repository ) {
$defaultId = $this->db->selectField(
'reading_list', 'rl_id',
@@ -378,7 +404,18 @@
public function testAddListEntry() {
$repository = new ReadingListRepository( 1, $this->db,
$this->db, $this->lbFactory );
$repository->setupForUser();
- $listId = $repository->addList( 'foo' );
+ list( $listId, $deletedListId ) = $this->addLists( 1, [
+ [
+ 'rl_name' => 'foo',
+ 'rl_deleted' => '0',
+ 'rls_index' => 1,
+ ],
+ [
+ 'rl_name' => 'bar',
+ 'rl_deleted' => '1',
+ 'rls_index' => 2,
+ ],
+ ] );
$entryId = $repository->addListEntry( $listId,
'en.wikipedia.org', 'Foo' );
/** @var ReadingListEntryRow $row */
@@ -411,6 +448,11 @@
function () use ( $listId ) {
$repository = new ReadingListRepository( 123,
$this->db, $this->db, $this->lbFactory );
$repository->addListEntry( $listId,
'en.wikipedia.org', 'B' );
+ }
+ );
+ $this->assertFailsWith( 'readinglists-db-error-list-deleted',
+ function () use ( $repository, $deletedListId ) {
+ $repository->addListEntry( $deletedListId,
'en.wikipedia.org', 'C' );
}
);
$this->assertFailsWith( 'readinglists-db-error-duplicate-page',
@@ -566,7 +608,7 @@
public function testDeleteListEntry() {
$repository = new ReadingListRepository( 1, $this->db,
$this->db, $this->lbFactory );
$repository->setupForUser();
- list( $listId ) = $this->addLists( 1, [
+ list( $listId, $deletedListId ) = $this->addLists( 1, [
[
'rl_is_default' => 0,
'rl_name' => 'test',
@@ -575,8 +617,13 @@
'rl_deleted' => 0,
'rls_index' => 1,
],
+ [
+ 'rl_name' => 'bar',
+ 'rl_deleted' => '1',
+ 'rls_index' => 2,
+ ],
] );
- list( $fooId, $foo2Id ) = $this->addListEntries( $listId, 1, [
+ list( $fooId, $foo2Id, $deletedId ) = $this->addListEntries(
$listId, 1, [
[
'rle_project' => 'foo',
'rle_title' => 'bar',
@@ -592,6 +639,24 @@
'rle_date_updated' => wfTimestampNow(),
'rle_deleted' => 0,
'rles_index' => 2,
+ ],
+ [
+ 'rle_project' => 'foo3',
+ 'rle_title' => 'bar3',
+ 'rle_date_created' => wfTimestampNow(),
+ 'rle_date_updated' => wfTimestampNow(),
+ 'rle_deleted' => 1,
+ 'rles_index' => 3,
+ ],
+ ] );
+ list( $parentDeletedId ) = $this->addListEntries(
$deletedListId, 1, [
+ [
+ 'rle_project' => 'foo4',
+ 'rle_title' => 'bar4',
+ 'rle_date_created' => wfTimestampNow(),
+ 'rle_date_updated' => wfTimestampNow(),
+ 'rle_deleted' => 0,
+ 'rles_index' => 1,
],
] );
@@ -613,6 +678,16 @@
function () use ( $foo2Id ) {
$repository = new ReadingListRepository( 123,
$this->db, $this->db, $this->lbFactory );
$repository->deleteListEntry( $foo2Id );
+ }
+ );
+ $this->assertFailsWith(
'readinglists-db-error-list-entry-deleted',
+ function () use ( $repository, $deletedId ) {
+ $repository->deleteListEntry( $deletedId );
+ }
+ );
+ $this->assertFailsWith( 'readinglists-db-error-list-deleted',
+ function () use ( $repository, $parentDeletedId ) {
+ $repository->deleteListEntry( $parentDeletedId
);
}
);
}
@@ -686,9 +761,22 @@
public function testListEntryOrder() {
$repository = new ReadingListRepository( 1, $this->db,
$this->db, $this->lbFactory );
$repository->setupForUser();
- list( $emptyListId ) = $this->addLists( 1, [ [ 'rl_name' =>
'empty', 'rls_index' => 10 ] ] );
- list( $listId ) = $this->addLists( 1, [ [ 'rl_name' => 'foo',
'rls_index' => 1,
- 'rl_date_updated' => '20100101000000' ] ] );
+ list( $emptyListId, $listId, $deletedListId ) =
$this->addLists( 1, [
+ [
+ 'rl_name' => 'empty',
+ 'rls_index' => 10,
+ ],
+ [
+ 'rl_name' => 'foo',
+ 'rls_index' => 1,
+ 'rl_date_updated' => '20100101000000',
+ ],
+ [
+ 'rl_name' => 'deleted',
+ 'rl_deleted' => '1',
+ 'rls_index' => 2,
+ ],
+ ] );
list( $entry1, $entry2, $entry3, $deletedEntry ) =
$this->addListEntries( $listId, 1, [
[
'rle_project' => 'foo',
@@ -712,6 +800,8 @@
'rles_index' => 4,
],
] );
+ list( $parentDeletedEntry ) = $this->addListEntries(
$deletedListId, 1,
+ [ [ 'rle_project' => 'foo5', 'rle_title' => 'bar5',
'rles_index' => 1 ] ] );
list( $foreignListId ) = $this->addLists( 100, [ [ 'rl_name' =>
'foo', 'rls_index' => 1 ] ] );
list( $foreignEntry ) = $this->addListEntries( $foreignListId,
100,
[ [ 'rle_project' => 'foo', 'rle_title' => 'bar' ] ] );
@@ -729,6 +819,22 @@
[ 'rl_id' => $listId ] );
$this->assertTimestampEquals( wfTimestampNow(), $listTimestamp
);
+ $this->assertFailsWith( 'readinglists-db-error-not-own-list',
+ function () use ( $repository, $foreignListId ) {
+ $repository->getListEntryOrder( $foreignListId
);
+ }
+ );
+ $this->assertFailsWith( 'readinglists-db-error-list-deleted',
+ function () use ( $repository, $deletedListId ) {
+ $repository->getListEntryOrder( $deletedListId
);
+ }
+ );
+ $this->assertFailsWith( 'readinglists-db-error-no-such-list',
+ function () use ( $repository ) {
+ $repository->getListEntryOrder( 123 );
+ }
+ );
+
$this->assertFailsWith( 'readinglists-db-error-empty-order',
function () use ( $repository, $listId ) {
$repository->setListEntryOrder( $listId, [] );
@@ -744,6 +850,11 @@
$repository->setListEntryOrder( $emptyListId, [
$entry1 ] );
}
);
+ $this->assertFailsWith( 'readinglists-db-error-list-deleted',
+ function () use ( $repository, $deletedListId,
$parentDeletedEntry ) {
+ $repository->setListEntryOrder( $deletedListId,
[ $parentDeletedEntry ] );
+ }
+ );
$this->assertFailsWith(
'readinglists-db-error-list-entry-deleted',
function () use ( $repository, $listId, $entry1,
$entry2, $entry3, $deletedEntry ) {
$repository->setListEntryOrder( $listId, [
$entry1, $entry2, $entry3, $deletedEntry ] );
--
To view, visit https://gerrit.wikimedia.org/r/387185
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If8db3dee951be7038bb104eaf3a7c9f58b5f1723
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/ReadingLists
Gerrit-Branch: master
Gerrit-Owner: Gergő Tisza <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
