Clean up ApiQueryImageInfo continuation

Reviewing this code, I spotted a few issues:
* 'imagerepository' is added to non-images, and to images that were
  skipped this round due to iicontinue. The latter is particularly
  troublesome, as clients may wind up with an incorrect value when
  merging continued results.
* Say two images are being queried, A.jpg and B.jpg. If the query needs
  to be continued somewhere in the middle of A.jpg's old versions, but
  then A.jpg is deleted before the client sends the continuation query,
  it will start in the middle of B.jpg's old versions instead of at the
  beginning of B.jpg's revisions.
* If the query needs to be continued somewhere in the middle of A.jpg's
  old versions, but in the continuation query some other module that is
  also being continued fills the result object, iicontinue will be
  reset to the *beginning* of A.jpg's old versions instead of preserving
  the position in the middle.

Change-Id: I08e2941010c7a70ff90b6244bfddd5ed0540fc9f

diff --git a/RELEASE-NOTES-1.21 b/RELEASE-NOTES-1.21
index bf1a20b..6ffe112 100644 (file)
--- a/RELEASE-NOTES-1.21
+++ b/RELEASE-NOTES-1.21
@@ -202,6 +202,10 @@ production.
   redirects.
 * On error, any warnings generated before that error will be shown in the result.
 * action=help suports generalized submodules (modules=query+value), querymodules obsolete
+* ApiQueryImageInfo continuation is more reliable. The only major change is
+  that the imagerepository property will no longer be set on page objects not
+  processed in the current query (i.e. non-images or those skipped due to
+  iicontinue).
 
 === API internal changes in 1.21 ===
 * For debugging only, a new global $wgDebugAPI removes many API restrictions when true.

diff --git a/includes/api/ApiQueryImageInfo.php b/includes/api/ApiQueryImageInfo.php
index 4b9bc98..351753c 100644 (file)
--- a/includes/api/ApiQueryImageInfo.php
+++ b/includes/api/ApiQueryImageInfo.php
@@ -52,9 +52,8 @@ class ApiQueryImageInfo extends ApiQueryBase {
                        $titles = array_keys( $pageIds[NS_FILE] );
                        asort( $titles ); // Ensure the order is always the same
 
-                       $skip = false;
+                       $fromTitle = null;
                        if ( !is_null( $params['continue'] ) ) {
-                               $skip = true;
                                $cont = explode( '|', $params['continue'] );
                                $this->dieContinueUsageIf( count( $cont ) != 2 );
                                $fromTitle = strval( $cont[0] );
@@ -77,12 +76,18 @@ class ApiQueryImageInfo extends ApiQueryBase {
                                $images = RepoGroup::singleton()->findFiles( $titles );
                        }
                        foreach ( $titles as $title ) {
+                               $pageId = $pageIds[NS_FILE][$title];
+                               $start = $title === $fromTitle ? $fromTimestamp : $params['start'];
+
                                if ( !isset( $images[$title] ) ) {
+                                       $result->addValue(
+                                               array( 'query', 'pages', intval( $pageId ) ),
+                                               'imagerepository', ''
+                                       );
+                                       // The above can't fail because it doesn't increase the result size
                                        continue;
                                }
 
-                               $start = $skip ? $fromTimestamp : $params['start'];
-                               $pageId = $pageIds[NS_FILE][$title];
                                $img = $images[$title];
 
                                $fit = $result->addValue(
@@ -97,10 +102,11 @@ class ApiQueryImageInfo extends ApiQueryBase {
                                                // thing again. When the violating queries have been
                                                // out-continued, the result will get through
                                                $this->setContinueEnumParameter( 'start',
-                                                       wfTimestamp( TS_ISO_8601, $img->getTimestamp() ) );
+                                                       $start !== null ? $start : wfTimestamp( TS_ISO_8601, $img->getTimestamp() )
+                                               );
                                        } else {
                                                $this->setContinueEnumParameter( 'continue',
-                                                       $this->getContinueStr( $img ) );
+                                                       $this->getContinueStr( $img, $start ) );
                                        }
                                        break;
                                }
@@ -164,18 +170,6 @@ class ApiQueryImageInfo extends ApiQueryBase {
                                if ( !$fit ) {
                                        break;
                                }
-                               $skip = false;
-                       }
-
-                       $data = $this->getResultData();
-                       foreach ( $data['query']['pages'] as $pageid => $arr ) {
-                               if ( is_array( $arr ) && !isset( $arr['imagerepository'] ) ) {
-                                       $result->addValue(
-                                               array( 'query', 'pages', $pageid ),
-                                               'imagerepository', ''
-                                       );
-                               }
-                               // The above can't fail because it doesn't increase the result size
                        }
                }
        }
@@ -433,9 +427,11 @@ class ApiQueryImageInfo extends ApiQueryBase {
         * @param $img File
         * @return string
         */
-       protected function getContinueStr( $img ) {
-               return $img->getOriginalTitle()->getText() .
-                       '|' . $img->getTimestamp();
+       protected function getContinueStr( $img, $start = null ) {
+               if ( $start === null ) {
+                       $start = $img->getTimestamp();
+               }
+               return $img->getOriginalTitle()->getText() . '|' . $start;
        }
 
        public function getAllowedParams() {