r110039 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110038‎ | r110039 | r110040 >
Date:00:06, 26 January 2012
Author:mglaser
Status:resolved (Comments)
Tags:
Comment:
* implemented overwrite param
* implemented missing methods (isPathUsableInternal, doSecureInternal)
* additional checks for invalid paths
* fixed bug in getFileList that lead to thumbnails not being regenerated
* all FileBackendTests pass
Modified paths:
  • /trunk/extensions/WindowsAzureStorage/includes/filerepo/backend/WindowsAzureFileBackend.php (modified) (history)

Diff [purge]

Index: trunk/extensions/WindowsAzureStorage/includes/filerepo/backend/WindowsAzureFileBackend.php
——@@ -54,18 +54,12 @@
5555 * @see FileBackend::resolveContainerPath()
5656 */
5757 protected function resolveContainerPath( $container, $relStoragePath ) {
58 - //Azure container naming conventions; http://msdn.microsoft.com/en-us/library/dd135715.aspx
 58+ //Azure blob naming conventions; http://msdn.microsoft.com/en-us/library/dd135715.aspx
5959
6060 if ( strlen( urlencode( $relStoragePath ) ) > 1024 ) {
6161 return null;
6262 }
63 -
64 - // Get absolute path given the container base dir
65 - if ( isset( $this->containerPaths[$container] ) ) {
66 - return $this->containerPaths[$container] . "/{$relStoragePath}";
67 - }
68 - // TODO: Should storagepath not be urlencoded?
69 - // TODO: return null
 63+
7064 return $relStoragePath;
7165 }
7266
——@@ -75,11 +69,22 @@
7670 function doStoreInternal( array $params ) {
7771 $status = Status::newGood();
7872 list( $dstCont, $dstRel ) = $this->resolveStoragePath( $params['dst'] );
 73+ if ( $dstRel === null ) {
 74+ $status->fatal( 'backend-fail-invalidpath', $params['dst'] );
 75+ return $status;
 76+ }
 77+
 78+ // Check if the destination object already exists
 79+ $blobExists = $this->storageClient->blobExists( $dstCont, $dstRel );
 80+ if ( $blobExists && empty( $params['overwrite'] ) ) { //Blob exists _and_ should not be overridden
 81+ $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
 82+ return $status;
 83+ }
 84+
7985 try {
8086 $result = $this->storageClient->putBlob( $dstCont, $dstRel, $params['src']);
8187 }
8288 catch ( Exception $e ) {
83 - // TODO: Read exception. Are there different ones?
8489 $status->fatal( 'backend-fail-store', $dstRel, $dstCont );
8590 }
8691 return $status;
——@@ -92,8 +97,21 @@
9398 $status = Status::newGood();
9499 list( $srcContainer, $srcDir ) = $this->resolveStoragePath( $params['src'] );
95100 list( $dstContainer, $dstDir ) = $this->resolveStoragePath( $params['dst'] );
 101+ if ( $srcDir === null ) {
 102+ $status->fatal( 'backend-fail-invalidpath', $params['src'] );
 103+ return $status;
 104+ }
 105+ if ( $dstDir === null ) {
 106+ $status->fatal( 'backend-fail-invalidpath', $params['dst'] );
 107+ return $status;
 108+ }
 109+
 110+ $blobExists = $this->storageClient->blobExists( $dstContainer, $dstDir );
 111+ if ( $blobExists && empty( $params['overwrite'] ) ) { //Blob exists _and_ should not be overridden
 112+ $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
 113+ return $status;
 114+ }
96115
97 - // TODO: check for null
98116 try {
99117 $result = $this->storageClient->copyBlob( $srcContainer, $srcDir, $dstContainer, $dstDir);
100118 }
——@@ -115,8 +133,8 @@
116134 return $status;
117135 }
118136
119 - // (a) Check the source container
120 - try { //TODO: Unnecessary --> remove (or better, check in resolveStoragePath?)
 137+ // Check the source container
 138+ try {
121139 $container = $this->storageClient->getContainer( $srcCont );
122140 }
123141 catch ( Exception $e ) {
——@@ -124,7 +142,7 @@
125143 return $status;
126144 }
127145
128 - // (b) Actually delete the object
 146+ // Actually delete the object
129147 try {
130148 $this->storageClient->deleteBlob( $srcCont, $srcRel );
131149 }
——@@ -147,27 +165,25 @@
148166 return $status;
149167 }
150168
151 - // (a) Check if the destination object already exists
 169+ // Check if the destination object already exists
152170 $blobExists = $this->storageClient->blobExists( $dstCont, $dstRel );
153171 if ( $blobExists && empty( $params['overwrite'] ) ) { //Blob exists _and_ should not be overridden
154172 $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
155173 return $status;
156174 }
157175
158 - // (b) Actually create the object
 176+ // Actually create the object
159177 try {
160 - // TODO: how do I know the container exists? Should we call prepare?
161178 $this->storageClient->putBlobData( $dstCont, $dstRel, $params['content'] );
162179 }
163180 catch ( Exception $e ) {
164181 $status->fatal( 'backend-fail-internal' );
165182 }
166 -
167183 return $status;
168184 }
169185
170186 /**
171 - * @see FileBackend::prepare()
 187+ * @see FileBackend::doPrepare()
172188 */
173189
174190 function doPrepareInternal( $container, $dir, array $params ) {
——@@ -180,13 +196,7 @@
181197 }
182198 try {
183199 $this->storageClient->createContainerIfNotExists( $c );
184 - // TODO: must this be set anytime prepare is called?
185200 $this->storageClient->setContainerAcl( $c, Microsoft_WindowsAzure_Storage_Blob::ACL_PUBLIC );
186 -
187 - // TODO: check if readable and writeable
188 - //$container = $this->storageClient->getContainer( $c );
189 - //$status->fatal( 'directoryreadonlyerror', $params['dir'] );
190 - //$status->fatal( 'directorynotreadableerror', $params['dir'] );
191201 }
192202 catch (Exception $e ) {
193203 $status->fatal( 'directorycreateerror', $params['dir'] );
——@@ -203,7 +213,8 @@
204214 //Azure container naming conventions; http://msdn.microsoft.com/en-us/library/dd135715.aspx
205215 $container = strtolower($container);
206216 $container = preg_replace( '#[^a-z0-9\-]#', '', $container );
207 - // TODO: "-test" and "test-" are invalid, too
 217+ $container = preg_replace( '#^-#', '', $container );
 218+ $container = preg_replace( '#-$#', '', $container );
208219 $container = preg_replace( '#-{2,}#', '-', $container );
209220
210221 return $container;
——@@ -212,20 +223,27 @@
213224 /**
214225 * @see FileBackend::secure()
215226 */
216 - /*
217 - // TODO: check if we need to override doSecure
218 - function secure( array $params ) {
 227+ function doSecureInternal( $container, $dir, array $params ) {
219228 $status = Status::newGood();
220 - return $status; // badgers? We don't need no steenking badgers!
 229+
 230+ try {
 231+ if ( $this->storageClient->containerExists( $container ) ) {
 232+ if ( $params['noAccess'] == true ) {
 233+ $this->storageClient->setContainerAcl( $container, Microsoft_WindowsAzure_Storage_Blob::ACL_PRIVATE );
 234+ }
 235+ }
 236+ }
 237+ catch (Exception $e ) {
 238+ $status->fatal( 'directorycreateerror', $container );
 239+ return $status;
 240+ }
 241+ return $status;
221242 }
222 - */
223243
224244 /**
225245 * @see FileBackend::fileExists()
226246 */
227 - function doFileExists( array $params ) {die();
228 - // TODO: Merely renamed this functino from fileExists. Check if more is neccessarey
229 - // TODO: Off topic here: Prepare function might call some internal function which can be overridden.
 247+ function doFileExists( array $params ) {
230248 list( $srcCont, $srcRel ) = $this->resolveStoragePath( $params['src'] );
231249 if ( $srcRel === null ) {
232250 return false; // invalid storage path
——@@ -236,45 +254,34 @@
237255 }
238256
239257 /**
240 - * @see FileBackend::getFileTimestamp()
241 - */
242 - // TODO: merely renamed from getFileTimestamp. Check if there are any probs.
243 - // TODO: Is this method still used?
244 - function doGetFileTimestamp( array $params ) {
245 - list( $srcCont, $srcRel ) = $this->resolveStoragePath( $params['src'] );
246 - if ( $srcRel === null ) {
247 - return false; // invalid storage path
248 - }
249 -
250 - $timestamp= false;
251 - try {
252 - //TODO Maybe use getBlobData()?
253 - $blob = $this->storageClient->getBlobInstance( $srcCont, $srcRel );
254 - $timestamp = wfTimestamp( TS_MW, $blob->LastModified ); //TODO: Timezone?
255 - } catch ( Exception $e ) {
256 - // TODO: Log it? What about different types of exceptions?
257 - }
258 - return $timestamp;
259 - }
260 -
261 - /**
262258 * @see FileBackend::getFileList()
263259 */
264260 function getFileListInternal( $container, $dir, array $params ) {
265 - // TODO: merely renamed from getFileList. Check implications (i assume, the list thing is no longer needed.
266 - //list( $c, $dir ) = $this->resolveStoragePath( $params['dir'] );
267261 $files = array();
 262+
268263 try {
269 - if ( $dir === null ) {
 264+ if ( trim($dir) == '' ) {
270265 $blobs = $this->storageClient->listBlobs($container);
271266 }
272267 else {
273 - //TODO: Check if $dir really is a startsequence of the blob name
 268+ if ( strrpos($dir, '/') != strlen($dir)-1 ) {
 269+ $dir = $dir.'/';
 270+ }
274271 $blobs = $this->storageClient->listBlobs($container, $dir);
275272 }
276273
277274 foreach( $blobs as $blob ) {
278 - $files[] = $blob->name;
 275+ // Only return the actual file name without the /
 276+ $tempName = $blob->name;
 277+ if ( trim($dir) != '' ) {
 278+ if ( strstr( $tempName, $dir ) !== false ) {
 279+ $tempName = substr($tempName, strpos( $tempName, $dir ) + strlen( $dir ) );
 280+ $files[] = $tempName;
 281+ }
 282+ } else {
 283+ $files[] = $tempName;
 284+ }
 285+
279286 }
280287 }
281288 catch( Exception $e ) {
——@@ -297,7 +304,6 @@
298305 // Get source file extension
299306 $ext = FileBackend::extensionFromPath( $srcRel );
300307 // Create a new temporary file...
301 - // TODO: Caution: tempfile should not write a local file.
302308 $tmpFile = TempFSFile::factory( wfBaseName( $srcRel ) . '_', $ext );
303309 if ( !$tmpFile ) {
304310 return null;
——@@ -308,17 +314,16 @@
309315 $this->storageClient->getBlob( $srcCont, $srcRel, $tmpPath );
310316 }
311317 catch ( Exception $e ) {
312 - $tmpFile = null;
 318+ return null;
313319 }
314320
315321 return $tmpFile;
316322 }
317323
318324 /**
319 - * @see FileBackend::doFileExists()
 325+ * @see FileBackend::getFileStat()
320326 */
321327 protected function doGetFileStat( array $params ) {
322 - // TODO: Refactor
323328 list( $srcCont, $srcRel ) = $this->resolveStoragePathReal( $params['src'] );
324329 if ( $srcRel === null ) {
325330 return false; // invalid storage path
——@@ -326,19 +331,39 @@
327332
328333 $timestamp= false;
329334 $size = false;
330 - // TODO: need additional checks??
 335+
331336 try {
332 - // TODO: Maybe use getBlobData()?
333337 $blob = $this->storageClient->getBlobInstance( $srcCont, $srcRel );
334 - $timestamp = wfTimestamp( TS_MW, $blob->LastModified ); //TODO: Timezone?
 338+ $timestamp = wfTimestamp( TS_MW, $blob->LastModified );
335339 $size = $blob->Size;
336340 return array(
337341 'mtime' => $timestamp,
338342 'size' => $size
339343 );
340344 } catch ( Exception $e ) {
341 - // TODO: Log it? What about different types of exceptions?
 345+ $stat = null;
342346 }
343347 return false;
344348 }
 349+
 350+ /**
 351+ * Check if a file can be created at a given storage path.
 352+ * FS backends should check if the parent directory exists and the file is writable.
 353+ * Backends using key/value stores should check if the container exists.
 354+ *
 355+ * @param $storagePath string
 356+ * @return bool
 357+ */
 358+
 359+ public function isPathUsableInternal( $storagePath ) {
 360+ list( $c, $dir ) = $this->resolveStoragePath( $storagePath );
 361+ if ( $dir === null ) {
 362+ return false;
 363+ }
 364+ if ( !$this->storageClient->containerExists( $c ) ) {
 365+ return false;
 366+ }
 367+
 368+ return true;
 369+ }
345370 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r110042* follow up r110039...mglaser01:02, 26 January 2012

Comments

#Comment by Aaron Schulz (talk | contribs)   00:48, 26 January 2012

I see $this->storageClient->blobExists() checks were they are only needed if there is no 'overwrite' param. They should only be called in that case.

Also, I'd kill doFileExists() since it won't be called.

#Comment by Mglaser (talk | contribs)   01:03, 26 January 2012

Thanks, Aaron. Fixed both issues in r110042

Status & tagging log