Yurik has uploaded a new change for review.
https://gerrit.wikimedia.org/r/198181
Change subject: Added gl_perm & gl_updated columns to gather_list
......................................................................
Added gl_perm & gl_updated columns to gather_list
INCOMPLETE! this patch is checked in so that we can
discuss DB schema changes. The tests are not passing yet.
* Two new fields for the lists table
* New list index by permission + updated TS
This will allow api to iterate over public+last updated
* The original SQL schema has been updated but commented out
for now until betalabs is updated. I plan to delete the
GatherListPermissions.php file once all dev machines
and betalabs are updated - will create it properly on
production cluster without updating.
Change-Id: I6ab326f98b2fa4778f83f867507b8e52dcc38e62
---
M includes/api/ApiEditList.php
M includes/api/ApiQueryListPages.php
M includes/api/ApiQueryLists.php
A schema/GatherListPermissions.php
M schema/Updater.hooks.php
A schema/gather_list-perm-ts.sql
M schema/gather_list.sql
M schema/gather_list_item.sql
8 files changed, 172 insertions(+), 63 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Gather
refs/changes/81/198181/1
diff --git a/includes/api/ApiEditList.php b/includes/api/ApiEditList.php
index 6af5668..d5cca7b 100644
--- a/includes/api/ApiEditList.php
+++ b/includes/api/ApiEditList.php
@@ -126,10 +126,7 @@
$listId = $this->createRow( $dbw, $user, $params,
$isWatchlist );
} else {
// Find existing list
- $row = $dbw->selectRow( 'gather_list',
- array( 'gl_id', 'gl_user', 'gl_label',
'gl_info' ), array( 'gl_id' => $listId ),
- __METHOD__
- );
+ $row = $this->getListRow( $dbw, array( 'gl_id' =>
$listId ) );
if ( $row === false ) {
// No database record with the given ID
$this->dieUsage( "List {$p}id was not found",
'badid' );
@@ -230,13 +227,18 @@
private function updateInfo( stdClass $v, array $params, $isWatchlist )
{
$updated = false;
+ if ( $isWatchlist && $params['perm'] === 'public' ) {
+ // Per team discussion, introducing artificial
limitation for now
+ // until we establish that making watchlist public
would cause no harm.
+ // This check can be deleted at any time since all
other API code supports it.
+ $this->dieUsage( 'Making watchlist public is not
supported for security reasons',
+ 'publicwatchlist' );
+ }
+
//
// Set default
if ( !property_exists( $v, 'description' ) ) {
$v->description = '';
- }
- if ( !property_exists( $v, 'perm' ) ) {
- $v->perm = 'private';
}
if ( !property_exists( $v, 'image' ) ) {
$v->image = '';
@@ -246,17 +248,6 @@
// Update from api parameters
if ( $params['description'] !== null && $v->description !==
$params['description'] ) {
$v->description = $params['description'];
- $updated = true;
- }
- if ( $params['perm'] !== null && $v->perm !== $params['perm'] )
{
- if ( $isWatchlist && $params['perm'] !== 'private' ) {
- // Per team discussion, introducing artificial
limitation for now
- // until we establish that making watchlist
public would cause no harm.
- // This check can be deleted at any time since
all other API code supports it.
- $this->dieUsage( 'Making watchlist public is
not supported for security reasons',
- 'publicwatchlist' );
- }
- $v->perm = $params['perm'];
$updated = true;
}
if ( $params['image'] !== null && $v->image !==
$params['image'] ) {
@@ -279,6 +270,7 @@
* Check if the info is a public list
* @param stdClass $info
* @return bool
+ * @deprecated must use DB column instead (To be deleted once DB is
updated)
*/
public static function isPublic( $info ) {
if ( $info && property_exists( $info, 'perm' ) ) {
@@ -308,6 +300,8 @@
'gl_user' => $user->getId(),
'gl_label' => $label,
'gl_info' => $info,
+ 'gl_perm' => $params['perm'] === 'public' ? 1 :
0,
+ 'gl_updated' => $dbw->timestamp(
wfTimestampNow() ),
), __METHOD__, 'IGNORE' );
$id = $dbw->insertId();
} else {
@@ -316,11 +310,9 @@
if ( $id === 0 ) {
// List already exists, update instead, or might not
need it
- $row = $dbw->selectRow( 'gather_list',
- array( 'gl_id', 'gl_user', 'gl_label',
'gl_info' ),
- array( 'gl_user' => $user->getId(), 'gl_label'
=> $label ),
- __METHOD__
- );
+ $row = $this->getListRow( $dbw, array(
+ 'gl_user' => $user->getId(), 'gl_label' =>
$label
+ ) );
if ( $row === false ) {
if ( $createRow ) {
// If creation failed, the second query
should have succeeded
@@ -355,6 +347,12 @@
$update = array();
if ( $params['label'] !== null && $row->gl_label !==
$params['label'] ) {
$update['gl_label'] = $params['label'];
+ }
+ if ( $params['perm'] !== null ) {
+ $perm = $params['perm'] === 'public' ? 1 : 0;
+ if ( $row->gl_perm !== $perm ) {
+ $update['gl_perm'] = $perm;
+ }
}
$info = self::parseListInfo( $row->gl_info, $row->gl_id, true );
$json = $this->updateInfo( $info, $params, $row->gl_label ===
'' );
@@ -522,4 +520,16 @@
}
return new stdClass();
}
+
+ /**
+ * @param DatabaseBase $dbw
+ * @param array $conds
+ * @return bool|stdClass
+ */
+ private function getListRow( DatabaseBase $dbw, $conds ) {
+ return $dbw->selectRow( 'gather_list',
+ array( 'gl_id', 'gl_user', 'gl_label', 'gl_perm',
'gl_info' ),
+ $conds,
+ __METHOD__ );
+ }
}
diff --git a/includes/api/ApiQueryListPages.php
b/includes/api/ApiQueryListPages.php
index 7ecf1f9..dca0811 100644
--- a/includes/api/ApiQueryListPages.php
+++ b/includes/api/ApiQueryListPages.php
@@ -79,7 +79,7 @@
// Id was given, this could be public or private list,
legacy watchlist or regular
// Allow access to any public list/watchlist, and to
private with proper owner/self
$db = $this->getDB();
- $listRow = $db->selectRow( 'gather_list', array(
'gl_label', 'gl_user', 'gl_info' ),
+ $listRow = $db->selectRow( 'gather_list', array(
'gl_label', 'gl_user' ),
array( 'gl_id' => $params['id'] ), __METHOD__ );
if ( $listRow === false ) {
$this->dieUsage( "List does not exist", 'badid'
);
@@ -101,11 +101,8 @@
}
// Check if this is a public list (if required)
- if ( !$showPrivate ) {
- $info = ApiEditList::parseListInfo(
$listRow->gl_info, $params['id'], false );
- if ( !ApiEditList::isPublic( $info ) ) {
- $this->dieUsage( "You have no rights to
see this list", 'badid' );
- }
+ if ( !$showPrivate && $listRow->perm !== 1 ) {
+ $this->dieUsage( "You have no rights to see
this list", 'badid' );
}
if ( $listRow->gl_label === '' ) {
diff --git a/includes/api/ApiQueryLists.php b/includes/api/ApiQueryLists.php
index 17c3f9f..08ba289 100644
--- a/includes/api/ApiQueryLists.php
+++ b/includes/api/ApiQueryLists.php
@@ -98,11 +98,13 @@
$this->addTables( 'gather_list' );
$this->addFields( 'gl_id' );
$this->addFieldsIf( 'gl_label', $fld_label || !$modeAll );
- // won't know if private until later
- $this->addFieldsIf( 'gl_user', $showPrivate === null ||
$modeAll );
+ $this->addFieldsIf( 'gl_user', $modeAll );
+ $this->addFieldsIf( 'gl_perm', $fld_public );
+
if ( $owner ) {
$this->addWhereFld( 'gl_user', $owner->getId() );
}
+
if ( $ids || $findWatchlist ) {
$cond = array();
if ( $ids ) {
@@ -110,6 +112,14 @@
}
if ( $findWatchlist ) {
$cond['gl_label'] = '';
+ }
+ $this->addWhere( $db->makeList( $cond, LIST_OR ) );
+ }
+
+ if ( $showPrivate !== true ) {
+ $cond = array( 'gl_perm' => 1 );
+ if ( $showPrivate === null ) {
+ $cond['gl_user'] = $this->getUser()->getId();
}
$this->addWhere( $db->makeList( $cond, LIST_OR ) );
}
@@ -147,14 +157,11 @@
}
}
- // If we need it for privacy checks or we need to return a prop
field
- $useInfo = $showPrivate !== true || $fld_description ||
$fld_public || $fld_image;
-
+ $useInfo = $fld_description || $fld_image;
$this->addFieldsIf( 'gl_info', $useInfo );
$limit = $params['limit'];
- // TODO: Disabling this until we stop skipping rows in
processing
- // $this->addOption( 'LIMIT', $limit + 1 );
+ $this->addOption( 'LIMIT', $limit + 1 );
if ( $modeAll ) {
$this->addOption( 'ORDER BY', 'gl_id' );
@@ -164,12 +171,10 @@
$count = 0;
$path = array( 'query', $this->getModuleName() );
- $currUserId = strval( $this->getUser()->getId() );
// This closure will process one row, even if that row is fake
watchlist
$processRow = function( $row ) use ( &$count, $modeAll, $limit,
$useInfo, $title,
- $fld_label, $fld_description, $fld_public, $fld_image,
$path, $showPrivate,
- $currUserId, $userId
+ $fld_label, $fld_description, $fld_public, $fld_image,
$path, $userId
) {
if ( $row === null ) {
// Fake watchlist row
@@ -179,21 +184,6 @@
'gl_user' => false,
'gl_info' => '',
);
- }
-
- $info = $useInfo ?
- ApiEditList::parseListInfo( $row->gl_info,
$row->gl_id, false ) : null;
-
- // Determine if this gather_list row is viewable by the
current user
- // TODO: this should be part of the SQL query once
fields are cerated
- $show = $showPrivate;
- if ( $show === null && $row->gl_user === $currUserId ) {
- $show = true;
- } elseif ( $show !== true && ApiEditList::isPublic(
$info ) ) {
- $show = true;
- }
- if ( !$show ) {
- return true;
}
$count++;
@@ -226,12 +216,13 @@
$data['title'] = $row->isIn !== null;
}
}
+ if ( $fld_public ) {
+ $data['public'] = $row->gl_perm === 1;
+ }
if ( $useInfo ) {
+ $info = ApiEditList::parseListInfo(
$row->gl_info, $row->gl_id, false );
if ( $fld_description ) {
$data['description'] = property_exists(
$info, 'description' ) ? $info->description : '';
- }
- if ( $fld_public ) {
- $data['public'] =
ApiEditList::isPublic( $info );
}
if ( $fld_image ) {
if ( property_exists( $info, 'image' )
&& $info->image ) {
@@ -380,7 +371,7 @@
if ( !$this->getUser()->isLoggedIn() ) {
$showPrivate = false;
} else {
- $showPrivate = null; // check each id against
$currUserId
+ $showPrivate = null; // check each id against
current user's ID
}
}
if ( $showPrivate !== false ) {
diff --git a/schema/GatherListPermissions.php b/schema/GatherListPermissions.php
new file mode 100644
index 0000000..3cb6b43
--- /dev/null
+++ b/schema/GatherListPermissions.php
@@ -0,0 +1,85 @@
+<?php
+/**
+ * @ingroup Maintenance
+ */
+
+namespace Gather;
+
+use FormatJson;
+use Gather\api\ApiEditList;
+use LoggedUpdateMaintenance;
+
+
+$IP = getenv( 'MW_INSTALL_PATH' );
+if ( $IP === false ) {
+ $IP = __DIR__ . '/../../..';
+}
+require_once( "$IP/maintenance/Maintenance.php" );
+
+/**
+ * @ingroup Maintenance
+ */
+class GatherListPermissions extends LoggedUpdateMaintenance {
+ public function __construct() {
+ parent::__construct();
+ $this->mDescription = 'Fix permissions and timestamp fields on
gather_list';
+ }
+
+ protected function getUpdateKey() {
+ return 'update gather list permissions';
+ }
+
+ protected function updateSkippedMessage() {
+ return 'gather list permissions already updated';
+ }
+
+ protected function doDBUpdates() {
+ $this->output( "Populating gl_perm column in gather_list
table\n" );
+
+ $db = wfGetDB( DB_MASTER );
+
+ $totalCount = 0;
+ $batchSize = $count = $this->mBatchSize;
+
+ while ( $batchSize === $count ) {
+ $count = 0;
+
+ $res = $db->select(
+ 'gather_list',
+ array( 'gl_id', 'gl_info', 'gl_label' ),
+ array( 'gl_perm' => NULL ),
+ __METHOD__,
+ array( 'LIMIT' => $batchSize )
+ );
+
+ foreach ( $res as $row ) {
+ $count++;
+
+ $info = ApiEditList::parseListInfo(
$row->gl_info, $row->gl_id, true );
+ $perm = ( ApiEditList::isPublic( $info ) &&
$row->gl_label !== '' ) ? 1 : 0;
+ unset( $info->perm );
+ unset( $info->public );
+
+ $db->update(
+ 'gather_list',
+ array(
+ 'gl_perm' => $perm,
+ 'gl_info' =>
FormatJson::encode( $info, false, FormatJson::ALL_OK ),
+ 'gl_updated' => $db->timestamp(
wfTimestampNow() ),
+ ),
+ array( 'gl_id' => $row->gl_id ),
+ __METHOD__ );
+ }
+
+ $totalCount += $count;
+
+ wfWaitForSlaves();
+ }
+
+ $this->output( "Done, $totalCount rows updated.\n" );
+ return true;
+ }
+}
+
+$maintClass = 'GatherListPermissions';
+require_once( RUN_MAINTENANCE_IF_MAIN );
diff --git a/schema/Updater.hooks.php b/schema/Updater.hooks.php
index a9ffd98..1e5b339 100644
--- a/schema/Updater.hooks.php
+++ b/schema/Updater.hooks.php
@@ -9,10 +9,12 @@
*/
class UpdaterHooks {
public static function onLoadExtensionSchemaUpdates( DatabaseUpdater
$du ) {
- $base = dirname( __FILE__ );
+ $du->addExtensionTable( 'gather_list', __DIR__ .
"/gather_list.sql", true );
+ $du->addExtensionTable( 'gather_list_item', __DIR__ .
"/gather_list_item.sql", true );
+ $du->addExtensionField( 'gather_list', 'gl_perm', __DIR__ .
"/gather_list-perm-ts.sql" );
- $du->addExtensionTable( 'gather_list', "$base/gather_list.sql",
true );
- $du->addExtensionTable( 'gather_list_item',
"$base/gather_list_item.sql", true );
+ require_once __DIR__ . '/GatherListPermissions.php';
+ $du->addPostDatabaseUpdateMaintenance(
'Gather\GatherListPermissions' );
return true;
}
diff --git a/schema/gather_list-perm-ts.sql b/schema/gather_list-perm-ts.sql
new file mode 100644
index 0000000..76d4b68
--- /dev/null
+++ b/schema/gather_list-perm-ts.sql
@@ -0,0 +1,13 @@
+-- Licence: GNU GPL v2+
+
+ALTER TABLE /*$wgDBprefix*/gather_list
+ -- The list permissions type (NULL=not migrated, GATHER_PRIVATE=0,
GATHER_PUBLIC=1)
+ ADD gl_perm TINYINT UNSIGNED NULL DEFAULT NULL AFTER gl_label,
+
+ -- The timestamp is updated whenever the list's meta data is modified.
+ -- It is possible we might update this field when modifying watchlist / list
pages
+ ADD gl_updated VARBINARY(14) NOT NULL DEFAULT '' AFTER gl_perm;
+
+
+-- Show all public lists, sorted by the last updated timestamp
+CREATE INDEX /*i*/gl_user_perm_updated ON /*_*/gather_list (gl_perm,
gl_updated DESC);
diff --git a/schema/gather_list.sql b/schema/gather_list.sql
index cd4f745..de1f72f 100644
--- a/schema/gather_list.sql
+++ b/schema/gather_list.sql
@@ -16,6 +16,13 @@
-- Must be unique per user - makes it easier for querying/looking up
gl_label VARCHAR(255) BINARY NOT NULL,
+ -- The list permissions type (NULL=not migrated, GATHER_PRIVATE=0,
GATHER_PUBLIC=1)
+ -- gl_perm TINYINT UNSIGNED NOT NULL DEFAULT 0,
+
+ -- The timestamp is updated whenever the list's meta data is modified.
+ -- It is possible we might update this field when modifying watchlist / list
pages
+ -- gl_updated VARBINARY(14) NOT NULL DEFAULT '',
+
-- All other values are stored here to allow for rapid design changes.
-- At this point we do not foresee any value in using indexes
-- thus blob is a perfect storage medium. Stored as JSON blob.
@@ -24,5 +31,8 @@
) /*$wgDBTableOptions*/;
--- Index to iterate by label
+-- Gets lists of a specific user, sorted by label, watchlist first
CREATE UNIQUE INDEX /*i*/gl_user_label ON /*_*/gather_list (gl_user, gl_label);
+
+-- Show all public lists, sorted by the last updated timestamp
+-- CREATE INDEX /*i*/gl_user_perm_updated ON /*_*/gather_list (gl_perm,
gl_updated DESC);
diff --git a/schema/gather_list_item.sql b/schema/gather_list_item.sql
index 7bdb7f5..f6b1358 100644
--- a/schema/gather_list_item.sql
+++ b/schema/gather_list_item.sql
@@ -17,13 +17,14 @@
-- Sort order uses real to simplify item insertion
-- without modifying other items
- gli_order REAL NOT NULL DEFAULT 0
+ gli_order FLOAT NOT NULL DEFAULT 0
) /*$wgDBTableOptions*/;
-- Define clustered index (must be first unique) -- most common operations
will enumerate in order
CREATE UNIQUE INDEX /*i*/gli_id_order_ns_title ON /*_*/gather_list_item
(gli_gl_id, gli_order, gli_namespace, gli_title);
+
-- In case all lists for a specific title need to be shown / updated
-- Also, enforce one title per list uniqueness
CREATE UNIQUE INDEX /*i*/gli_ns_title ON /*_*/gather_list_item (gli_namespace,
gli_title, gli_gl_id);
--
To view, visit https://gerrit.wikimedia.org/r/198181
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ab326f98b2fa4778f83f867507b8e52dcc38e62
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Gather
Gerrit-Branch: master
Gerrit-Owner: Yurik <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
