r92081 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r92080‎ | r92081 | r92082 >
Date:19:08, 13 July 2011
Author:raindrift
Status:resolved (Comments)
Tags:todo 
Comment:
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
Modified paths:
  • /trunk/phase3/includes/api/ApiUpload.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadFromStash.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadStash.php (modified) (history)
  • /trunk/phase3/maintenance/archives/patch-uploadstash.sql (modified) (history)
  • /trunk/phase3/maintenance/cleanupUploadStash.php (modified) (history)
  • /trunk/phase3/maintenance/tables.sql (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/cleanupUploadStash.php
——@@ -35,32 +35,33 @@
3636 }
3737
3838 public function execute() {
39 - $dbr = wfGetDB( DB_SLAVE );
 39+ $repo = RepoGroup::singleton()->getLocalRepo();
 40+
 41+ $dbr = $repo->getSlaveDb();
4042
4143 $this->output( "Getting list of files to clean up...\n" );
4244 $res = $dbr->select(
4345 'uploadstash',
4446 'us_key',
45 - 'us_timestamp < ' . wfTimestamp( TS_MW, time() - UploadStash::REPO_AGE * 3600 ),
 47+ 'us_timestamp < ' . $dbr->timestamp( time() - UploadStash::REPO_AGE * 3600 ),
4648 __METHOD__
4749 );
4850
49 - if( !is_object( $res ) ) {
 51+ if( !is_object( $res ) || $res->numRows() == 0 ) {
5052 // nothing to do.
5153 return false;
5254 }
5355
5456 // finish the read before starting writes.
5557 $keys = array();
56 - while( $row = $dbr->fetchRow( $res ) ) {
57 - array_push( $keys, $row['us_key'] );
 58+ foreach($res as $row) {
 59+ array_push( $keys, $row->us_key );
5860 }
5961
6062 $this->output( 'Removing ' . count($keys) . " file(s)...\n" );
6163 // this could be done some other, more direct/efficient way, but using
6264 // UploadStash's own methods means it's less likely to fall accidentally
6365 // out-of-date someday
64 - $repo = RepoGroup::singleton()->getLocalRepo();
6566 $stash = new UploadStash( $repo );
6667
6768 foreach( $keys as $key ) {
Index: trunk/phase3/maintenance/archives/patch-uploadstash.sql
——@@ -1,5 +1,5 @@
22 --
 3+-- Store information about newly uploaded files before they're
34 -- moved into the actual filestore
45 --
56 CREATE TABLE /*_*/uploadstash (
——@@ -22,7 +22,9 @@
2323 us_source_type varchar(50),
2424
2525 -- the date/time on which the file was added
26 - us_timestamp varchar(14) not null,
 26+ us_timestamp varbinary(14) not null,
 27+
 28+ us_status varchar(50) not null,
2729
2830 -- file properties from File::getPropsFromPath. these may prove unnecessary.
2931 --
——@@ -30,7 +32,8 @@
3133 -- this hash comes from File::sha1Base36(), and is 31 characters
3234 us_sha1 varchar(31) NOT NULL,
3335 us_mime varchar(255),
34 - us_media_type varchar(255),
 36+ -- Media type as defined by the MEDIATYPE_xxx constants, should duplicate definition in the image table
 37+ us_media_type ENUM("UNKNOWN", "BITMAP", "DRAWING", "AUDIO", "VIDEO", "MULTIMEDIA", "OFFICE", "TEXT", "EXECUTABLE", "ARCHIVE") default NULL,
3538 -- image-specific properties
3639 us_image_width int unsigned,
3740 us_image_height int unsigned,
——@@ -43,4 +46,4 @@
4447 -- pick out files by key, enforce key uniqueness
4548 CREATE UNIQUE INDEX /*i*/us_key ON /*_*/uploadstash (us_key);
4649 -- the abandoned upload cleanup script needs this
47 -CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp);
\ No newline at end of file
 50+CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp);
Index: trunk/phase3/maintenance/tables.sql
——@@ -962,7 +962,9 @@
963963 us_source_type varchar(50),
964964
965965 -- the date/time on which the file was added
966 - us_timestamp varchar(14) not null,
 966+ us_timestamp varbinary(14) not null,
 967+
 968+ us_status varchar(50) not null,
967969
968970 -- file properties from File::getPropsFromPath. these may prove unnecessary.
969971 --
——@@ -970,7 +972,8 @@
971973 -- this hash comes from File::sha1Base36(), and is 31 characters
972974 us_sha1 varchar(31) NOT NULL,
973975 us_mime varchar(255),
974 - us_media_type varchar(255),
 976+ -- Media type as defined by the MEDIATYPE_xxx constants, should duplicate definition in the image table
 977+ us_media_type ENUM("UNKNOWN", "BITMAP", "DRAWING", "AUDIO", "VIDEO", "MULTIMEDIA", "OFFICE", "TEXT", "EXECUTABLE", "ARCHIVE") default NULL,
975978 -- image-specific properties
976979 us_image_width int unsigned,
977980 us_image_height int unsigned,
Index: trunk/phase3/includes/upload/UploadFromStash.php
——@@ -16,15 +16,23 @@
1717 //LocalFile repo
1818 private $repo;
1919
20 - public function __construct( $stash = false, $repo = false ) {
21 - if( !$this->repo ) {
 20+ public function __construct( $user = false, $stash = false, $repo = false ) {
 21+ // user object. sometimes this won't exist, as when running from cron.
 22+ $this->user = $user;
 23+
 24+ if( $repo ) {
 25+ $this->repo = $repo;
 26+ } else {
2227 $this->repo = RepoGroup::singleton()->getLocalRepo();
2328 }
2429
25 - if( !$this->stash ) {
26 - $this->stash = new UploadStash( $this->repo );
 30+ if( $stash ) {
 31+ $this->stash = $stash;
 32+ } else {
 33+ wfDebug( __METHOD__ . " creating new UploadStash instance for " . $user->getId() . "\n" );
 34+ $this->stash = new UploadStash( $this->repo, $this->user );
2735 }
28 -
 36+
2937 return true;
3038 }
3139
Index: trunk/phase3/includes/upload/UploadStash.php
——@@ -42,6 +42,9 @@
4343
4444 // fileprops cache
4545 protected $fileProps = array();
 46+
 47+ // current user
 48+ protected $user, $userId, $isLoggedIn;
4649
4750 /**
4851 * Represents a temporary filestore, with metadata in the database.
——@@ -49,16 +52,30 @@
5053 *
5154 * @param $repo FileRepo
5255 */
53 - public function __construct( $repo ) {
 56+ public function __construct( $repo, $user = null ) {
5457 // this might change based on wiki's configuration.
5558 $this->repo = $repo;
 59+
 60+ // if a user was passed, use it. otherwise, attempt to use the global.
 61+ // this keeps FileRepo from breaking when it creates an UploadStash object
 62+ if( $user ) {
 63+ $this->user = $user;
 64+ } else {
 65+ global $wgUser;
 66+ $this->user = $wgUser;
 67+ }
 68+
 69+ if( is_object($this->user) ) {
 70+ $this->userId = $this->user->getId();
 71+ $this->isLoggedIn = $this->user->isLoggedIn();
 72+ }
5673 }
5774
5875 /**
5976 * Get a file and its metadata from the stash.
6077 *
6178 * @param $key String: key under which file information is stored
62 - * @param $noauth Boolean (optional) Don't check authentication. Used by maintenance scripts.
 79+ * @param $noAuth Boolean (optional) Don't check authentication. Used by maintenance scripts.
6380 * @throws UploadStashFileNotFoundException
6481 * @throws UploadStashNotLoggedInException
6582 * @throws UploadStashWrongOwnerException
——@@ -66,19 +83,19 @@
6784 * @return UploadStashFile
6885 */
6986 public function getFile( $key, $noAuth = false ) {
70 - global $wgUser;
7187
7288 if ( ! preg_match( self::KEY_FORMAT_REGEX, $key ) ) {
7389 throw new UploadStashBadPathException( "key '$key' is not in a proper format" );
7490 }
7591
7692 if( !$noAuth ) {
77 - $userId = $wgUser->getId();
78 - if( !$userId ) {
79 - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
 93+ if( !$this->isLoggedIn ) {
 94+ throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
8095 }
8196 }
8297
 98+ $dbr = $this->repo->getSlaveDb();
 99+
83100 if ( !isset( $this->fileMetadata[$key] ) ) {
84101 // try this first. if it fails to find the row, check for lag, wait, try again. if its still missing, throw an exception.
85102 // this more complex solution keeps things moving for page loads with many requests
——@@ -115,7 +132,7 @@
116133 }
117134
118135 if( !$noAuth ) {
119 - if( $this->fileMetadata[$key]['us_user'] != $userId ) {
 136+ if( $this->fileMetadata[$key]['us_user'] != $this->userId ) {
120137 throw new UploadStashWrongOwnerException( "This file ($key) doesn't belong to the current user." );
121138 }
122139 }
——@@ -157,7 +174,6 @@
158175 * @return UploadStashFile: file, or null on failure
159176 */
160177 public function stashFile( $path, $sourceType = null, $key = null ) {
161 - global $wgUser;
162178 if ( ! file_exists( $path ) ) {
163179 wfDebug( __METHOD__ . " tried to stash file at '$path', but it doesn't exist\n" );
164180 throw new UploadStashBadPathException( "path doesn't exist" );
——@@ -214,13 +230,17 @@
215231 $stashPath = $storeResult->value;
216232
217233 // fetch the current user ID
218 - $userId = $wgUser->getId();
219 - if( !$userId ) {
220 - throw new UploadStashNotLoggedInException( "No user is logged in, files must belong to users" );
 234+ if( !$this->isLoggedIn ) {
 235+ wfDebugCallstack();
 236+ throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
221237 }
222238
 239+ // insert the file metadata into the db.
 240+ wfDebug( __METHOD__ . " inserting $stashPath under $key\n" );
 241+ $dbw = $this->repo->getMasterDb();
 242+
223243 $this->fileMetadata[$key] = array(
224 - 'us_user' => $userId,
 244+ 'us_user' => $this->userId,
225245 'us_key' => $key,
226246 'us_orig_path' => $path,
227247 'us_path' => $stashPath,
——@@ -232,12 +252,11 @@
233253 'us_image_height' => $fileProps['height'],
234254 'us_image_bits' => $fileProps['bits'],
235255 'us_source_type' => $sourceType,
236 - 'us_timestamp' => wfTimestamp( TS_MW )
 256+ 'us_timestamp' => $dbw->timestamp(),
 257+ 'us_status' => 'finished'
237258 );
238259
239 - // insert the file metadata into the db.
240 - wfDebug( __METHOD__ . " inserting $stashPath under $key\n" );
241 - $dbw = wfGetDB( DB_MASTER );
 260+
242261 $dbw->insert(
243262 'uploadstash',
244263 $this->fileMetadata[$key],
——@@ -261,18 +280,15 @@
262281 * @return boolean: success
263282 */
264283 public function clear() {
265 - global $wgUser;
266 -
267 - $userId = $wgUser->getId();
268 - if( !$userId ) {
269 - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
 284+ if( !$this->isLoggedIn ) {
 285+ throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
270286 }
271287
272288 wfDebug( __METHOD__ . " clearing all rows for user $userId\n" );
273 - $dbw = wfGetDB( DB_MASTER );
 289+ $dbw = $this->repo->getMasterDb();
274290 $dbw->delete(
275291 'uploadstash',
276 - array( 'us_user' => $userId ),
 292+ array( 'us_user' => $this->userId ),
277293 __METHOD__
278294 );
279295
——@@ -291,14 +307,11 @@
292308 * @return boolean: success
293309 */
294310 public function removeFile( $key ){
295 - global $wgUser;
296 -
297 - $userId = $wgUser->getId();
298 - if( !$userId ) {
299 - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
 311+ if( !$this->isLoggedIn ) {
 312+ throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
300313 }
301314
302 - $dbw = wfGetDB( DB_MASTER );
 315+ $dbw = $this->repo->getMasterDb();
303316
304317 // this is a cheap query. it runs on the master so that this function still works when there's lag.
305318 // it won't be called all that often.
——@@ -309,7 +322,7 @@
310323 __METHOD__
311324 );
312325
313 - if( $row->us_user != $userId ) {
 326+ if( $row->us_user != $this->userId ) {
314327 throw new UploadStashWrongOwnerException( "Can't delete: the file ($key) doesn't belong to this user." );
315328 }
316329
——@@ -325,7 +338,7 @@
326339 public function removeFileNoAuth( $key ) {
327340 wfDebug( __METHOD__ . " clearing row $key\n" );
328341
329 - $dbw = wfGetDB( DB_MASTER );
 342+ $dbw = $this->repo->getMasterDb();
330343
331344 // this gets its own transaction since it's called serially by the cleanupUploadStash maintenance script
332345 $dbw->begin();
——@@ -353,14 +366,11 @@
354367 * @return Array
355368 */
356369 public function listFiles() {
357 - global $wgUser;
358 -
359 - $userId = $wgUser->getId();
360 - if( !$userId ) {
361 - throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
 370+ if( !$this->isLoggedIn ) {
 371+ throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
362372 }
363373
364 - $dbw = wfGetDB( DB_SLAVE );
 374+ $dbr = $this->repo->getSlaveDb();
365375 $res = $dbr->select(
366376 'uploadstash',
367377 'us_key',
——@@ -368,14 +378,15 @@
369379 __METHOD__
370380 );
371381
372 - if( !is_object( $res ) ) {
373 - // nothing there.
 382+ if( !is_object( $res ) || $res->numRows() == 0 ) {
 383+ // nothing to do.
374384 return false;
375385 }
376386
 387+ // finish the read before starting writes.
377388 $keys = array();
378 - while( $row = $dbr->fetchRow( $res ) ) {
379 - array_push( $keys, $row['us_key'] );
 389+ foreach($res as $row) {
 390+ array_push( $keys, $row->us_key );
380391 }
381392
382393 return $keys;
——@@ -419,7 +430,7 @@
420431 */
421432 protected function fetchFileMetadata( $key ) {
422433 // populate $fileMetadata[$key]
423 - $dbr = wfGetDB( DB_SLAVE );
 434+ $dbr = $this->repo->getSlaveDb();
424435 $row = $dbr->selectRow(
425436 'uploadstash',
426437 '*',
——@@ -444,7 +455,9 @@
445456 'us_image_width' => $row->us_image_width,
446457 'us_image_height' => $row->us_image_height,
447458 'us_image_bits' => $row->us_image_bits,
448 - 'us_source_type' => $row->us_source_type
 459+ 'us_source_type' => $row->us_source_type,
 460+ 'us_timestamp' => $row->us_timestamp,
 461+ 'us_status' => $row->us_status
449462 );
450463
451464 return true;
Index: trunk/phase3/includes/api/ApiUpload.php
——@@ -220,7 +220,9 @@
221221 $this->dieUsageMsg( 'invalid-file-key' );
222222 }
223223
224 - $this->mUpload = new UploadFromStash();
 224+ // context allows access to the current user without creating new $wgUser references
 225+ $context = $this->createContext();
 226+ $this->mUpload = new UploadFromStash( $context->getUser() );
225227 $this->mUpload->initialize( $this->mParams['filekey'], $this->mParams['filename'] );
226228
227229 } elseif ( isset( $this->mParams['file'] ) ) {

Sign-offs

UserFlagDate
Bryaninspected19:30, 13 July 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r92269No longer using the content hash as a file key. This solves a concurrency pr...raindrift18:33, 15 July 2011
r105322Followup r92081: SQL fix for cleanupUploadStash.php on PostgreSQL (bug 32822)brion18:15, 6 December 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r92009Refactored UploadStash and related classes to use the database for file metad...raindrift21:11, 12 July 2011

Comments

#Comment by 😂 (talk | contribs)   15:48, 15 July 2011

This revision breaks unit tests for me. After svn upping to this revision I get the following (pretty sure the second one is due to the first one failing): 

$ ./phpunit.php --filter Upload
PHPUnit 3.5.13 by Sebastian Bergmann.

.......EE...

Time: 9 seconds, Memory: 69.50Mb

There were 2 errors:

1) ApiUploadTest::testUploadStash
UploadStashFileNotFoundException: key '5v1oo6zzxdwzo5hclzffuund22bad7s.png' not found in stash

/opt/local/apache2/htdocs/phase3/includes/upload/UploadStash.php:120
/opt/local/apache2/htdocs/phase3/includes/upload/UploadStash.php:150
/opt/local/apache2/htdocs/phase3/includes/upload/UploadFromStash.php:60
/opt/local/apache2/htdocs/phase3/includes/api/ApiUpload.php:227
/opt/local/apache2/htdocs/phase3/includes/api/ApiUpload.php:68
/opt/local/apache2/htdocs/phase3/includes/api/ApiMain.php:679
/opt/local/apache2/htdocs/phase3/includes/api/ApiMain.php:340
/opt/local/apache2/htdocs/phase3/tests/phpunit/includes/api/ApiTestCase.php:46
/opt/local/apache2/htdocs/phase3/tests/phpunit/includes/api/ApiTestCase.php:69
/opt/local/apache2/htdocs/phase3/tests/phpunit/includes/api/ApiUploadTest.php:420
/opt/local/apache2/htdocs/phase3/tests/phpunit/MediaWikiTestCase.php:60
/opt/local/apache2/htdocs/phase3/tests/phpunit/MediaWikiPHPUnitCommand.php:20
/opt/local/apache2/htdocs/phase3/tests/phpunit/phpunit.php:60

2) UploadStashTest::testBug29408
Trying to get property of non-object

/opt/local/apache2/htdocs/phase3/includes/upload/UploadStash.php:325
/opt/local/apache2/htdocs/phase3/tests/phpunit/includes/upload/UploadStashTest.php:44
/opt/local/apache2/htdocs/phase3/tests/phpunit/MediaWikiTestCase.php:64
/opt/local/apache2/htdocs/phase3/tests/phpunit/MediaWikiPHPUnitCommand.php:20
/opt/local/apache2/htdocs/phase3/tests/phpunit/phpunit.php:60

FAILURES!
Tests: 12, Assertions: 52, Errors: 2.
#Comment by 😂 (talk | contribs)   15:49, 15 July 2011

To follow up: the second failure is because of the first failure, but really we should be more robust in UploadStash::removeFile() in case selectRow() doesn't return a $row.

#Comment by Raindrift (talk | contribs)   23:23, 19 July 2011

Tested, and it passes unit tests as of r92269 (my ImageMagick got broken, so those tests were ignored and I missed it). I'd appreciate suggestions for how to make the code more robust than it already is... if the key isn't there, there's not much that can be done.

#Comment by 😂 (talk | contribs)   23:34, 19 July 2011

Tests are working fine for me now again, thanks :) I actually fixed the robustness in removeFile() in r92250, so you can ignore that bit :)

#Comment by NeilK (talk | contribs)   03:20, 23 July 2011

Seems to be okay with followups

#Comment by Brion VIBBER (talk | contribs)   18:12, 6 December 2011

This line: 

 'us_timestamp < ' . $dbr->timestamp( time() - UploadStash::REPO_AGE * 3600 ),

fails for DBs where native timestamps contain formatting chars and spaces, such as PostgreSQL (bug 32822). You need to quote the resulting string.

#Comment by Brion VIBBER (talk | contribs)   18:19, 6 December 2011

Resolved in r105322.

#Comment by Platonides (talk | contribs)   21:14, 19 December 2011

What values can us_status contain? just 'finished'? Please document.

Status & tagging log