WIP simplification of api

Author: matthewhilton

classes/local/tag/tag_manager.php

@@ -78,6 +78,11 @@ public static function is_tagging_enabled_and_supported(): bool {
         return $enabledinconfig && $supportedbyfs;
     }
 
+    // TODO unit tests for this functionality
+    public static function can_override_existing_tags(): bool {
+        return !empty(get_config('tool_objectfs', 'overridetags'));
+    }
+
     /**
      * Gathers the tag values for a given content hash
      * @param string $contenthash
@@ -98,41 +103,6 @@ private static function gather_tags(string $contenthash): array {
         return $tags;
     }
 
-    /**
-     * Returns the tags stored locally in the DB for the given file contenthash
-     * @param int $objectid
-     * @return array array of key=>value pairs, the tags for the given file that are stored in the database. 
-     */
-    public static function query_local_tags(string $contenthash): array {
-        global $DB;
-        $records = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $contenthash], '', 'tagkey,tagvalue');
-
-        $tags = [];
-        foreach ($records as $record) {
-            $tags[$record->tagkey] = $record->tagvalue;
-        }
-        return $tags;
-    }
-
-    /**
-     * Synchronises the tags for a given file.
-     *
-     * @param string $contenthash file to update tags for
-     * @param bool $force By default, will not update stored tags unless the gathered tags differ from the locally stored tags. If this is true, it will always store the newest tags.
-     */
-    public static function update_tags_if_necessary(string $contenthash, bool $force = false) {
-        // Gather the exact current and compare against local db.
-        $gatheredtags = self::gather_tags($contenthash);
-        $localtags = self::query_local_tags($contenthash);	
-
-        // Update if forced to, or if what we just gathered is not the same as what is stored in the database.
-        $shouldupdate = $force || self::are_tags_different($gatheredtags, $localtags);
-
-        if ($shouldupdate) {
-            self::store_tags($contenthash, $gatheredtags);
-        }
-    }
-
     /**
      * Updates the local db tag records, and marks object as needing sync to sync local tags to external.
      * @param int $objectid
@@ -141,39 +111,23 @@ public static function update_tags_if_necessary(string $contenthash, bool $force
     private static function store_tags(string $contenthash, array $tags) {
         global $DB;
 
-        // Get a lock, since we are modifying the tags in the DB.
-        $lock = self::get_object_tag_lock($contenthash);
-
-        if (!$lock) {
-            throw new coding_exception("Unable to obtain lock for object " . $contenthash); // TODO different ex type ?
-        }
-
-        try {
-            // Purge any existing tags for this object.
-            $DB->delete_records('tool_objectfs_object_tags', ['contenthash' => $contenthash]);
-
-            // Record time in var, so that they all have the same time.
-            $timemodified = time();
-
-            // Store new records.
-            $recordstostore = [];
-            foreach ($tags as $key => $value) {
-                $recordstostore[] = [
-                    'contenthash' => $contenthash,
-                    'tagkey' => $key,
-                    'tagvalue' => $value,
-                    'timemodified' => $timemodified
-                ];
-            }
-            $DB->insert_records('tool_objectfs_object_tags', $recordstostore);
-
-            // We always mark them needing sync after changing the values.
-            self::mark_object_tag_sync_status($contenthash, self::SYNC_STATUS_NEEDS_SYNC);
-        } catch (Exception $e) {
-            // Failed - release lock and rethrow.
-            $lock->release();
-            throw $e;
+        // Purge any existing tags for this object.
+        $DB->delete_records('tool_objectfs_object_tags', ['contenthash' => $contenthash]);
+
+        // Record time in var, so that they all have the same time.
+        $timemodified = time();
+
+        // Store new records.
+        $recordstostore = [];
+        foreach ($tags as $key => $value) {
+            $recordstostore[] = [
+                'contenthash' => $contenthash,
+                'tagkey' => $key,
+                'tagvalue' => $value,
+                'timemodified' => $timemodified
+            ];
         }
+        $DB->insert_records('tool_objectfs_object_tags', $recordstostore);
     }
 
     /**
@@ -188,15 +142,19 @@ public static function get_objects_needing_sync(int $limit) {
         [$insql, $inparams] = $DB->get_in_or_equal([OBJECT_LOCATION_DUPLICATED, OBJECT_LOCATION_EXTERNAL, OBJECT_LOCATION_ORPHANED], SQL_PARAMS_NAMED);
         $inparams['syncstatus'] = self::SYNC_STATUS_NEEDS_SYNC;
 
+        // TODO somehow exclude ones already queued ?
         $records = $DB->get_records_select('tool_objectfs_objects', 'tagsyncstatus = :syncstatus AND location ' . $insql, $inparams, '', 'contenthash', 0, $limit);
         return array_column($records, 'contenthash');
     }
 
     /**
-     * Replicates the tags stored locally in the DB for a given object to the external object store.
+     * Gathers the tag values for a given object contenthash, stores the values in the DB and then replicates these to the external store.
+     * Will mark the object sync status to sync status not required.
      * @param string $contenthash
      */
-    public static function replicate_local_to_external_tags_for_object(string $contenthash) {
+    public static function calculate_tags_store_locally_and_replicate_to_external(string $contenthash) {
+        // TODO someway to genericise this, so we can call it with and without replicating ,etc...
+
         // Take lock, to ensure tags are not updated while we sync them.
         $lock = self::get_object_tag_lock($contenthash);
 
@@ -205,18 +163,24 @@ public static function replicate_local_to_external_tags_for_object(string $conte
         }
 
         try {
-            // TODO refactor so all func use contenthash, no objectid.
-            // Then also probably remove from DB.
-            $tags = self::query_local_tags($contenthash);
-
-            // Get current client.
+            // Get current client, ensure it is configured correctly.
             $client = manager::get_client(manager::get_objectfs_config());
 
             if (empty($client)) {
                 throw new coding_exception("No client set, cannot replicate tags"); // TODO moodle ex ?
             }
 
-            $client->set_object_tags($contenthash, $tags);
+            // Sanity check, ensure supports object tagging.
+            if (!$client->supports_object_tagging()) {
+                throw new coding_exception("Client does not support object tagging"); // TODO different ex ?
+            }
+
+            // Gather tags from sources & store locally.
+            $gatheredtags = self::gather_tags($contenthash);
+            self::store_tags($contenthash, $gatheredtags);
+
+            // Replicate to external.
+            $client->set_object_tags($contenthash, $gatheredtags);
 
             // Mark as synced, this stops it from re-syncing in the future.
             self::mark_object_tag_sync_status($contenthash, self::SYNC_STATUS_SYNC_NOT_REQUIRED);
@@ -226,6 +190,23 @@ public static function replicate_local_to_external_tags_for_object(string $conte
         }
     }
 
+    /**
+     * This should only ever be called as part of uploading an object for the first time.
+     */
+    public static function calculate_tags_and_store_locally_and_return(string $contenthash): array {
+        // TODO.
+    }
+
+    private static function should_set_object_tags(string $contenthash) {
+        // If can override, always return true.
+        if (self::can_override_existing_tags()) {
+            return true;
+        }
+
+        // Else lookup if object already has tags, or if it even exists.
+        // TODO.
+    }
+
     /**
      * Gets a tag writing lock on the given object, ensuring tags are not written/updated while being synced, for example.
      * @param string $contenthash
@@ -249,17 +230,4 @@ private static function mark_object_tag_sync_status(string $contenthash, int $st
         }
         $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]);
     }
-
-    /**
-     * Returns true if the tags given are different. They are different when their keys or values do not match, order does not matter.
-     * @param array $a array of key=>value where keys and values are both strings
-     * @param array $b array of key=>value where keys and values are both strings
-     * @return bool
-     */
-    private static function are_tags_different(array $a, array $b): bool {
-        // Order the keys in both arrays, then do a simple equality check.
-        ksort($a);
-        ksort($b);
-        return $a !== $b;
-    }
 }

classes/task/update_object_tags.php

@@ -45,10 +45,6 @@ public function execute() {
             return;
         }
 
-        // Update local tags in db.
-        tag_manager::update_tags_if_necessary($contenthash);
-
-        // Sync these to external store.
-        tag_manager::replicate_local_to_external_tags_for_object($contenthash);
+        tag_manager::calculate_tags_store_locally_and_replicate_to_external($contenthash);
     }
 }

lang/en/tool_objectfs.php

@@ -278,4 +278,6 @@
 $string['check:tagging:ok'] = 'Object tagging ok';
 $string['check:tagging:error'] = 'Error trying to tag object';
 $string['settings:maxtaggingperrun'] = 'Max objects tagged per run';
-$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to be queued for tag updates per run of the queue_objects_needing_tags scheduled task.';
+$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to be queued for tag updates per run of the queue_objects_needing_tags scheduled task.';
+$string['settings:overridetags'] = 'Override existing object tags';
+$string['settings:overridetags:desc'] = 'If enabled, ObjectFS will override any existing object tags as needed. If disabled and object already has tags, they will be left untouched. Primarily for use in situations where multiple environments target the same storage bucket. Generally, you will enable this in production, but disable everywhere else. See the README for more information.';

settings.php

@@ -255,13 +255,17 @@
         new lang_string('settings:taggingheader', 'tool_objectfs'), ''));
 
     // TODO some info here about tagging, maybe a link to doc in git.
-
     $settings->add(new admin_setting_configcheckbox('tool_objectfs/taggingenabled',
-        new lang_string('settings:taggingenabled', 'tool_objectfs'), '', ''));
+        new lang_string('settings:taggingenabled', 'tool_objectfs'), '', 0));
 
     $settings->add(new admin_setting_configtext('tool_objectfs/maxtaggingperrun',
         new lang_string('settings:maxtaggingperrun', 'tool_objectfs'),
         get_string('settings:maxtaggingperrun:desc', 'tool_objectfs'),
         10000,
         PARAM_INT));
+
+    $settings->add(new admin_setting_configcheckbox('tool_objectfs/overridetags',
+        new lang_string('settings:overridetags', 'tool_objectfs'),
+        get_string('settings:overridetags:desc', 'tool_objectfs'),
+        1));
 }