WIP more cleanups and refactors

Author: matthewhilton

classes/local/manager.php

@@ -384,4 +384,8 @@ public static function get_filename_from_header($header) {
         }
         return $filename;
     }
+    
+    public static function can_override_existing_objects() {
+        return !empty(get_config('tool_objectfs', 'canoverrideobjects'));
+    }
 }

classes/local/store/object_file_system.php

@@ -37,7 +37,6 @@
 use file_storage;
 use BlobRestProxy;
 use tool_objectfs\local\manager;
-use tool_objectfs\local\tag\tag_manager;
 
 defined('MOODLE_INTERNAL') || die();
 
@@ -1155,7 +1154,4 @@ private function update_object(array $result): array {
 
         return $result;
     }
-
-    public function sync_tags(string $contenthash) {
-    }
 }

classes/local/store/s3/client.php

@@ -25,6 +25,7 @@
 
 namespace tool_objectfs\local\store\s3;
 
+use coding_exception;
 use tool_objectfs\local\manager;
 use tool_objectfs\local\store\object_client_base;
 use tool_objectfs\local\store\signed_url;
@@ -62,10 +63,11 @@ class client extends object_client_base {
      */
     private $signingmethod;
 
+    /**
+     * @var mixed
+     */
     protected $bucketkeyprefix;
 
-    protected $allowoverride;
-
     /**
      * construct
      * @param mixed $config
@@ -86,7 +88,6 @@ public function __construct($config) {
             $this->enablepresignedurls = $config->enablepresignedurls;
             $this->signingmethod = $config->signingmethod;
             $this->bucketkeyprefix = $config->key_prefix;
-            $this->allowoverride = $config->s3_allowoverride;
             $this->set_client($config);
         } else {
             parent::__construct($config);
@@ -488,10 +489,6 @@ public function define_client_section($settings, $config) {
         $settings->add(new \admin_setting_configtext('tool_objectfs/key_prefix',
             new \lang_string('settings:aws:key_prefix', 'tool_objectfs'),
             new \lang_string('settings:aws:key_prefix_help', 'tool_objectfs'), ''));
-        
-        $settings->add(new \admin_setting_configcheckbox('tool_objectfs/s3_overrideobjects',
-            new \lang_string('settings:aws:overrideobjects', 'tool_objectfs'),
-            new \lang_string('settings:aws:overrideobjects_help', 'tool_objectfs'), 0));
 
         return $settings;
     }
@@ -515,15 +512,20 @@ public function upload_to_s3($localpath, $contenthash, array $tags) {
         try {
             $externalpath = $this->get_filepath_from_hash($contenthash);
 
-            // TODO upload with tags.
             $uploader = new \Aws\S3\ObjectUploader(
                 $this->client, $this->bucket,
                 $this->bucketkeyprefix . $externalpath,
                 $filehandle,
-                'private',
             );
             $uploader->upload();
             fclose($filehandle);
+
+            // The S3 php api does not support uploading object tags as part of
+            // the object uploader, so upload them directly after uploading the object.
+            if (!empty($tags)) {
+                $this->set_object_tags($contenthash, $tags);
+            }
+
         } catch (\Aws\Exception\MultipartUploadException $e) {
             $params = $e->getState()->getId();
             $this->client->abortMultipartUpload($params);
@@ -923,11 +925,14 @@ public function test_set_object_tag(): stdClass {
         return (object) ['success' => true, 'details' => ''];
     }
 
-    private function convert_array_to_s3_tag_format(array $tags): array {
-
-        return [
-            'TagSet' => $s3tags
-        ];
+    private function convert_tags_to_s3_format(array $tags) {
+        foreach($tags as $key => $value) {
+            $s3tags[] = [
+                'Key' => $key,
+                'Value' => $value,
+            ];
+        }
+        return $s3tags;
     }
 
     /**
@@ -936,23 +941,14 @@ private function convert_array_to_s3_tag_format(array $tags): array {
      * @param array $tags array of key=>value pairs to set as tags.
      */
     public function set_object_tags(string $contenthash, array $tags) {
-        $key = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash);
-
-        // Convert to S3 format.
-        $s3tags = [];
-        foreach($tags as $key => $value) {
-            $s3tags[] = [
-                'Key' => $key,
-                'Value' => $value,
-            ];
-        }
+        $objectkey = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash);
 
         // Then put onto object.
         $this->client->putObjectTagging([
             'Bucket' => $this->bucket,
-            'Key' => $key,
+            'Key' => $objectkey,
             'Tagging' => [
-                'TagSet' => $s3tags,
+                'TagSet' => $this->convert_tags_to_s3_format($tags),
             ],
         ]);
     }
@@ -966,14 +962,19 @@ public function get_object_tags(string $contenthash): array {
         $key = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash);
 
         // Query from S3.
-        $tags = $this->client->getObjectTagging([
+        $result = $this->client->getObjectTagging([
             'Bucket' => $this->bucket,
             'Key' => $key,
         ]);
 
+        // Ensure tags are what we expect, and AWS have not changed the format.
+        if (empty($result->toArray()['TagSet'])) {
+            throw new coding_exception("Unexpected tag format received"); // TODO different ex type.
+        }
+
         // Convert from S3 format to key=>value format.
         $tagkv = [];
-        foreach($tags['Tagging']['TagSet'] as $tag) {
+        foreach($result->toArray()['TagSet'] as $tag) {
             $tagkv[$tag['Key']] = $tag['Value'];
         }
 
@@ -987,34 +988,4 @@ public function get_object_tags(string $contenthash): array {
     public function supports_object_tagging(): bool {
         return true;
     }
-
-    // TODO maybe this moves to 'global' objectfs config ?
-    public function can_override_objects(): bool {
-        return $this->allowoverride;
-    }
-
-    public function object_exists(string $contenthash): bool {
-        $key = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash);
-
-        // Query from S3.
-        try {
-            $this->client->headObject([
-                'Bucket' => $this->bucket,
-                'Key' => $key,
-            ]);
-
-            // Exists, headObject succeeded.
-            return true;
-        } catch (\AWS\Exception\AwsException $e) {
-            $is404 = false; // TOOD determine if was 404, i.e. missing object
-
-            // Object does not exist.
-            if($is404) {
-                return false;
-            }
-
-            // Other error - rethrow.
-            throw $e;
-        }
-    }
 }

classes/local/store/s3/file_system.php

@@ -89,7 +89,7 @@ public function copy_from_local_to_external($contenthash) {
 
         try {
             // If can't override, check if it exists already, since upload object will override.
-            if (!manager::can_override_existing_objects() && $this->get_external_client()->object_exists($contenthash)) {
+            if (!manager::can_override_existing_objects() && $this->is_file_readable_externally_by_hash($contenthash)) {
                 // TODO somehow log it was skipped here?
                 return true;
             }

classes/local/tag/tag_manager.php

@@ -21,6 +21,7 @@
 use core\lock\lock_config;
 use Exception;
 use tool_objectfs\local\manager;
+use tool_objectfs\local\store\object_file_system;
 
 defined('MOODLE_INTERNAL') || die();
 require_once($CFG->dirroot . '/admin/tool/objectfs/lib.php');
@@ -160,24 +161,26 @@ public static function mark_object_tag_sync_status(string $contenthash, int $sta
         $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]);
     }
 
-    public static function sync_object_tags_post_upload(string $contenthash) {
+    public static function sync_object_tags_post_upload(string $contenthash, object_file_system $fs) {
         // First get object lock
         // TODO
 
         $client = manager::get_client(manager::get_objectfs_config());
 
-        // If object does not exist, cannot sync tags, abort.
-        if (!$client->object_exists($contenthash)) {
+        // If object does not exist, cannot sync tags to nothing, abort.
+        if (!$fs->is_file_readable_externally_by_hash($contenthash)) {
+            // 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.
-        if (!$client->can_override_objects()) {
+        // Cannot override and object exists, query existing and store.
+        if (!manager::can_override_existing_objects()) {
             // Query existing tags and store them.
             $existingtags = $client->get_object_tags($contenthash);
             self::store_tags_locally($contenthash, $existingtags);
 
-        // Else can override, upload new.
+        // Else can override, upload new and store.
         } else {
             $tags = self::gather_object_tags_for_upload($contenthash);
             $client->set_object_tags($contenthash, $tags);

classes/task/update_object_tags.php

@@ -18,6 +18,8 @@
 
 use coding_exception;
 use core\task\adhoc_task;
+use Exception;
+use Throwable;
 use tool_objectfs\local\manager;
 use tool_objectfs\local\tag\tag_manager;
 
@@ -34,8 +36,6 @@ class update_object_tags extends adhoc_task {
      * Execute task
      */
     public function execute() {
-        // TODO some way to trigger this from the frontend.
-
         if (!tag_manager::is_tagging_enabled_and_supported()) {
             mtrace("Tagging feature not enabled or supported by filesystem, exiting.");
             return;
@@ -50,12 +50,36 @@ public function execute() {
             return;
         }
 
+        // 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")) {
+            mtrace("File system is not object file system, exiting");
+            return;
+        }
+
         // For each, try to sync their tags.
         foreach ($contenthashes as $contenthash) {
-            tag_manager::sync_object_tags_post_upload($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();
         }
 
         // Re-queue self to process more in another iteration.
+        mtrace("Requeing self for another iteration");
         $task = new update_object_tags();
         \core\task\manager::queue_adhoc_task($task);
     }

db/tasks.php

@@ -107,14 +107,5 @@
         'dayofweek' => '*',
         'month'     => '*'
     ),
-	array (
-        'classname' => 'tool_objectfs\task\queue_objects_needing_tags',
-        'blocking'  => 0,
-        'minute'    => 'R',
-        'hour'      => '*',
-        'day'       => '*',
-        'dayofweek' => '*',
-        'month'     => '*'
-	),
 );
 

lang/en/tool_objectfs.php

@@ -273,13 +273,10 @@
 $string['check:proxyrangerequestsdisabled'] = 'The proxy range request setting is disabled.';
 $string['checkproxy_range_request'] = 'Pre-signed URL range request proxy';
 
-$string['task:queueobjectsneedingtags'] = 'Queue objects needing tagging';
 $string['checktagging_status'] = 'Object tagging';
 $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: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.';
-$string['settings:s3:overrideobjects'] = 'Override existing objects';
-$string['settings:s3:overrideobjects_help'] = 'If objects are allowed to be overwritten. Turning this off will require S3 to make an additional API call beforeupload to check if the object exists. If you are using tagging, and are in a multi env setup where multiple environments target the same bucket, you should turn this on for prod, and off for everywhere else.';
+$string['settings:maxtaggingperrun'] = 'Object tagging migration limit';
+$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to sync tags for per migration adhoc task iteration.';
+$string['settings:overrideobjects'] = 'Allow object override';
+$string['settings:overrideobjects:desc'] = 'If disabled, ObjectFS will check that an object does not exist before uploading.';

settings.php

@@ -23,6 +23,8 @@
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 
+use tool_objectfs\task\update_object_tags;
+
 defined('MOODLE_INTERNAL') || die();
 
 require_once(__DIR__ . '/classes/local/manager.php');
@@ -125,6 +127,10 @@
     $settings->add(new admin_setting_configduration('tool_objectfs/consistencydelay',
         new lang_string('settings:consistencydelay', 'tool_objectfs'), '', 10 * MINSECS, MINSECS));
 
+    $settings->add(new admin_setting_configcheckbox('tool_objectfs/overrideobjects',
+        new lang_string('settings:overrideobjects', 'tool_objectfs'),
+        get_string('settings:overrideobjects:desc', 'tool_objectfs'),
+        1));
 
     $settings->add(new admin_setting_heading('tool_objectfs/storagefilesystemselection',
         new lang_string('settings:clientselection:header', 'tool_objectfs'), ''));
@@ -263,9 +269,6 @@
         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));
+    
+    // TODO some way to scheduled the migration adhoc task from the frontend
 }

version.php

@@ -25,8 +25,8 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2023051704;      // The current plugin version (Date: YYYYMMDDXX).
-$plugin->release   = 2023051704;      // Same as version.
+$plugin->version   = 2023051705;      // The current plugin version (Date: YYYYMMDDXX).
+$plugin->release   = 2023051705;      // Same as version.
 $plugin->requires  = 2020110900;      // Requires Filesystem API.
 $plugin->component = "tool_objectfs";
 $plugin->maturity  = MATURITY_STABLE;