From c1ce064ef7939a920f4b7f69b2a0e4d5e00e4118 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Thu, 18 Jul 2024 15:44:51 +1000 Subject: [PATCH 01/16] feat: add object tagging --- TAGGING.md | 86 +++++ classes/check/tagging_status.php | 62 ++++ classes/local/manager.php | 1 + classes/local/report/objectfs_report.php | 1 + .../local/report/tag_count_report_builder.php | 51 +++ classes/local/store/object_client.php | 28 ++ classes/local/store/object_client_base.php | 36 ++ classes/local/store/object_file_system.php | 56 +++ classes/local/store/s3/client.php | 112 ++++++ classes/local/store/s3/file_system.php | 1 + classes/local/tag/environment_source.php | 61 ++++ classes/local/tag/mime_type_source.php | 65 ++++ classes/local/tag/tag_manager.php | 183 ++++++++++ classes/local/tag/tag_source.php | 50 +++ classes/task/trigger_update_object_tags.php | 48 +++ classes/task/update_object_tags.php | 85 +++++ classes/tests/test_client.php | 37 ++ classes/tests/testcase.php | 3 +- db/install.xml | 20 +- db/tasks.php | 12 + db/upgrade.php | 45 +++ lang/en/tool_objectfs.php | 19 + lib.php | 2 + settings.php | 42 ++- tests/local/report/object_status_test.php | 2 +- tests/local/tagging_test.php | 340 ++++++++++++++++++ tests/object_file_system_test.php | 104 ++++++ tests/task/populate_objects_filesize_test.php | 1 + .../task/trigger_update_object_tags_test.php | 49 +++ tests/task/update_object_tags_test.php | 170 +++++++++ 30 files changed, 1767 insertions(+), 5 deletions(-) create mode 100644 TAGGING.md create mode 100644 classes/check/tagging_status.php create mode 100644 classes/local/report/tag_count_report_builder.php create mode 100644 classes/local/tag/environment_source.php create mode 100644 classes/local/tag/mime_type_source.php create mode 100644 classes/local/tag/tag_manager.php create mode 100644 classes/local/tag/tag_source.php create mode 100644 classes/task/trigger_update_object_tags.php create mode 100644 classes/task/update_object_tags.php create mode 100644 tests/local/tagging_test.php create mode 100644 tests/task/trigger_update_object_tags_test.php create mode 100644 tests/task/update_object_tags_test.php diff --git a/TAGGING.md b/TAGGING.md new file mode 100644 index 00000000..0c2155e8 --- /dev/null +++ b/TAGGING.md @@ -0,0 +1,86 @@ +# Tagging +Tagging allows extra metadata about your files to be send to the external object store. These sources are defined in code, and currently cannot be configured on/off from the UI. + +Currently, this is only implemented for the S3 file system client. +**Tagging vs metadata** + +Note object tags are different from object metadata. + +Object metadata is immutable, and attached to the object on upload. With metadata, if you wish to update it (for example during a migration, or the sources changed), you have to copy the object with the new metadata, and delete the old object. This is problematic, since deletion is optional in objectfs. + +Object tags are more suitable, since their permissions can be managed separately (e.g. a client can be allowed to modify tags, but not delete objects). + +## File system setup +### S3 +[See the S3 docs for more information about tagging](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-tagging.html). + +You must allow `s3:GetObjectTagging` and `s3:PutObjectTagging` permission to the objectfs client. + +## Sources +The following sources are implemented currently: +### Environment +What environment the file was uploaded in. Configure the environment using `$CFG->objectfs_environment_name` + +### Mimetype +What mimetype the file is stored as under the `mdl_files` table. + +## Multiple environments pointing to single bucket +It is possible you are using objectfs with multiple environments (e.g. prod, staging) that both point to the same bucket. Since files are referenced by contenthash, it generally does not matter where they come from, so this isn't a problem. However to ensure the tags remain accurate, you should turn off `overwriteobjecttags` in the plugin settings for every environment except production. + +This means that staging is unable to overwrite tags for files uploaded elsewhere, but can set it on files only uploaded only from staging. However, files uploaded from production will always have the correct tags, and will overwrite any existing tags. + +```mermaid +graph LR + subgraph S3 + Object("`**Object** + contenthash: xyz + tags: env=prod`") + end + subgraph Prod + UploadObjectProd["`**Upload object** + contenthash: xyz + tags: env=prod`"] --> Object + end + subgraph Staging + UploadObjectStaging["`**Upload object** + contenthash: xyz + tags: env=staging`"] + end + Blocked["Blocked - does not have permissions\nto overwrite existing object tags"] + UploadObjectStaging --- Blocked + Blocked -.-> Object + + style Object fill:#ffffff00,stroke:#ffa812 + style S3 fill:#ffffff00,stroke:#ffa812 + style Prod fill:#ffffff00,stroke:#26ff4a + style UploadObjectProd fill:#ffffff00,stroke:#26ff4a + style Staging fill:#ffffff00,stroke:#978aff + style UploadObjectStaging fill:#ffffff00,stroke:#978aff + style Blocked fill:#ffffff00,stroke:#ff0000 +``` + +## Migration +If the way a tag was calculated has changed, or new tags are added (or removed) or this feature was turned on for the first time (or turned on after being off), you must do the following: +- Manually run `trigger_update_object_tags` scheduled task from the UI, which queues a `update_object_tags` adhoc task that will process all objects marked as needing sync (default is true) +or +- Call the CLI to execute a `update_object_tags` adhoc task manually. + +## Reporting +There is an additional graph added to the object summary report showing the tag value combinations and counts of each. + +Note, this is only for files that have been uploaded from this environment, and may not be consistent for environments where `overwriteobjecttags` is disabled (because the site does not know if a file was overwritten in the external store by another client). + +## For developers + +### Adding a new source +Note the rules about sources: +- Identifier must be < 32 chars long. +- Value must be < 128 chars long. + +While external providers allow longer key/values, we intentionally limit it to reserve space for future use. These limits may change in the future as the feature matures. + +To add a new source: +- Implement `tag_source` +- Add to the `tag_manager` class +- As part of an upgrade step, mark all objects `tagsyncstatus` to needing sync (using `tag_manager` class, or manually in the DB) +- As part of an upgrade step, queue a `update_object_tags` adhoc task to process the tag migration. \ No newline at end of file diff --git a/classes/check/tagging_status.php b/classes/check/tagging_status.php new file mode 100644 index 00000000..df3e68d5 --- /dev/null +++ b/classes/check/tagging_status.php @@ -0,0 +1,62 @@ +. + +namespace tool_objectfs\check; + +use core\check\check; +use core\check\result; +use tool_objectfs\local\tag\tag_manager; + +/** + * Tagging status check + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tagging_status extends check { + /** + * Link to ObjectFS settings page. + * + * @return \action_link|null + */ + public function get_action_link(): ?\action_link { + $url = new \moodle_url('/admin/category.php', ['category' => 'tool_objectfs']); + return new \action_link($url, get_string('pluginname', 'tool_objectfs')); + } + + /** + * Get result + * @return result + */ + public function get_result(): result { + if (!tag_manager::is_tagging_enabled_and_supported()) { + return new result(result::NA, get_string('check:tagging:na', 'tool_objectfs')); + } + + // Do a tag set test. + $config = \tool_objectfs\local\manager::get_objectfs_config(); + $client = \tool_objectfs\local\manager::get_client($config); + $result = $client->test_set_object_tag(); + + if ($result->success) { + return new result(result::OK, get_string('check:tagging:ok', 'tool_objectfs'), $result->details); + } else { + return new result(result::ERROR, get_string('check:tagging:error', 'tool_objectfs'), $result->details); + } + } +} diff --git a/classes/local/manager.php b/classes/local/manager.php index acd2b30e..188df9fc 100644 --- a/classes/local/manager.php +++ b/classes/local/manager.php @@ -64,6 +64,7 @@ public static function get_objectfs_config() { $config->batchsize = 10000; $config->useproxy = 0; $config->deleteexternal = 0; + $config->enabletagging = false; $config->filesystem = ''; $config->enablepresignedurls = 0; diff --git a/classes/local/report/objectfs_report.php b/classes/local/report/objectfs_report.php index cc9eb910..56869cd7 100644 --- a/classes/local/report/objectfs_report.php +++ b/classes/local/report/objectfs_report.php @@ -166,6 +166,7 @@ public static function get_report_types() { 'location', 'log_size', 'mime_type', + 'tag_count', ]; } diff --git a/classes/local/report/tag_count_report_builder.php b/classes/local/report/tag_count_report_builder.php new file mode 100644 index 00000000..0364b3fc --- /dev/null +++ b/classes/local/report/tag_count_report_builder.php @@ -0,0 +1,51 @@ +. + +namespace tool_objectfs\local\report; + +/** + * Tag count report builder. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tag_count_report_builder extends objectfs_report_builder { + /** + * Builds report + * @param int $reportid + * @return objectfs_report + */ + public function build_report($reportid) { + global $DB; + $report = new objectfs_report('tag_count', $reportid); + + // Returns counts + sizes of key:value. + $sql = " + SELECT CONCAT(COALESCE(object_tags.tagkey, '(untagged)'), ': ', COALESCE(object_tags.tagvalue, '')) as datakey, + COUNT(objects.id) as objectcount, + SUM(objects.filesize) as objectsum + FROM {tool_objectfs_objects} objects + LEFT JOIN {tool_objectfs_object_tags} object_tags + ON objects.contenthash = object_tags.contenthash + GROUP BY object_tags.tagkey, object_tags.tagvalue + "; + $result = $DB->get_records_sql($sql); + $report->add_rows($result); + return $report; + } +} diff --git a/classes/local/store/object_client.php b/classes/local/store/object_client.php index 80e3f6eb..ffa59bf2 100644 --- a/classes/local/store/object_client.php +++ b/classes/local/store/object_client.php @@ -16,6 +16,8 @@ namespace tool_objectfs\local\store; +use stdClass; + /** * Objectfs client interface. * @@ -141,6 +143,32 @@ public function test_range_request($filesystem); * @return int unix timestamp the token set expires at */ public function get_token_expiry_time(): int; + + /* + * Tests setting an objects tag. + * @return stdClass containing 'success' and 'details' properties + */ + public function test_set_object_tag(): stdClass; + + /** + * Set the given objects tags in the external store. + * @param string $contenthash file content hash + * @param array $tags array of key=>value pairs to set as tags. + */ + public function set_object_tags(string $contenthash, array $tags); + + /** + * Returns given objects tags queried from the external store. External object must exist. + * @param string $contenthash file content has + * @return array array of key=>value tag pairs + */ + public function get_object_tags(string $contenthash): array; + + /** + * If the client supports object tagging feature. + * @return bool true if supports, else false + */ + public function supports_object_tagging(): bool; } diff --git a/classes/local/store/object_client_base.php b/classes/local/store/object_client_base.php index fa1b7e8f..7361d72f 100644 --- a/classes/local/store/object_client_base.php +++ b/classes/local/store/object_client_base.php @@ -25,6 +25,8 @@ namespace tool_objectfs\local\store; +use stdClass; + /** * [Description object_client_base] */ @@ -196,4 +198,38 @@ public function get_token_expiry_time(): int { // Returning -1 = not implemented. return -1; } + + /* + * Tests setting an objects tag. + * @return stdClass containing 'success' and 'details' properties + */ + public function test_set_object_tag(): stdClass { + return (object)['success' => false, 'details' => '']; + } + + /** + * Set the given objects tags in the external store. + * @param string $contenthash file content hash + * @param array $tags array of key=>value pairs to set as tags. + */ + public function set_object_tags(string $contenthash, array $tags) { + return []; + } + + /** + * Returns given objects tags queried from the external store. External object must exist. + * @param string $contenthash file content has + * @return array array of key=>value tag pairs + */ + public function get_object_tags(string $contenthash): array { + return []; + } + + /** + * If the client supports object tagging feature. + * @return bool true if supports, else false + */ + public function supports_object_tagging(): bool { + return false; + } } diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index de30f1d2..286a2d23 100644 --- 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(); @@ -360,6 +363,12 @@ public function copy_object_from_local_to_external_by_hash($contenthash, $object } } + // If tagging is enabled, ensure tags are synced regardless of if object is local or duplicated, etc... + // The file may exist in external store because it was uploaded by another site, but we may want to put our tags onto it. + if (tag_manager::is_tagging_enabled_and_supported()) { + $this->push_object_tags($contenthash); + } + $this->logger->log_object_move('copy_object_from_local_to_external', $initiallocation, $finallocation, @@ -1154,4 +1163,51 @@ private function update_object(array $result): array { return $result; } + + /** + * Pushes tags to the external store (post upload) for a given hash. + * External client must support tagging. + * + * @param string $contenthash file to sync tags for + */ + public function push_object_tags(string $contenthash) { + if (!$this->get_external_client()->supports_object_tagging()) { + throw new coding_exception("Cannot sync tags, external client does not support tagging."); + } + + // Get a lock before syncing, to ensure other parts of objectfs are not moving/interacting with this object. + $lock = $this->acquire_object_lock($contenthash, 10); + + // No lock - just skip it. + if (!$lock) { + throw new coding_exception("Could not get object lock"); + } + + try { + $objectexists = $this->is_file_readable_externally_by_hash($contenthash); + + // Object must exist, and we can overwrite (and not care about existing tags) + // or cannot overwrite, and the tags are empty. + // Avoid unnecessarily checking tags, since this is an extra API call. + $canset = $objectexists && (tag_manager::can_overwrite_object_tags() || + empty($this->get_external_client()->get_object_tags($contenthash))); + + if ($canset) { + $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); + } + + // Regardless, it has synced. + tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED); + } catch (Throwable $e) { + $lock->release(); + + // Mark object as tag sync error, this should stop it re-trying until fixed manually. + tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_ERROR); + + throw $e; + } + $lock->release(); + } } diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index a6e2598e..5e73d974 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -25,10 +25,13 @@ 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; use local_aws\admin_settings_aws_region; +use stdClass; +use Throwable; define('AWS_API_VERSION', '2006-03-01'); define('AWS_CAN_READ_OBJECT', 0); @@ -875,4 +878,113 @@ public function test_range_request($filesystem) { } return (object)['result' => false, 'error' => get_string('fixturefilemissing', 'tool_objectfs')]; } + + /** + * Tests setting an objects tag. + * @return stdClass containing 'success' and 'details' properties + */ + public function test_set_object_tag(): stdClass { + try { + // First ensure a test object exists to put tags on. + // Note this will override the existing object if exists. + $key = $this->bucketkeyprefix . 'tagging_check_file'; + $this->client->putObject([ + 'Bucket' => $this->bucket, + 'Key' => $key, + 'Body' => 'test content', + ]); + + // Next try to tag it - this will throw an exception if cannot set + // (for example, because it does not have permissions to). + $this->client->putObjectTagging([ + 'Bucket' => $this->bucket, + 'Key' => $key, + 'Tagging' => [ + 'TagSet' => [ + [ + 'Key' => 'test', + 'Value' => 'test', + ], + ], + ], + ]); + } catch (Throwable $e) { + return (object) [ + 'success' => false, + 'details' => $e->getMessage(), + ]; + } + + // Success - no exceptions thrown. + return (object) ['success' => true, 'details' => '']; + } + + /** + * Convert key=>value to s3 tag format + * @param array $tags + * @return array tags in s3 format. + */ + private function convert_tags_to_s3_format(array $tags): array { + foreach ($tags as $key => $value) { + $s3tags[] = [ + 'Key' => $key, + 'Value' => $value, + ]; + } + return $s3tags; + } + + /** + * Set the given objects tags in the external store. + * @param string $contenthash file content hash + * @param array $tags array of key=>value pairs to set as tags. + */ + public function set_object_tags(string $contenthash, array $tags) { + $objectkey = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash); + + // Then put onto object. + $this->client->putObjectTagging([ + 'Bucket' => $this->bucket, + 'Key' => $objectkey, + 'Tagging' => [ + 'TagSet' => $this->convert_tags_to_s3_format($tags), + ], + ]); + } + + /** + * Returns given objects tags queried from the external store. Object must exist. + * @param string $contenthash file content has + * @return array array of key=>value tag pairs + */ + public function get_object_tags(string $contenthash): array { + $key = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash); + + // Query from S3. + $result = $this->client->getObjectTagging([ + 'Bucket' => $this->bucket, + 'Key' => $key, + ]); + + // Ensure tags are what we expect, and AWS have not changed the format. + if (!array_key_exists('TagSet', $result->toArray())) { + throw new coding_exception("Unexpected tag format received. Result did not contain a TagSet"); + } + + // Convert from S3 format to key=>value format. + $tagkv = []; + foreach ($result->toArray()['TagSet'] as $tag) { + $tagkv[$tag['Key']] = $tag['Value']; + } + + return $tagkv; + } + + /** + * If the client supports object tagging feature. + * @return bool true if supports, else false + */ + public function supports_object_tagging(): bool { + return true; + } } diff --git a/classes/local/store/s3/file_system.php b/classes/local/store/s3/file_system.php index 93637dd4..b272f33b 100644 --- a/classes/local/store/s3/file_system.php +++ b/classes/local/store/s3/file_system.php @@ -32,6 +32,7 @@ use tool_objectfs\local\manager; use tool_objectfs\local\store\object_file_system; +use tool_objectfs\local\tag\tag_manager; require_once($CFG->dirroot . '/admin/tool/objectfs/lib.php'); diff --git a/classes/local/tag/environment_source.php b/classes/local/tag/environment_source.php new file mode 100644 index 00000000..d7715ddb --- /dev/null +++ b/classes/local/tag/environment_source.php @@ -0,0 +1,61 @@ +. + +namespace tool_objectfs\local\tag; + +/** + * Provides environment a file was uploaded in. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class environment_source implements tag_source { + /** + * Identifier used in tagging file. Is the 'key' of the tag. + * @return string + */ + public static function get_identifier(): string { + return 'environment'; + } + + /** + * Description for source displayed in the admin settings. + * @return string + */ + public static function get_description(): string { + return get_string('tagsource:environment', 'tool_objectfs', self::get_env()); + } + + /** + * Returns current env value from $CFG + * @return string|null string if set, else null + */ + private static function get_env(): ?string { + global $CFG; + return !empty($CFG->objectfs_environment_name) ? $CFG->objectfs_environment_name : null; + } + + /** + * Returns the tag value for the given file contenthash + * @param string $contenthash + * @return string|null mime type for file. + */ + public function get_value_for_contenthash(string $contenthash): ?string { + return self::get_env(); + } +} diff --git a/classes/local/tag/mime_type_source.php b/classes/local/tag/mime_type_source.php new file mode 100644 index 00000000..7546cdcf --- /dev/null +++ b/classes/local/tag/mime_type_source.php @@ -0,0 +1,65 @@ +. + +namespace tool_objectfs\local\tag; + +/** + * Provides mime type of file. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class mime_type_source implements tag_source { + /** + * Identifier used in tagging file. Is the 'key' of the tag. + * @return string + */ + public static function get_identifier(): string { + return 'mimetype'; + } + + /** + * Description for source displayed in the admin settings. + * @return string + */ + public static function get_description(): string { + return get_string('tagsource:mimetype', 'tool_objectfs'); + } + + /** + * Returns the tag value for the given file contenthash + * @param string $contenthash + * @return string|null mime type for file. + */ + public function get_value_for_contenthash(string $contenthash): ?string { + global $DB; + // Sometimes multiple with same hash are uploaded (e.g. real vs draft), + // in this case, just take the first (mimetype is the same regardless). + $mime = $DB->get_field_sql('SELECT mimetype + FROM {files} + WHERE contenthash = :hash + LIMIT 1', + ['hash' => $contenthash]); + + if (empty($mime)) { + return null; + } + + return $mime; + } +} diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php new file mode 100644 index 00000000..36aada41 --- /dev/null +++ b/classes/local/tag/tag_manager.php @@ -0,0 +1,183 @@ +. + +namespace tool_objectfs\local\tag; + +use coding_exception; +use tool_objectfs\local\manager; + +defined('MOODLE_INTERNAL') || die(); +require_once($CFG->dirroot . '/admin/tool/objectfs/lib.php'); + +/** + * Manages object tagging feature. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tag_manager { + + /** + * @var int If object needs sync. These will periodically be picked up by scheduled tasks and queued for syncing. + */ + public const SYNC_STATUS_NEEDS_SYNC = 0; + + /** + * @var int Object does not need sync. Will be essentially ignored in tagging process. + */ + public const SYNC_STATUS_SYNC_NOT_REQUIRED = 1; + + /** + * @var int Object tried to sync but there was an error. Will make it ignored and must be corrected manually. + */ + public const SYNC_STATUS_ERROR = 2; + + /** + * @var array All possible tag sync statuses. + */ + public const SYNC_STATUSES = [ + self::SYNC_STATUS_NEEDS_SYNC, + self::SYNC_STATUS_SYNC_NOT_REQUIRED, + self::SYNC_STATUS_ERROR, + ]; + + /** + * Returns an array of tag_source instances that are currently defined. + * @return array + */ + public static function get_defined_tag_sources(): array { + // All possible tag sources should be defined here. + // Note this should be a maximum of 10 sources, as this is an AWS limit. + return [ + new mime_type_source(), + new environment_source(), + ]; + } + + /** + * Is the tagging feature enabled and supported by the configured fs? + * @return bool + */ + public static function is_tagging_enabled_and_supported(): bool { + $enabledinconfig = !empty(get_config('tool_objectfs', 'taggingenabled')); + + $client = manager::get_client(manager::get_objectfs_config()); + $supportedbyfs = !empty($client) && $client->supports_object_tagging(); + + return $enabledinconfig && $supportedbyfs; + } + + /** + * Gathers the tag values for a given content hash + * @param string $contenthash + * @return array array of key=>value pairs, the tags for the given file. + */ + public static function gather_object_tags_for_upload(string $contenthash): array { + $tags = []; + foreach (self::get_defined_tag_sources() as $source) { + $val = $source->get_value_for_contenthash($contenthash); + + // Null means not set for this object. + if (is_null($val)) { + continue; + } + + $tags[$source->get_identifier()] = $val; + } + return $tags; + } + + /** + * Stores tag records for contenthash locally + * @param string $contenthash + * @param array $tags + */ + public static function store_tags_locally(string $contenthash, array $tags) { + global $DB; + + // 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); + } + + /** + * Returns objects that are candidates for tag syncing. + * @param int $limit max number of records to return + * @return array array of contenthashes, which need tags calculated and synced. + */ + public static function get_objects_needing_sync(int $limit) { + global $DB; + + // Find object records where the status is NEEDS_SYNC and is replicated. + [$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; + $records = $DB->get_records_select('tool_objectfs_objects', 'tagsyncstatus = :syncstatus AND location ' . $insql, + $inparams, '', 'contenthash', 0, $limit); + return array_column($records, 'contenthash'); + } + + /** + * Marks a given object as the given status. + * @param string $contenthash + * @param int $status one of SYNC_STATUS_* constants + */ + public static function mark_object_tag_sync_status(string $contenthash, int $status) { + global $DB; + if (!in_array($status, self::SYNC_STATUSES)) { + throw new coding_exception("Invalid object tag sync status " . $status); + } + $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]); + } + + /** + * Returns a simple list of all the sources and their descriptions. + * @return string html string + */ + public static function get_tag_summary_html(): string { + $sources = self::get_defined_tag_sources(); + $html = ''; + + foreach ($sources as $source) { + $html .= $source->get_identifier() . ': ' . $source->get_description() . '
'; + } + return $html; + } + + /** + * If the current env is allowed to overwrite tags on objects that already have tags. + * @return bool + */ + public static function can_overwrite_object_tags(): bool { + return (bool) get_config('tool_objectfs', 'overwriteobjecttags'); + } +} diff --git a/classes/local/tag/tag_source.php b/classes/local/tag/tag_source.php new file mode 100644 index 00000000..a915c82a --- /dev/null +++ b/classes/local/tag/tag_source.php @@ -0,0 +1,50 @@ +. + +namespace tool_objectfs\local\tag; + +/** + * Tag source interface + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +interface tag_source { + /** + * Returns an unchanging identifier for this source. + * Must never change, otherwise it will lose connection with the tags replicated to objects. + * If it ever must change, a migration step must be completed to trigger all objects to recalculate their tags. + * Must not exceed 128 chars. + * @return string + */ + public static function get_identifier(): string; + + /** + * Description for source displayed in the admin settings. + * @return string + */ + public static function get_description(): string; + + /** + * Returns the value of this tag for the file with the given content hash. + * This must be deterministic, and should never exceed 128 chars. + * @param string $contenthash + * @return string + */ + public function get_value_for_contenthash(string $contenthash): ?string; +} diff --git a/classes/task/trigger_update_object_tags.php b/classes/task/trigger_update_object_tags.php new file mode 100644 index 00000000..6cf64dd9 --- /dev/null +++ b/classes/task/trigger_update_object_tags.php @@ -0,0 +1,48 @@ +. + +namespace tool_objectfs\task; + +use core\task\manager; +use core\task\scheduled_task; + +/** + * Queues update_object_tags adhoc task periodically, or manually from the frontend. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class trigger_update_object_tags extends scheduled_task { + /** + * Task name + */ + public function get_name() { + return get_string('task:triggerupdateobjecttags', 'tool_objectfs'); + } + /** + * Execute task + */ + public function execute() { + // Queue adhoc task, nothing else. + $task = new update_object_tags(); + $task->set_custom_data([ + 'iteration' => 1, + ]); + manager::queue_adhoc_task($task, true); + } +} diff --git a/classes/task/update_object_tags.php b/classes/task/update_object_tags.php new file mode 100644 index 00000000..cc3bbfcb --- /dev/null +++ b/classes/task/update_object_tags.php @@ -0,0 +1,85 @@ +. + +namespace tool_objectfs\task; + +use core\task\adhoc_task; +use tool_objectfs\local\tag\tag_manager; + +/** + * Calculates and updates an objects tags in the external store. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class update_object_tags extends adhoc_task { + /** + * Execute task + */ + public function execute() { + if (!tag_manager::is_tagging_enabled_and_supported()) { + mtrace("Tagging feature not enabled or supported by filesystem, exiting."); + return; + } + + // Since this adhoc task can requeue itself, ensure there is a fixed limit on the number + // of times this can happen, to avoid any accidental runaways. + $iterationlimit = get_config('tool_objectfs', 'maxtaggingiterations') ?: 0; + $iteration = !empty($this->get_custom_data()->iteration) ? $this->get_custom_data()->iteration : 0; + + if (empty($iterationlimit) || empty($iteration)) { + mtrace("Invalid number of iterations, exiting."); + return; + } + + if (abs($iteration) > abs($iterationlimit)) { + mtrace("Maximum number of iterations reached: " . $iteration . ", exiting."); + return; + } + + // Get the maximum num of objects to update as configured. + $limit = get_config('tool_objectfs', 'maxtaggingperrun'); + $contenthashes = tag_manager::get_objects_needing_sync($limit); + + if (empty($contenthashes)) { + mtrace("No more objects found that need tagging, exiting."); + return; + } + + // Sanity check that fs is object file system and not anything else. + $fs = get_file_storage()->get_file_system(); + + if (!method_exists($fs, "push_object_tags")) { + mtrace("File system is not object file system, exiting."); + return; + } + + // For each, try to sync their tags. + foreach ($contenthashes as $contenthash) { + $fs->push_object_tags($contenthash); + } + + // Re-queue self to process more in another iteration. + mtrace("Requeing self for another iteration."); + $task = new update_object_tags(); + $task->set_custom_data([ + 'iteration' => $iteration + 1, + ]); + \core\task\manager::queue_adhoc_task($task); + } +} diff --git a/classes/tests/test_client.php b/classes/tests/test_client.php index bda43c5e..7ca00d56 100644 --- a/classes/tests/test_client.php +++ b/classes/tests/test_client.php @@ -16,6 +16,7 @@ namespace tool_objectfs\tests; +use coding_exception; use tool_objectfs\local\store\object_client_base; /** @@ -37,6 +38,11 @@ class test_client extends object_client_base { */ private $bucketpath; + /** + * @var array in-memory tags used for unit tests + */ + public $tags; + /** * string * @param \stdClass $config @@ -169,5 +175,36 @@ public function get_token_expiry_time(): int { global $CFG; return $CFG->objectfs_phpunit_token_expiry_time; } + + /* + * Sets object tags - uses in-memory store for unit tests + * @param string $contenthash + * @param array $tags + */ + public function set_object_tags(string $contenthash, array $tags) { + global $CFG; + if (!empty($CFG->phpunit_objectfs_simulate_tag_set_error)) { + throw new coding_exception("Simulated tag set error"); + } + $this->tags[$contenthash] = $tags; + } + + /** + * Gets object tags - uses in-memory store for unit tests + * @param string $contenthash + * @return array + */ + public function get_object_tags(string $contenthash): array { + return $this->tags[$contenthash] ?? []; + } + + /** + * Object tagging support, for unit testing + * @return bool + */ + public function supports_object_tagging(): bool { + global $CFG; + return $CFG->phpunit_objectfs_supports_object_tagging; + } } diff --git a/classes/tests/testcase.php b/classes/tests/testcase.php index f3efc8cf..36ad5d8c 100644 --- a/classes/tests/testcase.php +++ b/classes/tests/testcase.php @@ -33,7 +33,6 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ abstract class testcase extends \advanced_testcase { - /** @var test_file_system Filesystem */ public $filesystem; @@ -48,8 +47,10 @@ 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/db/install.xml b/db/install.xml index 855c9786..5d6b6f4e 100644 --- a/db/install.xml +++ b/db/install.xml @@ -1,5 +1,5 @@ - @@ -11,6 +11,7 @@ + @@ -37,7 +38,7 @@ - + @@ -49,5 +50,20 @@ + + + + + + + + + + + + + + +
diff --git a/db/tasks.php b/db/tasks.php index ac98e3ff..43e3cfa6 100644 --- a/db/tasks.php +++ b/db/tasks.php @@ -107,5 +107,17 @@ 'dayofweek' => '*', 'month' => '*', ], + [ + 'classname' => 'tool_objectfs\task\trigger_update_object_tags', + 'blocking' => 0, + 'minute' => 'R', + 'hour' => '*', + 'day' => '*', + 'dayofweek' => '*', + 'month' => '*', + // Default disabled - intended to be manually run. + // Also, objectfs tagging support is default off. + 'disabled' => true, + ], ]; diff --git a/db/upgrade.php b/db/upgrade.php index 19c70089..4554b7e4 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -170,5 +170,50 @@ function xmldb_tool_objectfs_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2023013100, 'tool', 'objectfs'); } + + if ($oldversion < 2024093000) { + + // Define table tool_objectfs_object_tags to be created. + $table = new xmldb_table('tool_objectfs_object_tags'); + + // Adding fields to table tool_objectfs_object_tags. + $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table->add_field('contenthash', XMLDB_TYPE_CHAR, '40', null, XMLDB_NOTNULL, null, null); + $table->add_field('tagkey', XMLDB_TYPE_CHAR, '32', null, XMLDB_NOTNULL, null, null); + $table->add_field('tagvalue', XMLDB_TYPE_CHAR, '128', null, XMLDB_NOTNULL, null, null); + $table->add_field('timemodified', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); + + // Adding keys to table tool_objectfs_object_tags. + $table->add_key('primary', XMLDB_KEY_PRIMARY, ['id']); + + // Adding indexes to table tool_objectfs_object_tags. + $table->add_index('objecttagkey_idx', XMLDB_INDEX_UNIQUE, ['contenthash', 'tagkey']); + + // Conditionally launch create table for tool_objectfs_object_tags. + if (!$dbman->table_exists($table)) { + $dbman->create_table($table); + } + + // Define field tagsyncstatus to be added to tool_objectfs_objects. + $table = new xmldb_table('tool_objectfs_objects'); + $field = new xmldb_field('tagsyncstatus', XMLDB_TYPE_INTEGER, '2', null, XMLDB_NOTNULL, null, '0', 'filesize'); + + // Conditionally launch add field tagsyncstatus. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Changing precision of field datakey on table tool_objectfs_report_data, + // to (255) to allow for tag key + value pairs to fit in. + $table = new xmldb_table('tool_objectfs_report_data'); + $field = new xmldb_field('datakey', XMLDB_TYPE_CHAR, '255', null, XMLDB_NOTNULL, null, null, 'reporttype'); + + // Launch change of precision for field datakey. + $dbman->change_field_precision($table, $field); + + // Objectfs savepoint reached. + upgrade_plugin_savepoint(true, 2024093000, 'tool', 'objectfs'); + } + return true; } diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 74b383a5..98ff7f9f 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -275,3 +275,22 @@ $string['check:tokenexpiry:expired'] = 'Token expired for {$a->dayssince} days. Expired on {$a->time}'; $string['check:tokenexpiry:na'] = 'Token expired not implemented for filesystem, or no token is set'; $string['settings:tokenexpirywarnperiod'] = 'Token expiry warn period'; + +$string['settings:taggingheader'] = 'Tagging settings'; +$string['settings:taggingenabled'] = 'Tagging enabled'; +$string['checktagging_status'] = 'Object tagging'; +$string['check:tagging:ok'] = 'Object tagging ok'; +$string['check:tagging:na'] = 'Tagging not enabled or is not supported by file system'; +$string['check:tagging:error'] = 'Error trying to tag object'; +$string['settings:maxtaggingperrun'] = 'Object tagging adhoc sync maximum objects per run'; +$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to sync tags for per tagging sync adhoc task iteration.'; +$string['settings:maxtaggingiterations'] = 'Object tagging adhoc sync maximum number of iterations '; +$string['settings:maxtaggingiterations:desc'] = 'The maximum number of times the tagging sync adhoc task will requeue itself. To avoid accidental infinite runaway.'; +$string['settings:overrideobjecttags'] = 'Allow object tag override'; +$string['settings:overrideobjecttags:desc'] = 'Allows ObjectFS to overwrite tags on objects that already exist in the external store.'; +$string['tagsource:environment'] = 'Environment defined by $CFG->objectfs_environment_name, currently: "{$a}".'; +$string['tagsource:mimetype'] = 'File mimetype as stored in {files} table'; +$string['settings:tagsources'] = 'Tag sources'; +$string['task:triggerupdateobjecttags'] = 'Queue adhoc task to update object tags'; +$string['settings:tagging:help'] = 'Object tagging allows extra metadata to be attached to objects in the external store. Please read TAGGING.md in the plugin Github repository for detailed setup and considerations. This is currently only supported by the S3 external client.'; +$string['object_status:tag_count'] = 'Object tags'; diff --git a/lib.php b/lib.php index 708063a8..82b23d25 100644 --- a/lib.php +++ b/lib.php @@ -24,6 +24,7 @@ */ use tool_objectfs\local\object_manipulator\manipulator_builder; +use tool_objectfs\local\tag\tag_manager; define('OBJECTFS_PLUGIN_NAME', 'tool_objectfs'); @@ -103,6 +104,7 @@ function tool_objectfs_pluginfile($course, $cm, context $context, $filearea, arr function tool_objectfs_status_checks() { $checks = [ new tool_objectfs\check\token_expiry(), + new tool_objectfs\check\tagging_status(), ]; if (get_config('tool_objectfs', 'proxyrangerequests')) { diff --git a/settings.php b/settings.php index aef3c571..3bde1957 100644 --- a/settings.php +++ b/settings.php @@ -23,6 +23,9 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use tool_objectfs\local\tag\tag_manager; +use tool_objectfs\task\update_object_tags; + defined('MOODLE_INTERNAL') || die(); require_once(__DIR__ . '/classes/local/manager.php'); @@ -129,7 +132,6 @@ $settings->add(new admin_setting_configduration('tool_objectfs/consistencydelay', new lang_string('settings:consistencydelay', 'tool_objectfs'), '', 10 * MINSECS, MINSECS)); - $settings->add(new admin_setting_heading('tool_objectfs/storagefilesystemselection', new lang_string('settings:clientselection:header', 'tool_objectfs'), '')); @@ -253,4 +255,42 @@ $settings->add(new admin_setting_configcheckbox('tool_objectfs/preferexternal', new lang_string('settings:preferexternal', 'tool_objectfs'), '', '')); + + // Tagging settings. + $settings->add(new admin_setting_heading('tool_objectfs/taggingsettings', + new lang_string('settings:taggingheader', 'tool_objectfs'), '')); + + $settings->add(new admin_setting_description('tool_objectfs/tagginghelp', + '', + get_string('settings:tagging:help', 'tool_objectfs') + )); + + $settings->add(new admin_setting_configcheckbox('tool_objectfs/taggingenabled', + new lang_string('settings:taggingenabled', 'tool_objectfs'), '', 0)); + + $settings->add(new admin_setting_description('tool_objectfs/tagsources', + new lang_string('settings:tagsources', 'tool_objectfs'), + tag_manager::get_tag_summary_html() + )); + + $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_configtext('tool_objectfs/maxtaggingiterations', + new lang_string('settings:maxtaggingiterations', 'tool_objectfs'), + get_string('settings:maxtaggingiterations:desc', 'tool_objectfs'), + 1000, + PARAM_INT + )); + + $settings->add(new admin_setting_configcheckbox('tool_objectfs/overwriteobjecttags', + new lang_string('settings:overrideobjecttags', 'tool_objectfs'), + get_string('settings:overrideobjecttags:desc', 'tool_objectfs'), + 1 + )); + } diff --git a/tests/local/report/object_status_test.php b/tests/local/report/object_status_test.php index bbc895d4..a06caa53 100644 --- a/tests/local/report/object_status_test.php +++ b/tests/local/report/object_status_test.php @@ -66,7 +66,7 @@ public function test_generate_status_report_historic() { public function test_get_report_types() { $reporttypes = objectfs_report::get_report_types(); $this->assertEquals('array', gettype($reporttypes)); - $this->assertEquals(3, count($reporttypes)); + $this->assertEquals(4, count($reporttypes)); } /** diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php new file mode 100644 index 00000000..95443451 --- /dev/null +++ b/tests/local/tagging_test.php @@ -0,0 +1,340 @@ +. + +namespace tool_objectfs\local; + +use coding_exception; +use Throwable; +use tool_objectfs\local\manager; +use tool_objectfs\local\tag\tag_manager; +use tool_objectfs\local\tag\tag_source; +use tool_objectfs\tests\testcase; + +/** + * Tests tagging + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tagging_test extends testcase { + /** + * Tests get_defined_tag_sources + * @covers \tool_objectfs\local\tag_manager::get_defined_tag_sources + */ + public function test_get_defined_tag_sources() { + $sources = tag_manager::get_defined_tag_sources(); + $this->assertIsArray($sources); + + // Both AWS and Azure limit 10 tags per object, so ensure never more than 10 sources defined. + $this->assertLessThanOrEqual(10, count($sources)); + } + + /** + * Provides values to various tag source tests + * @return array + */ + public static function tag_source_provider(): array { + $sources = tag_manager::get_defined_tag_sources(); + $tests = []; + + foreach ($sources as $source) { + $tests[$source->get_identifier()] = [ + 'source' => $source, + ]; + } + + return $tests; + } + + /** + * Tests the source identifier + * @param tag_source $source + * @dataProvider tag_source_provider + * @covers \tool_objectfs\local\tag_source::get_identifier + */ + public function test_tag_sources_identifier(tag_source $source) { + $count = strlen($source->get_identifier()); + + // Ensure < 32 chars, the max length as defined in our docs. + $this->assertLessThan(32, $count); + $this->assertGreaterThan(0, $count); + } + + /** + * Tests the source value + * @param tag_source $source + * @dataProvider tag_source_provider + * @covers \tool_objectfs\local\tag_source::get_value_for_contenthash + */ + public function test_tag_sources_value(tag_source $source) { + $file = $this->create_duplicated_object('tag source value test ' . $source->get_identifier()); + $value = $source->get_value_for_contenthash($file->contenthash); + + // Null value - allowed, but means we cannot test. + if (is_null($value)) { + return; + } + + $count = strlen($value); + + // Ensure < 128 chars, the max length as defined in our docs. + $this->assertLessThan(128, $count); + $this->assertGreaterThan(0, $count); + } + + /** + * Provides values to test_is_tagging_enabled_and_supported + * @return array + */ + public static function is_tagging_enabled_and_supported_provider(): array { + return [ + 'neither config nor fs supports' => [ + 'enabledinconfig' => false, + 'supportedbyfs' => false, + 'expected' => false, + ], + 'enabled in config but fs does not support' => [ + 'enabledinconfig' => true, + 'supportedbyfs' => false, + 'expected' => false, + ], + 'enabled in config and fs does support' => [ + 'enabledinconfig' => true, + 'supportedbyfs' => true, + 'expected' => true, + ], + ]; + } + + /** + * Tests is_tagging_enabled_and_supported + * @param bool $enabledinconfig if tagging feature is turned on + * @param bool $supportedbyfs if the filesystem supports tagging + * @param bool $expected expected return result + * @dataProvider is_tagging_enabled_and_supported_provider + * @covers \tool_objectfs\local\tag_manager::is_tagging_enabled_and_supported + */ + public function test_is_tagging_enabled_and_supported(bool $enabledinconfig, bool $supportedbyfs, bool $expected) { + global $CFG; + // Set config. + set_config('taggingenabled', $enabledinconfig, 'tool_objectfs'); + + // Set supported by fs. + $config = manager::get_objectfs_config(); + $config->taggingenabled = $enabledinconfig; + $config->enabletasks = true; + $config->filesystem = '\\tool_objectfs\\tests\\test_file_system'; + manager::set_objectfs_config($config); + $CFG->phpunit_objectfs_supports_object_tagging = $supportedbyfs; + + $this->assertEquals($expected, tag_manager::is_tagging_enabled_and_supported()); + } + + /** + * Tests gather_object_tags_for_upload + * @covers \tool_objectfs\local\tag_manager::gather_object_tags_for_upload + */ + public function test_gather_object_tags_for_upload() { + $object = $this->create_duplicated_object('gather tags for upload test'); + $tags = tag_manager::gather_object_tags_for_upload($object->contenthash); + + $this->assertArrayHasKey('mimetype', $tags); + $this->assertArrayHasKey('environment', $tags); + $this->assertEquals('text', $tags['mimetype']); + $this->assertEquals('test', $tags['environment']); + } + + /** + * Tests store_tags_locally + * @covers \tool_objectfs\local\tag_manager::store_tags_locally + */ + public function test_store_tags_locally() { + global $DB; + + $tags = [ + 'test1' => 'abc', + 'test2' => 'xyz', + ]; + $hash = 'thisisatest'; + + // Ensure no tags for hash intially. + $this->assertEmpty($DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash])); + + // Store. + tag_manager::store_tags_locally($hash, $tags); + + // Confirm they are stored. + $queriedtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash]); + $this->assertCount(2, $queriedtags); + $tagtimebefore = current($queriedtags)->timemodified; + + // Re-store, confirm times changed. + $this->waitForSecond(); + 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); + } + + /** + * Provides values to test_get_objects_needing_sync + * @return array + */ + public static function get_objects_needing_sync_provider(): array { + return [ + 'duplicated, needs sync' => [ + 'location' => OBJECT_LOCATION_DUPLICATED, + 'status' => tag_manager::SYNC_STATUS_NEEDS_SYNC, + 'expectedneedssync' => true, + ], + 'remote, needs sync' => [ + 'location' => OBJECT_LOCATION_EXTERNAL, + 'status' => tag_manager::SYNC_STATUS_NEEDS_SYNC, + 'expectedneedssync' => true, + ], + 'local, needs sync' => [ + 'location' => OBJECT_LOCATION_LOCAL, + 'status' => tag_manager::SYNC_STATUS_NEEDS_SYNC, + 'expectedneedssync' => false, + ], + 'duplicated, does not need sync' => [ + 'location' => OBJECT_LOCATION_DUPLICATED, + 'status' => tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, + 'expectedneedssync' => false, + ], + 'local, does not need sync' => [ + 'location' => OBJECT_LOCATION_LOCAL, + 'status' => tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, + 'expectedneedssync' => false, + ], + 'duplicated, sync error' => [ + 'location' => OBJECT_LOCATION_DUPLICATED, + 'status' => tag_manager::SYNC_STATUS_ERROR, + 'expectedneedssync' => false, + ], + 'local, sync error' => [ + 'location' => OBJECT_LOCATION_LOCAL, + 'status' => tag_manager::SYNC_STATUS_ERROR, + 'expectedneedssync' => false, + ], + ]; + } + + /** + * Tests get_objects_needing_sync + * @param int $location object location + * @param int $syncstatus sync status to set on object record + * @param bool $expectedneedssync if the object should be included in the return of the function + * @dataProvider get_objects_needing_sync_provider + * @covers \tool_objectfs\local\tag_manager::get_objects_needing_sync + */ + public function test_get_objects_needing_sync(int $location, int $syncstatus, bool $expectedneedssync) { + global $DB; + + // Create the test object at the required location. + switch ($location) { + case OBJECT_LOCATION_DUPLICATED: + $object = $this->create_duplicated_object('tagging test object duplicated'); + break; + case OBJECT_LOCATION_LOCAL: + $object = $this->create_local_object('tagging test object local'); + break; + case OBJECT_LOCATION_EXTERNAL: + $object = $this->create_remote_object('tagging test object remote'); + break; + default: + throw new coding_exception("Object location not handled in test"); + } + + // Set the sync status. + $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $syncstatus, ['id' => $object->id]); + + // Check if it is included in the list. + $needssync = tag_manager::get_objects_needing_sync(1); + + if ($expectedneedssync) { + $this->assertContains($object->contenthash, $needssync); + } else { + $this->assertNotContains($object->contenthash, $needssync); + } + } + + /** + * Tests the limit input to get_objects_needing_sync + * @covers \tool_objectfs\local\tag_manager::get_objects_needing_sync + */ + public function test_get_objects_needing_sync_limit() { + global $DB; + + // Create two duplicated objects needing sync. + $object = $this->create_duplicated_object('sync limit test duplicated'); + $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', tag_manager::SYNC_STATUS_NEEDS_SYNC, ['id' => $object->id]); + $object = $this->create_remote_object('sync limit test remote'); + $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', tag_manager::SYNC_STATUS_NEEDS_SYNC, ['id' => $object->id]); + + // Ensure a limit of 2 returns 2, and limit of 1 returns 1. + $this->assertCount(2, tag_manager::get_objects_needing_sync(2)); + $this->assertCount(1, tag_manager::get_objects_needing_sync(1)); + } + + /** + * Test get_tag_summary_html + * @covers \tool_objectfs\local\tag_manager::get_tag_summary_html + */ + 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); + } + + /** + * Tests when fails to sync object tags, that the sync status is updated to SYNC_STATUS_ERROR. + */ + public function test_object_tag_sync_error() { + global $CFG, $DB; + + // Setup FS for tagging. + $config = manager::get_objectfs_config(); + $config->taggingenabled = true; + $config->enabletasks = true; + $config->filesystem = '\\tool_objectfs\\tests\\test_file_system'; + manager::set_objectfs_config($config); + $CFG->phpunit_objectfs_supports_object_tagging = true; + $this->assertTrue(tag_manager::is_tagging_enabled_and_supported()); + + // Create a good duplicated object. + $object = $this->create_duplicated_object('sync limit test duplicated'); + $status = $DB->get_field('tool_objectfs_objects', 'tagsyncstatus', ['id' => $object->id]); + $this->assertEquals(tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, $status); + + // Now try push tags, but trigger a simulated tag set error. + $CFG->phpunit_objectfs_simulate_tag_set_error = true; + $didthrow = false; + try { + $this->filesystem->push_object_tags($object->contenthash); + } catch (Throwable $e) { + $didthrow = true; + } + $this->assertTrue($didthrow); + + // Ensure tag sync status set to error. + $status = $DB->get_field('tool_objectfs_objects', 'tagsyncstatus', ['id' => $object->id]); + $this->assertEquals(tag_manager::SYNC_STATUS_ERROR, $status); + } +} diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php index 3621c064..0d53105e 100644 --- a/tests/object_file_system_test.php +++ b/tests/object_file_system_test.php @@ -16,8 +16,10 @@ namespace tool_objectfs; +use coding_exception; use tool_objectfs\local\store\object_file_system; use tool_objectfs\local\manager; +use tool_objectfs\local\tag\tag_manager; use tool_objectfs\tests\test_file_system; /** @@ -1018,4 +1020,106 @@ public function test_add_file_from_string_update_object_fail() { $this->assertEquals(\core_text::strlen($content), $result[1]); $this->assertTrue($result[2]); } + + /** + * Test syncing tags throws exception when client does not support tagging. + */ + public function test_push_object_tags_not_supported() { + global $CFG; + $CFG->phpunit_objectfs_supports_object_tagging = false; + $this->expectException(coding_exception::class); + $this->expectExceptionMessage('Cannot sync tags, external client does not support tagging'); + $this->filesystem->push_object_tags('123'); + } + + /** + * Tests syncing object tags where the file is not replicated. + */ + public function test_push_object_tags_object_not_replicated() { + global $CFG, $DB; + $CFG->phpunit_objectfs_supports_object_tagging = true; + + // Create object - not replicated to 'external' store yet. + $object = $this->create_local_object('test syncing local'); + + // Sync, this should do nothing but change sync status - cannot sync object tags + // where the object is not replicated. + $this->filesystem->push_object_tags($object->contenthash); + $object = $DB->get_record('tool_objectfs_objects', ['contenthash' => $object->contenthash]); + $this->assertEquals($object->tagsyncstatus, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED); + } + + /** + * Provides values to push_object_tags_replicated + * @return array + */ + public static function push_object_tags_replicated_provider(): array { + return [ + 'can override' => [ + 'can override' => true, + ], + 'cannot override' => [ + 'cannot override' => false, + ], + ]; + } + + /** + * Tests push_object_tags when the object is replicated. + * @param bool $canoverride if filesystem should be able to overwrite existing objects + * @dataProvider push_object_tags_replicated_provider + */ + public function test_push_object_tags_replicated(bool $canoverride) { + global $CFG, $DB; + $CFG->phpunit_objectfs_supports_object_tagging = true; + + set_config('overwriteobjecttags', $canoverride, 'tool_objectfs'); + $this->assertEquals($canoverride, tag_manager::can_overwrite_object_tags()); + + $object = $this->create_duplicated_object('test syncing replicated'); + + $testtags = [ + 'test' => 123, + 'test2' => 123, + 'test3' => 123, + 'test4' => 123, + ]; + + // Fake set the tags in the external store. + $this->filesystem->get_external_client()->tags[$object->contenthash] = $testtags; + + // Ensure tags are set 'externally'. + $tags = $this->filesystem->get_external_client()->get_object_tags($object->contenthash); + $this->assertCount(count($testtags), $tags); + + // But tags will not be stored locally (yet). + $localtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $object->contenthash]); + $this->assertCount(0, $localtags); + + // Sync the file. + $this->filesystem->push_object_tags($object->contenthash); + + // Tags should now be replicated locally. + $localtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $object->contenthash]); + $externaltags = $this->filesystem->get_external_client()->get_object_tags($object->contenthash); + + if ($canoverride) { + // If can override, we expect it to be overwritten by the tags defined in the sources. + $expectednum = count(tag_manager::get_defined_tag_sources()); + $this->assertCount($expectednum, $localtags); + + // Also expect the external store to be updated. + $this->assertCount($expectednum, $externaltags); + } else { + // If cannot overwrite, no tags should be synced. + $this->assertCount(0, $localtags); + + // External store should not be changed. + $this->assertCount(count($testtags), $externaltags); + } + + // Ensure status changed to not needing sync. + $object = $DB->get_record('tool_objectfs_objects', ['contenthash' => $object->contenthash]); + $this->assertEquals($object->tagsyncstatus, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED); + } } diff --git a/tests/task/populate_objects_filesize_test.php b/tests/task/populate_objects_filesize_test.php index 9ff08ee0..dd1402e1 100644 --- a/tests/task/populate_objects_filesize_test.php +++ b/tests/task/populate_objects_filesize_test.php @@ -179,6 +179,7 @@ public function test_that_non_null_values_are_not_updated() { */ public function test_orphaned_objects_are_not_updated() { global $DB; + $objects = $DB->get_records('tool_objectfs_objects'); $file1 = $this->create_local_file("Test 1"); $this->create_local_file("Test 2"); $this->create_local_file("Test 3"); diff --git a/tests/task/trigger_update_object_tags_test.php b/tests/task/trigger_update_object_tags_test.php new file mode 100644 index 00000000..cd3082d8 --- /dev/null +++ b/tests/task/trigger_update_object_tags_test.php @@ -0,0 +1,49 @@ +. + +namespace tool_objectfs\task; + +use advanced_testcase; +use core\task\manager; + +/** + * Tests trigger_update_object_tags + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class trigger_update_object_tags_test extends advanced_testcase { + /** + * Tests executing scheduled task. + */ + public function test_execute() { + $this->resetAfterTest(); + + $task = new trigger_update_object_tags(); + $task->execute(); + + // Ensure it spawned an adhoc task. + $queuedadhoctasks = manager::get_adhoc_tasks(update_object_tags::class); + $this->assertCount(1, $queuedadhoctasks); + + // Ensure the adhoc task spawned has an iteration of 1. + $adhoctask = current($queuedadhoctasks); + $this->assertNotEmpty($adhoctask->get_custom_data()); + $this->assertEquals(1, $adhoctask->get_custom_data()->iteration); + } +} diff --git a/tests/task/update_object_tags_test.php b/tests/task/update_object_tags_test.php new file mode 100644 index 00000000..f4866aba --- /dev/null +++ b/tests/task/update_object_tags_test.php @@ -0,0 +1,170 @@ +. + +namespace tool_objectfs\task; + +use core\task\manager; +use tool_objectfs\local\tag\tag_manager; +use tool_objectfs\tests\testcase; + +/** + * Tests update_object_tags + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class update_object_tags_test extends testcase { + /** + * Enables tagging in config and sets up the filesystem to allow tagging + */ + private function set_tagging_enabled() { + global $CFG; + $config = \tool_objectfs\local\manager::get_objectfs_config(); + $config->taggingenabled = true; + $config->enabletasks = true; + $config->filesystem = '\\tool_objectfs\\tests\\test_file_system'; + \tool_objectfs\local\manager::set_objectfs_config($config); + $CFG->phpunit_objectfs_supports_object_tagging = true; + } + + /** + * Creates object with tags needing to be synced + * @param string $contents contents of object to create. + * @return stdClass object record + */ + private function create_object_needing_tag_sync(string $contents) { + global $DB; + $object = $this->create_duplicated_object($contents); + $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', tag_manager::SYNC_STATUS_NEEDS_SYNC, ['id' => $object->id]); + return $object; + } + + /** + * Tests task exits when the tagging feature is disabled. + */ + public function test_not_enabled() { + $this->resetAfterTest(); + + // By default filesystem does not support and tagging not enabled, so should error. + $task = new update_object_tags(); + + $this->expectOutputString("Tagging feature not enabled or supported by filesystem, exiting.\n"); + $task->execute(); + } + + /** + * Tests handles an invalid iteration limit + */ + public function test_invalid_iteration_limit() { + $this->resetAfterTest(); + $this->set_tagging_enabled(); + + // This should be greater than 1, if zero should error. + set_config('maxtaggingiterations', 0, 'tool_objectfs'); + + // Give it a valid iteration number though. + $task = new update_object_tags(); + $task->set_custom_data(['iteration' => 5]); + + $this->expectOutputString("Invalid number of iterations, exiting.\n"); + $task->execute(); + } + + /** + * Tests handles an invalid number of iterations in custom data + */ + public function test_invalid_iteration_number() { + $this->resetAfterTest(); + $this->set_tagging_enabled(); + + // Give it a valid max iteration number. + set_config('maxtaggingiterations', 5, 'tool_objectfs'); + + // But don't set the iteration number on the customdata at all. + $task = new update_object_tags(); + $this->expectOutputString("Invalid number of iterations, exiting.\n"); + $task->execute(); + } + + /** + * Tests exits when there are no more objects needing to be synced + */ + public function test_no_more_objects_to_sync() { + $this->resetAfterTest(); + $this->set_tagging_enabled(); + set_config('maxtaggingiterations', 5, 'tool_objectfs'); + $task = new update_object_tags(); + $task->set_custom_data(['iteration' => 1]); + + $this->expectOutputString("No more objects found that need tagging, exiting.\n"); + $task->execute(); + } + + /** + * Tests maxtaggingiterations is correctly checked + */ + public function test_max_iterations() { + $this->resetAfterTest(); + $this->set_tagging_enabled(); + + // Set max 1 iteration. + set_config('maxtaggingiterations', 1, 'tool_objectfs'); + set_config('maxtaggingperrun', 100, 'tool_objectfs'); + + $task = new update_object_tags(); + + // Give it an iteration number higher. + $task->set_custom_data(['iteration' => 5]); + + $this->expectOutputString("Maximum number of iterations reached: 5, exiting.\n"); + $task->execute(); + } + + /** + * Tests a successful tagging run where it needs to requeue for further processing + */ + public function test_tagging_run_with_requeue() { + $this->resetAfterTest(); + $this->set_tagging_enabled(); + + // Set max 1 object per run. + set_config('maxtaggingperrun', 1, 'tool_objectfs'); + set_config('maxtaggingiterations', 5, 'tool_objectfs'); + + // Create two objects needing sync. + $this->create_object_needing_tag_sync('object 1'); + $this->create_object_needing_tag_sync('object 2'); + $this->assertCount(2, tag_manager::get_objects_needing_sync(100)); + + $task = new update_object_tags(); + $task->set_custom_data(['iteration' => 1]); + + $this->expectOutputString("Requeing self for another iteration.\n"); + $task->execute(); + + // Ensure that 1 object had its sync status updated. + $this->assertCount(1, tag_manager::get_objects_needing_sync(100)); + + // Ensure there is another task that was re-queued with the iteration incremented. + $tasks = manager::get_adhoc_tasks(update_object_tags::class); + $this->assertCount(1, $tasks); + $task = current($tasks); + $this->assertNotEmpty($task->get_custom_data()); + $this->assertEquals(2, $task->get_custom_data()->iteration); + } +} From 012ca60071fac59fb523e7c3e2328a63acae4b81 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 29 Jul 2024 09:10:07 +1000 Subject: [PATCH 02/16] test: fix unit test count checking --- tests/task/populate_objects_filesize_test.php | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/task/populate_objects_filesize_test.php b/tests/task/populate_objects_filesize_test.php index dd1402e1..a02f9372 100644 --- a/tests/task/populate_objects_filesize_test.php +++ b/tests/task/populate_objects_filesize_test.php @@ -27,13 +27,6 @@ */ class populate_objects_filesize_test extends \tool_objectfs\tests\testcase { - /** - * This method runs before every test. - */ - public function setUp(): void { - $this->resetAfterTest(); - } - /** * Test multiple objects have their filesize updated. */ @@ -179,7 +172,8 @@ public function test_that_non_null_values_are_not_updated() { */ public function test_orphaned_objects_are_not_updated() { global $DB; - $objects = $DB->get_records('tool_objectfs_objects'); + $numstart = $DB->count_records('tool_objectfs_objects'); + $file1 = $this->create_local_file("Test 1"); $this->create_local_file("Test 2"); $this->create_local_file("Test 3"); @@ -203,8 +197,8 @@ public function test_orphaned_objects_are_not_updated() { }); // Test that 4 records have now been updated. - $this->assertCount(5, $objects); - $this->assertCount(4, $updatedobjects); + $this->assertEquals(5, count($objects) - $numstart); + $this->assertEquals(4, count($updatedobjects) - $numstart); } /** @@ -212,6 +206,8 @@ public function test_orphaned_objects_are_not_updated() { */ public function test_objects_with_error_are_not_updated() { global $DB; + $numstart = $DB->count_records('tool_objectfs_objects'); + $file1 = $this->create_local_file("Test 1"); $this->create_local_file("Test 2"); $this->create_local_file("Test 3"); @@ -235,7 +231,7 @@ public function test_objects_with_error_are_not_updated() { }); // Test that 4 records have now been updated. - $this->assertCount(5, $objects); - $this->assertCount(4, $updatedobjects); + $this->assertEquals(5, count($objects) - $numstart); + $this->assertEquals(4, count($updatedobjects) - $numstart); } } From be1595628d637d0315bb60adb5adb53970d37d14 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 16 Aug 2024 15:45:35 +1000 Subject: [PATCH 03/16] tagging: don't wait for object lock --- classes/local/store/object_file_system.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index 286a2d23..085514e1 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -1176,7 +1176,8 @@ public function push_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, 10); + // Don't wait for it, we want to fail fast. + $lock = $this->acquire_object_lock($contenthash, 0); // No lock - just skip it. if (!$lock) { From 16077edf8c58e4f7f7ae824743b59d7d8820c272 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 16 Aug 2024 15:50:11 +1000 Subject: [PATCH 04/16] tagging: improve migration controls and progress visibility --- classes/local/store/object_file_system.php | 3 +- classes/local/store/s3/client.php | 4 +- classes/local/store/s3/file_system.php | 1 - classes/local/tag/tag_manager.php | 77 +++++++++++++-- classes/task/update_object_tags.php | 99 +++++++++++++++---- db/install.xml | 2 +- db/upgrade.php | 10 +- lang/en/tool_objectfs.php | 20 ++++ settings.php | 22 ++++- tests/local/tagging_test.php | 62 ++++++++---- tests/object_file_system_test.php | 4 +- .../task/trigger_update_object_tags_test.php | 1 + tests/task/update_object_tags_test.php | 66 ++++++++++++- version.php | 4 +- 14 files changed, 312 insertions(+), 63 deletions(-) diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index 085514e1..9fbdb749 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -1197,10 +1197,11 @@ public function push_object_tags(string $contenthash) { $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); + tag_manager::record_tag_pushed_time($contenthash, time()); } // Regardless, it has synced. - tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED); + tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_COMPLETE); } catch (Throwable $e) { $lock->release(); diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 5e73d974..767e177e 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -958,12 +958,12 @@ public function set_object_tags(string $contenthash, array $tags) { * @return array array of key=>value tag pairs */ public function get_object_tags(string $contenthash): array { - $key = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash); + $objectkey = $this->bucketkeyprefix . $this->get_filepath_from_hash($contenthash); // Query from S3. $result = $this->client->getObjectTagging([ 'Bucket' => $this->bucket, - 'Key' => $key, + 'Key' => $objectkey, ]); // Ensure tags are what we expect, and AWS have not changed the format. diff --git a/classes/local/store/s3/file_system.php b/classes/local/store/s3/file_system.php index b272f33b..93637dd4 100644 --- a/classes/local/store/s3/file_system.php +++ b/classes/local/store/s3/file_system.php @@ -32,7 +32,6 @@ use tool_objectfs\local\manager; use tool_objectfs\local\store\object_file_system; -use tool_objectfs\local\tag\tag_manager; require_once($CFG->dirroot . '/admin/tool/objectfs/lib.php'); diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index 36aada41..b5170978 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -17,6 +17,8 @@ namespace tool_objectfs\local\tag; use coding_exception; +use html_table; +use html_writer; use tool_objectfs\local\manager; defined('MOODLE_INTERNAL') || die(); @@ -40,7 +42,7 @@ class tag_manager { /** * @var int Object does not need sync. Will be essentially ignored in tagging process. */ - public const SYNC_STATUS_SYNC_NOT_REQUIRED = 1; + public const SYNC_STATUS_COMPLETE = 1; /** * @var int Object tried to sync but there was an error. Will make it ignored and must be corrected manually. @@ -52,7 +54,7 @@ class tag_manager { */ public const SYNC_STATUSES = [ self::SYNC_STATUS_NEEDS_SYNC, - self::SYNC_STATUS_SYNC_NOT_REQUIRED, + self::SYNC_STATUS_COMPLETE, self::SYNC_STATUS_ERROR, ]; @@ -113,9 +115,6 @@ public static function store_tags_locally(string $contenthash, array $tags) { // 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) { @@ -123,7 +122,6 @@ public static function store_tags_locally(string $contenthash, array $tags) { 'contenthash' => $contenthash, 'tagkey' => $key, 'tagvalue' => $value, - 'timemodified' => $timemodified, ]; } $DB->insert_records('tool_objectfs_object_tags', $recordstostore); @@ -159,18 +157,33 @@ public static function mark_object_tag_sync_status(string $contenthash, int $sta $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]); } + /** + * Updates the tagslastpushed time for a given object. + * This should only be done once successfully pushed to the external store. + * @param string $contenthash + * @param int $time time to set + */ + public static function record_tag_pushed_time(string $contenthash, int $time) { + global $DB; + $DB->set_field('tool_objectfs_objects', 'tagslastpushed', $time, ['contenthash' => $contenthash]); + } + /** * Returns a simple list of all the sources and their descriptions. * @return string html string */ - public static function get_tag_summary_html(): string { + public static function get_tag_source_summary_html(): string { $sources = self::get_defined_tag_sources(); - $html = ''; + $table = new html_table(); + $table->head = [ + get_string('table:tagsource', 'tool_objectfs'), + get_string('table:tagsourcemeaning', 'tool_objectfs'), + ]; foreach ($sources as $source) { - $html .= $source->get_identifier() . ': ' . $source->get_description() . '
'; + $table->data[$source->get_identifier()] = [$source->get_identifier(), $source->get_description()]; } - return $html; + return html_writer::table($table); } /** @@ -180,4 +193,48 @@ public static function get_tag_summary_html(): string { public static function can_overwrite_object_tags(): bool { return (bool) get_config('tool_objectfs', 'overwriteobjecttags'); } + + /** + * Get the string for a given tag sync status + * @param int $tagsyncstatus one of SYNC_STATUS_* + * @return string + */ + private static function get_sync_status_string(int $tagsyncstatus): string { + $strmap = [ + self::SYNC_STATUS_ERROR => 'error', + self::SYNC_STATUS_NEEDS_SYNC => 'needssync', + self::SYNC_STATUS_COMPLETE => 'notrequired', + ]; + + if (!array_key_exists($tagsyncstatus, $strmap)) { + throw new coding_exception('No status string is mapped for status: ' . $tagsyncstatus); + } + + return get_string('tagsyncstatus:' . $strmap[$tagsyncstatus], 'tool_objectfs'); + } + + /** + * Returns a html table with summaries of the sync statuses and the object count for each. + * @return string + */ + public static function get_tag_sync_status_summary_html(): string { + global $DB; + $statuses = $DB->get_records_sql("SELECT tagsyncstatus, COUNT(tagsyncstatus) + FROM {tool_objectfs_objects} + GROUP BY tagsyncstatus"); + + $table = new html_table(); + $table->head = [ + get_string('table:status', 'tool_objectfs'), + get_string('table:objectcount', 'tool_objectfs'), + ]; + + foreach (self::SYNC_STATUSES as $status) { + // If no objects have a status, they won't appear in the SQL above. + // In this case, just show zero (so the use knows it exists, but is zero). + $count = isset($statuses[$status]) ? $statuses[$status]->count : 0; + $table->data[$status] = [self::get_sync_status_string($status), $count]; + } + return html_writer::table($table); + } } diff --git a/classes/task/update_object_tags.php b/classes/task/update_object_tags.php index cc3bbfcb..d979d0ca 100644 --- a/classes/task/update_object_tags.php +++ b/classes/task/update_object_tags.php @@ -16,7 +16,12 @@ namespace tool_objectfs\task; +use coding_exception; use core\task\adhoc_task; +use core\task\manager; +use html_table; +use html_writer; +use moodle_exception; use tool_objectfs\local\tag\tag_manager; /** @@ -28,28 +33,95 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class update_object_tags extends adhoc_task { + + /** + * Generates a html table summarising the update object tag tasks that exist + * @return string + */ + public static function get_summary_html(): string { + $tasks = manager::get_adhoc_tasks(self::class); + + if (empty($tasks)) { + return get_string('tagging:migration:nothingrunning'); + } + + $table = new html_table(); + $table->head = [ + get_string('table:taskid', 'tool_objectfs'), + get_string('table:iteration', 'tool_objectfs'), + get_string('table:status', 'tool_objectfs'), + ]; + + foreach ($tasks as $task) { + $table->data[$task->get_id()] = [$task->get_id(), $task->get_iteration(), $task->get_status_badge()]; + } + + return html_writer::table($table); + } + + /** + * Returns a status badge depending on the health of the task + * @return string + */ + public function get_status_badge(): string { + $identifier = ''; + $class = ''; + + if ($this->get_fail_delay() > 0) { + $identifier = 'failing'; + $class = 'badge-warning'; + } else if (!is_null($this->get_timestarted()) && $this->get_timestarted() > 0) { + $identifier = 'running'; + $class = 'badge-info'; + } else { + $identifier = 'waiting'; + $class = 'badge-info'; + } + + return html_writer::span(get_string('status:'.$identifier, 'tool_objectfs', $this->get_fail_delay()), 'badge ' . $class); + } + + /** + * Returns iteration count + * @return int + */ + public function get_iteration(): int { + return !empty($this->get_custom_data()->iteration) ? $this->get_custom_data()->iteration : 0; + } + /** * Execute task */ public function execute() { if (!tag_manager::is_tagging_enabled_and_supported()) { - mtrace("Tagging feature not enabled or supported by filesystem, exiting."); - return; + // Site admin should know if this migration is running but the fs doesn't support tagging + // (maybe they changed fs mid-run?). + throw new moodle_exception('tagging:migration:notsupported', 'tool_objectfs'); } // Since this adhoc task can requeue itself, ensure there is a fixed limit on the number // of times this can happen, to avoid any accidental runaways. $iterationlimit = get_config('tool_objectfs', 'maxtaggingiterations') ?: 0; - $iteration = !empty($this->get_custom_data()->iteration) ? $this->get_custom_data()->iteration : 0; + $iteration = $this->get_iteration(); - if (empty($iterationlimit) || empty($iteration)) { - mtrace("Invalid number of iterations, exiting."); - return; + if (empty($iterationlimit) || empty($iteration) || $iterationlimit < 0 || $iteration < 0) { + // This should never hit here, if it does something is very wrong. + // Throw exception so it causes a retry and alerts. + throw new moodle_exception('tagging:migration:invaliditerations', 'tool_objectfs'); } - if (abs($iteration) > abs($iterationlimit)) { - mtrace("Maximum number of iterations reached: " . $iteration . ", exiting."); - return; + if ($iteration > $iterationlimit) { + // Generally this means the site has too many objects or not enough configured iterations. + // Regardless it should throw an exception to get the site admins attention. + throw new moodle_exception('tagging:migration:limitreached', 'tool_objectfs', '', $iteration); + } + + $fs = get_file_storage()->get_file_system(); + + // This is checked above in tag_manager::is_tagging_enabled_and_supported, but as a sanity check + // ensure this specific method is defined. + if (!method_exists($fs, "push_object_tags")) { + throw new coding_exception("Filesystem does not define push_object_tags"); } // Get the maximum num of objects to update as configured. @@ -57,18 +129,11 @@ public function execute() { $contenthashes = tag_manager::get_objects_needing_sync($limit); if (empty($contenthashes)) { + // This is ok, it means we are done. Exit silently. mtrace("No more objects found that need tagging, exiting."); return; } - // Sanity check that fs is object file system and not anything else. - $fs = get_file_storage()->get_file_system(); - - if (!method_exists($fs, "push_object_tags")) { - mtrace("File system is not object file system, exiting."); - return; - } - // For each, try to sync their tags. foreach ($contenthashes as $contenthash) { $fs->push_object_tags($contenthash); diff --git a/db/install.xml b/db/install.xml index 5d6b6f4e..5c5fc2ca 100644 --- a/db/install.xml +++ b/db/install.xml @@ -12,6 +12,7 @@ + @@ -56,7 +57,6 @@ - diff --git a/db/upgrade.php b/db/upgrade.php index 4554b7e4..078cecf4 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -181,7 +181,6 @@ function xmldb_tool_objectfs_upgrade($oldversion) { $table->add_field('contenthash', XMLDB_TYPE_CHAR, '40', null, XMLDB_NOTNULL, null, null); $table->add_field('tagkey', XMLDB_TYPE_CHAR, '32', null, XMLDB_NOTNULL, null, null); $table->add_field('tagvalue', XMLDB_TYPE_CHAR, '128', null, XMLDB_NOTNULL, null, null); - $table->add_field('timemodified', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); // Adding keys to table tool_objectfs_object_tags. $table->add_key('primary', XMLDB_KEY_PRIMARY, ['id']); @@ -211,6 +210,15 @@ function xmldb_tool_objectfs_upgrade($oldversion) { // Launch change of precision for field datakey. $dbman->change_field_precision($table, $field); + // Define field tagslastpushed to be added to tool_objectfs_objects. + $table = new xmldb_table('tool_objectfs_objects'); + $field = new xmldb_field('tagslastpushed', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, 0, 'tagsyncstatus'); + + // Conditionally launch add field tagslastpushed. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + // Objectfs savepoint reached. upgrade_plugin_savepoint(true, 2024093000, 'tool', 'objectfs'); } diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 98ff7f9f..58d65495 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -291,6 +291,26 @@ $string['tagsource:environment'] = 'Environment defined by $CFG->objectfs_environment_name, currently: "{$a}".'; $string['tagsource:mimetype'] = 'File mimetype as stored in {files} table'; $string['settings:tagsources'] = 'Tag sources'; +$string['settings:taggingstatus'] = 'Tagging status'; $string['task:triggerupdateobjecttags'] = 'Queue adhoc task to update object tags'; $string['settings:tagging:help'] = 'Object tagging allows extra metadata to be attached to objects in the external store. Please read TAGGING.md in the plugin Github repository for detailed setup and considerations. This is currently only supported by the S3 external client.'; $string['object_status:tag_count'] = 'Object tags'; +$string['tagsyncstatus:error'] = 'Errored'; +$string['tagsyncstatus:notrequired'] = 'Not required / synced'; +$string['tagsyncstatus:needssync'] = 'Waiting for sync'; +$string['settings:taggingstatuscounts'] = 'Tag sync status overview'; +$string['tagging:migration:notsupported'] = 'Tagging not enabled or supported by filesystem. Cannot execute tag migration task'; +$string['tagging:migration:invaliditerations'] = 'Invalid iteration number or iteration count'; +$string['tagging:migration:limitreached'] = 'Current iteration {$a} is >= the maximum number of iterations. Please investigate if this is expected and you need to increase the limit, or if there is a problem syncing the tags causing an infinite loop'; +$string['settings:taggingmigrationstatus'] = 'Tagging adhoc migration progress'; +$string['tagging:migration:nothingrunning'] = 'No tagging migration adhoc tasks are currently running'; +$string['tagging:migration:help'] = 'Run the trigger_update_object_tags scheduled task from the frontend or CLI to start a migration task.'; +$string['table:taskid'] = 'Task ID'; +$string['table:iteration'] = 'Iteration number'; +$string['table:status'] = 'Status'; +$string['table:objectcount'] = 'Object count'; +$string['table:tagsource'] = 'Tag source'; +$string['table:tagsourcemeaning'] = 'Description'; +$string['status:waiting'] = 'Waiting'; +$string['status:running'] = 'Running'; +$string['status:failing'] = 'Faildelay {$a}'; diff --git a/settings.php b/settings.php index 3bde1957..2b2e6fe6 100644 --- a/settings.php +++ b/settings.php @@ -258,10 +258,7 @@ // Tagging settings. $settings->add(new admin_setting_heading('tool_objectfs/taggingsettings', - new lang_string('settings:taggingheader', 'tool_objectfs'), '')); - - $settings->add(new admin_setting_description('tool_objectfs/tagginghelp', - '', + new lang_string('settings:taggingheader', 'tool_objectfs'), get_string('settings:tagging:help', 'tool_objectfs') )); @@ -270,7 +267,7 @@ $settings->add(new admin_setting_description('tool_objectfs/tagsources', new lang_string('settings:tagsources', 'tool_objectfs'), - tag_manager::get_tag_summary_html() + tag_manager::get_tag_source_summary_html() )); $settings->add(new admin_setting_configtext('tool_objectfs/maxtaggingperrun', @@ -293,4 +290,19 @@ 1 )); + // Tagging status. + $settings->add(new admin_setting_heading('tool_objectfs/taggingstatus', + new lang_string('settings:taggingstatus', 'tool_objectfs'), '')); + + // Build overview of tag statuses. + $settings->add(new admin_setting_description('tool_objectfs/taggingstatuscounts', + new lang_string('settings:taggingstatuscounts', 'tool_objectfs'), + tag_manager::get_tag_sync_status_summary_html() + )); + + // Overview of tagging migration. + $settings->add(new admin_setting_description('tool_objectfs/taggingmigrationstatus', + new lang_string('settings:taggingmigrationstatus', 'tool_objectfs'), + update_object_tags::get_summary_html(), + )); } diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index 95443451..52954c0a 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -113,7 +113,7 @@ public static function is_tagging_enabled_and_supported_provider(): array { 'supportedbyfs' => false, 'expected' => false, ], - 'enabled in config and fs does support' => [ + 'enabled in config and fs does support' => [ 'enabledinconfig' => true, 'supportedbyfs' => true, 'expected' => true, @@ -181,15 +181,6 @@ public function test_store_tags_locally() { // Confirm they are stored. $queriedtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash]); $this->assertCount(2, $queriedtags); - $tagtimebefore = current($queriedtags)->timemodified; - - // Re-store, confirm times changed. - $this->waitForSecond(); - 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); } /** @@ -215,12 +206,12 @@ public static function get_objects_needing_sync_provider(): array { ], 'duplicated, does not need sync' => [ 'location' => OBJECT_LOCATION_DUPLICATED, - 'status' => tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, + 'status' => tag_manager::SYNC_STATUS_COMPLETE, 'expectedneedssync' => false, ], 'local, does not need sync' => [ 'location' => OBJECT_LOCATION_LOCAL, - 'status' => tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, + 'status' => tag_manager::SYNC_STATUS_COMPLETE, 'expectedneedssync' => false, ], 'duplicated, sync error' => [ @@ -294,17 +285,18 @@ public function test_get_objects_needing_sync_limit() { } /** - * Test get_tag_summary_html - * @covers \tool_objectfs\local\tag_manager::get_tag_summary_html + * Test get_tag_source_summary_html + * @covers \tool_objectfs\local\tag_manager::get_tag_source_summary_html */ - public function test_get_tag_summary_html() { + public function test_get_tag_source_summary_html() { // Quick test just to ensure it generates and nothing explodes. - $html = tag_manager::get_tag_summary_html(); + $html = tag_manager::get_tag_source_summary_html(); $this->assertIsString($html); } /** * Tests when fails to sync object tags, that the sync status is updated to SYNC_STATUS_ERROR. + * @covers \tool_objectfs\local\tag_manager */ public function test_object_tag_sync_error() { global $CFG, $DB; @@ -321,7 +313,7 @@ public function test_object_tag_sync_error() { // Create a good duplicated object. $object = $this->create_duplicated_object('sync limit test duplicated'); $status = $DB->get_field('tool_objectfs_objects', 'tagsyncstatus', ['id' => $object->id]); - $this->assertEquals(tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED, $status); + $this->assertEquals(tag_manager::SYNC_STATUS_COMPLETE, $status); // Now try push tags, but trigger a simulated tag set error. $CFG->phpunit_objectfs_simulate_tag_set_error = true; @@ -337,4 +329,40 @@ public function test_object_tag_sync_error() { $status = $DB->get_field('tool_objectfs_objects', 'tagsyncstatus', ['id' => $object->id]); $this->assertEquals(tag_manager::SYNC_STATUS_ERROR, $status); } + + /** + * Tests tag_manager::record_tag_pushed_time + * @covers \tool_objectfs\local\tag_manager::record_tag_pushed_time + */ + public function test_record_tag_pushed_time() { + global $DB; + $object = $this->create_remote_object(); + $initialtime = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['contenthash' => $object->contenthash]); + $this->assertEquals(0, $initialtime); + + tag_manager::record_tag_pushed_time($object->contenthash, 100); + $newtime = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['contenthash' => $object->contenthash]); + $this->assertEquals(100, $newtime); + } + + /** + * Tests getting sync status summary html. + * @covers \tool_objectfs\local\tag_manager::get_tag_sync_status_summary_html. + */ + public function test_get_tag_sync_status_summary_html() { + // Make two objects each with a different status. + $object1 = $this->create_remote_object('o1'); + $object2 = $this->create_remote_object('o2'); + + tag_manager::mark_object_tag_sync_status($object1->contenthash, tag_manager::SYNC_STATUS_COMPLETE); + tag_manager::mark_object_tag_sync_status($object2->contenthash, tag_manager::SYNC_STATUS_ERROR); + + // Generate report. + $reporthtml = tag_manager::get_tag_sync_status_summary_html(); + + // Ensure report contains a column for every status (even thought there are none of some status in the db). + // (-1 to remove the header row). + $rowcount = substr_count($reporthtml, 'assertEquals(count(tag_manager::SYNC_STATUSES), $rowcount); + } } diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php index 0d53105e..fd296a75 100644 --- a/tests/object_file_system_test.php +++ b/tests/object_file_system_test.php @@ -1046,7 +1046,7 @@ public function test_push_object_tags_object_not_replicated() { // where the object is not replicated. $this->filesystem->push_object_tags($object->contenthash); $object = $DB->get_record('tool_objectfs_objects', ['contenthash' => $object->contenthash]); - $this->assertEquals($object->tagsyncstatus, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED); + $this->assertEquals($object->tagsyncstatus, tag_manager::SYNC_STATUS_COMPLETE); } /** @@ -1120,6 +1120,6 @@ public function test_push_object_tags_replicated(bool $canoverride) { // Ensure status changed to not needing sync. $object = $DB->get_record('tool_objectfs_objects', ['contenthash' => $object->contenthash]); - $this->assertEquals($object->tagsyncstatus, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED); + $this->assertEquals($object->tagsyncstatus, tag_manager::SYNC_STATUS_COMPLETE); } } diff --git a/tests/task/trigger_update_object_tags_test.php b/tests/task/trigger_update_object_tags_test.php index cd3082d8..53413003 100644 --- a/tests/task/trigger_update_object_tags_test.php +++ b/tests/task/trigger_update_object_tags_test.php @@ -30,6 +30,7 @@ class trigger_update_object_tags_test extends advanced_testcase { /** * Tests executing scheduled task. + * @covers \tool_objectfs\task\trigger_update_object_tags::execute */ public function test_execute() { $this->resetAfterTest(); diff --git a/tests/task/update_object_tags_test.php b/tests/task/update_object_tags_test.php index f4866aba..7382f9fe 100644 --- a/tests/task/update_object_tags_test.php +++ b/tests/task/update_object_tags_test.php @@ -17,6 +17,7 @@ namespace tool_objectfs\task; use core\task\manager; +use moodle_exception; use tool_objectfs\local\tag\tag_manager; use tool_objectfs\tests\testcase; @@ -27,6 +28,7 @@ * @author Matthew Hilton * @copyright Catalyst IT * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \tool_objectfs\task\update_object_tags */ class update_object_tags_test extends testcase { /** @@ -63,7 +65,8 @@ public function test_not_enabled() { // By default filesystem does not support and tagging not enabled, so should error. $task = new update_object_tags(); - $this->expectOutputString("Tagging feature not enabled or supported by filesystem, exiting.\n"); + $this->expectException(moodle_exception::class); + $this->expectExceptionMessage(get_string('tagging:migration:notsupported', 'tool_objectfs')); $task->execute(); } @@ -81,7 +84,8 @@ public function test_invalid_iteration_limit() { $task = new update_object_tags(); $task->set_custom_data(['iteration' => 5]); - $this->expectOutputString("Invalid number of iterations, exiting.\n"); + $this->expectException(moodle_exception::class); + $this->expectExceptionMessage(get_string('tagging:migration:invaliditerations', 'tool_objectfs')); $task->execute(); } @@ -96,8 +100,10 @@ public function test_invalid_iteration_number() { set_config('maxtaggingiterations', 5, 'tool_objectfs'); // But don't set the iteration number on the customdata at all. + $this->expectException(moodle_exception::class); + $this->expectExceptionMessage(get_string('tagging:migration:invaliditerations', 'tool_objectfs')); + $task = new update_object_tags(); - $this->expectOutputString("Invalid number of iterations, exiting.\n"); $task->execute(); } @@ -111,6 +117,7 @@ public function test_no_more_objects_to_sync() { $task = new update_object_tags(); $task->set_custom_data(['iteration' => 1]); + // This should not error, only output a string since it is successfully completed. $this->expectOutputString("No more objects found that need tagging, exiting.\n"); $task->execute(); } @@ -131,7 +138,8 @@ public function test_max_iterations() { // Give it an iteration number higher. $task->set_custom_data(['iteration' => 5]); - $this->expectOutputString("Maximum number of iterations reached: 5, exiting.\n"); + $this->expectException(moodle_exception::class); + $this->expectExceptionMessage(get_string('tagging:migration:limitreached', 'tool_objectfs', 5)); $task->execute(); } @@ -167,4 +175,54 @@ public function test_tagging_run_with_requeue() { $this->assertNotEmpty($task->get_custom_data()); $this->assertEquals(2, $task->get_custom_data()->iteration); } + + /** + * Tests get_iteration + * @covers \tool_objectfs\task\update_object_tags::get_iteration + */ + public function test_get_iteration() { + $task = new update_object_tags(); + + // No custom data, should return zero. + $this->assertEquals(0, $task->get_iteration()); + + // Set iteration, it should return that. + $task->set_custom_data([ + 'iteration' => 5, + ]); + $this->assertEquals(5, $task->get_iteration()); + } + + /** + * Tests getting status badge and summary html + * @covers \tool_objectfs\task\update_object_tags::get_status_badge + * @covers \tool_objectfs\task\update_object_tags::get_summary_html + */ + public function test_get_summary_html_and_status_badge() { + // Spawn three tasks and break each one in a different way. + // Test their badge output. + $task1 = new update_object_tags(); + $this->assertStringContainsString(get_string('status:waiting', 'tool_objectfs'), + $task1->get_status_badge()); + + $task2 = new update_object_tags(); + $task2->set_fail_delay(1000); + $this->assertStringContainsString(get_string('status:failing', 'tool_objectfs', 1000), + $task2->get_status_badge()); + + $task3 = new update_object_tags(); + $task3->set_timestarted(1000); + $this->assertStringContainsString(get_string('status:running', 'tool_objectfs'), + $task3->get_status_badge()); + + // Now queue them so we can test report generation. + manager::queue_adhoc_task($task1); + manager::queue_adhoc_task($task2); + manager::queue_adhoc_task($task3); + + // Ensure row count is expected (excluding header row). + $report = update_object_tags::get_summary_html(); + $rowcount = substr_count($report, 'assertEquals(3, $rowcount); + } } diff --git a/version.php b/version.php index 25c5d28d..096818a8 100644 --- a/version.php +++ b/version.php @@ -25,8 +25,8 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2024091700; // The current plugin version (Date: YYYYMMDDXX). -$plugin->release = 2024091700; // Same as version. +$plugin->version = 2024093000; // The current plugin version (Date: YYYYMMDDXX). +$plugin->release = 2024093000; // Same as version. $plugin->requires = 2023042400; // Requires 4.2. $plugin->component = "tool_objectfs"; $plugin->maturity = MATURITY_STABLE; From 59bf1ae43f0de02098b9ffae14e8aa3032a99e93 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 19 Aug 2024 15:44:47 +1000 Subject: [PATCH 05/16] feat: move tagging status reports to check api --- classes/check/tagging_migration_status.php | 80 +++++++++++++++++++ classes/check/tagging_sync_status.php | 74 +++++++++++++++++ classes/local/tag/tag_manager.php | 28 ++----- classes/task/update_object_tags.php | 26 ------ classes/tests/testcase.php | 17 ++++ lang/en/tool_objectfs.php | 7 ++ lib.php | 2 + settings.php | 20 +++-- tests/check/tagging_migration_status_test.php | 73 +++++++++++++++++ tests/check/tagging_sync_status_test.php | 68 ++++++++++++++++ tests/local/tagging_test.php | 64 ++++++++++++--- tests/task/update_object_tags_test.php | 38 ++------- 12 files changed, 396 insertions(+), 101 deletions(-) create mode 100644 classes/check/tagging_migration_status.php create mode 100644 classes/check/tagging_sync_status.php create mode 100644 tests/check/tagging_migration_status_test.php create mode 100644 tests/check/tagging_sync_status_test.php diff --git a/classes/check/tagging_migration_status.php b/classes/check/tagging_migration_status.php new file mode 100644 index 00000000..abaa9de4 --- /dev/null +++ b/classes/check/tagging_migration_status.php @@ -0,0 +1,80 @@ +. + +namespace tool_objectfs\check; + +use core\check\check; +use core\check\result; +use core\task\manager; +use html_table; +use html_writer; +use tool_objectfs\task\update_object_tags; + +/** + * Tagging migration status check + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tagging_migration_status extends check { + /** + * Link to ObjectFS settings page. + * + * @return \action_link|null + */ + public function get_action_link(): ?\action_link { + $url = new \moodle_url('/admin/category.php', ['category' => 'tool_objectfs']); + return new \action_link($url, get_string('pluginname', 'tool_objectfs')); + } + + /** + * Get result + * @return result + */ + public function get_result(): result { + // We want to check this regardless if enabled or supported and not exit early. + // Because it may have been turned off accidentally thus causing the migration to fail. + $tasks = manager::get_adhoc_tasks(update_object_tags::class); + + if (empty($tasks)) { + return new result(result::NA, get_string('tagging:migration:nothingrunning', 'tool_objectfs')); + } + + $table = new html_table(); + $table->head = [ + get_string('table:taskid', 'tool_objectfs'), + get_string('table:iteration', 'tool_objectfs'), + get_string('table:status', 'tool_objectfs'), + ]; + + foreach ($tasks as $task) { + $table->data[$task->get_id()] = [$task->get_id(), $task->get_iteration(), $task->get_status_badge()]; + } + $html = html_writer::table($table); + + $ataskisfailing = !empty(array_filter($tasks, function($task) { + return $task->get_fail_delay() > 0; + })); + + if ($ataskisfailing) { + return new result(result::WARNING, get_string('check:tagging:migrationerror', 'tool_objectfs'), $html); + } + + return new result(result::OK, get_string('check:tagging:migrationok', 'tool_objectfs'), $html); + } +} diff --git a/classes/check/tagging_sync_status.php b/classes/check/tagging_sync_status.php new file mode 100644 index 00000000..868687fa --- /dev/null +++ b/classes/check/tagging_sync_status.php @@ -0,0 +1,74 @@ +. + +namespace tool_objectfs\check; + +use core\check\check; +use core\check\result; +use html_table; +use html_writer; +use tool_objectfs\local\tag\tag_manager; + +/** + * Tagging sync status check + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tagging_sync_status extends check { + /** + * Link to ObjectFS settings page. + * + * @return \action_link|null + */ + public function get_action_link(): ?\action_link { + $url = new \moodle_url('/admin/category.php', ['category' => 'tool_objectfs']); + return new \action_link($url, get_string('pluginname', 'tool_objectfs')); + } + + /** + * Get result + * @return result + */ + public function get_result(): result { + if (!tag_manager::is_tagging_enabled_and_supported()) { + return new result(result::NA, get_string('check:tagging:na', 'tool_objectfs')); + } + + $statuses = tag_manager::get_tag_sync_status_summary(); + $table = new html_table(); + $table->head = [ + get_string('table:status', 'tool_objectfs'), + get_string('table:objectcount', 'tool_objectfs'), + ]; + + foreach (tag_manager::SYNC_STATUSES as $status) { + // If no objects have a status, they won't appear in the SQL above. + // In this case, just show zero (so the use knows it exists, but is zero). + $count = isset($statuses[$status]) ? $statuses[$status]->count : 0; + $table->data[$status] = [tag_manager::get_sync_status_string($status), $count]; + } + $table = html_writer::table($table); + + if (!empty($statuses[tag_manager::SYNC_STATUS_ERROR])) { + return new result(result::WARNING, get_string('check:tagging:syncerror', 'tool_objectfs'), $table); + } + + return new result(result::OK, get_string('check:tagging:syncok', 'tool_objectfs'), $table); + } +} diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index b5170978..8691315e 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -199,7 +199,7 @@ public static function can_overwrite_object_tags(): bool { * @param int $tagsyncstatus one of SYNC_STATUS_* * @return string */ - private static function get_sync_status_string(int $tagsyncstatus): string { + public static function get_sync_status_string(int $tagsyncstatus): string { $strmap = [ self::SYNC_STATUS_ERROR => 'error', self::SYNC_STATUS_NEEDS_SYNC => 'needssync', @@ -214,27 +214,13 @@ private static function get_sync_status_string(int $tagsyncstatus): string { } /** - * Returns a html table with summaries of the sync statuses and the object count for each. - * @return string + * Returns a summary of the object tag sync statuses. + * @return array */ - public static function get_tag_sync_status_summary_html(): string { + public static function get_tag_sync_status_summary(): array { global $DB; - $statuses = $DB->get_records_sql("SELECT tagsyncstatus, COUNT(tagsyncstatus) - FROM {tool_objectfs_objects} - GROUP BY tagsyncstatus"); - - $table = new html_table(); - $table->head = [ - get_string('table:status', 'tool_objectfs'), - get_string('table:objectcount', 'tool_objectfs'), - ]; - - foreach (self::SYNC_STATUSES as $status) { - // If no objects have a status, they won't appear in the SQL above. - // In this case, just show zero (so the use knows it exists, but is zero). - $count = isset($statuses[$status]) ? $statuses[$status]->count : 0; - $table->data[$status] = [self::get_sync_status_string($status), $count]; - } - return html_writer::table($table); + return $DB->get_records_sql("SELECT tagsyncstatus, COUNT(tagsyncstatus) + FROM {tool_objectfs_objects} + GROUP BY tagsyncstatus"); } } diff --git a/classes/task/update_object_tags.php b/classes/task/update_object_tags.php index d979d0ca..28b5b13c 100644 --- a/classes/task/update_object_tags.php +++ b/classes/task/update_object_tags.php @@ -33,32 +33,6 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class update_object_tags extends adhoc_task { - - /** - * Generates a html table summarising the update object tag tasks that exist - * @return string - */ - public static function get_summary_html(): string { - $tasks = manager::get_adhoc_tasks(self::class); - - if (empty($tasks)) { - return get_string('tagging:migration:nothingrunning'); - } - - $table = new html_table(); - $table->head = [ - get_string('table:taskid', 'tool_objectfs'), - get_string('table:iteration', 'tool_objectfs'), - get_string('table:status', 'tool_objectfs'), - ]; - - foreach ($tasks as $task) { - $table->data[$task->get_id()] = [$task->get_id(), $task->get_iteration(), $task->get_status_badge()]; - } - - return html_writer::table($table); - } - /** * Returns a status badge depending on the health of the task * @return string diff --git a/classes/tests/testcase.php b/classes/tests/testcase.php index 36ad5d8c..9ee21957 100644 --- a/classes/tests/testcase.php +++ b/classes/tests/testcase.php @@ -54,6 +54,23 @@ protected function setUp(): void { $this->resetAfterTest(true); } + /** + * Enables the test object filesystem and sets the tagging value. + * @param bool $tagging if tagging should be enabled or not. + */ + protected function enable_filesystem_and_set_tagging(bool $tagging) { + global $CFG; + set_config('taggingenabled', $tagging, 'tool_objectfs'); + + // Set supported by fs. + $config = manager::get_objectfs_config(); + $config->taggingenabled = $tagging; + $config->enabletasks = true; + $config->filesystem = '\\tool_objectfs\\tests\\test_file_system'; + manager::set_objectfs_config($config); + $CFG->phpunit_objectfs_supports_object_tagging = $tagging; + } + /** * reset_file_system * @return void diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 58d65495..78576878 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -280,6 +280,10 @@ $string['settings:taggingenabled'] = 'Tagging enabled'; $string['checktagging_status'] = 'Object tagging'; $string['check:tagging:ok'] = 'Object tagging ok'; +$string['check:tagging:syncerror'] = 'Objects have tag sync errors'; +$string['check:tagging:syncok'] = 'No objects reporting sync errors'; +$string['check:tagging:migrationerror'] = 'Object tagging migration task(s) have faildelay > 0'; +$string['check:tagging:migrationok'] = 'Object tagging migration tasks OK'; $string['check:tagging:na'] = 'Tagging not enabled or is not supported by file system'; $string['check:tagging:error'] = 'Error trying to tag object'; $string['settings:maxtaggingperrun'] = 'Object tagging adhoc sync maximum objects per run'; @@ -314,3 +318,6 @@ $string['status:waiting'] = 'Waiting'; $string['status:running'] = 'Running'; $string['status:failing'] = 'Faildelay {$a}'; +$string['checktagging_sync_status'] = 'Object tagging sync status'; +$string['checktagging_migration_status'] = 'Object tagging migration status'; + diff --git a/lib.php b/lib.php index 82b23d25..ad6fe853 100644 --- a/lib.php +++ b/lib.php @@ -105,6 +105,8 @@ function tool_objectfs_status_checks() { $checks = [ new tool_objectfs\check\token_expiry(), new tool_objectfs\check\tagging_status(), + new tool_objectfs\check\tagging_sync_status(), + new tool_objectfs\check\tagging_migration_status(), ]; if (get_config('tool_objectfs', 'proxyrangerequests')) { diff --git a/settings.php b/settings.php index 2b2e6fe6..bd03718e 100644 --- a/settings.php +++ b/settings.php @@ -294,15 +294,13 @@ $settings->add(new admin_setting_heading('tool_objectfs/taggingstatus', new lang_string('settings:taggingstatus', 'tool_objectfs'), '')); - // Build overview of tag statuses. - $settings->add(new admin_setting_description('tool_objectfs/taggingstatuscounts', - new lang_string('settings:taggingstatuscounts', 'tool_objectfs'), - tag_manager::get_tag_sync_status_summary_html() - )); - - // Overview of tagging migration. - $settings->add(new admin_setting_description('tool_objectfs/taggingmigrationstatus', - new lang_string('settings:taggingmigrationstatus', 'tool_objectfs'), - update_object_tags::get_summary_html(), - )); + // TODO when this branch supports 4.4+, use admin_setting_check https://tracker.moodle.org/browse/MDL-67898. + $settings->add(new admin_setting_description('taggingstatuslink', '', html_writer::link( + new moodle_url('/report/status/index.php', ['detail' => 'tool_objectfs_tagging_sync_status']), + get_string('settings:taggingstatus', 'tool_objectfs') + ))); + $settings->add(new admin_setting_description('taggingmigrationstatuslink', '', html_writer::link( + new moodle_url('/report/status/index.php', ['detail' => 'tool_objectfs_tagging_migration_status']), + get_string('settings:taggingmigrationstatus', 'tool_objectfs') + ))); } diff --git a/tests/check/tagging_migration_status_test.php b/tests/check/tagging_migration_status_test.php new file mode 100644 index 00000000..3b9763c7 --- /dev/null +++ b/tests/check/tagging_migration_status_test.php @@ -0,0 +1,73 @@ +. + +namespace tool_objectfs\check; + +use core\check\result; +use core\task\manager; +use tool_objectfs\check\tagging_migration_status; +use tool_objectfs\task\update_object_tags; +use tool_objectfs\tests\testcase; + +/** + * Tagging migration status check tests + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \tool_objectfs\check\tagging_migration_status + */ +class tagging_migration_status_test extends testcase { + /** + * Tests scenario that returns N/A + */ + public function test_get_result_na() { + // Regardless if this is disabled, the check should still return a non n/a status. + $this->enable_filesystem_and_set_tagging(false); + $check = new tagging_migration_status(); + $this->assertEquals(result::NA, $check->get_result()->get_status()); + } + + /* + * Test scenario that returns WARNING + */ + public function test_get_result_warning() { + // Regardless if this is disabled, the check should still return a non n/a status. + $this->enable_filesystem_and_set_tagging(false); + + $task = new update_object_tags(); + $task->set_fail_delay(64); + manager::queue_adhoc_task($task); + + $check = new tagging_migration_status(); + $this->assertEquals(result::WARNING, $check->get_result()->get_status()); + } + + /* + * Test scenario that returns OK + */ + public function test_get_result_ok() { + // Regardless if this is disabled, the check should still return a non n/a status. + $this->enable_filesystem_and_set_tagging(false); + + $task = new update_object_tags(); + manager::queue_adhoc_task($task); + + $check = new tagging_migration_status(); + $this->assertEquals(result::OK, $check->get_result()->get_status()); + } +} diff --git a/tests/check/tagging_sync_status_test.php b/tests/check/tagging_sync_status_test.php new file mode 100644 index 00000000..eba545cf --- /dev/null +++ b/tests/check/tagging_sync_status_test.php @@ -0,0 +1,68 @@ +. + +namespace tool_objectfs\check; + +use core\check\result; +use tool_objectfs\check\tagging_sync_status; +use tool_objectfs\local\tag\tag_manager; +use tool_objectfs\tests\testcase; + +/** + * Tagging sync status check tests + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \tool_objectfs\check\tagging_sync_status + */ +class tagging_sync_status_test extends testcase { + /** + * Tests scenario that returns N/A + */ + public function test_get_result_na() { + // Not enabled by default, should return N/A. + $check = new tagging_sync_status(); + $this->assertEquals(result::NA, $check->get_result()->get_status()); + } + + /** + * Test scenario that returns OK + */ + public function test_get_result_ok() { + $this->enable_filesystem_and_set_tagging(true); + $object = $this->create_remote_object(); + tag_manager::mark_object_tag_sync_status($object->contenthash, tag_manager::SYNC_STATUS_COMPLETE); + + // All objects OK, should return ok. + $check = new tagging_sync_status(); + $this->assertEquals(result::OK, $check->get_result()->get_status()); + } + + /** + * Tests scenario that returns WARNING + */ + public function test_get_result_warning() { + $this->enable_filesystem_and_set_tagging(true); + $object = $this->create_remote_object(); + tag_manager::mark_object_tag_sync_status($object->contenthash, tag_manager::SYNC_STATUS_ERROR); + + // An object has error, should return warning. + $check = new tagging_sync_status(); + $this->assertEquals(result::WARNING, $check->get_result()->get_status()); + } +} diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index 52954c0a..00052480 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -346,23 +346,63 @@ public function test_record_tag_pushed_time() { } /** - * Tests getting sync status summary html. - * @covers \tool_objectfs\local\tag_manager::get_tag_sync_status_summary_html. + * Tests tag_manger::get_tag_sync_status_summary + * @covers \tool_objectfs\local\tag_manager::get_tag_sync_status_summary */ - public function test_get_tag_sync_status_summary_html() { - // Make two objects each with a different status. - $object1 = $this->create_remote_object('o1'); - $object2 = $this->create_remote_object('o2'); + public function test_get_tag_sync_status_summary() { + // Ensure clean slate before test starts. + global $DB; + $DB->delete_records('tool_objectfs_objects'); + + // Create an object with each status. + $object1 = $this->create_remote_object('test1'); + $object2 = $this->create_remote_object('test2'); + $object3 = $this->create_remote_object('test3'); tag_manager::mark_object_tag_sync_status($object1->contenthash, tag_manager::SYNC_STATUS_COMPLETE); tag_manager::mark_object_tag_sync_status($object2->contenthash, tag_manager::SYNC_STATUS_ERROR); + tag_manager::mark_object_tag_sync_status($object3->contenthash, tag_manager::SYNC_STATUS_NEEDS_SYNC); + + // Ensure correctly counted. + $statuses = tag_manager::get_tag_sync_status_summary(); + $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_COMPLETE]->count); + $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_ERROR]->count); + $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_NEEDS_SYNC]->count); + } + + /** + * Provides sync statuses to tests + * @return array + */ + public static function sync_status_provider(): array { + $tests = []; + foreach (tag_manager::SYNC_STATUSES as $status) { + $tests[$status] = [ + 'status' => $status, + ]; + } + return $tests; + } - // Generate report. - $reporthtml = tag_manager::get_tag_sync_status_summary_html(); + /** + * Tests get_sync_status_string + * @param int $status + * @dataProvider sync_status_provider + * @covers \tool_objectfs\local\tag_manager::get_sync_status_string + */ + public function test_get_sync_status_string(int $status) { + $string = tag_manager::get_sync_status_string($status); + // Cheap check to ensure placeholder string not returned. + $this->assertStringNotContainsString('[', $string); + } - // Ensure report contains a column for every status (even thought there are none of some status in the db). - // (-1 to remove the header row). - $rowcount = substr_count($reporthtml, 'assertEquals(count(tag_manager::SYNC_STATUSES), $rowcount); + /** + * Tests get_sync_status_string when an invalid status is provided + * @covers \tool_objectfs\local\tag_manager::get_sync_status_string + */ + public function test_get_sync_status_string_does_not_exist() { + $this->expectException(coding_exception::class); + $this->expectExceptionMessage('No status string is mapped for status: 5'); + tag_manager::get_sync_status_string(5); } } diff --git a/tests/task/update_object_tags_test.php b/tests/task/update_object_tags_test.php index 7382f9fe..76fbaccc 100644 --- a/tests/task/update_object_tags_test.php +++ b/tests/task/update_object_tags_test.php @@ -31,19 +31,6 @@ * @covers \tool_objectfs\task\update_object_tags */ class update_object_tags_test extends testcase { - /** - * Enables tagging in config and sets up the filesystem to allow tagging - */ - private function set_tagging_enabled() { - global $CFG; - $config = \tool_objectfs\local\manager::get_objectfs_config(); - $config->taggingenabled = true; - $config->enabletasks = true; - $config->filesystem = '\\tool_objectfs\\tests\\test_file_system'; - \tool_objectfs\local\manager::set_objectfs_config($config); - $CFG->phpunit_objectfs_supports_object_tagging = true; - } - /** * Creates object with tags needing to be synced * @param string $contents contents of object to create. @@ -75,7 +62,7 @@ public function test_not_enabled() { */ public function test_invalid_iteration_limit() { $this->resetAfterTest(); - $this->set_tagging_enabled(); + $this->enable_filesystem_and_set_tagging(true); // This should be greater than 1, if zero should error. set_config('maxtaggingiterations', 0, 'tool_objectfs'); @@ -94,7 +81,7 @@ public function test_invalid_iteration_limit() { */ public function test_invalid_iteration_number() { $this->resetAfterTest(); - $this->set_tagging_enabled(); + $this->enable_filesystem_and_set_tagging(true); // Give it a valid max iteration number. set_config('maxtaggingiterations', 5, 'tool_objectfs'); @@ -112,7 +99,7 @@ public function test_invalid_iteration_number() { */ public function test_no_more_objects_to_sync() { $this->resetAfterTest(); - $this->set_tagging_enabled(); + $this->enable_filesystem_and_set_tagging(true); set_config('maxtaggingiterations', 5, 'tool_objectfs'); $task = new update_object_tags(); $task->set_custom_data(['iteration' => 1]); @@ -127,7 +114,7 @@ public function test_no_more_objects_to_sync() { */ public function test_max_iterations() { $this->resetAfterTest(); - $this->set_tagging_enabled(); + $this->enable_filesystem_and_set_tagging(true); // Set max 1 iteration. set_config('maxtaggingiterations', 1, 'tool_objectfs'); @@ -148,7 +135,7 @@ public function test_max_iterations() { */ public function test_tagging_run_with_requeue() { $this->resetAfterTest(); - $this->set_tagging_enabled(); + $this->enable_filesystem_and_set_tagging(true); // Set max 1 object per run. set_config('maxtaggingperrun', 1, 'tool_objectfs'); @@ -194,11 +181,10 @@ public function test_get_iteration() { } /** - * Tests getting status badge and summary html + * Tests getting status badge * @covers \tool_objectfs\task\update_object_tags::get_status_badge - * @covers \tool_objectfs\task\update_object_tags::get_summary_html */ - public function test_get_summary_html_and_status_badge() { + public function test_get_status_badge() { // Spawn three tasks and break each one in a different way. // Test their badge output. $task1 = new update_object_tags(); @@ -214,15 +200,5 @@ public function test_get_summary_html_and_status_badge() { $task3->set_timestarted(1000); $this->assertStringContainsString(get_string('status:running', 'tool_objectfs'), $task3->get_status_badge()); - - // Now queue them so we can test report generation. - manager::queue_adhoc_task($task1); - manager::queue_adhoc_task($task2); - manager::queue_adhoc_task($task3); - - // Ensure row count is expected (excluding header row). - $report = update_object_tags::get_summary_html(); - $rowcount = substr_count($report, 'assertEquals(3, $rowcount); } } From cbb9ab75f5d2fd25ede8f8d36e0073d0bc3b1066 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 20 Aug 2024 09:09:40 +1000 Subject: [PATCH 06/16] feat: display header with status report --- object_status.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/object_status.php b/object_status.php index 7161aa9a..84ece33c 100644 --- a/object_status.php +++ b/object_status.php @@ -57,6 +57,9 @@ echo $OUTPUT->box_start(); $table = new object_status_history_table($reporttype, $reportid); $table->baseurl = $pageurl; + + $heading = get_string('object_status:' . $reporttype, 'tool_objectfs'); + echo $OUTPUT->heading($heading, 2); $table->out(0, false); echo $OUTPUT->box_end(); } From cb2a90ad2ba061b3ac54d837374b0c8e7e68db7b Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 20 Aug 2024 09:31:11 +1000 Subject: [PATCH 07/16] refactor: integrate tagpushedtime into single update query --- classes/local/store/object_file_system.php | 8 +++++-- classes/local/tag/tag_manager.php | 28 ++++++++++++---------- tests/local/tagging_test.php | 15 ------------ tests/object_file_system_test.php | 7 ++++++ 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index 9fbdb749..b9375989 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -1192,16 +1192,20 @@ public function push_object_tags(string $contenthash) { // Avoid unnecessarily checking tags, since this is an extra API call. $canset = $objectexists && (tag_manager::can_overwrite_object_tags() || empty($this->get_external_client()->get_object_tags($contenthash))); + $timepushed = 0; if ($canset) { $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); - tag_manager::record_tag_pushed_time($contenthash, time()); + + // Record the time it was actually pushed to the external store + // (i.e. not when it existed already and we pulled the tags down). + $timepushed = time(); } // Regardless, it has synced. - tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_COMPLETE); + tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_COMPLETE, $timepushed); } catch (Throwable $e) { $lock->release(); diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index 8691315e..ab7cdb5b 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -148,24 +148,28 @@ public static function get_objects_needing_sync(int $limit) { * Marks a given object as the given status. * @param string $contenthash * @param int $status one of SYNC_STATUS_* constants + * @param int $tagpushedtime if tags were actually sent to the external store, + * this should be the time that happened, or zero if not. */ - public static function mark_object_tag_sync_status(string $contenthash, int $status) { + public static function mark_object_tag_sync_status(string $contenthash, int $status, int $tagpushedtime = 0) { global $DB; if (!in_array($status, self::SYNC_STATUSES)) { throw new coding_exception("Invalid object tag sync status " . $status); } - $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]); - } - /** - * Updates the tagslastpushed time for a given object. - * This should only be done once successfully pushed to the external store. - * @param string $contenthash - * @param int $time time to set - */ - public static function record_tag_pushed_time(string $contenthash, int $time) { - global $DB; - $DB->set_field('tool_objectfs_objects', 'tagslastpushed', $time, ['contenthash' => $contenthash]); + $timeupdate = !empty($tagpushedtime) ? ',tagslastpushed = :time' : ''; + $params = [ + 'status' => $status, + 'contenthash' => $contenthash, + 'time' => $tagpushedtime, + ]; + + // Need raw execute since update_records requires an id column, but we use contenthash instead. + $DB->execute("UPDATE {tool_objectfs_objects} + SET tagsyncstatus = :status + {$timeupdate} + WHERE contenthash = :contenthash", + $params); } /** diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index 00052480..23d660f9 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -330,21 +330,6 @@ public function test_object_tag_sync_error() { $this->assertEquals(tag_manager::SYNC_STATUS_ERROR, $status); } - /** - * Tests tag_manager::record_tag_pushed_time - * @covers \tool_objectfs\local\tag_manager::record_tag_pushed_time - */ - public function test_record_tag_pushed_time() { - global $DB; - $object = $this->create_remote_object(); - $initialtime = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['contenthash' => $object->contenthash]); - $this->assertEquals(0, $initialtime); - - tag_manager::record_tag_pushed_time($object->contenthash, 100); - $newtime = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['contenthash' => $object->contenthash]); - $this->assertEquals(100, $newtime); - } - /** * Tests tag_manger::get_tag_sync_status_summary * @covers \tool_objectfs\local\tag_manager::get_tag_sync_status_summary diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php index fd296a75..9f77ea4e 100644 --- a/tests/object_file_system_test.php +++ b/tests/object_file_system_test.php @@ -1102,6 +1102,7 @@ public function test_push_object_tags_replicated(bool $canoverride) { // Tags should now be replicated locally. $localtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $object->contenthash]); $externaltags = $this->filesystem->get_external_client()->get_object_tags($object->contenthash); + $time = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['id' => $object->id]); if ($canoverride) { // If can override, we expect it to be overwritten by the tags defined in the sources. @@ -1110,12 +1111,18 @@ public function test_push_object_tags_replicated(bool $canoverride) { // Also expect the external store to be updated. $this->assertCount($expectednum, $externaltags); + + // Tag push time should be set, since it actually pushed the tags. + $this->assertNotEquals(0, $time); } else { // If cannot overwrite, no tags should be synced. $this->assertCount(0, $localtags); // External store should not be changed. $this->assertCount(count($testtags), $externaltags); + + // The tag last push time should remain unchanged, since it didn't actually push any tags. + $this->assertEquals(0, $time); } // Ensure status changed to not needing sync. From 7b887c9a6fa1df5c57f271fdcd259aa9491eeac1 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 20 Aug 2024 14:47:10 +1000 Subject: [PATCH 08/16] refactor: store tags against object id instead of hash --- classes/local/report/tag_count_report_builder.php | 2 +- classes/local/tag/tag_manager.php | 7 +++++-- db/install.xml | 4 ++-- db/upgrade.php | 4 ++-- tests/local/tagging_test.php | 8 ++++---- tests/object_file_system_test.php | 4 ++-- 6 files changed, 16 insertions(+), 13 deletions(-) diff --git a/classes/local/report/tag_count_report_builder.php b/classes/local/report/tag_count_report_builder.php index 0364b3fc..bbc22f51 100644 --- a/classes/local/report/tag_count_report_builder.php +++ b/classes/local/report/tag_count_report_builder.php @@ -41,7 +41,7 @@ public function build_report($reportid) { SUM(objects.filesize) as objectsum FROM {tool_objectfs_objects} objects LEFT JOIN {tool_objectfs_object_tags} object_tags - ON objects.contenthash = object_tags.contenthash + ON objects.id = object_tags.objectid GROUP BY object_tags.tagkey, object_tags.tagvalue "; $result = $DB->get_records_sql($sql); diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index ab7cdb5b..48983e90 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -112,14 +112,17 @@ public static function gather_object_tags_for_upload(string $contenthash): array public static function store_tags_locally(string $contenthash, array $tags) { global $DB; + // Lookup object id. + $objectid = $DB->get_field('tool_objectfs_objects', 'id', ['contenthash' => $contenthash], MUST_EXIST); + // Purge any existing tags for this object. - $DB->delete_records('tool_objectfs_object_tags', ['contenthash' => $contenthash]); + $DB->delete_records('tool_objectfs_object_tags', ['objectid' => $objectid]); // Store new records. $recordstostore = []; foreach ($tags as $key => $value) { $recordstostore[] = [ - 'contenthash' => $contenthash, + 'objectid' => $objectid, 'tagkey' => $key, 'tagvalue' => $value, ]; diff --git a/db/install.xml b/db/install.xml index 5c5fc2ca..0065b1d7 100644 --- a/db/install.xml +++ b/db/install.xml @@ -54,7 +54,7 @@ - + @@ -62,7 +62,7 @@ - +
diff --git a/db/upgrade.php b/db/upgrade.php index 078cecf4..a8b6d0e9 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -178,7 +178,7 @@ function xmldb_tool_objectfs_upgrade($oldversion) { // Adding fields to table tool_objectfs_object_tags. $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); - $table->add_field('contenthash', XMLDB_TYPE_CHAR, '40', null, XMLDB_NOTNULL, null, null); + $table->add_field('objectid', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); $table->add_field('tagkey', XMLDB_TYPE_CHAR, '32', null, XMLDB_NOTNULL, null, null); $table->add_field('tagvalue', XMLDB_TYPE_CHAR, '128', null, XMLDB_NOTNULL, null, null); @@ -186,7 +186,7 @@ function xmldb_tool_objectfs_upgrade($oldversion) { $table->add_key('primary', XMLDB_KEY_PRIMARY, ['id']); // Adding indexes to table tool_objectfs_object_tags. - $table->add_index('objecttagkey_idx', XMLDB_INDEX_UNIQUE, ['contenthash', 'tagkey']); + $table->add_index('objecttagkey_idx', XMLDB_INDEX_UNIQUE, ['objectid', 'tagkey']); // Conditionally launch create table for tool_objectfs_object_tags. if (!$dbman->table_exists($table)) { diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index 23d660f9..d4fc7be6 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -170,16 +170,16 @@ public function test_store_tags_locally() { 'test1' => 'abc', 'test2' => 'xyz', ]; - $hash = 'thisisatest'; + $object = $this->create_remote_object(); // Ensure no tags for hash intially. - $this->assertEmpty($DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash])); + $this->assertEmpty($DB->get_records('tool_objectfs_object_tags', ['objectid' => $object->id])); // Store. - tag_manager::store_tags_locally($hash, $tags); + tag_manager::store_tags_locally($object->contenthash, $tags); // Confirm they are stored. - $queriedtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $hash]); + $queriedtags = $DB->get_records('tool_objectfs_object_tags', ['objectid' => $object->id]); $this->assertCount(2, $queriedtags); } diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php index 9f77ea4e..69acd436 100644 --- a/tests/object_file_system_test.php +++ b/tests/object_file_system_test.php @@ -1093,14 +1093,14 @@ public function test_push_object_tags_replicated(bool $canoverride) { $this->assertCount(count($testtags), $tags); // But tags will not be stored locally (yet). - $localtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $object->contenthash]); + $localtags = $DB->get_records('tool_objectfs_object_tags', ['objectid' => $object->id]); $this->assertCount(0, $localtags); // Sync the file. $this->filesystem->push_object_tags($object->contenthash); // Tags should now be replicated locally. - $localtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $object->contenthash]); + $localtags = $DB->get_records('tool_objectfs_object_tags', ['objectid' => $object->id]); $externaltags = $this->filesystem->get_external_client()->get_object_tags($object->contenthash); $time = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['id' => $object->id]); From ec8220119de38dd3dc13d7509dc3c91b10722261 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 20 Aug 2024 14:50:00 +1000 Subject: [PATCH 09/16] chore: organise tagging lang strings --- lang/en/tool_objectfs.php | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 78576878..18dd43b7 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -278,7 +278,22 @@ $string['settings:taggingheader'] = 'Tagging settings'; $string['settings:taggingenabled'] = 'Tagging enabled'; +$string['settings:maxtaggingperrun'] = 'Object tagging adhoc sync maximum objects per run'; +$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to sync tags for per tagging sync adhoc task iteration.'; +$string['settings:maxtaggingiterations'] = 'Object tagging adhoc sync maximum number of iterations '; +$string['settings:maxtaggingiterations:desc'] = 'The maximum number of times the tagging sync adhoc task will requeue itself. To avoid accidental infinite runaway.'; +$string['settings:overrideobjecttags'] = 'Allow object tag override'; +$string['settings:overrideobjecttags:desc'] = 'Allows ObjectFS to overwrite tags on objects that already exist in the external store.'; +$string['settings:tagsources'] = 'Tag sources'; +$string['settings:taggingstatus'] = 'Tagging status'; +$string['settings:taggingstatuscounts'] = 'Tag sync status overview'; +$string['settings:taggingmigrationstatus'] = 'Tagging adhoc migration progress'; +$string['settings:tagging:help'] = 'Object tagging allows extra metadata to be attached to objects in the external store. Please read TAGGING.md in the plugin Github repository for detailed setup and considerations. This is currently only supported by the S3 external client.'; + $string['checktagging_status'] = 'Object tagging'; +$string['checktagging_sync_status'] = 'Object tagging sync status'; +$string['checktagging_migration_status'] = 'Object tagging migration status'; + $string['check:tagging:ok'] = 'Object tagging ok'; $string['check:tagging:syncerror'] = 'Objects have tag sync errors'; $string['check:tagging:syncok'] = 'No objects reporting sync errors'; @@ -286,38 +301,31 @@ $string['check:tagging:migrationok'] = 'Object tagging migration tasks OK'; $string['check:tagging:na'] = 'Tagging not enabled or is not supported by file system'; $string['check:tagging:error'] = 'Error trying to tag object'; -$string['settings:maxtaggingperrun'] = 'Object tagging adhoc sync maximum objects per run'; -$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to sync tags for per tagging sync adhoc task iteration.'; -$string['settings:maxtaggingiterations'] = 'Object tagging adhoc sync maximum number of iterations '; -$string['settings:maxtaggingiterations:desc'] = 'The maximum number of times the tagging sync adhoc task will requeue itself. To avoid accidental infinite runaway.'; -$string['settings:overrideobjecttags'] = 'Allow object tag override'; -$string['settings:overrideobjecttags:desc'] = 'Allows ObjectFS to overwrite tags on objects that already exist in the external store.'; + $string['tagsource:environment'] = 'Environment defined by $CFG->objectfs_environment_name, currently: "{$a}".'; $string['tagsource:mimetype'] = 'File mimetype as stored in {files} table'; -$string['settings:tagsources'] = 'Tag sources'; -$string['settings:taggingstatus'] = 'Tagging status'; + $string['task:triggerupdateobjecttags'] = 'Queue adhoc task to update object tags'; -$string['settings:tagging:help'] = 'Object tagging allows extra metadata to be attached to objects in the external store. Please read TAGGING.md in the plugin Github repository for detailed setup and considerations. This is currently only supported by the S3 external client.'; -$string['object_status:tag_count'] = 'Object tags'; + $string['tagsyncstatus:error'] = 'Errored'; $string['tagsyncstatus:notrequired'] = 'Not required / synced'; $string['tagsyncstatus:needssync'] = 'Waiting for sync'; -$string['settings:taggingstatuscounts'] = 'Tag sync status overview'; + $string['tagging:migration:notsupported'] = 'Tagging not enabled or supported by filesystem. Cannot execute tag migration task'; $string['tagging:migration:invaliditerations'] = 'Invalid iteration number or iteration count'; $string['tagging:migration:limitreached'] = 'Current iteration {$a} is >= the maximum number of iterations. Please investigate if this is expected and you need to increase the limit, or if there is a problem syncing the tags causing an infinite loop'; -$string['settings:taggingmigrationstatus'] = 'Tagging adhoc migration progress'; $string['tagging:migration:nothingrunning'] = 'No tagging migration adhoc tasks are currently running'; $string['tagging:migration:help'] = 'Run the trigger_update_object_tags scheduled task from the frontend or CLI to start a migration task.'; + $string['table:taskid'] = 'Task ID'; $string['table:iteration'] = 'Iteration number'; $string['table:status'] = 'Status'; $string['table:objectcount'] = 'Object count'; $string['table:tagsource'] = 'Tag source'; $string['table:tagsourcemeaning'] = 'Description'; + $string['status:waiting'] = 'Waiting'; $string['status:running'] = 'Running'; $string['status:failing'] = 'Faildelay {$a}'; -$string['checktagging_sync_status'] = 'Object tagging sync status'; -$string['checktagging_migration_status'] = 'Object tagging migration status'; +$string['object_status:tag_count'] = 'Object tags'; From 56f79b81465eb530f0b2d6f3304925735dec9578 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 20 Aug 2024 15:48:56 +1000 Subject: [PATCH 10/16] bugfix: fix mysql query compatibility --- classes/check/tagging_sync_status.php | 2 +- classes/local/tag/tag_manager.php | 2 +- tests/local/tagging_test.php | 15 +++++++++------ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/classes/check/tagging_sync_status.php b/classes/check/tagging_sync_status.php index 868687fa..ba4d9302 100644 --- a/classes/check/tagging_sync_status.php +++ b/classes/check/tagging_sync_status.php @@ -60,7 +60,7 @@ public function get_result(): result { foreach (tag_manager::SYNC_STATUSES as $status) { // If no objects have a status, they won't appear in the SQL above. // In this case, just show zero (so the use knows it exists, but is zero). - $count = isset($statuses[$status]) ? $statuses[$status]->count : 0; + $count = isset($statuses[$status]->statuscount) ? $statuses[$status]->statuscount : 0; $table->data[$status] = [tag_manager::get_sync_status_string($status), $count]; } $table = html_writer::table($table); diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index 48983e90..43fdf7a4 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -226,7 +226,7 @@ public static function get_sync_status_string(int $tagsyncstatus): string { */ public static function get_tag_sync_status_summary(): array { global $DB; - return $DB->get_records_sql("SELECT tagsyncstatus, COUNT(tagsyncstatus) + return $DB->get_records_sql("SELECT tagsyncstatus, COUNT(tagsyncstatus) as statuscount FROM {tool_objectfs_objects} GROUP BY tagsyncstatus"); } diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index d4fc7be6..144e2191 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -340,9 +340,12 @@ public function test_get_tag_sync_status_summary() { $DB->delete_records('tool_objectfs_objects'); // Create an object with each status. - $object1 = $this->create_remote_object('test1'); - $object2 = $this->create_remote_object('test2'); - $object3 = $this->create_remote_object('test3'); + $object1 = $this->create_local_object('test1'); + $object2 = $this->create_local_object('test2'); + $object3 = $this->create_local_object('test3'); + + // Delete the unit test object that is automatically created, it has a filesize of zero. + $DB->delete_records('tool_objectfs_objects', ['filesize' => 0]); tag_manager::mark_object_tag_sync_status($object1->contenthash, tag_manager::SYNC_STATUS_COMPLETE); tag_manager::mark_object_tag_sync_status($object2->contenthash, tag_manager::SYNC_STATUS_ERROR); @@ -350,9 +353,9 @@ public function test_get_tag_sync_status_summary() { // Ensure correctly counted. $statuses = tag_manager::get_tag_sync_status_summary(); - $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_COMPLETE]->count); - $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_ERROR]->count); - $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_NEEDS_SYNC]->count); + $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_COMPLETE]->statuscount); + $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_ERROR]->statuscount); + $this->assertEquals(1, $statuses[tag_manager::SYNC_STATUS_NEEDS_SYNC]->statuscount); } /** From 79b12777e063e92ca9410cb11b60ad0a72e9df5f Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 23 Aug 2024 16:23:43 +1000 Subject: [PATCH 11/16] tagging: move mimetype to metadata, add location/orphan tag source --- TAGGING.md | 61 +++++---------- classes/local/manager.php | 24 ++++-- .../local/object_manipulator/manipulator.php | 2 +- classes/local/store/object_file_system.php | 75 +++++++++++++++++-- classes/local/store/s3/client.php | 11 ++- classes/local/store/s3/file_system.php | 3 +- classes/local/tag/environment_source.php | 4 +- ...me_type_source.php => location_source.php} | 22 ++---- classes/local/tag/tag_manager.php | 2 +- lang/en/tool_objectfs.php | 4 +- tests/local/tagging_test.php | 23 +++++- tests/object_file_system_test.php | 59 ++++++++++++--- 12 files changed, 197 insertions(+), 93 deletions(-) rename classes/local/tag/{mime_type_source.php => location_source.php} (68%) diff --git a/TAGGING.md b/TAGGING.md index 0c2155e8..c7795714 100644 --- a/TAGGING.md +++ b/TAGGING.md @@ -6,7 +6,7 @@ Currently, this is only implemented for the S3 file system client. Note object tags are different from object metadata. -Object metadata is immutable, and attached to the object on upload. With metadata, if you wish to update it (for example during a migration, or the sources changed), you have to copy the object with the new metadata, and delete the old object. This is problematic, since deletion is optional in objectfs. +Object metadata is immutable, and attached to the object on upload. With metadata, if you wish to update it (for example during a migration, or the sources changed), you have to copy the object with the new metadata, and delete the old object. This is not ideal, since deletion is optional in objectfs. Object tags are more suitable, since their permissions can be managed separately (e.g. a client can be allowed to modify tags, but not delete objects). @@ -21,54 +21,33 @@ The following sources are implemented currently: ### Environment What environment the file was uploaded in. Configure the environment using `$CFG->objectfs_environment_name` -### Mimetype -What mimetype the file is stored as under the `mdl_files` table. - -## Multiple environments pointing to single bucket -It is possible you are using objectfs with multiple environments (e.g. prod, staging) that both point to the same bucket. Since files are referenced by contenthash, it generally does not matter where they come from, so this isn't a problem. However to ensure the tags remain accurate, you should turn off `overwriteobjecttags` in the plugin settings for every environment except production. - -This means that staging is unable to overwrite tags for files uploaded elsewhere, but can set it on files only uploaded only from staging. However, files uploaded from production will always have the correct tags, and will overwrite any existing tags. - -```mermaid -graph LR - subgraph S3 - Object("`**Object** - contenthash: xyz - tags: env=prod`") - end - subgraph Prod - UploadObjectProd["`**Upload object** - contenthash: xyz - tags: env=prod`"] --> Object - end - subgraph Staging - UploadObjectStaging["`**Upload object** - contenthash: xyz - tags: env=staging`"] - end - Blocked["Blocked - does not have permissions\nto overwrite existing object tags"] - UploadObjectStaging --- Blocked - Blocked -.-> Object - - style Object fill:#ffffff00,stroke:#ffa812 - style S3 fill:#ffffff00,stroke:#ffa812 - style Prod fill:#ffffff00,stroke:#26ff4a - style UploadObjectProd fill:#ffffff00,stroke:#26ff4a - style Staging fill:#ffffff00,stroke:#978aff - style UploadObjectStaging fill:#ffffff00,stroke:#978aff - style Blocked fill:#ffffff00,stroke:#ff0000 -``` +This tag is also used by objectfs to determine if tags can be overwritten. See [Multiple environments setup](#multiple-environments-setup) for more information. + +### Location +Either `orphan` if the file no longer exists in the `files` table in Moodle, otherwise `active`. + +## Multiple environments setup +This feature is designed to work in situations where multiple environments (e.g. prod, staging) points to the same bucket, however, some setup is needed: + +1. Turn off `overwriteobjecttags` in every environment except the production environment. +2. Configure `$CFG->objectfs_environment_name` to be unique for all environments. + +By doing the above two steps, it will allow the production environment to always set its own tags, even if a file was first uploaded to staging and then to production. + +Lower environments can still update tags, but only if the `environment` matches theirs. This allows staging to manage object tags on objects only it knows about, but as soon as the file is uploaded from production (and therefore have it's environment tag replaced with `prod`), staging will no longer touch it. ## Migration -If the way a tag was calculated has changed, or new tags are added (or removed) or this feature was turned on for the first time (or turned on after being off), you must do the following: -- Manually run `trigger_update_object_tags` scheduled task from the UI, which queues a `update_object_tags` adhoc task that will process all objects marked as needing sync (default is true) +Only new objects uploaded after enabling this feature will have tags added. To backfill tags for previously uploaded objects, you must do the following: + +- Manually run `trigger_update_object_tags` scheduled task from the UI, which queues a `update_object_tags` adhoc task that will process all objects marked as needing sync. or - Call the CLI to execute a `update_object_tags` adhoc task manually. +You may need to update the DB to mark objects tag sync status as needing sync if the object has previously been synced before. ## Reporting There is an additional graph added to the object summary report showing the tag value combinations and counts of each. -Note, this is only for files that have been uploaded from this environment, and may not be consistent for environments where `overwriteobjecttags` is disabled (because the site does not know if a file was overwritten in the external store by another client). +Note, this is only for files that have been uploaded from the respective environment, and may not be consistent for environments where `overwriteobjecttags` is disabled (because the site does not know if a file was overwritten in the external store by another client). ## For developers diff --git a/classes/local/manager.php b/classes/local/manager.php index 188df9fc..19694318 100644 --- a/classes/local/manager.php +++ b/classes/local/manager.php @@ -27,6 +27,7 @@ use stdClass; use tool_objectfs\local\store\object_file_system; +use tool_objectfs\local\tag\tag_manager; /** * [Description manager] @@ -160,7 +161,7 @@ public static function update_object_by_hash($contenthash, $newlocation, $filesi $newobject->filesize = isset($oldobject->filesize) ? $oldobject->filesize : $DB->get_field('files', 'filesize', ['contenthash' => $contenthash], IGNORE_MULTIPLE); - return self::update_object($newobject, $newlocation); + return self::upsert_object($newobject, $newlocation); } $newobject->location = $newlocation; @@ -173,9 +174,7 @@ public static function update_object_by_hash($contenthash, $newlocation, $filesi $newobject->filesize = $filesize; $newobject->timeduplicated = time(); } - $DB->insert_record('tool_objectfs_objects', $newobject); - - return $newobject; + return self::upsert_object($newobject, $newlocation); } /** @@ -185,7 +184,7 @@ public static function update_object_by_hash($contenthash, $newlocation, $filesi * @return stdClass * @throws \dml_exception */ - public static function update_object(stdClass $object, $newlocation) { + public static function upsert_object(stdClass $object, $newlocation) { global $DB; // If location change is 'duplicated' we update timeduplicated. @@ -193,8 +192,21 @@ public static function update_object(stdClass $object, $newlocation) { $object->timeduplicated = time(); } + $locationchanged = !isset($object->location) || $object->location != $newlocation; $object->location = $newlocation; - $DB->update_record('tool_objectfs_objects', $object); + + // If id is set, update, else insert new. + if (empty($object->id)) { + $object->id = $DB->insert_record('tool_objectfs_objects', $object); + } else { + $DB->update_record('tool_objectfs_objects', $object); + } + + // Post update, notify tag manager since the location tag likely needs changing. + if ($locationchanged && tag_manager::is_tagging_enabled_and_supported()) { + $fs = get_file_storage()->get_file_system(); + $fs->push_object_tags($object->contenthash); + } return $object; } diff --git a/classes/local/object_manipulator/manipulator.php b/classes/local/object_manipulator/manipulator.php index f5108305..afd21808 100644 --- a/classes/local/object_manipulator/manipulator.php +++ b/classes/local/object_manipulator/manipulator.php @@ -111,7 +111,7 @@ public function execute(array $objectrecords) { $newlocation = $this->manipulate_object($objectrecord); if (!empty($objectrecord->id)) { - manager::update_object($objectrecord, $newlocation); + manager::upsert_object($objectrecord, $newlocation); } else { manager::update_object_by_hash($objectrecord->contenthash, $newlocation); } diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index b9375989..da148fab 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -39,6 +39,7 @@ use coding_exception; use Throwable; use tool_objectfs\local\manager; +use tool_objectfs\local\tag\environment_source; use tool_objectfs\local\tag\tag_manager; defined('MOODLE_INTERNAL') || die(); @@ -167,6 +168,23 @@ protected function get_local_path_from_hash($contenthash, $fetchifnotfound = fal return $path; } + /** + * Returns mimetype for a given hash + * @param string $contenthash + * @return string mimetype as stored in mdl_files + */ + protected function get_mimetype_from_hash(string $contenthash): string { + global $DB; + + // We limit 1 because multiple files can have the same contenthash. + // However, they all have the same mimetype so it does not matter which one we query. + return $DB->get_field_sql('SELECT mimetype + FROM {files} + WHERE contenthash = :hash + LIMIT 1', + ['hash' => $contenthash]); + } + /** * get_remote_path_from_storedfile * @param \stored_file $file @@ -1185,13 +1203,7 @@ public function push_object_tags(string $contenthash) { } try { - $objectexists = $this->is_file_readable_externally_by_hash($contenthash); - - // Object must exist, and we can overwrite (and not care about existing tags) - // or cannot overwrite, and the tags are empty. - // Avoid unnecessarily checking tags, since this is an extra API call. - $canset = $objectexists && (tag_manager::can_overwrite_object_tags() || - empty($this->get_external_client()->get_object_tags($contenthash))); + $canset = $this->can_set_object_tags($contenthash); $timepushed = 0; if ($canset) { @@ -1200,7 +1212,7 @@ public function push_object_tags(string $contenthash) { tag_manager::store_tags_locally($contenthash, $tags); // Record the time it was actually pushed to the external store - // (i.e. not when it existed already and we pulled the tags down). + // (i.e. not when it existed already and was skipped). $timepushed = time(); } @@ -1216,4 +1228,51 @@ public function push_object_tags(string $contenthash) { } $lock->release(); } + + /** + * Returns true if the current env can set the given object's tags. + * + * To set the tags: + * - The object must exist + * - We can overwrite tags (and not care about any existing) + * OR + * - We cannot overwrite tags, and the tags are empty or the environment is the same as ours. + * + * Avoids unnecessarily querying tags as this is an extra api call to the object store. + * + * @param string $contenthash + * @return bool + */ + private function can_set_object_tags(string $contenthash): bool { + $objectexists = $this->is_file_readable_externally_by_hash($contenthash); + + // Object must exist, we cannot set tags on an object that is missing. + if (!$objectexists) { + return false; + } + + // If can overwrite tags, we don't care then about any existing tags. + if (tag_manager::can_overwrite_object_tags()) { + return true; + } + + // Else we need to check the tags are empty, or the env matches ours. + $existingtags = $this->get_external_client()->get_object_tags($contenthash); + + // Not set yet, must be a new object. + if (empty($existingtags) || !isset($existingtags[environment_source::get_identifier()])) { + return true; + } + + $envsource = new environment_source(); + $currentenv = $envsource->get_value_for_contenthash($contenthash); + + // Env is the same as ours, allowed to set. + if ($existingtags[environment_source::get_identifier()] == $currentenv) { + return true; + } + + // Else no match, do not set. + return false; + } } diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 767e177e..e3ea37ef 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -496,10 +496,11 @@ public function define_client_section($settings, $config) { * * @param string $localpath Path to a local file. * @param string $contenthash Content hash of the file. + * @param string $mimetype the mimetype of the file being uploaded * * @throws \Exception if fails. */ - public function upload_to_s3($localpath, $contenthash) { + public function upload_to_s3($localpath, $contenthash, string $mimetype) { $filehandle = fopen($localpath, 'rb'); if (!$filehandle) { @@ -511,7 +512,13 @@ public function upload_to_s3($localpath, $contenthash) { $uploader = new \Aws\S3\ObjectUploader( $this->client, $this->bucket, $this->bucketkeyprefix . $externalpath, - $filehandle + $filehandle, + 'private', + [ + 'params' => [ + 'ContentType' => $mimetype, + ], + ] ); $uploader->upload(); fclose($filehandle); diff --git a/classes/local/store/s3/file_system.php b/classes/local/store/s3/file_system.php index 93637dd4..b1897d3d 100644 --- a/classes/local/store/s3/file_system.php +++ b/classes/local/store/s3/file_system.php @@ -85,9 +85,10 @@ public function readfile(\stored_file $file) { */ public function copy_from_local_to_external($contenthash) { $localpath = $this->get_local_path_from_hash($contenthash); + $mime = $this->get_mimetype_from_hash($contenthash); try { - $this->get_external_client()->upload_to_s3($localpath, $contenthash); + $this->get_external_client()->upload_to_s3($localpath, $contenthash, $mime); return true; } catch (\Exception $e) { $this->get_logger()->error_log( diff --git a/classes/local/tag/environment_source.php b/classes/local/tag/environment_source.php index d7715ddb..127b08fc 100644 --- a/classes/local/tag/environment_source.php +++ b/classes/local/tag/environment_source.php @@ -17,7 +17,7 @@ namespace tool_objectfs\local\tag; /** - * Provides environment a file was uploaded in. + * Provides current environment to file. * * @package tool_objectfs * @author Matthew Hilton @@ -53,7 +53,7 @@ private static function get_env(): ?string { /** * Returns the tag value for the given file contenthash * @param string $contenthash - * @return string|null mime type for file. + * @return string|null environment value. */ public function get_value_for_contenthash(string $contenthash): ?string { return self::get_env(); diff --git a/classes/local/tag/mime_type_source.php b/classes/local/tag/location_source.php similarity index 68% rename from classes/local/tag/mime_type_source.php rename to classes/local/tag/location_source.php index 7546cdcf..1353a03a 100644 --- a/classes/local/tag/mime_type_source.php +++ b/classes/local/tag/location_source.php @@ -17,20 +17,20 @@ namespace tool_objectfs\local\tag; /** - * Provides mime type of file. + * Provides location status for a file. * * @package tool_objectfs * @author Matthew Hilton * @copyright Catalyst IT * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class mime_type_source implements tag_source { +class location_source implements tag_source { /** * Identifier used in tagging file. Is the 'key' of the tag. * @return string */ public static function get_identifier(): string { - return 'mimetype'; + return 'location'; } /** @@ -38,7 +38,7 @@ public static function get_identifier(): string { * @return string */ public static function get_description(): string { - return get_string('tagsource:mimetype', 'tool_objectfs'); + return get_string('tagsource:location', 'tool_objectfs'); } /** @@ -48,18 +48,10 @@ public static function get_description(): string { */ public function get_value_for_contenthash(string $contenthash): ?string { global $DB; - // Sometimes multiple with same hash are uploaded (e.g. real vs draft), - // in this case, just take the first (mimetype is the same regardless). - $mime = $DB->get_field_sql('SELECT mimetype - FROM {files} - WHERE contenthash = :hash - LIMIT 1', - ['hash' => $contenthash]); - if (empty($mime)) { - return null; - } + $isorphaned = $DB->record_exists('tool_objectfs_objects', ['contenthash' => $contenthash, + 'location' => OBJECT_LOCATION_ORPHANED]); - return $mime; + return $isorphaned ? 'orphan' : 'active'; } } diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index 43fdf7a4..058b82f6 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -66,8 +66,8 @@ public static function get_defined_tag_sources(): array { // All possible tag sources should be defined here. // Note this should be a maximum of 10 sources, as this is an AWS limit. return [ - new mime_type_source(), new environment_source(), + new location_source(), ]; } diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 18dd43b7..9dbee628 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -283,7 +283,7 @@ $string['settings:maxtaggingiterations'] = 'Object tagging adhoc sync maximum number of iterations '; $string['settings:maxtaggingiterations:desc'] = 'The maximum number of times the tagging sync adhoc task will requeue itself. To avoid accidental infinite runaway.'; $string['settings:overrideobjecttags'] = 'Allow object tag override'; -$string['settings:overrideobjecttags:desc'] = 'Allows ObjectFS to overwrite tags on objects that already exist in the external store.'; +$string['settings:overrideobjecttags:desc'] = 'Allows ObjectFS to overwrite tags on objects that already exist in the external store. If not checked, objectfs will only set tags when the objects "environment" value is empty or is the same as currently defined.'; $string['settings:tagsources'] = 'Tag sources'; $string['settings:taggingstatus'] = 'Tagging status'; $string['settings:taggingstatuscounts'] = 'Tag sync status overview'; @@ -303,7 +303,7 @@ $string['check:tagging:error'] = 'Error trying to tag object'; $string['tagsource:environment'] = 'Environment defined by $CFG->objectfs_environment_name, currently: "{$a}".'; -$string['tagsource:mimetype'] = 'File mimetype as stored in {files} table'; +$string['tagsource:location'] = 'Location of file, either "orphan" or "active".'; $string['task:triggerupdateobjecttags'] = 'Queue adhoc task to update object tags'; diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index 144e2191..1b6a2d9a 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -153,10 +153,29 @@ public function test_gather_object_tags_for_upload() { $object = $this->create_duplicated_object('gather tags for upload test'); $tags = tag_manager::gather_object_tags_for_upload($object->contenthash); - $this->assertArrayHasKey('mimetype', $tags); $this->assertArrayHasKey('environment', $tags); - $this->assertEquals('text', $tags['mimetype']); $this->assertEquals('test', $tags['environment']); + $this->assertArrayHasKey('location', $tags); + $this->assertEquals('active', $tags['location']); + } + + /** + * Tests gather_object_tags_for_upload when orphaned + * @covers \tool_objectfs\local\tag_manager::gather_object_tags_for_upload + */ + public function test_gather_object_tags_for_upload_orphaned() { + global $DB; + $object = $this->create_duplicated_object('gather tags for upload test'); + + // Change the object record to be orphaned. + $DB->update_record('tool_objectfs_objects', ['id' => $object->id, 'location' => OBJECT_LOCATION_ORPHANED]); + + $tags = tag_manager::gather_object_tags_for_upload($object->contenthash); + + $this->assertArrayHasKey('environment', $tags); + $this->assertEquals('test', $tags['environment']); + $this->assertArrayHasKey('location', $tags); + $this->assertEquals('orphan', $tags['location']); } /** diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php index 69acd436..696eed4b 100644 --- a/tests/object_file_system_test.php +++ b/tests/object_file_system_test.php @@ -1055,35 +1055,68 @@ public function test_push_object_tags_object_not_replicated() { */ public static function push_object_tags_replicated_provider(): array { return [ - 'can override' => [ + // Can override, doesn't matter if envs are different. + 'can override - different env' => [ + 'object env' => 'prod', + 'push env' => 'staging', 'can override' => true, + 'expected override' => true, ], - 'cannot override' => [ - 'cannot override' => false, + 'can override - same env' => [ + 'object env' => 'prod', + 'push env' => 'prod', + 'can override' => true, + 'expected override' => true, + ], + 'can override - empty env' => [ + 'object env' => '', + 'push env' => 'prod', + 'can override' => true, + 'expected override' => true, + ], + // Cannot override, env must match or be empty. + 'cannot override - same env' => [ + 'object env' => 'prod', + 'push env' => 'prod', + 'can override' => false, + 'expected override' => true, + ], + 'cannot override - different env' => [ + 'object env' => 'prod', + 'push env' => 'staging', + 'can override' => false, + 'expected override' => false, + ], + 'cannot override - env is empty' => [ + 'object env' => '', + 'push env' => 'staging', + 'can override' => false, + 'expected override' => true, ], ]; } /** * Tests push_object_tags when the object is replicated. + * Tests rules around overriding are correctly applied. + * + * @param string $objectenv the env to set when 'uploading' the object + * @param string $pushenv the env to set when trying to push new tags * @param bool $canoverride if filesystem should be able to overwrite existing objects + * @param bool $expectedoverride if it was expected that the tags were overwritten. * @dataProvider push_object_tags_replicated_provider */ - public function test_push_object_tags_replicated(bool $canoverride) { + public function test_push_object_tags_replicated(string $objectenv, string $pushenv, bool $canoverride, + bool $expectedoverride) { global $CFG, $DB; $CFG->phpunit_objectfs_supports_object_tagging = true; + $CFG->objectfs_environment_name = $objectenv; set_config('overwriteobjecttags', $canoverride, 'tool_objectfs'); $this->assertEquals($canoverride, tag_manager::can_overwrite_object_tags()); $object = $this->create_duplicated_object('test syncing replicated'); - - $testtags = [ - 'test' => 123, - 'test2' => 123, - 'test3' => 123, - 'test4' => 123, - ]; + $testtags = tag_manager::gather_object_tags_for_upload($object->contenthash); // Fake set the tags in the external store. $this->filesystem->get_external_client()->tags[$object->contenthash] = $testtags; @@ -1096,6 +1129,8 @@ public function test_push_object_tags_replicated(bool $canoverride) { $localtags = $DB->get_records('tool_objectfs_object_tags', ['objectid' => $object->id]); $this->assertCount(0, $localtags); + $CFG->objectfs_environment_name = $pushenv; + // Sync the file. $this->filesystem->push_object_tags($object->contenthash); @@ -1104,7 +1139,7 @@ public function test_push_object_tags_replicated(bool $canoverride) { $externaltags = $this->filesystem->get_external_client()->get_object_tags($object->contenthash); $time = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['id' => $object->id]); - if ($canoverride) { + if ($expectedoverride) { // If can override, we expect it to be overwritten by the tags defined in the sources. $expectednum = count(tag_manager::get_defined_tag_sources()); $this->assertCount($expectednum, $localtags); From f117f121952b8a49debdffbb029cb9291909649f Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 3 Sep 2024 09:36:45 +1000 Subject: [PATCH 12/16] tagging: check environment config length --- classes/local/tag/environment_source.php | 14 +++++++++++++- lang/en/tool_objectfs.php | 1 + tests/local/tagging_test.php | 15 +++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/classes/local/tag/environment_source.php b/classes/local/tag/environment_source.php index 127b08fc..f26a87c1 100644 --- a/classes/local/tag/environment_source.php +++ b/classes/local/tag/environment_source.php @@ -16,6 +16,8 @@ namespace tool_objectfs\local\tag; +use moodle_exception; + /** * Provides current environment to file. * @@ -47,7 +49,17 @@ public static function get_description(): string { */ private static function get_env(): ?string { global $CFG; - return !empty($CFG->objectfs_environment_name) ? $CFG->objectfs_environment_name : null; + + if (empty($CFG->objectfs_environment_name)) { + return null; + } + + // Must never be greater than 128, unlikely, but we must enforce this. + if (strlen($CFG->objectfs_environment_name) > 128) { + throw new moodle_exception('tagsource:environment:toolong', 'tool_objectfs'); + } + + return $CFG->objectfs_environment_name; } /** diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 9dbee628..b5a1bd64 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -303,6 +303,7 @@ $string['check:tagging:error'] = 'Error trying to tag object'; $string['tagsource:environment'] = 'Environment defined by $CFG->objectfs_environment_name, currently: "{$a}".'; +$string['tagsource:environment:toolong'] = 'The value defined in objectfs_environment_name is too long. It must be < 128 chars'; $string['tagsource:location'] = 'Location of file, either "orphan" or "active".'; $string['task:triggerupdateobjecttags'] = 'Queue adhoc task to update object tags'; diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index 1b6a2d9a..c8ff3a54 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -17,8 +17,10 @@ namespace tool_objectfs\local; use coding_exception; +use moodle_exception; use Throwable; use tool_objectfs\local\manager; +use tool_objectfs\local\tag\environment_source; use tool_objectfs\local\tag\tag_manager; use tool_objectfs\local\tag\tag_source; use tool_objectfs\tests\testcase; @@ -412,4 +414,17 @@ public function test_get_sync_status_string_does_not_exist() { $this->expectExceptionMessage('No status string is mapped for status: 5'); tag_manager::get_sync_status_string(5); } + + /** + * Tests the length of the defined tag source is checked correctly + */ + public function test_environment_source_too_long() { + global $CFG; + $CFG->objectfs_environment_name = 'This is a really long string. It needs to be long because it needs to be more than 128 chars for the test to trigger an exception.'; + $source = new environment_source(); + + $this->expectException(moodle_exception::class); + $this->expectExceptionMessage(get_string('tagsource:environment:toolong', 'tool_objectfs')); + $source->get_value_for_contenthash('test'); + } } From f01c6393a9fd999667b28a02fa6e47fc289f8591 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 9 Sep 2024 10:52:18 +1000 Subject: [PATCH 13/16] settings: use admin_setting_check if available --- settings.php | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/settings.php b/settings.php index bd03718e..04fbe3da 100644 --- a/settings.php +++ b/settings.php @@ -23,6 +23,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use tool_objectfs\check\tagging_migration_status; +use tool_objectfs\check\tagging_sync_status; use tool_objectfs\local\tag\tag_manager; use tool_objectfs\task\update_object_tags; @@ -294,13 +296,19 @@ $settings->add(new admin_setting_heading('tool_objectfs/taggingstatus', new lang_string('settings:taggingstatus', 'tool_objectfs'), '')); - // TODO when this branch supports 4.4+, use admin_setting_check https://tracker.moodle.org/browse/MDL-67898. - $settings->add(new admin_setting_description('taggingstatuslink', '', html_writer::link( - new moodle_url('/report/status/index.php', ['detail' => 'tool_objectfs_tagging_sync_status']), - get_string('settings:taggingstatus', 'tool_objectfs') - ))); - $settings->add(new admin_setting_description('taggingmigrationstatuslink', '', html_writer::link( - new moodle_url('/report/status/index.php', ['detail' => 'tool_objectfs_tagging_migration_status']), - get_string('settings:taggingmigrationstatus', 'tool_objectfs') - ))); + // Only in 4.4+. + if (class_exists('admin_setting_check')) { + $settings->add(new admin_setting_check('tool_objectfs/check_taggingsyncstatus', new tagging_sync_status(), true)); + $settings->add(new admin_setting_check('tool_objectfs/check_taggingmigrationstatus', new tagging_migration_status(), true)); + } else { + // Fallback to links instead. + $settings->add(new admin_setting_description('taggingstatuslink', '', html_writer::link( + new moodle_url('/report/status/index.php', ['detail' => 'tool_objectfs_tagging_sync_status']), + get_string('settings:taggingstatus', 'tool_objectfs') + ))); + $settings->add(new admin_setting_description('taggingmigrationstatuslink', '', html_writer::link( + new moodle_url('/report/status/index.php', ['detail' => 'tool_objectfs_tagging_migration_status']), + get_string('settings:taggingmigrationstatus', 'tool_objectfs') + ))); + } } From f87531dfa24e04d85e3d734a01613b497117a409 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 9 Sep 2024 11:16:31 +1000 Subject: [PATCH 14/16] report: remove object size from tag count report --- classes/local/report/object_status_history_table.php | 5 +++++ classes/local/report/objectfs_report.php | 3 ++- classes/local/report/tag_count_report_builder.php | 9 +++------ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/classes/local/report/object_status_history_table.php b/classes/local/report/object_status_history_table.php index 906689ce..3a2bfc87 100644 --- a/classes/local/report/object_status_history_table.php +++ b/classes/local/report/object_status_history_table.php @@ -74,6 +74,11 @@ public function __construct($reporttype, $reportid) { $columnheaders['runningsize'] = get_string('object_status:runningsize', 'tool_objectfs'); } + // Tag count report does not display the size. + if ($this->reporttype == 'tag_count') { + unset($columnheaders['size']); + } + $this->set_attribute('class', 'table-sm'); $this->define_columns(array_keys($columnheaders)); $this->define_headers(array_values($columnheaders)); diff --git a/classes/local/report/objectfs_report.php b/classes/local/report/objectfs_report.php index 56869cd7..468e35db 100644 --- a/classes/local/report/objectfs_report.php +++ b/classes/local/report/objectfs_report.php @@ -78,7 +78,8 @@ public function add_row($datakey, $objectcount, $objectsum) { */ public function add_rows(array $rows) { foreach ($rows as $row) { - $this->add_row($row->datakey, $row->objectcount, $row->objectsum); + // Note objectsum is optional. + $this->add_row($row->datakey, $row->objectcount, $row->objectsum ?? 0); } } diff --git a/classes/local/report/tag_count_report_builder.php b/classes/local/report/tag_count_report_builder.php index bbc22f51..bcc6e80e 100644 --- a/classes/local/report/tag_count_report_builder.php +++ b/classes/local/report/tag_count_report_builder.php @@ -34,14 +34,11 @@ public function build_report($reportid) { global $DB; $report = new objectfs_report('tag_count', $reportid); - // Returns counts + sizes of key:value. + // Returns counts of key:value. $sql = " SELECT CONCAT(COALESCE(object_tags.tagkey, '(untagged)'), ': ', COALESCE(object_tags.tagvalue, '')) as datakey, - COUNT(objects.id) as objectcount, - SUM(objects.filesize) as objectsum - FROM {tool_objectfs_objects} objects - LEFT JOIN {tool_objectfs_object_tags} object_tags - ON objects.id = object_tags.objectid + COUNT(DISTINCT object_tags.objectid) as objectcount + FROM {tool_objectfs_object_tags} object_tags GROUP BY object_tags.tagkey, object_tags.tagvalue "; $result = $DB->get_records_sql($sql); From 8e9deac683fc66af08f4c74891192ef353b68e80 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 10 Sep 2024 11:12:38 +1000 Subject: [PATCH 15/16] tagging: ignore if cannot get lock --- classes/local/store/object_file_system.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index da148fab..e9be319e 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -1187,8 +1187,9 @@ private function update_object(array $result): array { * External client must support tagging. * * @param string $contenthash file to sync tags for + * @return bool true if set tags, false if could not get lock. */ - public function push_object_tags(string $contenthash) { + public function push_object_tags(string $contenthash): bool { if (!$this->get_external_client()->supports_object_tagging()) { throw new coding_exception("Cannot sync tags, external client does not support tagging."); } @@ -1199,7 +1200,7 @@ public function push_object_tags(string $contenthash) { // No lock - just skip it. if (!$lock) { - throw new coding_exception("Could not get object lock"); + return false; } try { @@ -1227,6 +1228,7 @@ public function push_object_tags(string $contenthash) { throw $e; } $lock->release(); + return true; } /** From a8cff29c3b1538f20b69fc503fcd12bb9f12d93e Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 30 Sep 2024 09:02:12 +1000 Subject: [PATCH 16/16] ci: small fixups --- classes/local/store/object_client.php | 2 +- classes/local/store/object_client_base.php | 2 +- classes/tests/test_client.php | 2 +- tests/local/tagging_test.php | 4 +++- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/classes/local/store/object_client.php b/classes/local/store/object_client.php index ffa59bf2..55c386b3 100644 --- a/classes/local/store/object_client.php +++ b/classes/local/store/object_client.php @@ -144,7 +144,7 @@ public function test_range_request($filesystem); */ public function get_token_expiry_time(): int; - /* + /** * Tests setting an objects tag. * @return stdClass containing 'success' and 'details' properties */ diff --git a/classes/local/store/object_client_base.php b/classes/local/store/object_client_base.php index 7361d72f..e448814e 100644 --- a/classes/local/store/object_client_base.php +++ b/classes/local/store/object_client_base.php @@ -199,7 +199,7 @@ public function get_token_expiry_time(): int { return -1; } - /* + /** * Tests setting an objects tag. * @return stdClass containing 'success' and 'details' properties */ diff --git a/classes/tests/test_client.php b/classes/tests/test_client.php index 7ca00d56..12088abc 100644 --- a/classes/tests/test_client.php +++ b/classes/tests/test_client.php @@ -176,7 +176,7 @@ public function get_token_expiry_time(): int { return $CFG->objectfs_phpunit_token_expiry_time; } - /* + /** * Sets object tags - uses in-memory store for unit tests * @param string $contenthash * @param array $tags diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index c8ff3a54..a5a5956e 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -417,10 +417,12 @@ public function test_get_sync_status_string_does_not_exist() { /** * Tests the length of the defined tag source is checked correctly + * @covers \tool_objectfs\local\environment_source */ public function test_environment_source_too_long() { global $CFG; - $CFG->objectfs_environment_name = 'This is a really long string. It needs to be long because it needs to be more than 128 chars for the test to trigger an exception.'; + $CFG->objectfs_environment_name = 'This is a really long string. + It needs to be long because it needs to be more than 128 chars for the test to trigger an exception.'; $source = new environment_source(); $this->expectException(moodle_exception::class);