jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/387186 )
Change subject: Fix handling of partial order parameters
......................................................................
Fix handling of partial order parameters
Throw an error when setListOrder() or setListEntryOrder() is called,
and the provided ids are valid, but not all valid ids are provided.
Bug: T177853
Change-Id: Id992ed0eefd67c9e0b67ce378a9654301880b52a
---
M i18n/en.json
M i18n/qqq.json
M src/ReadingListRepository.php
M tests/src/ReadingListRepositoryTest.php
4 files changed, 60 insertions(+), 22 deletions(-)
Approvals:
jenkins-bot: Verified
Mholloway: Looks good to me, approved
diff --git a/i18n/en.json b/i18n/en.json
index 5e88465..b7c4321 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -17,6 +17,8 @@
"readinglists-db-error-duplicate-page": "The list already contains this
page.",
"readinglists-db-error-empty-list-ids": "List ids parameter must not be
empty.",
"readinglists-db-error-empty-order": "Order parameter must not be
empty.",
+ "readinglists-db-error-missing-list": "All lists must be included in
the order ($1 missing).",
+ "readinglists-db-error-missing-list-entry": "All list entries must be
included in the order ($1 missing).",
"readinglists-db-error-entry-not-in-list": "List entry $1 does not
belong to this list.",
"readinglists-db-error-user-required": "This method cannot be called
without specifying the user.",
"readinglists-apierror-project-title-param": "<var>$1project</var> and
<var>$1title</var> must be used together.",
diff --git a/i18n/qqq.json b/i18n/qqq.json
index c7f8e33..b7b2f38 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -18,6 +18,8 @@
"readinglists-db-error-duplicate-page": "Error message used when trying
to add a page to a list that already contains it.",
"readinglists-db-error-empty-list-ids": "Error message used when
querying list entries without specifying any lists.",
"readinglists-db-error-empty-order": "Error message used when trying to
set the order of lists or list entries but passing an empty order array.",
+ "readinglists-db-error-missing-list": "Error message used when trying
to set the order of lists but omitting some list IDs.",
+ "readinglists-db-error-missing-list-entry": "Error message used when
trying to set the order of list entries but omitting some entry IDs.",
"readinglists-db-error-entry-not-in-list": "Error message used when
trying to set the order of list entries but some of them do not belong to the
list.",
"readinglists-db-error-user-required": "Error message used when calling
a method that operates on a single user, but the user was not specified when
the repository object was constructed.",
"readinglists-apierror-project-title-param": "{{doc-apierror}}\n$1 is
the module prefix.",
diff --git a/src/ReadingListRepository.php b/src/ReadingListRepository.php
index f22e298..5fdec56 100644
--- a/src/ReadingListRepository.php
+++ b/src/ReadingListRepository.php
@@ -534,28 +534,40 @@
throw new ReadingListRepositoryException(
'readinglists-db-error-not-set-up' );
}
- // Make sure the lists exist and the user owns them.
- $res = $this->dbw->select(
+ // Make sure the set of IDs match the actual lists.
+ $ids = $this->dbw->selectFieldValues(
'reading_list',
- [ 'rl_id', 'rl_user_id', 'rl_deleted' ],
- [ 'rl_id' => $order ]
- );
- $filtered = [];
- foreach ( $res as $row ) {
+ 'rl_id',
+ [
+ 'rl_user_id' => $this->userId,
+ 'rl_deleted' => 0,
+ ]
+ ) ?: [];
+ $nonExistent = array_diff( $order, $ids );
+ if ( $nonExistent ) {
/** @var ReadingListRow $row */
- if ( $row->rl_user_id != $this->userId ) {
+ $row = $this->dbw->selectRow(
+ 'reading_list',
+ [ 'rl_id', 'rl_user_id', 'rl_deleted' ],
+ [ 'rl_id' => reset( $nonExistent ) ]
+ );
+ if ( !$row ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list',
+ [ reset( $nonExistent ) ] );
+ } 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 ] );
+ } else {
+ throw new LogicException( 'setListOrder failed
for unknown reason' );
}
- $filtered[] = $row->rl_id;
}
- $missing = array_diff( $order, $filtered );
+ $missing = array_diff( $ids, $order );
if ( $missing ) {
throw new ReadingListRepositoryException(
- 'readinglists-db-error-no-such-list', [ reset(
$missing ) ] );
+ 'readinglists-db-error-missing-list', [ reset(
$missing ) ] );
}
$this->dbw->deleteJoin(
@@ -628,16 +640,27 @@
throw new ReadingListRepositoryException(
'readinglists-db-error-empty-order' );
}
- // Make sure the list entries exist and the user owns them.
- $res = $this->dbw->select(
+ // Make sure the set of IDs match the actual list entries.
+ $ids = $this->dbw->selectFieldValues(
'reading_list_entry',
- [ 'rle_id', 'rle_rl_id', 'rle_user_id', 'rle_deleted' ],
- [ 'rle_id' => $order ]
- );
- $filtered = [];
- foreach ( $res as $row ) {
+ 'rle_id',
+ [
+ 'rle_rl_id' => $id,
+ 'rle_deleted' => 0,
+ ]
+ ) ?: [];
+ $nonExistent = array_diff( $order, $ids );
+ if ( $nonExistent ) {
/** @var ReadingListEntryRow $row */
- if ( $row->rle_user_id != $this->userId ) {
+ $row = $this->dbw->selectRow(
+ 'reading_list_entry',
+ [ 'rle_id', 'rle_rl_id', 'rle_user_id',
'rle_deleted' ],
+ [ 'rle_id' => reset( $nonExistent ) ]
+ );
+ if ( !$row ) {
+ throw new ReadingListRepositoryException(
'readinglists-db-error-no-such-list-entry',
+ [ reset( $nonExistent ) ] );
+ } elseif ( $row->rle_user_id != $this->userId ) {
throw new ReadingListRepositoryException(
'readinglists-db-error-not-own-list-entry', [ $row->rle_id ] );
} elseif ( $row->rle_rl_id != $id ) {
@@ -646,13 +669,14 @@
} elseif ( $row->rle_deleted ) {
throw new ReadingListRepositoryException(
'readinglists-db-error-list-entry-deleted', [ $row->rle_id ] );
+ } else {
+ throw new LogicException( 'setListEntryOrder
failed for unknown reason' );
}
- $filtered[] = $row->rle_id;
}
- $missing = array_diff( $order, $filtered );
+ $missing = array_diff( $ids, $order );
if ( $missing ) {
throw new ReadingListRepositoryException(
- 'readinglists-db-error-no-such-list-entry', [
reset( $missing ) ] );
+ 'readinglists-db-error-missing-list-entry', [
reset( $missing ) ] );
}
$this->dbw->deleteJoin(
diff --git a/tests/src/ReadingListRepositoryTest.php
b/tests/src/ReadingListRepositoryTest.php
index 6747f63..c1d1f70 100644
--- a/tests/src/ReadingListRepositoryTest.php
+++ b/tests/src/ReadingListRepositoryTest.php
@@ -741,6 +741,11 @@
$repository->setListOrder( [] );
}
);
+ $this->assertFailsWith( 'readinglists-db-error-missing-list',
+ function () use ( $repository, $fooId, $foo2Id ) {
+ $repository->setListOrder( [ $fooId, $foo2Id ]
);
+ }
+ );
$this->assertFailsWith( 'readinglists-db-error-not-own-list',
function () use ( $repository, $fooId, $foo2Id,
$foreignId ) {
$repository->setListOrder( [ $fooId, $foo2Id,
$foreignId ] );
@@ -840,6 +845,11 @@
$repository->setListEntryOrder( $listId, [] );
}
);
+ $this->assertFailsWith(
'readinglists-db-error-missing-list-entry',
+ function () use ( $repository, $listId, $entry1,
$entry2 ) {
+ $repository->setListEntryOrder( $listId, [
$entry1, $entry2 ] );
+ }
+ );
$this->assertFailsWith(
'readinglists-db-error-not-own-list-entry',
function () use ( $repository, $listId, $foreignEntry )
{
$repository->setListEntryOrder( $listId, [
$foreignEntry ] );
--
To view, visit https://gerrit.wikimedia.org/r/387186
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id992ed0eefd67c9e0b67ce378a9654301880b52a
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/ReadingLists
Gerrit-Branch: master
Gerrit-Owner: Gergő Tisza <[email protected]>
Gerrit-Reviewer: BearND <[email protected]>
Gerrit-Reviewer: Mholloway <[email protected]>
Gerrit-Reviewer: Siebrand <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
