From c2e2c8270e46157f0514f0bf5d75718885716225 Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Wed, 13 Jul 2011 19:08:51 +0000 Subject: [PATCH] Added us_status column for future expansion re Bryan's suggestion Implemented changes suggested in code review on r92009: constructor bug handling passed repo/stash up-to-date timestamp generation fetching db handle from repo iterating over select results according to convention changed uploadstash.us_media_type to enum to mirror image.img_media_type removed (most) new references to $wgUser, instead using ApiBase::createContext to find the user --- includes/api/ApiUpload.php | 4 +- includes/upload/UploadFromStash.php | 18 ++-- includes/upload/UploadStash.php | 99 ++++++++++++---------- maintenance/archives/patch-uploadstash.sql | 11 ++- maintenance/cleanupUploadStash.php | 13 +-- maintenance/tables.sql | 7 +- 6 files changed, 91 insertions(+), 61 deletions(-) diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 6a5a1ac781..3a8326f428 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -220,7 +220,9 @@ class ApiUpload extends ApiBase { $this->dieUsageMsg( 'invalid-file-key' ); } - $this->mUpload = new UploadFromStash(); + // context allows access to the current user without creating new $wgUser references + $context = $this->createContext(); + $this->mUpload = new UploadFromStash( $context->getUser() ); $this->mUpload->initialize( $this->mParams['filekey'], $this->mParams['filename'] ); } elseif ( isset( $this->mParams['file'] ) ) { diff --git a/includes/upload/UploadFromStash.php b/includes/upload/UploadFromStash.php index 7f3789c17c..1ee720a158 100644 --- a/includes/upload/UploadFromStash.php +++ b/includes/upload/UploadFromStash.php @@ -16,15 +16,23 @@ class UploadFromStash extends UploadBase { //LocalFile repo private $repo; - public function __construct( $stash = false, $repo = false ) { - if( !$this->repo ) { + public function __construct( $user = false, $stash = false, $repo = false ) { + // user object. sometimes this won't exist, as when running from cron. + $this->user = $user; + + if( $repo ) { + $this->repo = $repo; + } else { $this->repo = RepoGroup::singleton()->getLocalRepo(); } - if( !$this->stash ) { - $this->stash = new UploadStash( $this->repo ); + if( $stash ) { + $this->stash = $stash; + } else { + wfDebug( __METHOD__ . " creating new UploadStash instance for " . $user->getId() . "\n" ); + $this->stash = new UploadStash( $this->repo, $this->user ); } - + return true; } diff --git a/includes/upload/UploadStash.php b/includes/upload/UploadStash.php index 61c337ad23..9c526b4faa 100644 --- a/includes/upload/UploadStash.php +++ b/includes/upload/UploadStash.php @@ -42,6 +42,9 @@ class UploadStash { // fileprops cache protected $fileProps = array(); + + // current user + protected $user, $userId, $isLoggedIn; /** * Represents a temporary filestore, with metadata in the database. @@ -49,16 +52,30 @@ class UploadStash { * * @param $repo FileRepo */ - public function __construct( $repo ) { + public function __construct( $repo, $user = null ) { // this might change based on wiki's configuration. $this->repo = $repo; + + // if a user was passed, use it. otherwise, attempt to use the global. + // this keeps FileRepo from breaking when it creates an UploadStash object + if( $user ) { + $this->user = $user; + } else { + global $wgUser; + $this->user = $wgUser; + } + + if( is_object($this->user) ) { + $this->userId = $this->user->getId(); + $this->isLoggedIn = $this->user->isLoggedIn(); + } } /** * Get a file and its metadata from the stash. * * @param $key String: key under which file information is stored - * @param $noauth Boolean (optional) Don't check authentication. Used by maintenance scripts. + * @param $noAuth Boolean (optional) Don't check authentication. Used by maintenance scripts. * @throws UploadStashFileNotFoundException * @throws UploadStashNotLoggedInException * @throws UploadStashWrongOwnerException @@ -66,19 +83,19 @@ class UploadStash { * @return UploadStashFile */ public function getFile( $key, $noAuth = false ) { - global $wgUser; if ( ! preg_match( self::KEY_FORMAT_REGEX, $key ) ) { throw new UploadStashBadPathException( "key '$key' is not in a proper format" ); } if( !$noAuth ) { - $userId = $wgUser->getId(); - if( !$userId ) { - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' ); + if( !$this->isLoggedIn ) { + throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' ); } } + $dbr = $this->repo->getSlaveDb(); + if ( !isset( $this->fileMetadata[$key] ) ) { // try this first. if it fails to find the row, check for lag, wait, try again. if its still missing, throw an exception. // this more complex solution keeps things moving for page loads with many requests @@ -115,7 +132,7 @@ class UploadStash { } if( !$noAuth ) { - if( $this->fileMetadata[$key]['us_user'] != $userId ) { + if( $this->fileMetadata[$key]['us_user'] != $this->userId ) { throw new UploadStashWrongOwnerException( "This file ($key) doesn't belong to the current user." ); } } @@ -157,7 +174,6 @@ class UploadStash { * @return UploadStashFile: file, or null on failure */ public function stashFile( $path, $sourceType = null, $key = null ) { - global $wgUser; if ( ! file_exists( $path ) ) { wfDebug( __METHOD__ . " tried to stash file at '$path', but it doesn't exist\n" ); throw new UploadStashBadPathException( "path doesn't exist" ); @@ -214,13 +230,17 @@ class UploadStash { $stashPath = $storeResult->value; // fetch the current user ID - $userId = $wgUser->getId(); - if( !$userId ) { - throw new UploadStashNotLoggedInException( "No user is logged in, files must belong to users" ); + if( !$this->isLoggedIn ) { + wfDebugCallstack(); + throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' ); } + // insert the file metadata into the db. + wfDebug( __METHOD__ . " inserting $stashPath under $key\n" ); + $dbw = $this->repo->getMasterDb(); + $this->fileMetadata[$key] = array( - 'us_user' => $userId, + 'us_user' => $this->userId, 'us_key' => $key, 'us_orig_path' => $path, 'us_path' => $stashPath, @@ -232,12 +252,11 @@ class UploadStash { 'us_image_height' => $fileProps['height'], 'us_image_bits' => $fileProps['bits'], 'us_source_type' => $sourceType, - 'us_timestamp' => wfTimestamp( TS_MW ) + 'us_timestamp' => $dbw->timestamp(), + 'us_status' => 'finished' ); - // insert the file metadata into the db. - wfDebug( __METHOD__ . " inserting $stashPath under $key\n" ); - $dbw = wfGetDB( DB_MASTER ); + $dbw->insert( 'uploadstash', $this->fileMetadata[$key], @@ -261,18 +280,15 @@ class UploadStash { * @return boolean: success */ public function clear() { - global $wgUser; - - $userId = $wgUser->getId(); - if( !$userId ) { - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' ); + if( !$this->isLoggedIn ) { + throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' ); } wfDebug( __METHOD__ . " clearing all rows for user $userId\n" ); - $dbw = wfGetDB( DB_MASTER ); + $dbw = $this->repo->getMasterDb(); $dbw->delete( 'uploadstash', - array( 'us_user' => $userId ), + array( 'us_user' => $this->userId ), __METHOD__ ); @@ -291,14 +307,11 @@ class UploadStash { * @return boolean: success */ public function removeFile( $key ){ - global $wgUser; - - $userId = $wgUser->getId(); - if( !$userId ) { - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' ); + if( !$this->isLoggedIn ) { + throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' ); } - $dbw = wfGetDB( DB_MASTER ); + $dbw = $this->repo->getMasterDb(); // this is a cheap query. it runs on the master so that this function still works when there's lag. // it won't be called all that often. @@ -309,7 +322,7 @@ class UploadStash { __METHOD__ ); - if( $row->us_user != $userId ) { + if( $row->us_user != $this->userId ) { throw new UploadStashWrongOwnerException( "Can't delete: the file ($key) doesn't belong to this user." ); } @@ -325,7 +338,7 @@ class UploadStash { public function removeFileNoAuth( $key ) { wfDebug( __METHOD__ . " clearing row $key\n" ); - $dbw = wfGetDB( DB_MASTER ); + $dbw = $this->repo->getMasterDb(); // this gets its own transaction since it's called serially by the cleanupUploadStash maintenance script $dbw->begin(); @@ -353,14 +366,11 @@ class UploadStash { * @return Array */ public function listFiles() { - global $wgUser; - - $userId = $wgUser->getId(); - if( !$userId ) { - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' ); + if( !$this->isLoggedIn ) { + throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' ); } - $dbw = wfGetDB( DB_SLAVE ); + $dbr = $this->repo->getSlaveDb(); $res = $dbr->select( 'uploadstash', 'us_key', @@ -368,14 +378,15 @@ class UploadStash { __METHOD__ ); - if( !is_object( $res ) ) { - // nothing there. + if( !is_object( $res ) || $res->numRows() == 0 ) { + // nothing to do. return false; } + // finish the read before starting writes. $keys = array(); - while( $row = $dbr->fetchRow( $res ) ) { - array_push( $keys, $row['us_key'] ); + foreach($res as $row) { + array_push( $keys, $row->us_key ); } return $keys; @@ -419,7 +430,7 @@ class UploadStash { */ protected function fetchFileMetadata( $key ) { // populate $fileMetadata[$key] - $dbr = wfGetDB( DB_SLAVE ); + $dbr = $this->repo->getSlaveDb(); $row = $dbr->selectRow( 'uploadstash', '*', @@ -444,7 +455,9 @@ class UploadStash { 'us_image_width' => $row->us_image_width, 'us_image_height' => $row->us_image_height, 'us_image_bits' => $row->us_image_bits, - 'us_source_type' => $row->us_source_type + 'us_source_type' => $row->us_source_type, + 'us_timestamp' => $row->us_timestamp, + 'us_status' => $row->us_status ); return true; diff --git a/maintenance/archives/patch-uploadstash.sql b/maintenance/archives/patch-uploadstash.sql index 2d29c3b494..2512076f49 100644 --- a/maintenance/archives/patch-uploadstash.sql +++ b/maintenance/archives/patch-uploadstash.sql @@ -1,5 +1,5 @@ -- --- Store information about newly uploaded files before they're +-- Store information about newly uploaded files before they're -- moved into the actual filestore -- CREATE TABLE /*_*/uploadstash ( @@ -22,7 +22,9 @@ CREATE TABLE /*_*/uploadstash ( us_source_type varchar(50), -- the date/time on which the file was added - us_timestamp varchar(14) not null, + us_timestamp varbinary(14) not null, + + us_status varchar(50) not null, -- file properties from File::getPropsFromPath. these may prove unnecessary. -- @@ -30,7 +32,8 @@ CREATE TABLE /*_*/uploadstash ( -- this hash comes from File::sha1Base36(), and is 31 characters us_sha1 varchar(31) NOT NULL, us_mime varchar(255), - us_media_type varchar(255), + -- Media type as defined by the MEDIATYPE_xxx constants, should duplicate definition in the image table + us_media_type ENUM("UNKNOWN", "BITMAP", "DRAWING", "AUDIO", "VIDEO", "MULTIMEDIA", "OFFICE", "TEXT", "EXECUTABLE", "ARCHIVE") default NULL, -- image-specific properties us_image_width int unsigned, us_image_height int unsigned, @@ -43,4 +46,4 @@ CREATE INDEX /*i*/us_user ON /*_*/uploadstash (us_user); -- pick out files by key, enforce key uniqueness CREATE UNIQUE INDEX /*i*/us_key ON /*_*/uploadstash (us_key); -- the abandoned upload cleanup script needs this -CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp); \ No newline at end of file +CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp); diff --git a/maintenance/cleanupUploadStash.php b/maintenance/cleanupUploadStash.php index 763f556a06..4b47f39753 100644 --- a/maintenance/cleanupUploadStash.php +++ b/maintenance/cleanupUploadStash.php @@ -35,32 +35,33 @@ class UploadStashCleanup extends Maintenance { } public function execute() { - $dbr = wfGetDB( DB_SLAVE ); + $repo = RepoGroup::singleton()->getLocalRepo(); + + $dbr = $repo->getSlaveDb(); $this->output( "Getting list of files to clean up...\n" ); $res = $dbr->select( 'uploadstash', 'us_key', - 'us_timestamp < ' . wfTimestamp( TS_MW, time() - UploadStash::REPO_AGE * 3600 ), + 'us_timestamp < ' . $dbr->timestamp( time() - UploadStash::REPO_AGE * 3600 ), __METHOD__ ); - if( !is_object( $res ) ) { + if( !is_object( $res ) || $res->numRows() == 0 ) { // nothing to do. return false; } // finish the read before starting writes. $keys = array(); - while( $row = $dbr->fetchRow( $res ) ) { - array_push( $keys, $row['us_key'] ); + foreach($res as $row) { + array_push( $keys, $row->us_key ); } $this->output( 'Removing ' . count($keys) . " file(s)...\n" ); // this could be done some other, more direct/efficient way, but using // UploadStash's own methods means it's less likely to fall accidentally // out-of-date someday - $repo = RepoGroup::singleton()->getLocalRepo(); $stash = new UploadStash( $repo ); foreach( $keys as $key ) { diff --git a/maintenance/tables.sql b/maintenance/tables.sql index 48e0d7bb91..65c8a72b11 100644 --- a/maintenance/tables.sql +++ b/maintenance/tables.sql @@ -962,7 +962,9 @@ CREATE TABLE /*_*/uploadstash ( us_source_type varchar(50), -- the date/time on which the file was added - us_timestamp varchar(14) not null, + us_timestamp varbinary(14) not null, + + us_status varchar(50) not null, -- file properties from File::getPropsFromPath. these may prove unnecessary. -- @@ -970,7 +972,8 @@ CREATE TABLE /*_*/uploadstash ( -- this hash comes from File::sha1Base36(), and is 31 characters us_sha1 varchar(31) NOT NULL, us_mime varchar(255), - us_media_type varchar(255), + -- Media type as defined by the MEDIATYPE_xxx constants, should duplicate definition in the image table + us_media_type ENUM("UNKNOWN", "BITMAP", "DRAWING", "AUDIO", "VIDEO", "MULTIMEDIA", "OFFICE", "TEXT", "EXECUTABLE", "ARCHIVE") default NULL, -- image-specific properties us_image_width int unsigned, us_image_height int unsigned, -- 2.20.1