Merge branch 'MDL-33330-files-reference' of git://github.com/mudrd8mz/moodle
authorDan Poltawski <dan@moodle.com>
Wed, 6 Jun 2012 08:18:30 +0000 (16:18 +0800)
committerDan Poltawski <dan@moodle.com>
Wed, 6 Jun 2012 08:18:30 +0000 (16:18 +0800)
Conflicts:
version.php
    lib/db/upgrade.php

lib/db/install.xml
lib/db/upgrade.php
lib/filestorage/file_exceptions.php
lib/filestorage/file_storage.php
lib/filestorage/tests/file_storage_test.php
version.php

index d4b7077..2cbd0e9 100644 (file)
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" ?>
-<XMLDB PATH="lib/db" VERSION="20120521" COMMENT="XMLDB file for core Moodle tables"
+<XMLDB PATH="lib/db" VERSION="20120531" COMMENT="XMLDB file for core Moodle tables"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:noNamespaceSchemaLocation="../../lib/xmldb/xmldb.xsd"
 >
         <FIELD NAME="repositoryid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" PREVIOUS="id" NEXT="lastsync"/>
         <FIELD NAME="lastsync" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false" COMMENT="Last time the proxy file was synced with repository" PREVIOUS="repositoryid" NEXT="lifetime"/>
         <FIELD NAME="lifetime" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false" COMMENT="How often do we have to sync proxy file with repository" PREVIOUS="lastsync" NEXT="reference"/>
-        <FIELD NAME="reference" TYPE="text" NOTNULL="false" SEQUENCE="false" PREVIOUS="lifetime"/>
+        <FIELD NAME="reference" TYPE="text" NOTNULL="false" SEQUENCE="false" COMMENT="Identification of the external file. Repository plugins are interpreting it to locate the external file." PREVIOUS="lifetime" NEXT="referencehash"/>
+        <FIELD NAME="referencehash" TYPE="char" LENGTH="40" NOTNULL="true" SEQUENCE="false" COMMENT="Internal implementation detail, contains SHA1 hash of the reference field. Can be indexed and used for comparison. Not meant to be used by a non-core code." PREVIOUS="reference"/>
       </FIELDS>
       <KEYS>
         <KEY NAME="primary" TYPE="primary" FIELDS="id" NEXT="repositoryid"/>
         <KEY NAME="repositoryid" TYPE="foreign" FIELDS="repositoryid" REFTABLE="repository_instances" REFFIELDS="id" PREVIOUS="primary"/>
       </KEYS>
+      <INDEXES>
+        <INDEX NAME="uq_external_file" UNIQUE="true" FIELDS="repositoryid, referencehash" COMMENT="The combination of repositoryid and reference field is supposed to be a unique identification of an external file. Because the reference is a TEXT field, we can't use to compose the index. So we use the referencehash instead and the file API is responsible to keep it up-to-date"/>
+      </INDEXES>
     </TABLE>
     <TABLE NAME="repository" COMMENT="This table contains one entry for every configured external repository instance." PREVIOUS="files_reference" NEXT="repository_instances">
       <FIELDS>
index ba53f99..f303a26 100644 (file)
@@ -769,5 +769,66 @@ function xmldb_main_upgrade($oldversion) {
         upgrade_main_savepoint(true, 2012052900.05);
     }
 
+    if ($oldversion < 2012060600.01) {
+        // Add field referencehash to files_reference
+        $table = new xmldb_table('files_reference');
+        $field = new xmldb_field('referencehash', XMLDB_TYPE_CHAR, '40', null, XMLDB_NOTNULL, null, null, 'reference');
+        if (!$dbman->field_exists($table, $field)) {
+            $dbman->add_field($table, $field);
+        }
+        upgrade_main_savepoint(true, 2012060600.01);
+    }
+
+    if ($oldversion < 2012060600.02) {
+        // Populate referencehash field with SHA1 hash of the reference - this shoudl affect only 2.3dev sites
+        // that were using the feature for testing. Production sites have the table empty.
+        $rs = $DB->get_recordset('files_reference', null, '', 'id, reference');
+        foreach ($rs as $record) {
+            $hash = sha1($record->reference);
+            $DB->set_field('files_reference', 'referencehash', $hash, array('id' => $record->id));
+        }
+        $rs->close();
+
+        upgrade_main_savepoint(true, 2012060600.02);
+    }
+
+    if ($oldversion < 2012060600.03) {
+        // Merge duplicate records in files_reference that were created during the development
+        // phase at 2.3dev sites. This is needed so we can create the unique index over
+        // (repositoryid, referencehash) fields.
+        $sql = "SELECT repositoryid, referencehash, MIN(id) AS minid
+                  FROM {files_reference}
+              GROUP BY repositoryid, referencehash
+                HAVING COUNT(*) > 1;";
+        $duprs = $DB->get_recordset_sql($sql);
+        foreach ($duprs as $duprec) {
+            // get the list of all ids in {files_reference} that need to be remapped
+            $dupids = $DB->get_records_select('files_reference', "repositoryid = ? AND referencehash = ? AND id > ?",
+                array($duprec->repositoryid, $duprec->referencehash, $duprec->minid), '', 'id');
+            $dupids = array_keys($dupids);
+            // relink records in {files} that are now referring to a duplicate record
+            // in {files_reference} to refer to the first one
+            list($subsql, $subparams) = $DB->get_in_or_equal($dupids);
+            $DB->set_field_select('files', 'referencefileid', $duprec->minid, "referencefileid $subsql", $subparams);
+            // and finally remove all orphaned records from {files_reference}
+            $DB->delete_records_list('files_reference', 'id', $dupids);
+        }
+        $duprs->close();
+
+        upgrade_main_savepoint(true, 2012060600.03);
+    }
+
+    if ($oldversion < 2012060600.04) {
+        // Add a unique index over repositoryid and referencehash fields in files_reference table
+        $table = new xmldb_table('files_reference');
+        $index = new xmldb_index('uq_external_file', XMLDB_INDEX_UNIQUE, array('repositoryid', 'referencehash'));
+
+        if (!$dbman->index_exists($table, $index)) {
+            $dbman->add_index($table, $index);
+        }
+
+        upgrade_main_savepoint(true, 2012060600.04);
+    }
+
     return true;
-}
\ No newline at end of file
+}
index 68c01de..425e304 100644 (file)
@@ -116,23 +116,31 @@ class file_pool_content_exception extends file_exception {
     }
 }
 
+
 /**
- * Exception related to external file support
+ * Problem with records in the {files_reference} table
  *
  * @package   core_files
- * @category  files
- * @copyright 2012 Dongsheng Cai {@link http://dongsheng.org}
+ * @catehory  files
+ * @copyright 2012 David Mudrak <david@moodle.com>
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class external_file_exception extends file_exception {
+class file_reference_exception extends file_exception {
     /**
      * Constructor
      *
-     * @param string   $errorcode error code
-     * @param stdClass $a extra information
-     * @param string   $debuginfo extra debug info
+     * @param int $repositoryid the id of the repository that provides the referenced file
+     * @param string $reference the information for the repository to locate the file
+     * @param int|null $referencefileid the id of the record in {files_reference} if known
+     * @param int|null $fileid the id of the referrer's record in {files} if known
+     * @param string|null $debuginfo extra debug info
      */
-    public function __construct($errorcode, $a = null, $debuginfo = null) {
-        parent::__construct($errorcode, '', '', $a, $debuginfo);
+    function __construct($repositoryid, $reference, $referencefileid=null, $fileid=null, $debuginfo=null) {
+        $a = new stdClass();
+        $a->repositoryid = $repositoryid;
+        $a->reference = $reference;
+        $a->referencefileid = $referencefileid;
+        $a->fileid = $fileid;
+        parent::__construct('filereferenceproblem', $a, $debuginfo);
     }
 }
index d5ce7c1..203337a 100644 (file)
@@ -859,19 +859,13 @@ class file_storage {
             return $this->get_file_instance($newrecord);
         }
 
+        // note: referencefileid is copied from the original file so that
+        // creating a new file from an existing alias creates new alias implicitly.
+        // here we just check the database consistency.
         if (!empty($newrecord->repositoryid)) {
-            try {
-                $referencerecord = new stdClass;
-                $referencerecord->repositoryid = $newrecord->repositoryid;
-                $referencerecord->reference = $newrecord->reference;
-                $referencerecord->lastsync  = $newrecord->referencelastsync;
-                $referencerecord->lifetime  = $newrecord->referencelifetime;
-                $referencerecord->id = $DB->insert_record('files_reference', $referencerecord);
-            } catch (dml_exception $e) {
-                throw new stored_file_creation_exception($newrecord->contextid, $newrecord->component, $newrecord->filearea, $newrecord->itemid,
-                                                         $newrecord->filepath, $newrecord->filename, $e->debuginfo);
+            if ($newrecord->referencefileid != $this->get_referencefileid($newrecord->repositoryid, $newrecord->reference, MUST_EXIST)) {
+                throw new file_reference_exception($newrecord->repositoryid, $newrecord->reference, $newrecord->referencefileid);
             }
-            $newrecord->referencefileid = $referencerecord->id;
         }
 
         try {
@@ -1180,12 +1174,12 @@ class file_storage {
     }
 
     /**
-     * Create a moodle file from file reference information
+     * Create a new alias/shortcut file from file reference information
      *
-     * @param stdClass $filerecord
-     * @param int $repositoryid
-     * @param string $reference
-     * @param array $options options for creating external file
+     * @param stdClass|array $filerecord object or array describing the new file
+     * @param int $repositoryid the id of the repository that provides the original file
+     * @param string $reference the information required by the repository to locate the original file
+     * @param array $options options for creating the new file
      * @return stored_file
      */
     public function create_file_from_reference($filerecord, $repositoryid, $reference, $options = array()) {
@@ -1268,20 +1262,13 @@ class file_storage {
 
         $transaction = $DB->start_delegated_transaction();
 
-        // Insert file reference record.
         try {
-            $referencerecord = new stdClass;
-            $referencerecord->repositoryid = $repositoryid;
-            $referencerecord->reference = $reference;
-            $referencerecord->lastsync = $filerecord->referencelastsync;
-            $referencerecord->lifetime = $filerecord->referencelifetime;
-            $referencerecord->id = $DB->insert_record('files_reference', $referencerecord);
-        } catch (dml_exception $e) {
-            throw $e;
+            $filerecord->referencefileid = $this->get_or_create_referencefileid($repositoryid, $reference,
+                $filerecord->referencelastsync, $filerecord->referencelifetime);
+        } catch (Exception $e) {
+            throw new file_reference_exception($repositoryid, $reference, null, null, $e->getMessage());
         }
 
-        $filerecord->referencefileid = $referencerecord->id;
-
         // External file doesn't have content in moodle.
         // So we create an empty file for it.
         list($filerecord->contenthash, $filerecord->filesize, $newfile) = $this->add_string_to_pool(null);
@@ -1672,59 +1659,74 @@ class file_storage {
     }
 
     /**
-     * Search references by providing reference content
+     * Returns all aliases that link to an external file identified by the given reference
      *
-     * @param string $str
-     * @return array
+     * Aliases in user draft areas are excluded from the returned list.
+     *
+     * @param string $reference identification of the referenced file
+     * @return array of stored_file indexed by its pathnamehash
      */
-    public function search_references($str) {
+    public function search_references($reference) {
         global $DB;
+
+        if (is_null($reference)) {
+            throw new coding_exception('NULL is not a valid reference to an external file');
+        }
+
+        $referencehash = sha1($reference);
+
         $sql = "SELECT ".self::instance_sql_fields('f', 'r')."
                   FROM {files} f
-             LEFT JOIN {files_reference} r
-                       ON f.referencefileid = r.id
-                 WHERE ".$DB->sql_compare_text('r.reference').' = '.$DB->sql_compare_text('?')."
-                 AND (f.component <> ? OR f.filearea <> ?)";
+                  JOIN {files_reference} r ON f.referencefileid = r.id
+                  JOIN {repository_instances} ri ON r.repositoryid = ri.id
+                 WHERE r.referencehash = ?
+                       AND (f.component <> ? OR f.filearea <> ?)";
 
-        $rs = $DB->get_recordset_sql($sql, array($str, 'user', 'draft'));
+        $rs = $DB->get_recordset_sql($sql, array($referencehash, 'user', 'draft'));
         $files = array();
         foreach ($rs as $filerecord) {
-            $file = $this->get_file_instance($filerecord);
-            if ($file->is_external_file()) {
-                $files[$filerecord->pathnamehash] = $file;
-            }
+            $files[$filerecord->pathnamehash] = $this->get_file_instance($filerecord);
         }
 
         return $files;
     }
 
     /**
-     * Search references count by providing reference content
+     * Returns the number of aliases that link to an external file identified by the given reference
      *
-     * @param string $str
+     * Aliases in user draft areas are not counted.
+     *
+     * @param string $reference identification of the referenced file
      * @return int
      */
-    public function search_references_count($str) {
+    public function search_references_count($reference) {
         global $DB;
+
+        if (is_null($reference)) {
+            throw new coding_exception('NULL is not a valid reference to an external file');
+        }
+
+        $referencehash = sha1($reference);
+
         $sql = "SELECT COUNT(f.id)
                   FROM {files} f
-             LEFT JOIN {files_reference} r
-                       ON f.referencefileid = r.id
-                 WHERE ".$DB->sql_compare_text('r.reference').' = '.$DB->sql_compare_text('?')."
-                 AND (f.component <> ? OR f.filearea <> ?)";
+                  JOIN {files_reference} r ON f.referencefileid = r.id
+                  JOIN {repository_instances} ri ON r.repositoryid = ri.id
+                 WHERE r.referencehash = ?
+                       AND (f.component <> ? OR f.filearea <> ?)";
 
-        $count = $DB->count_records_sql($sql, array($str, 'user', 'draft'));
-        return $count;
+        return $DB->count_records_sql($sql, array($referencehash, 'user', 'draft'));
     }
 
     /**
-     * Return all files referring to provided stored_file instance
-     * This won't work for draft files
+     * Returns all aliases that link to the given stored_file
+     *
+     * Aliases in user draft areas are excluded from the returned list.
      *
      * @param stored_file $storedfile
-     * @return array
+     * @return array of stored_file
      */
-    public function get_references_by_storedfile($storedfile) {
+    public function get_references_by_storedfile(stored_file $storedfile) {
         global $DB;
 
         $params = array();
@@ -1734,37 +1736,19 @@ class file_storage {
         $params['itemid']    = $storedfile->get_itemid();
         $params['filename']  = $storedfile->get_filename();
         $params['filepath']  = $storedfile->get_filepath();
-        $params['userid']    = $storedfile->get_userid();
-
-        $reference = self::pack_reference($params);
 
-        $sql = "SELECT ".self::instance_sql_fields('f', 'r')."
-                  FROM {files} f
-             LEFT JOIN {files_reference} r
-                       ON f.referencefileid = r.id
-                 WHERE ".$DB->sql_compare_text('r.reference').' = '.$DB->sql_compare_text('?')."
-                   AND (f.component <> ? OR f.filearea <> ?)";
-
-        $rs = $DB->get_recordset_sql($sql, array($reference, 'user', 'draft'));
-        $files = array();
-        foreach ($rs as $filerecord) {
-            $file = $this->get_file_instance($filerecord);
-            if ($file->is_external_file()) {
-                $files[$filerecord->pathnamehash] = $file;
-            }
-        }
-
-        return $files;
+        return $this->search_references(self::pack_reference($params));
     }
 
     /**
-     * Return the count files referring to provided stored_file instance
-     * This won't work for draft files
+     * Returns the number of aliases that link to the given stored_file
+     *
+     * Aliases in user draft areas are not counted.
      *
      * @param stored_file $storedfile
      * @return int
      */
-    public function get_references_count_by_storedfile($storedfile) {
+    public function get_references_count_by_storedfile(stored_file $storedfile) {
         global $DB;
 
         $params = array();
@@ -1774,19 +1758,8 @@ class file_storage {
         $params['itemid']    = $storedfile->get_itemid();
         $params['filename']  = $storedfile->get_filename();
         $params['filepath']  = $storedfile->get_filepath();
-        $params['userid']    = $storedfile->get_userid();
-
-        $reference = self::pack_reference($params);
-
-        $sql = "SELECT COUNT(f.id)
-                  FROM {files} f
-             LEFT JOIN {files_reference} r
-                       ON f.referencefileid = r.id
-                 WHERE ".$DB->sql_compare_text('r.reference').' = '.$DB->sql_compare_text('?')."
-                 AND (f.component <> ? OR f.filearea <> ?)";
 
-        $count = $DB->count_records_sql($sql, array($reference, 'user', 'draft'));
-        return $count;
+        return $this->search_references_count(self::pack_reference($params));
     }
 
     /**
@@ -1795,7 +1768,7 @@ class file_storage {
      * @param stored_file $storedfile a stored_file instances
      * @return stored_file stored_file
      */
-    public function import_external_file($storedfile) {
+    public function import_external_file(stored_file $storedfile) {
         global $CFG;
         require_once($CFG->dirroot.'/repository/lib.php');
         // sync external file
@@ -1925,5 +1898,60 @@ class file_storage {
 
         return implode(', ', $fields);
     }
-}
 
+    /**
+     * Returns the id of the record in {files_reference} that matches the passed repositoryid and reference
+     *
+     * If the record already exists, its id is returned. If there is no such record yet,
+     * new one is created (using the lastsync and lifetime provided, too) and its id is returned.
+     *
+     * @param int $repositoryid
+     * @param string $reference
+     * @return int
+     */
+    private function get_or_create_referencefileid($repositoryid, $reference, $lastsync = null, $lifetime = null) {
+        global $DB;
+
+        $id = $this->get_referencefileid($repositoryid, $reference, IGNORE_MISSING);
+
+        if ($id !== false) {
+            // bah, that was easy
+            return $id;
+        }
+
+        // no such record yet, create one
+        try {
+            $id = $DB->insert_record('files_reference', array(
+                'repositoryid'  => $repositoryid,
+                'reference'     => $reference,
+                'referencehash' => sha1($reference),
+                'lastsync'      => $lastsync,
+                'lifetime'      => $lifetime));
+        } catch (dml_exception $e) {
+            // if inserting the new record failed, chances are that the race condition has just
+            // occured and the unique index did not allow to create the second record with the same
+            // repositoryid + reference combo
+            $id = $this->get_referencefileid($repositoryid, $reference, MUST_EXIST);
+        }
+
+        return $id;
+    }
+
+    /**
+     * Returns the id of the record in {files_reference} that matches the passed parameters
+     *
+     * Depending on the required strictness, false can be returned. The behaviour is consistent
+     * with standard DML methods.
+     *
+     * @param int $repositoryid
+     * @param string $reference
+     * @param int $strictness either {@link IGNORE_MISSING}, {@link IGNORE_MULTIPLE} or {@link MUST_EXIST}
+     * @return int|bool
+     */
+    private function get_referencefileid($repositoryid, $reference, $strictness) {
+        global $DB;
+
+        return $DB->get_field('files_reference', 'id',
+            array('repositoryid' => $repositoryid, 'referencehash' => sha1($reference)), $strictness);
+    }
+}
index 41feea1..ad3fb49 100644 (file)
@@ -297,7 +297,7 @@ class filestoragelib_testcase extends advanced_testcase {
             $this->assertEquals($key, $file->get_pathnamehash());
         }
 
-        // Get area files ordered by id (was breaking on oracle).
+        // Get area files ordered by id.
         $filesbyid  = $fs->get_area_files($user->ctxid, 'user', 'private', false, 'id', false);
         // Should be the two files without folder.
         $this->assertEquals(3, count($filesbyid));
@@ -405,8 +405,6 @@ class filestoragelib_testcase extends advanced_testcase {
         $fs = get_file_storage();
 
         $areafiles = $fs->get_area_files($user->ctxid, 'user', 'private');
-        //Test get_file_by_hash
-
         $testfile = reset($areafiles);
         $references = $fs->get_references_by_storedfile($testfile);
         // TODO MDL-33368 Verify result!!
@@ -420,9 +418,57 @@ class filestoragelib_testcase extends advanced_testcase {
         $userrepository = reset($repos);
         $this->assertInstanceOf('repository', $userrepository);
 
-        // This should break on oracle.
-        $fs->get_external_files($userrepository->id, 'id');
-        // TODO MDL-33368 Verify result!!
+        // no aliases yet
+        $exfiles = $fs->get_external_files($userrepository->id, 'id');
+        $this->assertEquals(array(), $exfiles);
+
+        // create three aliases linking the same original: $aliasfile1 and $aliasfile2 are
+        // created via create_file_from_reference(), $aliasfile3 created from $aliasfile2
+        $originalfile = null;
+        foreach ($fs->get_area_files($user->ctxid, 'user', 'private') as $areafile) {
+            if (!$areafile->is_directory()) {
+                $originalfile = $areafile;
+                break;
+            }
+        }
+        $this->assertInstanceOf('stored_file', $originalfile);
+        $originalrecord = array(
+            'contextid' => $originalfile->get_contextid(),
+            'component' => $originalfile->get_component(),
+            'filearea'  => $originalfile->get_filearea(),
+            'itemid'    => $originalfile->get_itemid(),
+            'filepath'  => $originalfile->get_filepath(),
+            'filename'  => $originalfile->get_filename(),
+        );
+
+        $aliasrecord = $this->generate_file_record();
+        $aliasrecord->filepath = '/foo/';
+        $aliasrecord->filename = 'one.txt';
+
+        $ref = $fs->pack_reference($originalrecord);
+        $aliasfile1 = $fs->create_file_from_reference($aliasrecord, $userrepository->id, $ref);
+
+        $aliasrecord->filepath = '/bar/';
+        $aliasrecord->filename = 'uno.txt';
+        // change the order of the items in the array to make sure that it does not matter
+        ksort($originalrecord);
+        $ref = $fs->pack_reference($originalrecord);
+        $aliasfile2 = $fs->create_file_from_reference($aliasrecord, $userrepository->id, $ref);
+
+        $aliasrecord->filepath = '/bar/';
+        $aliasrecord->filename = 'jedna.txt';
+        $aliasfile3 = $fs->create_file_from_storedfile($aliasrecord, $aliasfile2);
+
+        // make sure we get three aliases now
+        $exfiles = $fs->get_external_files($userrepository->id, 'id');
+        $this->assertEquals(3, count($exfiles));
+        foreach ($exfiles as $exfile) {
+            $this->assertTrue($exfile->is_external_file());
+        }
+        // make sure they all link the same original (thence that all are linked with the same
+        // record in {files_reference})
+        $this->assertEquals($aliasfile1->get_referencefileid(), $aliasfile2->get_referencefileid());
+        $this->assertEquals($aliasfile3->get_referencefileid(), $aliasfile2->get_referencefileid());
     }
 
     public function test_create_directory_contextid_negative() {
index 813b25b..f6ee588 100644 (file)
@@ -30,7 +30,7 @@
 defined('MOODLE_INTERNAL') || die();
 
 
-$version  = 2012060400.00;              // YYYYMMDD      = weekly release date of this DEV branch
+$version  = 2012060600.04;              // YYYYMMDD      = weekly release date of this DEV branch
                                         //         RR    = release increments - 00 in DEV branches
                                         //           .XX = incremental changes