WIP docs and tests for refactored method

matthewhilton · matthewhilton · commit e7a8f785471f · 2024-07-26T12:38:28.000+10:00
diff --git a/TAGGING.md b/TAGGING.md
@@ -0,0 +1,39 @@
+# Tagging
+
+todo blurb about tagging
+
+You probably don't need this unless you have a lot of objects!
+
+Support level: currenly only implemented for AWS/S3
+
+Note extra cost, extra api calls + tagging cost per object per month
+
+## Sources
+
+Constraints:
+- Key - max 128 chars (aws + azure)
+- Value - max 128 chars (actual max is 256, but reserving half for future use e.g. versioning)
+- Deterministic
+
+Implemented:
+- Mimetype 
+- Env - defined by <insert cfg here>
+
+## Multi env
+- Turn off object override on every env except prod
+- TODO diagram here
+
+## Reporting
+TODO blurb about reporting
+Note reporting quirk because of multi env setup and what each env knows about.
+
+## Migration
+
+TODO
+If you change any of the tags e.g. the way they work and want to re-update tags, update tag status to needs sync and queue adhoc task to re-sync.
+
+## For developers
+
+### Adding a new source
+- Add to <insert place in tag manager here>
+- Run migration steps (TODO link to above)
diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php
@@ -36,7 +36,10 @@
 use stored_file;
 use file_storage;
 use BlobRestProxy;
+use coding_exception;
+use Throwable;
 use tool_objectfs\local\manager;
+use tool_objectfs\local\tag\tag_manager;
 
 defined('MOODLE_INTERNAL') || die();
 
@@ -1154,4 +1157,50 @@ private function update_object(array $result): array {
 
         return $result;
     }
+
+    /**
+     * Syncs tags post upload for a given hash.
+     * Note this function assumes tagging is enable and supported by the fs.
+     *
+     * @param string $contenthash file to sync tags for
+     */
+    public function sync_object_tags(string $contenthash) {
+        // Get a lock before syncing, to ensure other parts of objectfs are not moving/interacting with this object.
+        $lock = $this->acquire_object_lock($contenthash, 5);
+
+        // No lock - just skip it.
+        if (!$lock) {
+            throw new coding_exception("Could not get object lock"); // TODO different ex type?
+        }
+
+        try {
+            $objectexists = $this->is_file_readable_externally_by_hash($contenthash);
+            // If object does not exist, cannot sync tags to nothing, abort.
+            if (!$objectexists) {
+                // TODO maybe this should be a failed status?
+                tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED);
+                return;
+            }
+
+            // Cannot override and object exists, query existing and store.
+            if (!manager::can_override_existing_objects() && $objectexists) {
+                // Query existing tags and store them.
+                $existingtags = $this->get_external_client()->get_object_tags($contenthash);
+                tag_manager::store_tags_locally($contenthash, $existingtags);
+
+            // Else can override, upload new and store.
+            } else {
+                $tags = tag_manager::gather_object_tags_for_upload($contenthash);
+                $this->get_external_client()->set_object_tags($contenthash, $tags);
+                tag_manager::store_tags_locally($contenthash, $tags);
+            }
+
+            // Either way, it has synced.
+            tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED);
+        } catch (Throwable $e) {
+            $lock->release();
+            throw $e;
+        }
+        $lock->release();
+    }
 }
diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php
@@ -160,40 +160,6 @@ public static function mark_object_tag_sync_status(string $contenthash, int $sta
         $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]);
     }
 
-    /**
-     * Syncs an objects tags after they are uploaded.
-     * Note - you should take out the object lock before calling this function to avoid race conditions.
-     * @param string $contenthash
-     * @param object_file_system $fs
-     */
-    public static function sync_object_tags_post_upload(string $contenthash, object_file_system $fs) {
-        $client = $fs->get_external_client();
-        $objectexists = $fs->is_file_readable_externally_by_hash($contenthash);
-
-        // If object does not exist, cannot sync tags to nothing, abort.
-        if (!$objectexists) {
-            // TODO maybe this should be a failed status?
-            self::mark_object_tag_sync_status($contenthash, self::SYNC_STATUS_SYNC_NOT_REQUIRED);
-            return;
-        }
-
-        // Cannot override and object exists, query existing and store.
-        if (!manager::can_override_existing_objects() && $objectexists) {
-            // Query existing tags and store them.
-            $existingtags = $client->get_object_tags($contenthash);
-            self::store_tags_locally($contenthash, $existingtags);
-
-        // Else can override, upload new and store.
-        } else {
-            $tags = self::gather_object_tags_for_upload($contenthash);
-            $client->set_object_tags($contenthash, $tags);
-            self::store_tags_locally($contenthash, $tags);
-        }
-
-        // Either way, it has synced.
-        self::mark_object_tag_sync_status($contenthash, self::SYNC_STATUS_SYNC_NOT_REQUIRED);
-    }
-
     /**
      * Returns a simple list of all the sources and their descriptions.
      * @return string html string
diff --git a/classes/task/update_object_tags.php b/classes/task/update_object_tags.php
@@ -50,29 +50,14 @@ public function execute() {
         // Sanity check that fs is object file system and not anything else.
         $fs = get_file_storage()->get_file_system();
 
-        if (!method_exists($fs, "acquire_object_lock")) {
+        if (!method_exists($fs, "sync_object_tags")) {
             mtrace("File system is not object file system, exiting");
             return;
         }
 
         // For each, try to sync their tags.
         foreach ($contenthashes as $contenthash) {
-            // Get a lock before syncing, to ensure other parts of objectfs are not moving/interacting with this object.
-            $lock = $fs->acquire_object_lock($contenthash, 5);
-
-            // No lock - just skip it.
-            if (!$lock) {
-                continue;
-            }
-
-            try {
-                mtrace("Syncing tags for " . $contenthash);
-                tag_manager::sync_object_tags_post_upload($contenthash);
-            } catch (Throwable $e) {
-                $lock->release();
-                throw $e;
-            }
-            $lock->release();
+            $fs->sync_object_tags($contenthash);
         }
 
         // Re-queue self to process more in another iteration.
diff --git a/classes/tests/testcase.php b/classes/tests/testcase.php
@@ -39,6 +39,7 @@ protected function setUp(): void {
         global $CFG;
         $CFG->alternative_file_system_class = '\\tool_objectfs\\tests\\test_file_system';
         $CFG->forced_plugin_settings['tool_objectfs']['deleteexternal'] = false;
+        $CFG->objectfs_environment_name = 'test';
         $this->filesystem = new test_file_system();
         $this->logger = new \tool_objectfs\log\null_logger();
         $this->resetAfterTest(true);
diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php
@@ -76,11 +76,17 @@ public function test_tag_sources_identifier(tag_source $source) {
      * @dataProvider tag_source_provider
      */
     public function test_tag_sources_value(tag_source $source) {
-        // Ensure tag value is < 256 chars, to fit AWS & Azure spec.
+        // Ensure tag value is < 128 chars, AWS & Azure spec allow for 256, but we reserve 128 for future use.
         $file = $this->create_duplicated_object();
         $value = $source->get_value_for_contenthash($file->contenthash);
+
+        // Null value - allowed, but means we cannot test.
+        if (is_null($value)) {
+            return;
+        }
+
         $count = strlen($value);
-        $this->assertLessThan(256, $count);
+        $this->assertLessThan(128, $count);
         $this->assertGreaterThan(0, $count);
     }
 
@@ -135,66 +141,43 @@ public function test_is_tagging_enabled_and_supported(bool $enabledinconfig, boo
         $this->assertEquals($expected, tag_manager::is_tagging_enabled_and_supported());
     }
 
-    /**
-     * Tests query_local_tags
-     * @covers \tool_objectfs\local\tag_manager::query_local_tags
-     */
-    public function test_query_local_tags() {
-        global $DB;
+    public function test_gather_object_tags_for_upload() {
+        $object = $this->create_duplicated_object();
+        $tags = tag_manager::gather_object_tags_for_upload($object->contenthash);
 
-        // Setup some fake tag data.
-        $DB->insert_records('tool_objectfs_object_tags', [
-            [
-                'contenthash' => 'abc123',
-                'tagkey' => 'test',
-                'tagvalue' => 'test',
-                'timemodified' => time()
-            ],
-            [
-                'contenthash' => 'abc123',
-                'tagkey' => 'test2',
-                'tagvalue' => 'test2',
-                'timemodified' => time()
-            ]
-        ]);
-
-        $this->assertCount(2, tag_manager::query_local_tags('abc123'));
-        $this->assertCount(0, tag_manager::query_local_tags('doesnotexist'));
+        $this->assertArrayHasKey('mimetype', $tags);
+        $this->assertArrayHasKey('environment', $tags);
+        $this->assertEquals('text', $tags['mimetype']);
+        $this->assertEquals('test', $tags['environment']);
     }
 
-    /**
-     * Tests update_tags_if_necessary
-     * @covers \tool_objectfs\local\tag_manager::update_tags_if_necessary
-     */
-    public function test_update_tags_if_necessary() {
+    public function test_store_tags_locally() {
         global $DB;
 
-        // There should be no existing tags to begin with.
-        $this->assertEmpty(tag_manager::query_local_tags('test'));
-
-        // Update tags if necessary - should update the tags.
-        tag_manager::update_tags_if_necessary('test');
+        $tags = [
+            'test1' => 'abc',
+            'test2' => 'xyz'
+        ];
+        $hash = 'thisisatest';
 
-        // Query tags - should be equal to number defined by the manager.
-        $expectedcount = count(tag_manager::get_defined_tag_sources());
-        $this->assertCount($expectedcount, tag_manager::query_local_tags('test'), 'Tags created match the number defined by manager');
+        // Ensure no tags for hash intially.
+        $this->assertEmpty($DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash]));
 
-        // Note the timemodified of one of the tags.
-        $timemodified = $DB->get_field('tool_objectfs_object_tags', 'timemodified', ['contenthash' => 'test', 'tagkey' => tag_manager::get_defined_tag_sources()[0]->get_identifier()]);
-        
-        // Running again, timemodified should not change.
-        $this->waitForSecond();
-        tag_manager::update_tags_if_necessary('test');
+        // Store.
+        tag_manager::store_tags_locally($hash, $tags);
 
-        $newtimemodified = $DB->get_field('tool_objectfs_object_tags', 'timemodified', ['contenthash' => 'test', 'tagkey' => tag_manager::get_defined_tag_sources()[0]->get_identifier()]);
-        $this->assertEquals($timemodified, $newtimemodified, 'Tags timemodified must not change if not forced and values are not different');
+        // Confirm they are stored.
+        $queriedtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash]);
+        $this->assertCount(2, $queriedtags);
+        $tagtimebefore = current($queriedtags)->timemodified;
 
-        // Except if it is forced.
+        // Re-store, confirm times changed.
         $this->waitForSecond();
-        tag_manager::update_tags_if_necessary('test', true);
-        
-        $timemodifiedafterforce = $DB->get_field('tool_objectfs_object_tags', 'timemodified', ['contenthash' => 'test', 'tagkey' => tag_manager::get_defined_tag_sources()[0]->get_identifier()]);
-        $this->assertNotEquals($timemodified, $timemodifiedafterforce, 'Forced tag update changed time modified');
+        tag_manager::store_tags_locally($hash, $tags);
+        $queriedtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash]);
+        $tagtimeafter = current($queriedtags)->timemodified;
+
+        $this->assertNotSame($tagtimebefore, $tagtimeafter);
     }
 
     /**
@@ -288,27 +271,9 @@ public function test_get_objects_needing_sync_limit() {
         $this->assertCount(1, tag_manager::get_objects_needing_sync(1));
     }
     
-    /**
-     * Tests replicate_local_to_external_tags_for_object
-     * @covers \tool_objectfs\local\tag_manager::replicate_local_to_external_tags_for_object
-     */
-    public function test_replicate_local_to_external_tags_for_object() {
-        global $DB;
-
-        // Enable tagging, setup test fs.
-        $config = manager::get_objectfs_config();
-        $config->taggingenabled = true;
-        $config->enabletasks = true;
-        $config->filesystem = '\\tool_objectfs\\tests\\test_file_system';
-        manager::set_objectfs_config($config);
-
-        // Setup object, mark as needing sync.
-        $object = $this->create_remote_object();
-        $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', tag_manager::SYNC_STATUS_NEEDS_SYNC, ['id' => $object->id]);
-        tag_manager::replicate_local_to_external_tags_for_object($object->contenthash);
-
-        // Ensure status is now set as not needing sync.
-        $status = $DB->get_field('tool_objectfs_objects', 'tagsyncstatus', ['id' => $object->id]);
-        $this->assertEquals(tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, $status);
+    public function test_get_tag_summary_html() {
+        // Quick test just to ensure it generates and nothing explodes.
+        $html = tag_manager::get_tag_summary_html();
+        $this->assertIsString($html);
     }
 }
diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php
@@ -1016,4 +1016,6 @@ public function test_add_file_from_string_update_object_fail() {
         $this->assertEquals(\core_text::strlen($content), $result[1]);
         $this->assertTrue($result[2]);
     }
+
+    // TODO test tag syncing somehow ? Using a test FS ? Maybe test FS can store them in memory ?
 }

Original file line number	Diff line number	Diff line change
`@@ -1016,4 +1016,6 @@ public function test_add_file_from_string_update_object_fail() {`
`1016`	`1016`	`$this->assertEquals(\core_text::strlen($content), $result[1]);`
`1017`	`1017`	`$this->assertTrue($result[2]);`
`1018`	`1018`	`}`
	`1019`	`+`
	`1020`	`+ // TODO test tag syncing somehow ? Using a test FS ? Maybe test FS can store them in memory ?`
`1019`	`1021`	`}`