[MediaWiki-commits] [Gerrit] mediawiki...ReadingLists[master]: Fix recreation of deleted list entries
jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/386731 )
Change subject: Fix recreation of deleted list entries
..
Fix recreation of deleted list entries
We have a unique index on list + project + title, so a (soft)deleted
entry blocks the creation of another entry with the same project
and title in the same list. Instead, let's just repurpose the
deleted row.
Bug: T179120
Change-Id: Iddbc8af582c93aea182eb356745e34025b67ccae
---
M src/ReadingListRepository.php
M tests/src/ReadingListRepositoryTest.php
2 files changed, 49 insertions(+), 10 deletions(-)
Approvals:
jenkins-bot: Verified
Mholloway: Looks good to me, approved
diff --git a/src/ReadingListRepository.php b/src/ReadingListRepository.php
index 4bd7e89..c23eb9c 100644
--- a/src/ReadingListRepository.php
+++ b/src/ReadingListRepository.php
@@ -371,29 +371,56 @@
throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list', [ $id ] );
}
- $this->dbw->insert(
+ // due to the combination of soft deletion + unique constraint
on
+ // rle_rl_id + rle_project + rle_title, recreation needs
special handling
+ /** @var ReadingListEntryRow $row */
+ $row = $this->dbw->selectRow(
'reading_list_entry',
+ [ 'rle_id', 'rle_deleted' ],
[
'rle_rl_id' => $id,
- 'rle_user_id' => $this->userId,
'rle_project' => $project,
'rle_title' => $title,
- 'rle_date_created' => $this->dbw->timestamp(),
- 'rle_date_updated' => $this->dbw->timestamp(),
- 'rle_deleted' => 0,
],
__METHOD__,
- // throw custom exception for unique constraint on
rle_rl_id + rle_project + rle_title
- [ 'IGNORE' ]
+ // lock the row to avoid race conditions with
purgeOldDeleted() in the update case
+ [ 'FOR UPDATE' ]
);
- if ( !$this->dbw->affectedRows() ) {
+ if ( $row === false ) {
+ $this->dbw->insert(
+ 'reading_list_entry',
+ [
+ 'rle_rl_id' => $id,
+ 'rle_user_id' => $this->userId,
+ 'rle_project' => $project,
+ 'rle_title' => $title,
+ 'rle_date_created' =>
$this->dbw->timestamp(),
+ 'rle_date_updated' =>
$this->dbw->timestamp(),
+ 'rle_deleted' => 0,
+ ]
+ );
+ } elseif ( $row->rle_deleted ) {
+ $this->dbw->update(
+ 'reading_list_entry',
+ [
+ 'rle_date_created' =>
$this->dbw->timestamp(),
+ 'rle_date_updated' =>
$this->dbw->timestamp(),
+ 'rle_deleted' => 0,
+ ],
+ [
+ 'rle_id' => $row->rle_id,
+ ]
+ );
+ } else {
throw new ReadingListRepositoryException(
'readinglists-db-error-duplicate-page' );
}
+ $insertId = $row ? (int)$row->rle_id : $this->dbw->insertId();
$this->logger->info( 'Added entry {entry} for user {user}', [
- 'entry' => $this->dbw->insertId(),
+ 'entry' => $insertId,
'user' => $this->userId,
+ 'recreated' => (bool)$row,
] );
- return $this->dbw->insertId();
+ return $insertId;
}
/**
diff --git a/tests/src/ReadingListRepositoryTest.php
b/tests/src/ReadingListRepositoryTest.php
index 4c21b4f..dfb3071 100644
--- a/tests/src/ReadingListRepositoryTest.php
+++ b/tests/src/ReadingListRepositoryTest.php
@@ -390,6 +390,18 @@
$this->assertTimestampEquals( wfTimestampNow(),
$row->rle_date_updated );
$this->assertEquals( 0, $row->rle_deleted );
+ // test that deletion + recreation does not trip the unique
contstraint
+ $repository->deleteListEntry( $entryId );
+ $entryId2 = $repository->addListEntry( $listId,
[MediaWiki-commits] [Gerrit] mediawiki...ReadingLists[master]: Fix recreation of deleted list entries
Gergő Tisza has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/386731 )
Change subject: Fix recreation of deleted list entries
..
Fix recreation of deleted list entries
We have a unique index on list + project + title, so a (soft)deleted
entry blocks the creation of another entry with the same project
and title in the same list. Instead, let's just repurpose the
deleted row.
Bug: T179120
Change-Id: Iddbc8af582c93aea182eb356745e34025b67ccae
---
M src/ReadingListRepository.php
M tests/src/ReadingListRepositoryTest.php
2 files changed, 49 insertions(+), 10 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/ReadingLists
refs/changes/31/386731/1
diff --git a/src/ReadingListRepository.php b/src/ReadingListRepository.php
index 4bd7e89..c23eb9c 100644
--- a/src/ReadingListRepository.php
+++ b/src/ReadingListRepository.php
@@ -371,29 +371,56 @@
throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list', [ $id ] );
}
- $this->dbw->insert(
+ // due to the combination of soft deletion + unique constraint
on
+ // rle_rl_id + rle_project + rle_title, recreation needs
special handling
+ /** @var ReadingListEntryRow $row */
+ $row = $this->dbw->selectRow(
'reading_list_entry',
+ [ 'rle_id', 'rle_deleted' ],
[
'rle_rl_id' => $id,
- 'rle_user_id' => $this->userId,
'rle_project' => $project,
'rle_title' => $title,
- 'rle_date_created' => $this->dbw->timestamp(),
- 'rle_date_updated' => $this->dbw->timestamp(),
- 'rle_deleted' => 0,
],
__METHOD__,
- // throw custom exception for unique constraint on
rle_rl_id + rle_project + rle_title
- [ 'IGNORE' ]
+ // lock the row to avoid race conditions with
purgeOldDeleted() in the update case
+ [ 'FOR UPDATE' ]
);
- if ( !$this->dbw->affectedRows() ) {
+ if ( $row === false ) {
+ $this->dbw->insert(
+ 'reading_list_entry',
+ [
+ 'rle_rl_id' => $id,
+ 'rle_user_id' => $this->userId,
+ 'rle_project' => $project,
+ 'rle_title' => $title,
+ 'rle_date_created' =>
$this->dbw->timestamp(),
+ 'rle_date_updated' =>
$this->dbw->timestamp(),
+ 'rle_deleted' => 0,
+ ]
+ );
+ } elseif ( $row->rle_deleted ) {
+ $this->dbw->update(
+ 'reading_list_entry',
+ [
+ 'rle_date_created' =>
$this->dbw->timestamp(),
+ 'rle_date_updated' =>
$this->dbw->timestamp(),
+ 'rle_deleted' => 0,
+ ],
+ [
+ 'rle_id' => $row->rle_id,
+ ]
+ );
+ } else {
throw new ReadingListRepositoryException(
'readinglists-db-error-duplicate-page' );
}
+ $insertId = $row ? (int)$row->rle_id : $this->dbw->insertId();
$this->logger->info( 'Added entry {entry} for user {user}', [
- 'entry' => $this->dbw->insertId(),
+ 'entry' => $insertId,
'user' => $this->userId,
+ 'recreated' => (bool)$row,
] );
- return $this->dbw->insertId();
+ return $insertId;
}
/**
diff --git a/tests/src/ReadingListRepositoryTest.php
b/tests/src/ReadingListRepositoryTest.php
index 4c21b4f..dfb3071 100644
--- a/tests/src/ReadingListRepositoryTest.php
+++ b/tests/src/ReadingListRepositoryTest.php
@@ -390,6 +390,18 @@
$this->assertTimestampEquals( wfTimestampNow(),
$row->rle_date_updated );
$this->assertEquals( 0, $row->rle_deleted );
+ // test that deletion + recreation does not trip the unique
contstraint
+ $repository->deleteListEntry( $entryId );
+ $entryId2 = $repository->addListEntry(
