From 7352d2f8f535405ab41232db3a0e931c644e663e Mon Sep 17 00:00:00 2001 From: Peter Sistrom Date: Fri, 3 Jul 2026 09:29:50 +1000 Subject: [PATCH 1/4] Issue #752: Migrate all the location status to bits --- classes/local/location_helper.php | 156 ++++++++++++++++++ classes/local/manager.php | 43 ++++- .../candidates/bitmask_candidates.php | 127 ++++++++++++++ .../candidates/checker_candidates.php | 38 +++-- .../candidates/deleter_candidates.php | 12 +- .../candidates/manipulator_candidates.php | 14 -- .../manipulator_candidates_base.php | 12 ++ .../candidates/orphaner_candidates.php | 39 +++-- .../candidates/puller_candidates.php | 11 +- .../candidates/pusher_candidates.php | 10 +- .../candidates/recoverer_candidates.php | 11 +- .../local/report/location_report_builder.php | 32 ++-- .../report/object_location_history_table.php | 10 +- .../report/object_status_history_table.php | 6 +- classes/local/store/object_file_system.php | 39 ++--- classes/local/store/s3/file_system.php | 2 +- classes/local/table/files_table.php | 4 +- classes/local/tag/location_source.php | 5 +- classes/local/tag/tag_manager.php | 12 +- classes/log/aggregate_logger.php | 11 +- .../task/delete_orphaned_object_metadata.php | 7 +- classes/task/populate_objects_filesize.php | 11 +- classes/task/reconcile_filedir.php | 7 +- classes/tests/testcase.php | 4 +- db/install.xml | 6 +- db/upgrade.php | 62 +++++++ lang/en/tool_objectfs.php | 1 + lib.php | 58 +++++-- missing_files.php | 2 +- .../local/object_manipulator/checker_test.php | 18 +- .../local/object_manipulator/deleter_test.php | 10 +- .../object_manipulator/orphaner_test.php | 14 +- .../local/object_manipulator/puller_test.php | 6 +- .../local/object_manipulator/pusher_test.php | 8 +- .../object_manipulator/recoverer_test.php | 18 +- tests/local/tagging_test.php | 4 +- tests/object_file_system_test.php | 18 +- tests/task/populate_objects_filesize_test.php | 8 +- tests/task/task_reconcile_filedir_test.php | 16 +- version.php | 4 +- 40 files changed, 660 insertions(+), 216 deletions(-) create mode 100644 classes/local/location_helper.php create mode 100644 classes/local/object_manipulator/candidates/bitmask_candidates.php diff --git a/classes/local/location_helper.php b/classes/local/location_helper.php new file mode 100644 index 00000000..9810f3ba --- /dev/null +++ b/classes/local/location_helper.php @@ -0,0 +1,156 @@ +. + +/** + * Helper for converting between bitmask location values and individual DB columns. + * + * The PHP layer works with bitmask integers (OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES etc.) + * while the DB stores individual boolean columns (in_filedir, in_mdl_files, in_remote). + * + * @package tool_objectfs + * @author Catalyst IT + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace tool_objectfs\local; + +defined('MOODLE_INTERNAL') || die(); + +require_once(__DIR__ . '/../../lib.php'); + +/** + * Translates between the PHP bitmask representation and the DB boolean columns. + */ +class location_helper { + /** + * Get the registry mapping each bit flag constant to its corresponding DB column name. + * Must be a method rather than a class constant because OBJECT_LOCATION_* are defined at runtime. + * + * @return array [bit_value => column_name, ...] + */ + public static function get_bit_columns(): array { + return [ + OBJECT_LOCATION_IN_FILEDIR => 'in_filedir', + OBJECT_LOCATION_IN_MDL_FILES => 'in_mdl_files', + OBJECT_LOCATION_IN_REMOTE => 'in_remote', + ]; + } + + /** + * Convert a bitmask to an associative array of column => 0/1 values for DB storage. + * + * @param int $location Bitmask (e.g. OBJECT_LOCATION_DUPLICATED = 7). + * @return array ['in_filedir' => 1, 'in_mdl_files' => 1, 'in_remote' => 1] + */ + public static function bits_to_columns(int $location): array { + $columns = []; + foreach (self::get_bit_columns() as $bit => $column) { + $columns[$column] = ($location & $bit) ? 1 : 0; + } + return $columns; + } + + /** + * Reconstruct a bitmask from a DB row's boolean columns. + * + * @param object $row A DB record containing in_filedir, in_mdl_files, in_remote fields. + * @return int The composed bitmask. + */ + public static function columns_to_bits(object $row): int { + $location = 0; + foreach (self::get_bit_columns() as $bit => $column) { + if (!empty($row->$column)) { + $location |= $bit; + } + } + return $location; + } + + /** + * Generate SQL WHERE conditions for bitmask-based candidate queries. + * + * Converts has_mask/not_mask into column equality checks. + * Example: has_mask=3 (IN_FILEDIR|IN_MDL_FILES), not_mask=4 (IN_REMOTE) + * → "in_filedir = 1 AND in_mdl_files = 1 AND in_remote = 0" + * + * @param int $hasmask Bits that MUST be set. + * @param int $notmask Bits that must NOT be set. + * @return string SQL conditions (without leading WHERE/AND). + */ + public static function bits_to_sql_conditions(int $hasmask, int $notmask): string { + $conditions = []; + foreach (self::get_bit_columns() as $bit => $column) { + if ($hasmask & $bit) { + $conditions[] = $column . ' = 1'; + } + if ($notmask & $bit) { + $conditions[] = $column . ' = 0'; + } + } + return implode(' AND ', $conditions); + } + + /** + * Generate SQL WHERE conditions with a table alias prefix. + * + * @param int $hasmask Bits that MUST be set. + * @param int $notmask Bits that must NOT be set. + * @param string $alias Table alias (e.g. 'o'). + * @return string SQL conditions with alias prefix. + */ + public static function bits_to_sql_conditions_aliased(int $hasmask, int $notmask, string $alias): string { + $conditions = []; + foreach (self::get_bit_columns() as $bit => $column) { + if ($hasmask & $bit) { + $conditions[] = $alias . '.' . $column . ' = 1'; + } + if ($notmask & $bit) { + $conditions[] = $alias . '.' . $column . ' = 0'; + } + } + return implode(' AND ', $conditions); + } + + /** + * Get the column name for a given bit flag. + * + * @param int $bit A single bit flag constant. + * @return string|null Column name or null if not registered. + */ + public static function get_column_for_bit(int $bit): ?string { + return self::get_bit_columns()[$bit] ?? null; + } + + /** + * Generate SQL WHERE conditions for an exact location match. + * + * Unlike bits_to_sql_conditions which only checks specified has/not bits, + * this checks ALL registered bits - those in the bitmask must be 1, those not in it must be 0. + * + * @param int $location The exact bitmask to match. + * @param string $alias Optional table alias prefix. + * @return string SQL conditions. + */ + public static function bits_to_exact_sql(int $location, string $alias = ''): string { + $prefix = $alias !== '' ? $alias . '.' : ''; + $conditions = []; + foreach (self::get_bit_columns() as $bit => $column) { + $conditions[] = $prefix . $column . ' = ' . (($location & $bit) ? '1' : '0'); + } + return implode(' AND ', $conditions); + } +} diff --git a/classes/local/manager.php b/classes/local/manager.php index b5268667..f37d9838 100644 --- a/classes/local/manager.php +++ b/classes/local/manager.php @@ -148,11 +148,15 @@ public static function update_object_by_hash($contenthash, $newlocation, $filesi $oldobject = $DB->get_record('tool_objectfs_objects', ['contenthash' => $contenthash]); if ($oldobject) { + // Reconstruct bitmask from stored columns for comparison. + $oldlocation = location_helper::columns_to_bits($oldobject); + $newobject->timeduplicated = $oldobject->timeduplicated; $newobject->id = $oldobject->id; // If location hasn't changed we do not need to update unless filesize is not populated. - if ((int)$oldobject->location === $newlocation && isset($oldobject->filesize)) { + if ($oldlocation === $newlocation && isset($oldobject->filesize)) { + $oldobject->location = $oldlocation; return $oldobject; } @@ -162,7 +166,6 @@ public static function update_object_by_hash($contenthash, $newlocation, $filesi return self::upsert_object($newobject, $newlocation); } - $newobject->location = $newlocation; // Use existing file data related to the object if it exists. $filerecord = $DB->get_record('files', ['contenthash' => $contenthash], 'filesize,timecreated', IGNORE_MULTIPLE); @@ -191,14 +194,28 @@ public static function upsert_object(stdClass $object, $newlocation) { $object->timeduplicated = time(); } - $locationchanged = !isset($object->location) || $object->location != $newlocation; + // Determine if location actually changed. + $oldlocation = isset($object->location) ? $object->location : null; + $locationchanged = $oldlocation === null || $oldlocation != $newlocation; + + // Store the bitmask on the PHP object for callers. $object->location = $newlocation; + // Split bitmask into individual DB columns. + $columns = location_helper::bits_to_columns($newlocation); + foreach ($columns as $col => $val) { + $object->$col = $val; + } + + // Remove the virtual 'location' property before DB write - it's not a real column. + $dbobject = clone $object; + unset($dbobject->location); + // If id is set, update, else insert new. if (empty($object->id)) { - $object->id = $DB->insert_record('tool_objectfs_objects', $object); + $object->id = $DB->insert_record('tool_objectfs_objects', $dbobject); } else { - $DB->update_record('tool_objectfs_objects', $object); + $DB->update_record('tool_objectfs_objects', $dbobject); } // Post update, notify tag manager since the location tag likely needs changing. @@ -210,6 +227,22 @@ public static function upsert_object(stdClass $object, $newlocation) { return $object; } + /** + * Get the location bitmask for an object by its contenthash. + * + * @param string $contenthash + * @return int The location bitmask, or OBJECT_LOCATION_ERROR (0) if not found. + */ + public static function get_location_by_hash(string $contenthash): int { + global $DB; + $record = $DB->get_record('tool_objectfs_objects', ['contenthash' => $contenthash], + 'in_filedir, in_mdl_files, in_remote'); + if (!$record) { + return OBJECT_LOCATION_ERROR; + } + return location_helper::columns_to_bits($record); + } + /** * cloudfront_pem_exists * @return string diff --git a/classes/local/object_manipulator/candidates/bitmask_candidates.php b/classes/local/object_manipulator/candidates/bitmask_candidates.php new file mode 100644 index 00000000..84d4a400 --- /dev/null +++ b/classes/local/object_manipulator/candidates/bitmask_candidates.php @@ -0,0 +1,127 @@ +. + +/** + * Unified bitmask-based candidate class for object manipulation. + * + * Replaces separate pusher_candidates, puller_candidates, deleter_candidates, + * and recoverer_candidates with a single parameterized class. + * + * @package tool_objectfs + * @author Catalyst IT + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace tool_objectfs\local\object_manipulator\candidates; + +use stdClass; +use tool_objectfs\local\location_helper; + +/** + * Universal candidate finder using bitmask filters on the location column. + * + * Accepts two bitmasks: + * - "has" mask: bits the file MUST have (location & has_mask = has_mask) + * - "not" mask: bits the file must NOT have (location & not_mask = 0) + * + * Optional filters for filesize and timeduplicated are applied when provided. + */ +class bitmask_candidates implements manipulator_candidates { + /** @var string Query name for logging. */ + protected $queryname; + + /** @var stdClass Plugin config. */ + protected $config; + + /** @var int Bits that must be set in location. */ + private $hasmask; + + /** @var int Bits that must NOT be set in location. */ + private $notmask; + + /** @var array Optional filter options. */ + private $options; + + /** + * Constructor. + * + * @param stdClass $config Plugin config (must include batchsize). + * @param int $hasmask Bits the location MUST have. + * @param int $notmask Bits the location must NOT have. + * @param string $queryname Name for logging. + * @param array $options Optional filters: + * 'threshold' => int - filesize > threshold (minimum file size) + * 'max_filesize' => int - filesize < max_filesize (maximum file size) + * 'size_ceiling' => int - filesize <= size_ceiling (upper file size bound) + * 'maxage' => int - timeduplicated <= maxage (timestamp threshold) + */ + public function __construct(stdClass $config, int $hasmask, int $notmask, string $queryname, array $options = []) { + $this->config = $config; + $this->hasmask = $hasmask; + $this->notmask = $notmask; + $this->queryname = $queryname; + $this->options = $options; + } + + /** + * get_query_name + * @return string + */ + public function get_query_name() { + return $this->queryname; + } + + /** + * Get candidate objects matching the bitmask filters. + * + * @return array + */ + public function get() { + global $DB; + + // Convert bitmasks to column-based SQL conditions. + $locationconditions = location_helper::bits_to_sql_conditions($this->hasmask, $this->notmask); + + $conditions = [$locationconditions]; + $params = []; + + if (isset($this->options['threshold'])) { + $conditions[] = 'filesize > :threshold'; + $params['threshold'] = $this->options['threshold']; + } + if (isset($this->options['max_filesize'])) { + $conditions[] = 'filesize < :max_filesize'; + $params['max_filesize'] = $this->options['max_filesize']; + } + if (isset($this->options['size_ceiling'])) { + $conditions[] = 'filesize <= :size_ceiling'; + $params['size_ceiling'] = $this->options['size_ceiling']; + } + if (isset($this->options['maxage'])) { + $conditions[] = 'timeduplicated <= :maxage'; + $params['maxage'] = $this->options['maxage']; + } + + $where = implode("\n AND ", $conditions); + $sql = "SELECT contenthash, + filesize + FROM {tool_objectfs_objects} + WHERE {$where}"; + + return $DB->get_records_sql($sql, $params, 0, $this->config->batchsize); + } +} diff --git a/classes/local/object_manipulator/candidates/checker_candidates.php b/classes/local/object_manipulator/candidates/checker_candidates.php index e52af679..d390482d 100644 --- a/classes/local/object_manipulator/candidates/checker_candidates.php +++ b/classes/local/object_manipulator/candidates/checker_candidates.php @@ -27,31 +27,45 @@ /** * chcker_candiates */ -class checker_candidates extends manipulator_candidates_base { +class checker_candidates implements manipulator_candidates { /** * queryname * @var string */ protected $queryname = 'get_check_candidates'; + /** @var \stdClass $config */ + protected $config; + /** - * get_candiates_sql + * checker_candidates constructor. + * @param \stdClass $config + */ + public function __construct(\stdClass $config) { + $this->config = $config; + } + + /** + * get_query_name * @return string */ - public function get_candidates_sql() { - return 'SELECT f.contenthash - FROM {files} f - LEFT JOIN {tool_objectfs_objects} o ON f.contenthash = o.contenthash - WHERE f.filesize > 0 - AND o.location is NULL - GROUP BY f.contenthash'; + public function get_query_name() { + return $this->queryname; } /** - * get_candidates_sql_params + * Get files that exist in {files} but have no tracking row in {tool_objectfs_objects}. + * * @return array */ - public function get_candidates_sql_params() { - return []; + public function get() { + global $DB; + $sql = 'SELECT f.contenthash + FROM {files} f + LEFT JOIN {tool_objectfs_objects} o ON f.contenthash = o.contenthash + WHERE f.filesize > 0 + AND o.id IS NULL + GROUP BY f.contenthash'; + return $DB->get_records_sql($sql, [], 0, $this->config->batchsize); } } diff --git a/classes/local/object_manipulator/candidates/deleter_candidates.php b/classes/local/object_manipulator/candidates/deleter_candidates.php index 5985bd3b..014604c2 100644 --- a/classes/local/object_manipulator/candidates/deleter_candidates.php +++ b/classes/local/object_manipulator/candidates/deleter_candidates.php @@ -38,24 +38,24 @@ class deleter_candidates extends manipulator_candidates_base { * get_candiates_sql * @return string */ - public function get_candidates_sql() { - return 'SELECT contenthash, + protected function get_candidates_sql(): string { + $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_DUPLICATED); + return "SELECT contenthash, filesize FROM {tool_objectfs_objects} WHERE timeduplicated <= :consistancythreshold - AND location = :location - AND filesize > :sizethreshold'; + AND {$locationconditions} + AND filesize > :sizethreshold"; } /** * get_candiates_sql_params * @return array */ - public function get_candidates_sql_params() { + protected function get_candidates_sql_params(): array { $consistancythreshold = time() - $this->config->consistencydelay; return [ 'consistancythreshold' => $consistancythreshold, - 'location' => OBJECT_LOCATION_DUPLICATED, 'sizethreshold' => $this->config->sizethreshold, ]; } diff --git a/classes/local/object_manipulator/candidates/manipulator_candidates.php b/classes/local/object_manipulator/candidates/manipulator_candidates.php index 234ee2cb..99943cb3 100644 --- a/classes/local/object_manipulator/candidates/manipulator_candidates.php +++ b/classes/local/object_manipulator/candidates/manipulator_candidates.php @@ -33,20 +33,6 @@ interface manipulator_candidates { */ public function get_query_name(); - /** - * Returns SQL to retrieve objects for manipulation. - * - * @return string - */ - public function get_candidates_sql(); - - /** - * Returns a list of parameters for SQL from get_candidates_sql. - * - * @return array - */ - public function get_candidates_sql_params(); - /** * get * @return array diff --git a/classes/local/object_manipulator/candidates/manipulator_candidates_base.php b/classes/local/object_manipulator/candidates/manipulator_candidates_base.php index a542217c..2a038633 100644 --- a/classes/local/object_manipulator/candidates/manipulator_candidates_base.php +++ b/classes/local/object_manipulator/candidates/manipulator_candidates_base.php @@ -64,4 +64,16 @@ public function get() { $this->config->batchsize ); } + + /** + * Returns SQL to retrieve objects for manipulation. + * @return string + */ + abstract protected function get_candidates_sql(): string; + + /** + * Returns parameters for the SQL from get_candidates_sql. + * @return array + */ + abstract protected function get_candidates_sql_params(): array; } diff --git a/classes/local/object_manipulator/candidates/orphaner_candidates.php b/classes/local/object_manipulator/candidates/orphaner_candidates.php index e2cd1db4..3e9ec167 100644 --- a/classes/local/object_manipulator/candidates/orphaner_candidates.php +++ b/classes/local/object_manipulator/candidates/orphaner_candidates.php @@ -27,32 +27,45 @@ /** * orphaner_candidates */ -class orphaner_candidates extends manipulator_candidates_base { +class orphaner_candidates implements manipulator_candidates { /** * queryname * @var string */ protected $queryname = 'get_orphan_candidates'; + /** @var \stdClass $config */ + protected $config; + + /** + * orphaner_candidates constructor. + * @param \stdClass $config + */ + public function __construct(\stdClass $config) { + $this->config = $config; + } + /** - * get_candidates_sql + * get_query_name * @return string */ - public function get_candidates_sql() { - return 'SELECT o.id, o.contenthash, o.location - FROM {tool_objectfs_objects} o - LEFT JOIN {files} f ON o.contenthash = f.contenthash - WHERE f.id is null - AND o.location != :location'; + public function get_query_name() { + return $this->queryname; } /** - * get_candidates_sql_params + * Get tracked objects that no longer have a reference in {files}. + * Excludes objects already marked as orphaned (in_filedir=1, in_mdl_files=0, in_remote=0). + * * @return array */ - public function get_candidates_sql_params() { - return [ - 'location' => OBJECT_LOCATION_ORPHANED, - ]; + public function get() { + global $DB; + $sql = 'SELECT o.id, o.contenthash, o.in_filedir, o.in_mdl_files, o.in_remote + FROM {tool_objectfs_objects} o + LEFT JOIN {files} f ON o.contenthash = f.contenthash + WHERE f.id IS NULL + AND NOT (o.in_filedir = 1 AND o.in_mdl_files = 0 AND o.in_remote = 0)'; + return $DB->get_records_sql($sql, [], 0, $this->config->batchsize); } } diff --git a/classes/local/object_manipulator/candidates/puller_candidates.php b/classes/local/object_manipulator/candidates/puller_candidates.php index 4a497df4..13109654 100644 --- a/classes/local/object_manipulator/candidates/puller_candidates.php +++ b/classes/local/object_manipulator/candidates/puller_candidates.php @@ -38,19 +38,20 @@ class puller_candidates extends manipulator_candidates_base { * get_candidates_sql * @return string */ - public function get_candidates_sql() { - return 'SELECT contenthash, + protected function get_candidates_sql(): string { + $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_EXTERNAL); + return "SELECT contenthash, filesize FROM {tool_objectfs_objects} WHERE filesize <= :sizethreshold - AND location = :location'; + AND {$locationconditions}"; } /** * get_candidates_sql_params * @return array */ - public function get_candidates_sql_params() { - return ['sizethreshold' => $this->config->sizethreshold, 'location' => OBJECT_LOCATION_EXTERNAL]; + protected function get_candidates_sql_params(): array { + return ['sizethreshold' => $this->config->sizethreshold]; } } diff --git a/classes/local/object_manipulator/candidates/pusher_candidates.php b/classes/local/object_manipulator/candidates/pusher_candidates.php index ae45b05b..20aae41f 100644 --- a/classes/local/object_manipulator/candidates/pusher_candidates.php +++ b/classes/local/object_manipulator/candidates/pusher_candidates.php @@ -38,27 +38,27 @@ class pusher_candidates extends manipulator_candidates_base { * get_candidates_sql * @return string */ - public function get_candidates_sql() { - return 'SELECT contenthash, + protected function get_candidates_sql(): string { + $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_LOCAL); + return "SELECT contenthash, filesize FROM {tool_objectfs_objects} WHERE filesize > :threshold AND filesize < :maximum_file_size AND timeduplicated <= :maxcreatedtimestamp - AND location = :object_location'; + AND {$locationconditions}"; } /** * get_candidates_sql_params * @return array */ - public function get_candidates_sql_params() { + protected function get_candidates_sql_params(): array { $filesystem = new $this->config->filesystem(); return [ 'maxcreatedtimestamp' => time() - $this->config->minimumage, 'threshold' => $this->config->sizethreshold, 'maximum_file_size' => $filesystem->get_maximum_upload_filesize(), - 'object_location' => OBJECT_LOCATION_LOCAL, ]; } } diff --git a/classes/local/object_manipulator/candidates/recoverer_candidates.php b/classes/local/object_manipulator/candidates/recoverer_candidates.php index 9e29c868..f3c5fdc5 100644 --- a/classes/local/object_manipulator/candidates/recoverer_candidates.php +++ b/classes/local/object_manipulator/candidates/recoverer_candidates.php @@ -38,18 +38,19 @@ class recoverer_candidates extends manipulator_candidates_base { * get_candidates_sql * @return string */ - public function get_candidates_sql() { - return 'SELECT contenthash, + protected function get_candidates_sql(): string { + $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_MISSING); + return "SELECT contenthash, filesize FROM {tool_objectfs_objects} - WHERE location = :location'; + WHERE {$locationconditions}"; } /** * get_candidates_sql_params * @return array */ - public function get_candidates_sql_params() { - return ['location' => OBJECT_LOCATION_ERROR]; + protected function get_candidates_sql_params(): array { + return []; } } diff --git a/classes/local/report/location_report_builder.php b/classes/local/report/location_report_builder.php index 99cb4da4..8d96918e 100644 --- a/classes/local/report/location_report_builder.php +++ b/classes/local/report/location_report_builder.php @@ -25,6 +25,7 @@ namespace tool_objectfs\local\report; +use tool_objectfs\local\location_helper; use tool_objectfs\local\manager; use tool_objectfs\local\store\object_file_system; @@ -46,7 +47,7 @@ public function build_report($reportid) { OBJECT_LOCATION_DUPLICATED, OBJECT_LOCATION_EXTERNAL, OBJECT_LOCATION_ORPHANED, - OBJECT_LOCATION_ERROR, + OBJECT_LOCATION_MISSING, ]; $totalcount = 0; @@ -54,12 +55,14 @@ public function build_report($reportid) { $filedircount = 0; $filedirsum = 0; foreach ($locations as $location) { + $locationwhere = location_helper::bits_to_exact_sql($location, 'o'); + $sql = - 'WITH + "WITH cte_objects AS ( - SELECT o.contenthash, o.location + SELECT o.contenthash FROM {tool_objectfs_objects} o - WHERE o.location = ? ), + WHERE {$locationwhere} ), cte_obj_files AS ( SELECT f.contenthash, MAX(f.filesize) AS filesize FROM {files} f @@ -68,39 +71,40 @@ public function build_report($reportid) { GROUP BY f.contenthash, f.filesize) SELECT COALESCE(COUNT(cof.contenthash),0) AS objectcount, COALESCE(SUM(cof.filesize),0) AS objectsum - FROM cte_obj_files cof'; + FROM cte_obj_files cof"; if ($location == OBJECT_LOCATION_LOCAL) { + $localwhere = location_helper::bits_to_exact_sql($location, 'co'); $sql = - 'WITH + "WITH cte_objects AS ( - SELECT o.contenthash, o.location + SELECT o.contenthash FROM {tool_objectfs_objects} o ), cte_obj_files AS ( SELECT f.contenthash, MAX(f.filesize) AS filesize FROM {files} f LEFT JOIN cte_objects co ON f.contenthash = co.contenthash - WHERE filesize > 0 AND ( co.location = ? OR co.location IS NULL ) + WHERE filesize > 0 AND ( {$localwhere} OR co.in_filedir IS NULL ) GROUP BY f.contenthash, f.filesize) SELECT COALESCE(COUNT(cof.contenthash),0) AS objectcount, COALESCE(SUM(cof.filesize),0) AS objectsum - FROM cte_obj_files cof'; + FROM cte_obj_files cof"; } if ($location !== OBJECT_LOCATION_ORPHANED) { // Process the query normally. - $result = $DB->get_record_sql($sql, [$location]); + $result = $DB->get_record_sql($sql); } else if ($location === OBJECT_LOCATION_ORPHANED) { // Start the query from objectfs, for ORPHANED objects, they are not located in the files table. $sql = - 'WITH + "WITH cte_objects AS ( SELECT o.contenthash FROM {tool_objectfs_objects} o - WHERE o.location = ?) + WHERE {$locationwhere}) SELECT COALESCE(COUNT(co.contenthash),0) AS objectcount - FROM cte_objects co'; - $result = $DB->get_record_sql($sql, [$location]); + FROM cte_objects co"; + $result = $DB->get_record_sql($sql); $result->objectsum = 0; } diff --git a/classes/local/report/object_location_history_table.php b/classes/local/report/object_location_history_table.php index faee6163..cbcfac70 100644 --- a/classes/local/report/object_location_history_table.php +++ b/classes/local/report/object_location_history_table.php @@ -136,7 +136,7 @@ public function query_db($pagesize, $useinitialsbar = true) { $duplicatedrecord = $rawrecords[$id . OBJECT_LOCATION_DUPLICATED] ?? $emptyrecord; $orphanedrecord = $rawrecords[$id . OBJECT_LOCATION_ORPHANED] ?? $emptyrecord; $externalrecord = $rawrecords[$id . OBJECT_LOCATION_EXTERNAL] ?? $emptyrecord; - $errorrecord = $rawrecords[$id . OBJECT_LOCATION_ERROR] ?? $emptyrecord; + $missingrecord = $rawrecords[$id . OBJECT_LOCATION_MISSING] ?? $emptyrecord; $filedir = $rawrecords[$id . 'filedir'] ?? $emptyrecord; $total = $rawrecords[$id . 'total'] ?? $emptyrecord; @@ -154,8 +154,8 @@ public function query_db($pagesize, $useinitialsbar = true) { $row['orphaned_size'] = get_string('object_status:location:orphanedsizeunknown', 'tool_objectfs'); $row['external_count'] = $externalrecord->count; $row['external_size'] = $externalrecord->size; - $row['missing_count'] = $errorrecord->count; - $row['missing_size'] = $errorrecord->size; + $row['missing_count'] = $missingrecord->count; + $row['missing_size'] = $missingrecord->size; $row['total_count'] = $total->count; $row['total_size'] = $total->size; $row['filedir_count'] = $filedir->count; @@ -167,7 +167,7 @@ public function query_db($pagesize, $useinitialsbar = true) { $row['duplicated_count'] = number_format($duplicatedrecord->count); $row['orphaned_count'] = number_format($orphanedrecord->count); $row['external_count'] = number_format($externalrecord->count); - $row['missing_count'] = number_format($errorrecord->count); + $row['missing_count'] = number_format($missingrecord->count); $row['total_count'] = number_format($total->count); $row['filedir_count'] = number_format($filedir->count); $row['delta_count'] = number_format($deltacount); @@ -176,7 +176,7 @@ public function query_db($pagesize, $useinitialsbar = true) { $this->duplicatedsizes[] = $this->size_to_mb($duplicatedrecord->size); $this->orphanedsizes[] = $this->size_to_mb($orphanedrecord->size); $this->externalsizes[] = $this->size_to_mb($externalrecord->size); - $this->missingsizes[] = $this->size_to_mb($errorrecord->size); + $this->missingsizes[] = $this->size_to_mb($missingrecord->size); $this->totalsizes[] = $this->size_to_mb($total->size); $this->filedirsizes[] = $this->size_to_mb($filedir->size); $this->deltasizes[] = $this->size_to_mb($deltasize); diff --git a/classes/local/report/object_status_history_table.php b/classes/local/report/object_status_history_table.php index f037e294..c31d6606 100644 --- a/classes/local/report/object_status_history_table.php +++ b/classes/local/report/object_status_history_table.php @@ -240,11 +240,11 @@ public function get_file_location_string($filelocation) { 'filedir' => 'object_status:filedir', 'deltaa' => 'object_status:delta:a', 'deltab' => 'object_status:delta:b', - OBJECT_LOCATION_ERROR => 'object_status:location:error', + OBJECT_LOCATION_MISSING => 'object_status:location:missing', + OBJECT_LOCATION_ORPHANED => 'object_status:location:orphaned', OBJECT_LOCATION_LOCAL => 'object_status:location:local', OBJECT_LOCATION_DUPLICATED => 'object_status:location:duplicated', OBJECT_LOCATION_EXTERNAL => 'object_status:location:external', - OBJECT_LOCATION_ORPHANED => 'object_status:location:orphaned', ]; if (isset($locationstringmap[$filelocation])) { return get_string($locationstringmap[$filelocation], 'tool_objectfs'); @@ -262,7 +262,7 @@ public function get_file_location_class($filelocation): string { switch ($filelocation) { case 'deltaa': case 'deltab': - case OBJECT_LOCATION_ERROR: + case OBJECT_LOCATION_MISSING: case OBJECT_LOCATION_ORPHANED: $class = 'table-danger'; break; diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index d7df5b93..d5381d07 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -310,20 +310,21 @@ public function is_file_readable_externally_by_hash($contenthash) { * @return int */ public function get_object_location_from_hash($contenthash) { - $localreadable = $this->is_file_readable_locally_by_hash($contenthash); - $externalreadable = $this->is_file_readable_externally_by_hash($contenthash); - - if ($localreadable && $externalreadable) { - return OBJECT_LOCATION_DUPLICATED; - } else if ($localreadable && !$externalreadable) { - return OBJECT_LOCATION_LOCAL; - } else if (!$localreadable && $externalreadable) { - return OBJECT_LOCATION_EXTERNAL; - } else { - // Object is not anywhere - we toggle an error state in the DB. - manager::update_object_by_hash($contenthash, OBJECT_LOCATION_ERROR); - return OBJECT_LOCATION_ERROR; + // The mdl_files bit is always set because this function is only called + // for objects known to be referenced in the files table. + $location = OBJECT_LOCATION_IN_MDL_FILES; + if ($this->is_file_readable_locally_by_hash($contenthash)) { + $location |= OBJECT_LOCATION_IN_FILEDIR; + } + if ($this->is_file_readable_externally_by_hash($contenthash)) { + $location |= OBJECT_LOCATION_IN_REMOTE; } + + if ($location === OBJECT_LOCATION_MISSING) { + // Object exists in mdl_files but is not anywhere physically - record missing state. + manager::update_object_by_hash($contenthash, OBJECT_LOCATION_MISSING); + } + return $location; } /** @@ -487,7 +488,7 @@ public function readfile(\stored_file $file) { $this->logger->log_object_read('readfile', $path, $file->get_filesize()); if ($success === false) { - manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_ERROR); + manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_MISSING); } } } @@ -517,7 +518,7 @@ public function get_content(\stored_file $file) { $this->logger->log_object_read('file_get_contents', $path, $file->get_filesize()); if (!$contents) { - manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_ERROR); + manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_MISSING); } return $contents; @@ -627,7 +628,7 @@ public function get_content_file_handle(\stored_file $file, $type = \stored_file $this->logger->log_object_read('get_file_handle_for_path', $path, $file->get_filesize()); if (!$filehandle) { - manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_ERROR); + manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_MISSING); } return $filehandle; @@ -764,6 +765,7 @@ public function delete_object_from_hash($contenthash) { $this->delete_external_file_from_hash($contenthash); break; + case OBJECT_LOCATION_MISSING: case OBJECT_LOCATION_ERROR: default: return; @@ -1317,8 +1319,7 @@ private function can_set_object_tags(string $contenthash): bool { */ private function is_file_stored_externally_by_hash(string $contenthash): bool { global $DB; - $location = (int) $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $contenthash]); - - return $location === OBJECT_LOCATION_DUPLICATED || $location === OBJECT_LOCATION_EXTERNAL; + $record = $DB->get_record('tool_objectfs_objects', ['contenthash' => $contenthash], 'in_remote'); + return !empty($record->in_remote); } } diff --git a/classes/local/store/s3/file_system.php b/classes/local/store/s3/file_system.php index 029f66fd..8787d518 100644 --- a/classes/local/store/s3/file_system.php +++ b/classes/local/store/s3/file_system.php @@ -71,7 +71,7 @@ public function readfile(\stored_file $file) { $this->get_logger()->log_object_read('readfile', $path, $file->get_filesize()); if ($success === false) { - manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_ERROR); + manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_MISSING); throw new \file_exception('storedfilecannotreadfile', $file->get_filename()); } } diff --git a/classes/local/table/files_table.php b/classes/local/table/files_table.php index a4c6f5e7..84674d07 100644 --- a/classes/local/table/files_table.php +++ b/classes/local/table/files_table.php @@ -46,8 +46,8 @@ public function __construct($uniqueid, $objectlocation) { $from = '{files} f'; $from .= ' LEFT JOIN {tool_objectfs_objects} o on f.contenthash = o.contenthash'; $from .= ' LEFT JOIN {context} ctx ON f.contextid = ctx.id'; - $where = 'o.location = ?'; - $params = [$objectlocation]; + $where = \tool_objectfs\local\location_helper::bits_to_exact_sql((int)$objectlocation, 'o'); + $params = []; $this->columns = $this->headers = ['id', 'contextid', 'contenthash', 'localpath', 'link', 'component', 'filearea', 'filename', 'filepath', 'mimetype', 'filesize', 'timecreated']; diff --git a/classes/local/tag/location_source.php b/classes/local/tag/location_source.php index 1353a03a..9ff3ec2d 100644 --- a/classes/local/tag/location_source.php +++ b/classes/local/tag/location_source.php @@ -49,8 +49,9 @@ public static function get_description(): string { public function get_value_for_contenthash(string $contenthash): ?string { global $DB; - $isorphaned = $DB->record_exists('tool_objectfs_objects', ['contenthash' => $contenthash, - 'location' => OBJECT_LOCATION_ORPHANED]); + $columns = \tool_objectfs\local\location_helper::bits_to_columns(OBJECT_LOCATION_ORPHANED); + $conditions = array_merge(['contenthash' => $contenthash], $columns); + $isorphaned = $DB->record_exists('tool_objectfs_objects', $conditions); return $isorphaned ? 'orphan' : 'active'; } diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php index 2866329a..cb750bcf 100644 --- a/classes/local/tag/tag_manager.php +++ b/classes/local/tag/tag_manager.php @@ -138,13 +138,15 @@ 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; + $locationconditions = '(' . + '(' . \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_DUPLICATED) . ')' . + ' OR (' . \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_EXTERNAL) . ')' . + ' OR (' . \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ORPHANED) . ')' . + ')'; $records = $DB->get_records_select( 'tool_objectfs_objects', - 'tagsyncstatus = :syncstatus AND location ' . $insql, - $inparams, + "tagsyncstatus = :syncstatus AND {$locationconditions}", + ['syncstatus' => self::SYNC_STATUS_NEEDS_SYNC], '', 'contenthash', 0, diff --git a/classes/log/aggregate_logger.php b/classes/log/aggregate_logger.php index 406ea584..63694e66 100644 --- a/classes/log/aggregate_logger.php +++ b/classes/log/aggregate_logger.php @@ -64,7 +64,8 @@ class aggregate_logger extends objectfs_logger { public function __construct() { parent::__construct(); $this->movestatistics = [ - OBJECT_LOCATION_ERROR => [], + OBJECT_LOCATION_MISSING => [], + OBJECT_LOCATION_ORPHANED => [], OBJECT_LOCATION_LOCAL => [], OBJECT_LOCATION_DUPLICATED => [], OBJECT_LOCATION_EXTERNAL => [], @@ -157,16 +158,16 @@ protected function output_move_statistic($movestatistic, $initiallocation, $fina */ public function location_to_string($location) { switch ($location) { - case OBJECT_LOCATION_ERROR: - return 'error'; + case OBJECT_LOCATION_MISSING: + return 'missing'; + case OBJECT_LOCATION_ORPHANED: + return 'orphaned'; case OBJECT_LOCATION_LOCAL: return 'local'; case OBJECT_LOCATION_DUPLICATED: return 'duplicated'; case OBJECT_LOCATION_EXTERNAL: return 'remote'; - case OBJECT_LOCATION_ORPHANED: - return 'orphaned'; default: return $location; } diff --git a/classes/task/delete_orphaned_object_metadata.php b/classes/task/delete_orphaned_object_metadata.php index 10b4ac67..40c23453 100644 --- a/classes/task/delete_orphaned_object_metadata.php +++ b/classes/task/delete_orphaned_object_metadata.php @@ -51,8 +51,8 @@ public function execute() { return; } + $orphanconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ORPHANED, 'o'); $params = [ - 'location' => OBJECT_LOCATION_ORPHANED, 'ageforremoval' => time() - $ageforremoval, ]; @@ -64,7 +64,7 @@ public function execute() { $sql = 'SELECT o.* FROM {tool_objectfs_objects} o LEFT JOIN {files} f ON o.contenthash = f.contenthash - WHERE f.id is null AND o.location = :location AND timeduplicated < :ageforremoval'; + WHERE f.id is null AND {$orphanconditions} AND timeduplicated < :ageforremoval'; $objects = $DB->get_recordset_sql($sql, $params); $count = 0; @@ -79,7 +79,8 @@ public function execute() { mtrace("Deleted $count orphaned files and their metadata (orphaned tool_objectfs_objects)"); } else { // Delete external files is turned off, we only delete the metadata. - $wheresql = 'location = :location and timeduplicated < :ageforremoval'; + $orphanconditions2 = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ORPHANED); + $wheresql = "{$orphanconditions2} and timeduplicated < :ageforremoval"; $count = $DB->count_records_select('tool_objectfs_objects', $wheresql, $params); if (!empty($count)) { mtrace("Deleting $count records with orphaned metadata (orphaned tool_objectfs_objects)"); diff --git a/classes/task/populate_objects_filesize.php b/classes/task/populate_objects_filesize.php index 98936f0a..a76a1eed 100644 --- a/classes/task/populate_objects_filesize.php +++ b/classes/task/populate_objects_filesize.php @@ -42,14 +42,19 @@ public function execute() { $maxupdates = !empty($data->maxupdates) ? $data->maxupdates : self::MAX_UPDATES; // Get all objects without a filesize and join them to a filesize from the files table. - // Values less than 0 for object's location indicate an error for the object. - $sql = "SELECT o.id, o.contenthash, o.timeduplicated, o.location, f.filesize + // Exclude error objects (those with all location bits = 0). + $sql = "SELECT o.id, o.contenthash, o.timeduplicated, o.in_filedir, o.in_mdl_files, o.in_remote, f.filesize FROM {tool_objectfs_objects} o JOIN {files} f ON o.contenthash = f.contenthash WHERE o.filesize IS NULL - AND o.location >= 0 + AND o.in_mdl_files = 1 + AND (o.in_filedir = 1 OR o.in_remote = 1) GROUP BY o.id, o.contenthash, + o.timeduplicated, + o.in_filedir, + o.in_mdl_files, + o.in_remote, f.filesize"; $records = $DB->get_recordset_sql($sql, null, 0, $maxupdates + 1); diff --git a/classes/task/reconcile_filedir.php b/classes/task/reconcile_filedir.php index 98cdb895..bb3c01e8 100644 --- a/classes/task/reconcile_filedir.php +++ b/classes/task/reconcile_filedir.php @@ -64,7 +64,7 @@ public function execute(): void { // The config deletelocal must be enabled. if (!get_config('tool_objectfs', 'deletelocal')) { - mtrace('ObjectFS: deletelocal disabled — skipping filedir reconciliation.'); + mtrace('ObjectFS: deletelocal disabled - skipping filedir reconciliation.'); return; } @@ -75,7 +75,7 @@ public function execute(): void { try { $filesystem = new $this->config->filesystem(); } catch (\Throwable $e) { - mtrace('ObjectFS: Could not instantiate filesystem — will register new files as local only. ' . $e->getMessage()); + mtrace('ObjectFS: Could not instantiate filesystem - will register new files as local only. ' . $e->getMessage()); } } @@ -295,7 +295,8 @@ public function execute(): void { } else { // If ObjectFS thinks the file is remote-only, // update it so it knows the file is duplicated. - if ($object->location == OBJECT_LOCATION_EXTERNAL) { + $objectlocation = \tool_objectfs\local\location_helper::columns_to_bits($object); + if ($objectlocation == OBJECT_LOCATION_EXTERNAL) { manager::upsert_object($object, OBJECT_LOCATION_DUPLICATED); $updated++; } diff --git a/classes/tests/testcase.php b/classes/tests/testcase.php index 9384e120..df6f92da 100644 --- a/classes/tests/testcase.php +++ b/classes/tests/testcase.php @@ -180,7 +180,7 @@ protected function create_error_file() { $file = $this->create_local_file(); $path = $this->get_local_path_from_storedfile($file); unlink($path); - manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_ERROR); + manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_MISSING); return $file; } @@ -311,7 +311,7 @@ protected function create_remote_object($content = 'remote object content') { */ protected function create_error_object($content = 'error object content') { $file = $this->create_error_file($content); - return $this->create_object_record($file, OBJECT_LOCATION_ERROR); + return $this->create_object_record($file, OBJECT_LOCATION_MISSING); } /** diff --git a/db/install.xml b/db/install.xml index 0065b1d7..267ab4ac 100644 --- a/db/install.xml +++ b/db/install.xml @@ -9,7 +9,9 @@ - + + + @@ -19,7 +21,7 @@ - + diff --git a/db/upgrade.php b/db/upgrade.php index bf06b09c..95415994 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -220,5 +220,67 @@ function xmldb_tool_objectfs_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2024120600, 'tool', 'objectfs'); } + if ($oldversion < 2026041007) { + // Migrate from single 'location' integer column to individual boolean columns. + // Old values: ORPHANED=-2, ERROR(missing)=-1, LOCAL=0, DUPLICATED=1, EXTERNAL=2 + // New columns: in_filedir, in_mdl_files, in_remote (each 0 or 1). + + $table = new xmldb_table('tool_objectfs_objects'); + + // Add the new boolean columns. + $field = new xmldb_field('in_filedir', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'timeduplicated'); + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + $field = new xmldb_field('in_mdl_files', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'in_filedir'); + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + $field = new xmldb_field('in_remote', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'in_mdl_files'); + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Populate new columns from old location values. + // ORPHANED (-2): in_filedir=1, in_mdl_files=0, in_remote=0. + $DB->execute('UPDATE {tool_objectfs_objects} SET in_filedir = 1 WHERE location = -2'); + // ERROR/MISSING (-1): in_filedir=0, in_mdl_files=1, in_remote=0. + $DB->execute('UPDATE {tool_objectfs_objects} SET in_mdl_files = 1 WHERE location = -1'); + // LOCAL (0): in_filedir=1, in_mdl_files=1, in_remote=0. + $DB->execute('UPDATE {tool_objectfs_objects} SET in_filedir = 1, in_mdl_files = 1 WHERE location = 0'); + // DUPLICATED (1): in_filedir=1, in_mdl_files=1, in_remote=1. + $DB->execute('UPDATE {tool_objectfs_objects} SET in_filedir = 1, in_mdl_files = 1, in_remote = 1 WHERE location = 1'); + // EXTERNAL (2): in_filedir=0, in_mdl_files=1, in_remote=1. + $DB->execute('UPDATE {tool_objectfs_objects} SET in_mdl_files = 1, in_remote = 1 WHERE location = 2'); + + // Drop the old index that references location. + $index = new xmldb_index('toolobjeobje_con_idu_ix', XMLDB_INDEX_UNIQUE, ['contenthash', 'location']); + if ($dbman->index_exists($table, $index)) { + $dbman->drop_index($table, $index); + } + + // Drop the old location column. + $field = new xmldb_field('location'); + if ($dbman->field_exists($table, $field)) { + $dbman->drop_field($table, $field); + } + + // Add composite index on the new boolean columns. + $index = new xmldb_index('ix_location_bits', XMLDB_INDEX_NOTUNIQUE, ['in_filedir', 'in_mdl_files', 'in_remote']); + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + // Migrate report_data datakey values to the new bitmask values. + // Reports store the composite location value as a string key. + $DB->execute("UPDATE {tool_objectfs_report_data} SET datakey = '6' WHERE reporttype = 'location' AND datakey = '2'"); + $DB->execute("UPDATE {tool_objectfs_report_data} SET datakey = '7' WHERE reporttype = 'location' AND datakey = '1'"); + $DB->execute("UPDATE {tool_objectfs_report_data} SET datakey = '3' WHERE reporttype = 'location' AND datakey = '0'"); + $DB->execute("UPDATE {tool_objectfs_report_data} SET datakey = '2' WHERE reporttype = 'location' AND datakey = '-1'"); + $DB->execute("UPDATE {tool_objectfs_report_data} SET datakey = '1' WHERE reporttype = 'location' AND datakey = '-2'"); + + upgrade_plugin_savepoint(true, 2026041007, 'tool', 'objectfs'); + } + return true; } diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index b1884d74..816667d2 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -72,6 +72,7 @@ $string['object_status:location:duplicatedsize'] = 'Duplicated (size)'; $string['object_status:location:duplicatedsizechart'] = 'Duplicated (MB)'; $string['object_status:location:error'] = 'Missing from filedir and external storage (view files)'; +$string['object_status:location:missing'] = 'Missing from filedir and external storage (view files)'; $string['object_status:location:external'] = 'Only in external storage'; $string['object_status:location:externalcount'] = 'External (count)'; $string['object_status:location:externalsize'] = 'External (size)'; diff --git a/lib.php b/lib.php index 870e6f32..24dc7a69 100644 --- a/lib.php +++ b/lib.php @@ -29,36 +29,62 @@ define('OBJECTFS_PLUGIN_NAME', 'tool_objectfs'); /** - * Location enum of the object - * ORPHANED is when the {objectfs_objects} table contains a record linking to a - * moodle {files} record which is no longer present. + * Location bit flag: the object exists in the local filedir. + * Bit 1. */ -define('OBJECT_LOCATION_ORPHANED', -2); +define('OBJECT_LOCATION_IN_FILEDIR', 1); /** - * Location enum of the object - * ERROR is when the file is missing when it is expected to be there. + * Location bit flag: the object is referenced in the Moodle files table (mdl_files). + * Bit 2. + */ +define('OBJECT_LOCATION_IN_MDL_FILES', 2); + +/** + * Location bit flag: the object exists in the primary remote object store. + * Bit 4. + */ +define('OBJECT_LOCATION_IN_REMOTE', 4); + +/** + * Location of the object: completely absent — no bits set. + * This is an invalid state that should not normally be stored in the database. + * Value: 0. + */ +define('OBJECT_LOCATION_ERROR', 0); + +/** + * Location of the object: in filedir only, no mdl_files reference. + * This is a trashdir candidate marked for cleanup. + * Value: OBJECT_LOCATION_IN_FILEDIR = 1. + */ +define('OBJECT_LOCATION_ORPHANED', OBJECT_LOCATION_IN_FILEDIR); + +/** + * Location of the object: referenced in mdl_files but not present in filedir or any remote store. + * This is a missing-file error state. + * Value: OBJECT_LOCATION_IN_MDL_FILES = 2. * @see tests/object_file_system_test.php for examples. */ -define('OBJECT_LOCATION_ERROR', -1); +define('OBJECT_LOCATION_MISSING', OBJECT_LOCATION_IN_MDL_FILES); /** - * Location enum of the object - * LOCAL is when the object exists locally only. + * Location of the object: in filedir and mdl_files, not yet pushed to any remote store. + * Value: OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES = 3. */ -define('OBJECT_LOCATION_LOCAL', 0); +define('OBJECT_LOCATION_LOCAL', OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES); /** - * Location enum of the object - * DUPLICATED is when the object exists both locally, and remotely. + * Location of the object: in mdl_files and the primary remote store, no local filedir copy. + * Value: OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE = 6. */ -define('OBJECT_LOCATION_DUPLICATED', 1); +define('OBJECT_LOCATION_EXTERNAL', OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE); /** - * Location enum of the object - * EXTERNAL is when when the object lives remotely only. + * Location of the object: in filedir, mdl_files, and the primary remote store. + * Value: OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE = 7. */ -define('OBJECT_LOCATION_EXTERNAL', 2); +define('OBJECT_LOCATION_DUPLICATED', OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE); define('OBJECTFS_REPORT_OBJECT_LOCATION', 0); define('OBJECTFS_REPORT_LOG_SIZE', 1); diff --git a/missing_files.php b/missing_files.php index 1c22ff90..0436add3 100644 --- a/missing_files.php +++ b/missing_files.php @@ -37,7 +37,7 @@ $PAGE->set_context(context_system::instance()); $PAGE->set_cacheable(false); $output = $PAGE->get_renderer('tool_objectfs'); -$table = new files_table('missing-files', OBJECT_LOCATION_ERROR); +$table = new files_table('missing-files', OBJECT_LOCATION_MISSING); $table->define_baseurl('/admin/tool/objectfs/missing_files.php'); if ($table->is_downloading($download, get_string('filename:missingfiles', 'tool_objectfs'))) { diff --git a/tests/local/object_manipulator/checker_test.php b/tests/local/object_manipulator/checker_test.php index 3ec59e18..d8fc3f69 100644 --- a/tests/local/object_manipulator/checker_test.php +++ b/tests/local/object_manipulator/checker_test.php @@ -50,24 +50,24 @@ protected function tearDown(): void { public function test_checker_get_location_local_if_object_is_local(): void { global $DB; $file = $this->create_local_object(); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $file->contenthash]); - $this->assertEquals('string', gettype($location)); + $location = manager::get_location_by_hash($file->contenthash); + $this->assertIsInt($location); $this->assertEquals(OBJECT_LOCATION_LOCAL, $location); } public function test_checker_get_location_duplicated_if_object_is_duplicated(): void { global $DB; $file = $this->create_duplicated_object(); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $file->contenthash]); - $this->assertEquals('string', gettype($location)); + $location = manager::get_location_by_hash($file->contenthash); + $this->assertIsInt($location); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); } public function test_checker_get_location_external_if_object_is_external(): void { global $DB; $file = $this->create_remote_object(); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $file->contenthash]); - $this->assertEquals('string', gettype($location)); + $location = manager::get_location_by_hash($file->contenthash); + $this->assertIsInt($location); $this->assertEquals(OBJECT_LOCATION_EXTERNAL, $location); } @@ -95,9 +95,9 @@ public function test_checker_can_update_object(): void { $localobject->id = null; $DB->delete_records('tool_objectfs_objects', ['contenthash' => $localobject->contenthash]); $this->checker->execute([$localobject]); - $dblocation = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $localobject->contenthash]); + $dblocation = manager::get_location_by_hash($localobject->contenthash); - $this->assertEquals('string', gettype($dblocation)); + $this->assertIsInt($dblocation); $this->assertEquals(OBJECT_LOCATION_LOCAL, $dblocation); self::assertFalse($this->objects_contain_hash($localobject->contenthash)); } @@ -127,6 +127,6 @@ public function test_checker_manipulate_object_method_will_get_error_location_on $file = $this->create_error_object(); $reflection = new \ReflectionMethod(checker::class, "manipulate_object"); $reflection->setAccessible(true); - $this->assertEquals(OBJECT_LOCATION_ERROR, $reflection->invokeArgs($this->checker, [$file])); + $this->assertEquals(OBJECT_LOCATION_MISSING, $reflection->invokeArgs($this->checker, [$file])); } } diff --git a/tests/local/object_manipulator/deleter_test.php b/tests/local/object_manipulator/deleter_test.php index 09fd974f..b95d7088 100644 --- a/tests/local/object_manipulator/deleter_test.php +++ b/tests/local/object_manipulator/deleter_test.php @@ -91,7 +91,7 @@ public function test_deleter_can_delete_object(): void { $this->deleter->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_EXTERNAL, $location); $this->assertFalse($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -103,7 +103,7 @@ public function test_deleter_can_handle_local_object(): void { $this->deleter->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_LOCAL, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertFalse($this->is_externally_readable_by_hash($object->contenthash)); @@ -115,7 +115,7 @@ public function test_deleter_can_handle_remote_object(): void { $this->deleter->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_EXTERNAL, $location); $this->assertFalse($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -128,7 +128,7 @@ public function test_deleter_will_delete_no_objects_if_deletelocal_disabled(): v $this->deleter->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -144,7 +144,7 @@ public function test_deleter_can_delete_multiple_objects(): void { $this->deleter->execute($objects); foreach ($objects as $object) { - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_EXTERNAL, $location); $this->assertFalse($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); diff --git a/tests/local/object_manipulator/orphaner_test.php b/tests/local/object_manipulator/orphaner_test.php index 43e1a17e..1ee000c4 100644 --- a/tests/local/object_manipulator/orphaner_test.php +++ b/tests/local/object_manipulator/orphaner_test.php @@ -71,9 +71,9 @@ public function test_orphaner_can_orphan_files(): void { $object2 = $this->create_duplicated_object(), $object3 = $this->create_remote_object(), ]); - $location1 = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object1->contenthash]); - $location2 = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object2->contenthash]); - $location3 = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object3->contenthash]); + $location1 = manager::get_location_by_hash($object1->contenthash); + $location2 = manager::get_location_by_hash($object2->contenthash); + $location3 = manager::get_location_by_hash($object3->contenthash); $this->assertEquals(OBJECT_LOCATION_ORPHANED, $location1); $this->assertEquals(OBJECT_LOCATION_ORPHANED, $location2); $this->assertEquals(OBJECT_LOCATION_ORPHANED, $location3); @@ -105,7 +105,7 @@ public function test_orphaner_finds_correct_candidates(): void { $this->assertCount(1, $objects); // Ensure it ignores orphaned records during the find. - $DB->set_field('tool_objectfs_objects', 'location', OBJECT_LOCATION_ORPHANED, ['contenthash' => $object->contenthash]); + manager::update_object_by_hash($object->contenthash, OBJECT_LOCATION_ORPHANED); $objects = $finder->get(); $this->assertCount(0, $objects); // No candidates - only candidate has been orphaned. } @@ -117,9 +117,9 @@ public function test_orphaner_correctly_orphans_provided_files(): void { $object2 = $this->create_duplicated_object(), $object3 = $this->create_remote_object(), ]); - $location1 = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object1->contenthash]); - $location2 = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object2->contenthash]); - $location3 = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object3->contenthash]); + $location1 = manager::get_location_by_hash($object1->contenthash); + $location2 = manager::get_location_by_hash($object2->contenthash); + $location3 = manager::get_location_by_hash($object3->contenthash); $this->assertEquals(OBJECT_LOCATION_ORPHANED, $location1); $this->assertEquals(OBJECT_LOCATION_ORPHANED, $location2); $this->assertEquals(OBJECT_LOCATION_ORPHANED, $location3); diff --git a/tests/local/object_manipulator/puller_test.php b/tests/local/object_manipulator/puller_test.php index 1a061563..f055262c 100644 --- a/tests/local/object_manipulator/puller_test.php +++ b/tests/local/object_manipulator/puller_test.php @@ -91,7 +91,7 @@ public function test_puller_can_pull_remote_file(): void { $this->puller->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -103,7 +103,7 @@ public function test_puller_can_handle_duplicated_file(): void { $this->puller->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -115,7 +115,7 @@ public function test_puller_can_handle_local_file(): void { $this->puller->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_LOCAL, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertFalse($this->is_externally_readable_by_hash($object->contenthash)); diff --git a/tests/local/object_manipulator/pusher_test.php b/tests/local/object_manipulator/pusher_test.php index 1cf28e55..d0fb203a 100644 --- a/tests/local/object_manipulator/pusher_test.php +++ b/tests/local/object_manipulator/pusher_test.php @@ -109,7 +109,7 @@ public function test_pusher_can_push_local_file(): void { $this->pusher->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -121,7 +121,7 @@ public function test_pusher_can_handle_duplicated_file(): void { $this->pusher->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -133,7 +133,7 @@ public function test_pusher_can_handle_remote_file(): void { $this->pusher->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_EXTERNAL, $location); $this->assertFalse($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); @@ -149,7 +149,7 @@ public function test_pusher_can_push_multiple_objects(): void { $this->pusher->execute($objects); foreach ($objects as $object) { - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); $this->assertTrue($this->is_locally_readable_by_hash($object->contenthash)); $this->assertTrue($this->is_externally_readable_by_hash($object->contenthash)); diff --git a/tests/local/object_manipulator/recoverer_test.php b/tests/local/object_manipulator/recoverer_test.php index 03abfce0..7be22a44 100644 --- a/tests/local/object_manipulator/recoverer_test.php +++ b/tests/local/object_manipulator/recoverer_test.php @@ -61,44 +61,44 @@ public function test_recoverer_get_candidate_objects_will_get_error_objects(): v public function test_recoverer_will_recover_local_objects(): void { global $DB; $object = $this->create_local_object(); - $DB->set_field('tool_objectfs_objects', 'location', OBJECT_LOCATION_ERROR, ['contenthash' => $object->contenthash]); + manager::update_object_by_hash($object->contenthash, OBJECT_LOCATION_MISSING); $this->recoverer->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_LOCAL, $location); } public function test_recoverer_will_recover_duplicated_objects(): void { global $DB; $object = $this->create_duplicated_object(); - $DB->set_field('tool_objectfs_objects', 'location', OBJECT_LOCATION_ERROR, ['contenthash' => $object->contenthash]); + manager::update_object_by_hash($object->contenthash, OBJECT_LOCATION_MISSING); $this->recoverer->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_DUPLICATED, $location); } public function test_recoverer_will_recover_remote_objects(): void { global $DB; $object = $this->create_remote_object(); - $DB->set_field('tool_objectfs_objects', 'location', OBJECT_LOCATION_ERROR, ['contenthash' => $object->contenthash]); + manager::update_object_by_hash($object->contenthash, OBJECT_LOCATION_MISSING); $this->recoverer->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); + $location = manager::get_location_by_hash($object->contenthash); $this->assertEquals(OBJECT_LOCATION_EXTERNAL, $location); } public function test_recoverer_will_not_recover_error_objects(): void { global $DB; $object = $this->create_error_object(); - $DB->set_field('tool_objectfs_objects', 'location', OBJECT_LOCATION_ERROR, ['contenthash' => $object->contenthash]); + manager::update_object_by_hash($object->contenthash, OBJECT_LOCATION_MISSING); $this->recoverer->execute([$object]); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $object->contenthash]); - $this->assertEquals(OBJECT_LOCATION_ERROR, $location); + $location = manager::get_location_by_hash($object->contenthash); + $this->assertEquals(OBJECT_LOCATION_MISSING, $location); } } diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php index a3629028..fdc62a46 100644 --- a/tests/local/tagging_test.php +++ b/tests/local/tagging_test.php @@ -171,7 +171,9 @@ public function test_gather_object_tags_for_upload_orphaned(): void { $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]); + $columns = \tool_objectfs\local\location_helper::bits_to_columns(OBJECT_LOCATION_ORPHANED); + $columns['id'] = $object->id; + $DB->update_record('tool_objectfs_objects', $columns); $tags = tag_manager::gather_object_tags_for_upload($object->contenthash); diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php index 48d49db8..3cde3f43 100644 --- a/tests/object_file_system_test.php +++ b/tests/object_file_system_test.php @@ -162,7 +162,7 @@ public function test_copy_object_from_external_to_local_by_hash_if_not_local_and $location = $this->filesystem->copy_object_from_external_to_local_by_hash($fakehash); - $this->assertEquals(OBJECT_LOCATION_ERROR, $location); + $this->assertEquals(OBJECT_LOCATION_MISSING, $location); } public function test_copy_object_from_local_to_external_by_hash(): void { @@ -199,7 +199,7 @@ public function test_copy_object_from_local_to_external_by_hash_if_not_local_and $location = $this->filesystem->copy_object_from_local_to_external_by_hash($fakehash); - $this->assertEquals(OBJECT_LOCATION_ERROR, $location); + $this->assertEquals(OBJECT_LOCATION_MISSING, $location); } public function test_delete_object_from_local_by_hash(): void { @@ -229,7 +229,7 @@ public function test_delete_object_from_local_by_hash_if_not_local(): void { $location = $this->filesystem->delete_object_from_local_by_hash($fakehash); - $this->assertEquals(OBJECT_LOCATION_ERROR, $location); + $this->assertEquals(OBJECT_LOCATION_MISSING, $location); } public function test_delete_object_from_local_by_hash_if_can_verify_external_object(): void { @@ -321,8 +321,8 @@ public function test_readfile_updates_object_with_error_location_on_fail(): void $this->filesystem->readfile($fakefile); restore_error_handler(); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $fakefile->get_contenthash()]); - $this->assertEquals(OBJECT_LOCATION_ERROR, $location); + $location = manager::get_location_by_hash($fakefile->get_contenthash()); + $this->assertEquals(OBJECT_LOCATION_MISSING, $location); } public function test_get_content_if_object_is_local(): void { @@ -353,8 +353,8 @@ public function test_get_content_updates_object_with_error_location_on_fail(): v $this->filesystem->get_content($fakefile); restore_error_handler(); - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $fakefile->get_contenthash()]); - $this->assertEquals(OBJECT_LOCATION_ERROR, $location); + $location = manager::get_location_by_hash($fakefile->get_contenthash()); + $this->assertEquals(OBJECT_LOCATION_MISSING, $location); } /** @@ -426,8 +426,8 @@ public function test_get_content_file_handle_does_not_set_error_location_on_tran $this->assertStringContainsString('Failed to open', $e->getMessage()); } - $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $file->get_contenthash()]); - $this->assertNotEquals(OBJECT_LOCATION_ERROR, $location); + $location = manager::get_location_by_hash($file->get_contenthash()); + $this->assertNotEquals(OBJECT_LOCATION_MISSING, $location); } public function test_remove_file_will_remove_local_file(): void { diff --git a/tests/task/populate_objects_filesize_test.php b/tests/task/populate_objects_filesize_test.php index b7083dfd..b5fe840a 100644 --- a/tests/task/populate_objects_filesize_test.php +++ b/tests/task/populate_objects_filesize_test.php @@ -16,6 +16,8 @@ namespace tool_objectfs\task; +use tool_objectfs\local\manager; + /** * Test adhoc-task populate_objects_filesize. * @@ -198,7 +200,7 @@ public function test_orphaned_objects_are_not_updated(): void { $DB->set_field('tool_objectfs_objects', 'filesize', null); // Set first object to be orphaned. - $DB->set_field('tool_objectfs_objects', 'location', -2, ['contenthash' => $filehashes[0]]); + manager::update_object_by_hash($filehashes[0], OBJECT_LOCATION_ORPHANED); // Call ad-hoc task to populate filesizes. $task = new \tool_objectfs\task\populate_objects_filesize(); @@ -232,8 +234,8 @@ public function test_objects_with_error_are_not_updated(): void { // Set all objects to have a filesize of null. $DB->set_field('tool_objectfs_objects', 'filesize', null); - // Set first object to be orphaned. - $DB->set_field('tool_objectfs_objects', 'location', -1, ['contenthash' => $file1->get_contenthash()]); + // Set first object to have error/missing state. + manager::update_object_by_hash($file1->get_contenthash(), OBJECT_LOCATION_MISSING); // Call ad-hoc task to populate filesizes. $task = new \tool_objectfs\task\populate_objects_filesize(); diff --git a/tests/task/task_reconcile_filedir_test.php b/tests/task/task_reconcile_filedir_test.php index e06822b3..cb89f9dc 100644 --- a/tests/task/task_reconcile_filedir_test.php +++ b/tests/task/task_reconcile_filedir_test.php @@ -16,6 +16,7 @@ namespace tool_objectfs\task; +use tool_objectfs\local\manager; use tool_objectfs\tests\testcase; use tool_objectfs\task\reconcile_filedir; @@ -57,12 +58,7 @@ public function test_reconcile_filedir_behaviours(): void { // 4. File marked EXTERNAL but exists locally, should update to DUPLICATED. $remotefile = $this->create_local_file('remote content'); - $DB->set_field( - 'tool_objectfs_objects', - 'location', - OBJECT_LOCATION_EXTERNAL, - ['contenthash' => $remotefile->get_contenthash()] - ); + manager::update_object_by_hash($remotefile->get_contenthash(), OBJECT_LOCATION_EXTERNAL); // Execute scheduled task. ob_start(); @@ -99,15 +95,9 @@ public function test_reconcile_filedir_behaviours(): void { ); // 4. External file should now be marked as DUPLICATED. - $updatedobject = $DB->get_record( - 'tool_objectfs_objects', - ['contenthash' => $remotefile->get_contenthash()], - '*', - MUST_EXIST - ); $this->assertEquals( OBJECT_LOCATION_DUPLICATED, - $updatedobject->location, + manager::get_location_by_hash($remotefile->get_contenthash()), 'External file should now be marked as DUPLICATED.' ); diff --git a/version.php b/version.php index 3f1c91e1..4ad26fc3 100644 --- a/version.php +++ b/version.php @@ -25,8 +25,8 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2026041006; // The current plugin version (Date: YYYYMMDDXX). -$plugin->release = 2026041006; // Same as version. +$plugin->version = 2026041007; // The current plugin version (Date: YYYYMMDDXX). +$plugin->release = 2026041007; // Same as version. $plugin->requires = 2024042200; // Requires 4.4. $plugin->component = "tool_objectfs"; $plugin->maturity = MATURITY_STABLE; From 22a1affb2a94c9cd3fa61aec85fad33ed21dcda0 Mon Sep 17 00:00:00 2001 From: Peter Sistrom Date: Mon, 20 Jul 2026 15:09:39 +1000 Subject: [PATCH 2/4] Issue #752: Pass known locations bitmask to get_object_location_from_hash() --- classes/local/manager.php | 7 +++-- .../candidates/bitmask_candidates.php | 9 ++++--- .../candidates/checker_candidates.php | 11 +++++--- .../local/object_manipulator/recoverer.php | 4 ++- .../local/report/location_report_builder.php | 2 +- classes/local/store/object_file_system.php | 26 +++++++++++++------ lang/en/tool_objectfs.php | 8 +++--- tests/task/populate_objects_filesize_test.php | 12 ++++++--- 8 files changed, 51 insertions(+), 28 deletions(-) diff --git a/classes/local/manager.php b/classes/local/manager.php index f37d9838..069030c0 100644 --- a/classes/local/manager.php +++ b/classes/local/manager.php @@ -235,8 +235,11 @@ public static function upsert_object(stdClass $object, $newlocation) { */ public static function get_location_by_hash(string $contenthash): int { global $DB; - $record = $DB->get_record('tool_objectfs_objects', ['contenthash' => $contenthash], - 'in_filedir, in_mdl_files, in_remote'); + $record = $DB->get_record( + 'tool_objectfs_objects', + ['contenthash' => $contenthash], + 'in_filedir, in_mdl_files, in_remote' + ); if (!$record) { return OBJECT_LOCATION_ERROR; } diff --git a/classes/local/object_manipulator/candidates/bitmask_candidates.php b/classes/local/object_manipulator/candidates/bitmask_candidates.php index 84d4a400..6c76871e 100644 --- a/classes/local/object_manipulator/candidates/bitmask_candidates.php +++ b/classes/local/object_manipulator/candidates/bitmask_candidates.php @@ -32,11 +32,11 @@ use tool_objectfs\local\location_helper; /** - * Universal candidate finder using bitmask filters on the location column. + * Universal candidate finder using bitmask filters on the location bit columns. * * Accepts two bitmasks: - * - "has" mask: bits the file MUST have (location & has_mask = has_mask) - * - "not" mask: bits the file must NOT have (location & not_mask = 0) + * - "has" mask: bits the file MUST have (in_filedir/in_mdl_files/in_remote column = 1) + * - "not" mask: bits the file must NOT have (column = 0) * * Optional filters for filesize and timeduplicated are applied when provided. */ @@ -94,7 +94,8 @@ public function get() { global $DB; // Convert bitmasks to column-based SQL conditions. - $locationconditions = location_helper::bits_to_sql_conditions($this->hasmask, $this->notmask); + // Fallback so the WHERE clause remains valid when no location filtering is requested. + $locationconditions = location_helper::bits_to_sql_conditions($this->hasmask, $this->notmask) ?: '1=1'; $conditions = [$locationconditions]; $params = []; diff --git a/classes/local/object_manipulator/candidates/checker_candidates.php b/classes/local/object_manipulator/candidates/checker_candidates.php index d390482d..2fa5f222 100644 --- a/classes/local/object_manipulator/candidates/checker_candidates.php +++ b/classes/local/object_manipulator/candidates/checker_candidates.php @@ -54,18 +54,21 @@ public function get_query_name() { } /** - * Get files that exist in {files} but have no tracking row in {tool_objectfs_objects}. + * Get files that exist in {files} but either have no tracking row in {tool_objectfs_objects} + * or have an ERROR state (all location bits zero), which includes rows migrated from a NULL + * location in the previous schema. * * @return array */ public function get() { global $DB; - $sql = 'SELECT f.contenthash + $errorconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ERROR, 'o'); + $sql = "SELECT f.contenthash FROM {files} f LEFT JOIN {tool_objectfs_objects} o ON f.contenthash = o.contenthash WHERE f.filesize > 0 - AND o.id IS NULL - GROUP BY f.contenthash'; + AND (o.id IS NULL OR ({$errorconditions})) + GROUP BY f.contenthash"; return $DB->get_records_sql($sql, [], 0, $this->config->batchsize); } } diff --git a/classes/local/object_manipulator/recoverer.php b/classes/local/object_manipulator/recoverer.php index cceb6a59..93c2d2fe 100644 --- a/classes/local/object_manipulator/recoverer.php +++ b/classes/local/object_manipulator/recoverer.php @@ -37,6 +37,8 @@ class recoverer extends manipulator { * @return int */ public function manipulate_object(stdClass $objectrecord) { - return $this->filesystem->get_object_location_from_hash($objectrecord->contenthash); + // The recoverer only knows the object is in the objectfs table, not necessarily + // in mdl_files, so pass 0 to check all location bits fresh. + return $this->filesystem->get_object_location_from_hash($objectrecord->contenthash, 0); } } diff --git a/classes/local/report/location_report_builder.php b/classes/local/report/location_report_builder.php index 8d96918e..8bc50063 100644 --- a/classes/local/report/location_report_builder.php +++ b/classes/local/report/location_report_builder.php @@ -78,7 +78,7 @@ public function build_report($reportid) { $sql = "WITH cte_objects AS ( - SELECT o.contenthash + SELECT o.contenthash, o.in_filedir, o.in_mdl_files, o.in_remote FROM {tool_objectfs_objects} o ), cte_obj_files AS ( SELECT f.contenthash, MAX(f.filesize) AS filesize diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index d5381d07..0a18f94a 100644 --- a/classes/local/store/object_file_system.php +++ b/classes/local/store/object_file_system.php @@ -306,17 +306,27 @@ public function is_file_readable_externally_by_hash($contenthash) { /** * get_object_location_from_hash * @param mixed $contenthash - * + * @param int $knownlocations Bitmask of locations already known to be present without re-checking. + * Defaults to OBJECT_LOCATION_IN_MDL_FILES since most callers reach + * this function via a stored_file or mdl_files reference. + * Pass 0 when the caller only knows the object is in the objectfs + * table (e.g. the recoverer), so all bits are checked fresh. * @return int */ - public function get_object_location_from_hash($contenthash) { - // The mdl_files bit is always set because this function is only called - // for objects known to be referenced in the files table. - $location = OBJECT_LOCATION_IN_MDL_FILES; - if ($this->is_file_readable_locally_by_hash($contenthash)) { + public function get_object_location_from_hash($contenthash, $knownlocations = OBJECT_LOCATION_IN_MDL_FILES) { + $location = $knownlocations; + + // Only check each location if not already known — avoids redundant DB/filesystem calls. + if (!($location & OBJECT_LOCATION_IN_MDL_FILES)) { + global $DB; + if ($DB->record_exists('files', ['contenthash' => $contenthash])) { + $location |= OBJECT_LOCATION_IN_MDL_FILES; + } + } + if (!($location & OBJECT_LOCATION_IN_FILEDIR) && $this->is_file_readable_locally_by_hash($contenthash)) { $location |= OBJECT_LOCATION_IN_FILEDIR; } - if ($this->is_file_readable_externally_by_hash($contenthash)) { + if (!($location & OBJECT_LOCATION_IN_REMOTE) && $this->is_file_readable_externally_by_hash($contenthash)) { $location |= OBJECT_LOCATION_IN_REMOTE; } @@ -1320,6 +1330,6 @@ private function can_set_object_tags(string $contenthash): bool { private function is_file_stored_externally_by_hash(string $contenthash): bool { global $DB; $record = $DB->get_record('tool_objectfs_objects', ['contenthash' => $contenthash], 'in_remote'); - return !empty($record->in_remote); + return $record && !empty($record->in_remote); } } diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 816667d2..f3849645 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -72,7 +72,6 @@ $string['object_status:location:duplicatedsize'] = 'Duplicated (size)'; $string['object_status:location:duplicatedsizechart'] = 'Duplicated (MB)'; $string['object_status:location:error'] = 'Missing from filedir and external storage (view files)'; -$string['object_status:location:missing'] = 'Missing from filedir and external storage (view files)'; $string['object_status:location:external'] = 'Only in external storage'; $string['object_status:location:externalcount'] = 'External (count)'; $string['object_status:location:externalsize'] = 'External (size)'; @@ -84,9 +83,10 @@ $string['object_status:location:localcount'] = 'Local (count)'; $string['object_status:location:localsize'] = 'Local (size)'; $string['object_status:location:localsizechart'] = 'Local (MB)'; -$string['object_status:location:missingcount'] = 'Error (count)'; -$string['object_status:location:missingsize'] = 'Error (size)'; -$string['object_status:location:missingsizechart'] = 'Error (MB)'; +$string['object_status:location:missing'] = 'Missing from filedir and external storage (view files)'; +$string['object_status:location:missingcount'] = 'Missing (count)'; +$string['object_status:location:missingsize'] = 'Missing (size)'; +$string['object_status:location:missingsizechart'] = 'Missing (MB)'; $string['object_status:location:orphaned'] = 'Marked as orphaned (not in the {files} table)'; $string['object_status:location:orphanedcount'] = 'Orphaned (count)'; $string['object_status:location:orphanedsize'] = 'Orphaned (size)'; diff --git a/tests/task/populate_objects_filesize_test.php b/tests/task/populate_objects_filesize_test.php index b5fe840a..1f86d21e 100644 --- a/tests/task/populate_objects_filesize_test.php +++ b/tests/task/populate_objects_filesize_test.php @@ -199,8 +199,10 @@ public function test_orphaned_objects_are_not_updated(): void { // Set all objects to have a filesize of null. $DB->set_field('tool_objectfs_objects', 'filesize', null); - // Set first object to be orphaned. - manager::update_object_by_hash($filehashes[0], OBJECT_LOCATION_ORPHANED); + // Set first object to be orphaned (direct column write to preserve NULL filesize). + $columns = \tool_objectfs\local\location_helper::bits_to_columns(OBJECT_LOCATION_ORPHANED); + $columns['id'] = $DB->get_field('tool_objectfs_objects', 'id', ['contenthash' => $filehashes[0]]); + $DB->update_record('tool_objectfs_objects', $columns); // Call ad-hoc task to populate filesizes. $task = new \tool_objectfs\task\populate_objects_filesize(); @@ -234,8 +236,10 @@ public function test_objects_with_error_are_not_updated(): void { // Set all objects to have a filesize of null. $DB->set_field('tool_objectfs_objects', 'filesize', null); - // Set first object to have error/missing state. - manager::update_object_by_hash($file1->get_contenthash(), OBJECT_LOCATION_MISSING); + // Set first object to have error/missing state (direct column write to preserve NULL filesize). + $columns = \tool_objectfs\local\location_helper::bits_to_columns(OBJECT_LOCATION_MISSING); + $columns['id'] = $DB->get_field('tool_objectfs_objects', 'id', ['contenthash' => $file1->get_contenthash()]); + $DB->update_record('tool_objectfs_objects', $columns); // Call ad-hoc task to populate filesizes. $task = new \tool_objectfs\task\populate_objects_filesize(); From 17f84537942ddb0a60933c2b4b19fc4dd9243b33 Mon Sep 17 00:00:00 2001 From: Peter Sistrom Date: Tue, 21 Jul 2026 13:21:09 +1000 Subject: [PATCH 3/4] Replace candidate classes with unified bitmask_candidates --- .../candidates/bitmask_candidates.php | 17 +-- .../candidates/candidates_factory.php | 108 ++++++++++++++++-- .../candidates/checker_candidates.php | 25 +--- .../candidates/manipulator_candidates.php | 3 +- .../manipulator_candidates_base.php | 27 ----- .../candidates/orphaner_candidates.php | 32 ++---- 6 files changed, 110 insertions(+), 102 deletions(-) diff --git a/classes/local/object_manipulator/candidates/bitmask_candidates.php b/classes/local/object_manipulator/candidates/bitmask_candidates.php index 6c76871e..51648fa4 100644 --- a/classes/local/object_manipulator/candidates/bitmask_candidates.php +++ b/classes/local/object_manipulator/candidates/bitmask_candidates.php @@ -40,13 +40,10 @@ * * Optional filters for filesize and timeduplicated are applied when provided. */ -class bitmask_candidates implements manipulator_candidates { +class bitmask_candidates extends manipulator_candidates_base { /** @var string Query name for logging. */ protected $queryname; - /** @var stdClass Plugin config. */ - protected $config; - /** @var int Bits that must be set in location. */ private $hasmask; @@ -70,21 +67,13 @@ class bitmask_candidates implements manipulator_candidates { * 'maxage' => int - timeduplicated <= maxage (timestamp threshold) */ public function __construct(stdClass $config, int $hasmask, int $notmask, string $queryname, array $options = []) { - $this->config = $config; + parent::__construct($config); $this->hasmask = $hasmask; $this->notmask = $notmask; $this->queryname = $queryname; $this->options = $options; } - /** - * get_query_name - * @return string - */ - public function get_query_name() { - return $this->queryname; - } - /** * Get candidate objects matching the bitmask filters. * @@ -94,7 +83,7 @@ public function get() { global $DB; // Convert bitmasks to column-based SQL conditions. - // Fallback so the WHERE clause remains valid when no location filtering is requested. + // Fall back to a tautology so the WHERE clause remains valid when no location filtering is requested. $locationconditions = location_helper::bits_to_sql_conditions($this->hasmask, $this->notmask) ?: '1=1'; $conditions = [$locationconditions]; diff --git a/classes/local/object_manipulator/candidates/candidates_factory.php b/classes/local/object_manipulator/candidates/candidates_factory.php index f1479aaf..3500e561 100644 --- a/classes/local/object_manipulator/candidates/candidates_factory.php +++ b/classes/local/object_manipulator/candidates/candidates_factory.php @@ -35,31 +35,115 @@ /** * Candidates Factory + * + * Maps manipulator classes to candidate finders. Uses bitmask_candidates for + * manipulators whose candidates are determined by location bitmask filters. */ class candidates_factory { - /** @var array $manipulatormap */ - private static $manipulatormap = [ + /** + * Manipulators that use the legacy class-based mapping (non-bitmask queries). + * @var array + */ + private static $legacymap = [ checker::class => checker_candidates::class, - deleter::class => deleter_candidates::class, - puller::class => puller_candidates::class, - pusher::class => pusher_candidates::class, - recoverer::class => recoverer_candidates::class, orphaner::class => orphaner_candidates::class, ]; /** - * Finder - * @param mixed $manipulator - * @param stdClass $config + * Get the bitmask map for manipulators. + * Must be a method rather than a static property because the OBJECT_LOCATION_* + * constants are defined at runtime via define(). + * + * @return array + */ + private static function get_bitmask_map(): array { + return [ + pusher::class => [ + 'has_mask' => OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES, // Must be local and referenced. + 'not_mask' => OBJECT_LOCATION_IN_REMOTE, // Must not be in remote. + 'queryname' => 'get_push_candidates', + ], + puller::class => [ + 'has_mask' => OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE, // Must be referenced and in remote. + 'not_mask' => OBJECT_LOCATION_IN_FILEDIR, // Must not be local. + 'queryname' => 'get_pull_candidates', + ], + deleter::class => [ + 'has_mask' => OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE, + 'not_mask' => 0, // All bits set, nothing excluded. + 'queryname' => 'get_delete_candidates', + ], + recoverer::class => [ + 'has_mask' => OBJECT_LOCATION_IN_MDL_FILES, // Must be referenced. + 'not_mask' => OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_REMOTE, // Must not be local or remote. + 'queryname' => 'get_recover_candidates', + ], + ]; + } + + /** + * Create a candidate finder for the given manipulator. * - * @return mixed + * @param string $manipulator Manipulator class name. + * @param stdClass $config Plugin config. + * @return manipulator_candidates * @throws moodle_exception */ public static function finder($manipulator, stdClass $config) { - if (isset(self::$manipulatormap[$manipulator])) { - $classname = self::$manipulatormap[$manipulator]; + // Legacy candidates (checker, orphaner) use non-bitmask SQL. + if (isset(self::$legacymap[$manipulator])) { + $classname = self::$legacymap[$manipulator]; return new $classname($config); } + + // Bitmask-based candidates. + $bitmaskmap = self::get_bitmask_map(); + if (isset($bitmaskmap[$manipulator])) { + $entry = $bitmaskmap[$manipulator]; + $options = self::get_options_for_manipulator($manipulator, $config); + return new bitmask_candidates( + $config, + $entry['has_mask'], + $entry['not_mask'], + $entry['queryname'], + $options + ); + } + throw new moodle_exception('invalidclass', 'error', '', 'Invalid manipulator class'); } + + /** + * Build filter options for a bitmask manipulator based on config. + * + * @param string $manipulator Manipulator class name. + * @param stdClass $config Plugin config. + * @return array + */ + private static function get_options_for_manipulator(string $manipulator, stdClass $config): array { + switch ($manipulator) { + case pusher::class: + $filesystem = new $config->filesystem(); + return [ + 'threshold' => $config->sizethreshold, + 'max_filesize' => $filesystem->get_maximum_upload_filesize(), + 'maxage' => time() - $config->minimumage, + ]; + + case puller::class: + return [ + 'size_ceiling' => $config->sizethreshold, + ]; + + case deleter::class: + return [ + 'threshold' => $config->sizethreshold, + 'maxage' => time() - $config->consistencydelay, + ]; + + case recoverer::class: + default: + return []; + } + } } diff --git a/classes/local/object_manipulator/candidates/checker_candidates.php b/classes/local/object_manipulator/candidates/checker_candidates.php index 2fa5f222..34cf8170 100644 --- a/classes/local/object_manipulator/candidates/checker_candidates.php +++ b/classes/local/object_manipulator/candidates/checker_candidates.php @@ -27,36 +27,15 @@ /** * chcker_candiates */ -class checker_candidates implements manipulator_candidates { +class checker_candidates extends manipulator_candidates_base { /** * queryname * @var string */ protected $queryname = 'get_check_candidates'; - /** @var \stdClass $config */ - protected $config; - - /** - * checker_candidates constructor. - * @param \stdClass $config - */ - public function __construct(\stdClass $config) { - $this->config = $config; - } - - /** - * get_query_name - * @return string - */ - public function get_query_name() { - return $this->queryname; - } - /** - * Get files that exist in {files} but either have no tracking row in {tool_objectfs_objects} - * or have an ERROR state (all location bits zero), which includes rows migrated from a NULL - * location in the previous schema. + * Get files that exist in {files} but have no tracking row in {tool_objectfs_objects}. * * @return array */ diff --git a/classes/local/object_manipulator/candidates/manipulator_candidates.php b/classes/local/object_manipulator/candidates/manipulator_candidates.php index 99943cb3..679e7e30 100644 --- a/classes/local/object_manipulator/candidates/manipulator_candidates.php +++ b/classes/local/object_manipulator/candidates/manipulator_candidates.php @@ -34,7 +34,8 @@ interface manipulator_candidates { public function get_query_name(); /** - * get + * Get candidate objects for manipulation. + * * @return array * @throws dml_exception */ diff --git a/classes/local/object_manipulator/candidates/manipulator_candidates_base.php b/classes/local/object_manipulator/candidates/manipulator_candidates_base.php index 2a038633..87a31819 100644 --- a/classes/local/object_manipulator/candidates/manipulator_candidates_base.php +++ b/classes/local/object_manipulator/candidates/manipulator_candidates_base.php @@ -49,31 +49,4 @@ public function __construct(stdClass $config) { public function get_query_name() { return $this->queryname; } - - /** - * get - * @return array - * @throws dml_exception - */ - public function get() { - global $DB; - return $DB->get_records_sql( - $this->get_candidates_sql(), - $this->get_candidates_sql_params(), - 0, - $this->config->batchsize - ); - } - - /** - * Returns SQL to retrieve objects for manipulation. - * @return string - */ - abstract protected function get_candidates_sql(): string; - - /** - * Returns parameters for the SQL from get_candidates_sql. - * @return array - */ - abstract protected function get_candidates_sql_params(): array; } diff --git a/classes/local/object_manipulator/candidates/orphaner_candidates.php b/classes/local/object_manipulator/candidates/orphaner_candidates.php index 3e9ec167..ac3c83e2 100644 --- a/classes/local/object_manipulator/candidates/orphaner_candidates.php +++ b/classes/local/object_manipulator/candidates/orphaner_candidates.php @@ -27,45 +27,27 @@ /** * orphaner_candidates */ -class orphaner_candidates implements manipulator_candidates { +class orphaner_candidates extends manipulator_candidates_base { /** * queryname * @var string */ protected $queryname = 'get_orphan_candidates'; - /** @var \stdClass $config */ - protected $config; - - /** - * orphaner_candidates constructor. - * @param \stdClass $config - */ - public function __construct(\stdClass $config) { - $this->config = $config; - } - - /** - * get_query_name - * @return string - */ - public function get_query_name() { - return $this->queryname; - } - /** * Get tracked objects that no longer have a reference in {files}. - * Excludes objects already marked as orphaned (in_filedir=1, in_mdl_files=0, in_remote=0). * * @return array */ public function get() { global $DB; - $sql = 'SELECT o.id, o.contenthash, o.in_filedir, o.in_mdl_files, o.in_remote + $notorphaned = 'NOT (' . \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ORPHANED, 'o') . ')'; + $sql = "SELECT o.id, o.contenthash FROM {tool_objectfs_objects} o LEFT JOIN {files} f ON o.contenthash = f.contenthash - WHERE f.id IS NULL - AND NOT (o.in_filedir = 1 AND o.in_mdl_files = 0 AND o.in_remote = 0)'; - return $DB->get_records_sql($sql, [], 0, $this->config->batchsize); + WHERE f.id is null + AND {$notorphaned}"; + $params = []; + return $DB->get_records_sql($sql, $params, 0, $this->config->batchsize); } } From a7ee870f854f3f6a83aa9372b1ae83d6845801c1 Mon Sep 17 00:00:00 2001 From: Peter Sistrom Date: Mon, 10 Aug 2026 12:26:35 +1000 Subject: [PATCH 4/4] Delete candidate classes --- .../candidates/bitmask_candidates.php | 117 -------------- .../candidates/candidates_factory.php | 149 ------------------ .../candidates/candidates_finder.php | 62 -------- .../candidates/checker_candidates.php | 53 ------- .../candidates/deleter_candidates.php | 62 -------- .../candidates/manipulator_candidates.php | 43 ----- .../manipulator_candidates_base.php | 52 ------ .../candidates/orphaner_candidates.php | 53 ------- .../candidates/puller_candidates.php | 57 ------- .../candidates/pusher_candidates.php | 64 -------- .../candidates/recoverer_candidates.php | 56 ------- classes/local/object_manipulator/checker.php | 26 +++ classes/local/object_manipulator/deleter.php | 32 ++++ .../manipulator_builder.php | 9 +- classes/local/object_manipulator/orphaner.php | 25 +++ classes/local/object_manipulator/puller.php | 27 ++++ classes/local/object_manipulator/pusher.php | 35 ++++ .../local/object_manipulator/recoverer.php | 26 +++ classes/tests/testcase.php | 4 +- .../object_manipulator/orphaner_test.php | 10 +- .../local/object_manipulator/pusher_test.php | 6 +- .../object_manipulator/recoverer_test.php | 7 +- 22 files changed, 181 insertions(+), 794 deletions(-) delete mode 100644 classes/local/object_manipulator/candidates/bitmask_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/candidates_factory.php delete mode 100644 classes/local/object_manipulator/candidates/candidates_finder.php delete mode 100644 classes/local/object_manipulator/candidates/checker_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/deleter_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/manipulator_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/manipulator_candidates_base.php delete mode 100644 classes/local/object_manipulator/candidates/orphaner_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/puller_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/pusher_candidates.php delete mode 100644 classes/local/object_manipulator/candidates/recoverer_candidates.php diff --git a/classes/local/object_manipulator/candidates/bitmask_candidates.php b/classes/local/object_manipulator/candidates/bitmask_candidates.php deleted file mode 100644 index 51648fa4..00000000 --- a/classes/local/object_manipulator/candidates/bitmask_candidates.php +++ /dev/null @@ -1,117 +0,0 @@ -. - -/** - * Unified bitmask-based candidate class for object manipulation. - * - * Replaces separate pusher_candidates, puller_candidates, deleter_candidates, - * and recoverer_candidates with a single parameterized class. - * - * @package tool_objectfs - * @author Catalyst IT - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -use stdClass; -use tool_objectfs\local\location_helper; - -/** - * Universal candidate finder using bitmask filters on the location bit columns. - * - * Accepts two bitmasks: - * - "has" mask: bits the file MUST have (in_filedir/in_mdl_files/in_remote column = 1) - * - "not" mask: bits the file must NOT have (column = 0) - * - * Optional filters for filesize and timeduplicated are applied when provided. - */ -class bitmask_candidates extends manipulator_candidates_base { - /** @var string Query name for logging. */ - protected $queryname; - - /** @var int Bits that must be set in location. */ - private $hasmask; - - /** @var int Bits that must NOT be set in location. */ - private $notmask; - - /** @var array Optional filter options. */ - private $options; - - /** - * Constructor. - * - * @param stdClass $config Plugin config (must include batchsize). - * @param int $hasmask Bits the location MUST have. - * @param int $notmask Bits the location must NOT have. - * @param string $queryname Name for logging. - * @param array $options Optional filters: - * 'threshold' => int - filesize > threshold (minimum file size) - * 'max_filesize' => int - filesize < max_filesize (maximum file size) - * 'size_ceiling' => int - filesize <= size_ceiling (upper file size bound) - * 'maxage' => int - timeduplicated <= maxage (timestamp threshold) - */ - public function __construct(stdClass $config, int $hasmask, int $notmask, string $queryname, array $options = []) { - parent::__construct($config); - $this->hasmask = $hasmask; - $this->notmask = $notmask; - $this->queryname = $queryname; - $this->options = $options; - } - - /** - * Get candidate objects matching the bitmask filters. - * - * @return array - */ - public function get() { - global $DB; - - // Convert bitmasks to column-based SQL conditions. - // Fall back to a tautology so the WHERE clause remains valid when no location filtering is requested. - $locationconditions = location_helper::bits_to_sql_conditions($this->hasmask, $this->notmask) ?: '1=1'; - - $conditions = [$locationconditions]; - $params = []; - - if (isset($this->options['threshold'])) { - $conditions[] = 'filesize > :threshold'; - $params['threshold'] = $this->options['threshold']; - } - if (isset($this->options['max_filesize'])) { - $conditions[] = 'filesize < :max_filesize'; - $params['max_filesize'] = $this->options['max_filesize']; - } - if (isset($this->options['size_ceiling'])) { - $conditions[] = 'filesize <= :size_ceiling'; - $params['size_ceiling'] = $this->options['size_ceiling']; - } - if (isset($this->options['maxage'])) { - $conditions[] = 'timeduplicated <= :maxage'; - $params['maxage'] = $this->options['maxage']; - } - - $where = implode("\n AND ", $conditions); - $sql = "SELECT contenthash, - filesize - FROM {tool_objectfs_objects} - WHERE {$where}"; - - return $DB->get_records_sql($sql, $params, 0, $this->config->batchsize); - } -} diff --git a/classes/local/object_manipulator/candidates/candidates_factory.php b/classes/local/object_manipulator/candidates/candidates_factory.php deleted file mode 100644 index 3500e561..00000000 --- a/classes/local/object_manipulator/candidates/candidates_factory.php +++ /dev/null @@ -1,149 +0,0 @@ -. - -/** - * Class candidates_factory - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -use moodle_exception; -use stdClass; -use tool_objectfs\local\object_manipulator\checker; -use tool_objectfs\local\object_manipulator\deleter; -use tool_objectfs\local\object_manipulator\puller; -use tool_objectfs\local\object_manipulator\pusher; -use tool_objectfs\local\object_manipulator\recoverer; -use tool_objectfs\local\object_manipulator\orphaner; - -/** - * Candidates Factory - * - * Maps manipulator classes to candidate finders. Uses bitmask_candidates for - * manipulators whose candidates are determined by location bitmask filters. - */ -class candidates_factory { - /** - * Manipulators that use the legacy class-based mapping (non-bitmask queries). - * @var array - */ - private static $legacymap = [ - checker::class => checker_candidates::class, - orphaner::class => orphaner_candidates::class, - ]; - - /** - * Get the bitmask map for manipulators. - * Must be a method rather than a static property because the OBJECT_LOCATION_* - * constants are defined at runtime via define(). - * - * @return array - */ - private static function get_bitmask_map(): array { - return [ - pusher::class => [ - 'has_mask' => OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES, // Must be local and referenced. - 'not_mask' => OBJECT_LOCATION_IN_REMOTE, // Must not be in remote. - 'queryname' => 'get_push_candidates', - ], - puller::class => [ - 'has_mask' => OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE, // Must be referenced and in remote. - 'not_mask' => OBJECT_LOCATION_IN_FILEDIR, // Must not be local. - 'queryname' => 'get_pull_candidates', - ], - deleter::class => [ - 'has_mask' => OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE, - 'not_mask' => 0, // All bits set, nothing excluded. - 'queryname' => 'get_delete_candidates', - ], - recoverer::class => [ - 'has_mask' => OBJECT_LOCATION_IN_MDL_FILES, // Must be referenced. - 'not_mask' => OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_REMOTE, // Must not be local or remote. - 'queryname' => 'get_recover_candidates', - ], - ]; - } - - /** - * Create a candidate finder for the given manipulator. - * - * @param string $manipulator Manipulator class name. - * @param stdClass $config Plugin config. - * @return manipulator_candidates - * @throws moodle_exception - */ - public static function finder($manipulator, stdClass $config) { - // Legacy candidates (checker, orphaner) use non-bitmask SQL. - if (isset(self::$legacymap[$manipulator])) { - $classname = self::$legacymap[$manipulator]; - return new $classname($config); - } - - // Bitmask-based candidates. - $bitmaskmap = self::get_bitmask_map(); - if (isset($bitmaskmap[$manipulator])) { - $entry = $bitmaskmap[$manipulator]; - $options = self::get_options_for_manipulator($manipulator, $config); - return new bitmask_candidates( - $config, - $entry['has_mask'], - $entry['not_mask'], - $entry['queryname'], - $options - ); - } - - throw new moodle_exception('invalidclass', 'error', '', 'Invalid manipulator class'); - } - - /** - * Build filter options for a bitmask manipulator based on config. - * - * @param string $manipulator Manipulator class name. - * @param stdClass $config Plugin config. - * @return array - */ - private static function get_options_for_manipulator(string $manipulator, stdClass $config): array { - switch ($manipulator) { - case pusher::class: - $filesystem = new $config->filesystem(); - return [ - 'threshold' => $config->sizethreshold, - 'max_filesize' => $filesystem->get_maximum_upload_filesize(), - 'maxage' => time() - $config->minimumage, - ]; - - case puller::class: - return [ - 'size_ceiling' => $config->sizethreshold, - ]; - - case deleter::class: - return [ - 'threshold' => $config->sizethreshold, - 'maxage' => time() - $config->consistencydelay, - ]; - - case recoverer::class: - default: - return []; - } - } -} diff --git a/classes/local/object_manipulator/candidates/candidates_finder.php b/classes/local/object_manipulator/candidates/candidates_finder.php deleted file mode 100644 index 17c266f9..00000000 --- a/classes/local/object_manipulator/candidates/candidates_finder.php +++ /dev/null @@ -1,62 +0,0 @@ -. - -/** - * Class candidates_finder - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -use moodle_exception; -use stdClass; - -/** - * Candidates Finder - */ -class candidates_finder { - /** @var string $finder */ - private $finder = ''; - - /** - * candidates_finder constructor. - * @param string $manipulator - * @param stdClass $config - * @throws moodle_exception - */ - public function __construct($manipulator, stdClass $config) { - $this->finder = candidates_factory::finder($manipulator, $config); - } - - /** - * get - * @return array - */ - public function get() { - return $this->finder->get(); - } - - /** - * get_query_name - * @return string - */ - public function get_query_name() { - return $this->finder->get_query_name(); - } -} diff --git a/classes/local/object_manipulator/candidates/checker_candidates.php b/classes/local/object_manipulator/candidates/checker_candidates.php deleted file mode 100644 index 34cf8170..00000000 --- a/classes/local/object_manipulator/candidates/checker_candidates.php +++ /dev/null @@ -1,53 +0,0 @@ -. - -/** - * Class checker_candidates - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -/** - * chcker_candiates - */ -class checker_candidates extends manipulator_candidates_base { - /** - * queryname - * @var string - */ - protected $queryname = 'get_check_candidates'; - - /** - * Get files that exist in {files} but have no tracking row in {tool_objectfs_objects}. - * - * @return array - */ - public function get() { - global $DB; - $errorconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ERROR, 'o'); - $sql = "SELECT f.contenthash - FROM {files} f - LEFT JOIN {tool_objectfs_objects} o ON f.contenthash = o.contenthash - WHERE f.filesize > 0 - AND (o.id IS NULL OR ({$errorconditions})) - GROUP BY f.contenthash"; - return $DB->get_records_sql($sql, [], 0, $this->config->batchsize); - } -} diff --git a/classes/local/object_manipulator/candidates/deleter_candidates.php b/classes/local/object_manipulator/candidates/deleter_candidates.php deleted file mode 100644 index 014604c2..00000000 --- a/classes/local/object_manipulator/candidates/deleter_candidates.php +++ /dev/null @@ -1,62 +0,0 @@ -. - -/** - * Class deleter_candidates - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -/** - * deleter_candidates - */ -class deleter_candidates extends manipulator_candidates_base { - /** - * queryname - * @var string - */ - protected $queryname = 'get_delete_candidates'; - - /** - * get_candiates_sql - * @return string - */ - protected function get_candidates_sql(): string { - $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_DUPLICATED); - return "SELECT contenthash, - filesize - FROM {tool_objectfs_objects} - WHERE timeduplicated <= :consistancythreshold - AND {$locationconditions} - AND filesize > :sizethreshold"; - } - - /** - * get_candiates_sql_params - * @return array - */ - protected function get_candidates_sql_params(): array { - $consistancythreshold = time() - $this->config->consistencydelay; - return [ - 'consistancythreshold' => $consistancythreshold, - 'sizethreshold' => $this->config->sizethreshold, - ]; - } -} diff --git a/classes/local/object_manipulator/candidates/manipulator_candidates.php b/classes/local/object_manipulator/candidates/manipulator_candidates.php deleted file mode 100644 index 679e7e30..00000000 --- a/classes/local/object_manipulator/candidates/manipulator_candidates.php +++ /dev/null @@ -1,43 +0,0 @@ -. - -namespace tool_objectfs\local\object_manipulator\candidates; - -use dml_exception; - -/** - * Interface manipulator_candidates - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -interface manipulator_candidates { - /** - * Returns a manipulator query name for logging. - * - * @return string - */ - public function get_query_name(); - - /** - * Get candidate objects for manipulation. - * - * @return array - * @throws dml_exception - */ - public function get(); -} diff --git a/classes/local/object_manipulator/candidates/manipulator_candidates_base.php b/classes/local/object_manipulator/candidates/manipulator_candidates_base.php deleted file mode 100644 index 87a31819..00000000 --- a/classes/local/object_manipulator/candidates/manipulator_candidates_base.php +++ /dev/null @@ -1,52 +0,0 @@ -. - -/** - * Class candidates_factory - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -use dml_exception; -use stdClass; - -/** - * manipulator_candidates_base - */ -abstract class manipulator_candidates_base implements manipulator_candidates { - /** @var stdClass $config */ - protected $config; - - /** - * manipulator_candidates_base constructor. - * @param stdClass $config - */ - public function __construct(stdClass $config) { - $this->config = $config; - } - - /** - * get_query_name - * @return string - */ - public function get_query_name() { - return $this->queryname; - } -} diff --git a/classes/local/object_manipulator/candidates/orphaner_candidates.php b/classes/local/object_manipulator/candidates/orphaner_candidates.php deleted file mode 100644 index ac3c83e2..00000000 --- a/classes/local/object_manipulator/candidates/orphaner_candidates.php +++ /dev/null @@ -1,53 +0,0 @@ -. - -/** - * Class orphaner_candidates - * @package tool_objectfs - * @author Nathan Mares - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -/** - * orphaner_candidates - */ -class orphaner_candidates extends manipulator_candidates_base { - /** - * queryname - * @var string - */ - protected $queryname = 'get_orphan_candidates'; - - /** - * Get tracked objects that no longer have a reference in {files}. - * - * @return array - */ - public function get() { - global $DB; - $notorphaned = 'NOT (' . \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_ORPHANED, 'o') . ')'; - $sql = "SELECT o.id, o.contenthash - FROM {tool_objectfs_objects} o - LEFT JOIN {files} f ON o.contenthash = f.contenthash - WHERE f.id is null - AND {$notorphaned}"; - $params = []; - return $DB->get_records_sql($sql, $params, 0, $this->config->batchsize); - } -} diff --git a/classes/local/object_manipulator/candidates/puller_candidates.php b/classes/local/object_manipulator/candidates/puller_candidates.php deleted file mode 100644 index 13109654..00000000 --- a/classes/local/object_manipulator/candidates/puller_candidates.php +++ /dev/null @@ -1,57 +0,0 @@ -. - -/** - * Class puller_candidates - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -/** - * puller_candidates - */ -class puller_candidates extends manipulator_candidates_base { - /** - * queryname - * @var string - */ - protected $queryname = 'get_pull_candidates'; - - /** - * get_candidates_sql - * @return string - */ - protected function get_candidates_sql(): string { - $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_EXTERNAL); - return "SELECT contenthash, - filesize - FROM {tool_objectfs_objects} - WHERE filesize <= :sizethreshold - AND {$locationconditions}"; - } - - /** - * get_candidates_sql_params - * @return array - */ - protected function get_candidates_sql_params(): array { - return ['sizethreshold' => $this->config->sizethreshold]; - } -} diff --git a/classes/local/object_manipulator/candidates/pusher_candidates.php b/classes/local/object_manipulator/candidates/pusher_candidates.php deleted file mode 100644 index 20aae41f..00000000 --- a/classes/local/object_manipulator/candidates/pusher_candidates.php +++ /dev/null @@ -1,64 +0,0 @@ -. - -/** - * Class pusher_candidates - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -/** - * pusher_candidates - */ -class pusher_candidates extends manipulator_candidates_base { - /** - * queryname - * @var string - */ - protected $queryname = 'get_push_candidates'; - - /** - * get_candidates_sql - * @return string - */ - protected function get_candidates_sql(): string { - $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_LOCAL); - return "SELECT contenthash, - filesize - FROM {tool_objectfs_objects} - WHERE filesize > :threshold - AND filesize < :maximum_file_size - AND timeduplicated <= :maxcreatedtimestamp - AND {$locationconditions}"; - } - - /** - * get_candidates_sql_params - * @return array - */ - protected function get_candidates_sql_params(): array { - $filesystem = new $this->config->filesystem(); - return [ - 'maxcreatedtimestamp' => time() - $this->config->minimumage, - 'threshold' => $this->config->sizethreshold, - 'maximum_file_size' => $filesystem->get_maximum_upload_filesize(), - ]; - } -} diff --git a/classes/local/object_manipulator/candidates/recoverer_candidates.php b/classes/local/object_manipulator/candidates/recoverer_candidates.php deleted file mode 100644 index f3c5fdc5..00000000 --- a/classes/local/object_manipulator/candidates/recoverer_candidates.php +++ /dev/null @@ -1,56 +0,0 @@ -. - -/** - * Class recoverer_candidates - * @package tool_objectfs - * @author Gleimer Mora - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -namespace tool_objectfs\local\object_manipulator\candidates; - -/** - * recoverer_candidates - */ -class recoverer_candidates extends manipulator_candidates_base { - /** - * queryname - * @var string - */ - protected $queryname = 'get_recover_candidates'; - - /** - * get_candidates_sql - * @return string - */ - protected function get_candidates_sql(): string { - $locationconditions = \tool_objectfs\local\location_helper::bits_to_exact_sql(OBJECT_LOCATION_MISSING); - return "SELECT contenthash, - filesize - FROM {tool_objectfs_objects} - WHERE {$locationconditions}"; - } - - /** - * get_candidates_sql_params - * @return array - */ - protected function get_candidates_sql_params(): array { - return []; - } -} diff --git a/classes/local/object_manipulator/checker.php b/classes/local/object_manipulator/checker.php index 6a39e160..d737abbf 100644 --- a/classes/local/object_manipulator/checker.php +++ b/classes/local/object_manipulator/checker.php @@ -26,6 +26,7 @@ namespace tool_objectfs\local\object_manipulator; use stdClass; +use tool_objectfs\local\location_helper; use tool_objectfs\local\store\object_file_system; use tool_objectfs\log\aggregate_logger; @@ -33,6 +34,31 @@ * checker */ class checker extends manipulator { + /** + * Get query name for logging. + * @return string + */ + public static function get_query_name(): string { + return 'get_check_candidates'; + } + + /** + * Get files that exist in {files} but have no tracking row or are in error state. + * @param stdClass $config Plugin config. + * @return array + */ + public static function get_candidates(stdClass $config): array { + global $DB; + $errorconditions = location_helper::bits_to_exact_sql(OBJECT_LOCATION_ERROR, 'o'); + $sql = "SELECT f.contenthash + FROM {files} f + LEFT JOIN {tool_objectfs_objects} o ON f.contenthash = o.contenthash + WHERE f.filesize > 0 + AND (o.id IS NULL OR ({$errorconditions})) + GROUP BY f.contenthash"; + return $DB->get_records_sql($sql, [], 0, $config->batchsize * 10); + } + /** * Checker constructor. * This manipulator adds location for files that do not have records in {tool_objectfs_objects} table. diff --git a/classes/local/object_manipulator/deleter.php b/classes/local/object_manipulator/deleter.php index 7afa7b74..98432e2d 100644 --- a/classes/local/object_manipulator/deleter.php +++ b/classes/local/object_manipulator/deleter.php @@ -26,6 +26,7 @@ namespace tool_objectfs\local\object_manipulator; use stdClass; +use tool_objectfs\local\location_helper; use tool_objectfs\local\store\object_file_system; use tool_objectfs\log\aggregate_logger; @@ -49,6 +50,37 @@ class deleter extends manipulator { */ private $deletelocal; + /** + * Get query name for logging. + * @return string + */ + public static function get_query_name(): string { + return 'get_delete_candidates'; + } + + /** + * Get candidate objects to delete from local storage. + * @param stdClass $config Plugin config. + * @return array + */ + public static function get_candidates(stdClass $config): array { + global $DB; + $locationconds = location_helper::bits_to_sql_conditions( + OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE, + 0 + ) ?: '1=1'; + $sql = "SELECT contenthash, filesize + FROM {tool_objectfs_objects} + WHERE {$locationconds} + AND filesize > :threshold + AND timeduplicated <= :maxage"; + $params = [ + 'threshold' => $config->sizethreshold, + 'maxage' => time() - $config->consistencydelay, + ]; + return $DB->get_records_sql($sql, $params, 0, $config->batchsize); + } + /** * deleter constructor. * @param object_file_system $filesystem diff --git a/classes/local/object_manipulator/manipulator_builder.php b/classes/local/object_manipulator/manipulator_builder.php index 52745279..13e01d8b 100644 --- a/classes/local/object_manipulator/manipulator_builder.php +++ b/classes/local/object_manipulator/manipulator_builder.php @@ -29,7 +29,6 @@ use moodle_exception; use stdClass; use tool_objectfs\local\manager; -use tool_objectfs\local\object_manipulator\candidates\candidates_finder; use tool_objectfs\log\aggregate_logger; defined('MOODLE_INTERNAL') || die(); @@ -53,9 +52,6 @@ class manipulator_builder { /** @var string $manipulatorclass */ private $manipulatorclass; - /** @var candidates_finder $finder */ - private $finder; - /** @var stdClass $config */ private $config; @@ -107,10 +103,9 @@ private function build($manipulator) { $this->config = manager::get_objectfs_config(); $this->manipulatorclass = $manipulator; $this->logger = new aggregate_logger(); - $this->finder = new candidates_finder($manipulator, $this->config); - $this->candidates = $this->finder->get(); + $this->candidates = $manipulator::get_candidates($this->config); $countcandidates = count($this->candidates); - $this->logger->log_object_query($this->finder->get_query_name(), $countcandidates); + $this->logger->log_object_query($manipulator::get_query_name(), $countcandidates); if ($countcandidates === 0) { mtrace('No candidate objects found.'); } diff --git a/classes/local/object_manipulator/orphaner.php b/classes/local/object_manipulator/orphaner.php index 3576ddc3..cbaccdde 100644 --- a/classes/local/object_manipulator/orphaner.php +++ b/classes/local/object_manipulator/orphaner.php @@ -30,11 +30,36 @@ namespace tool_objectfs\local\object_manipulator; use stdClass; +use tool_objectfs\local\location_helper; /** * orphaner */ class orphaner extends manipulator { + /** + * Get query name for logging. + * @return string + */ + public static function get_query_name(): string { + return 'get_orphan_candidates'; + } + + /** + * Get tracked objects that no longer have a reference in {files}. + * @param stdClass $config Plugin config. + * @return array + */ + public static function get_candidates(stdClass $config): array { + global $DB; + $notorphaned = 'NOT (' . location_helper::bits_to_exact_sql(OBJECT_LOCATION_ORPHANED, 'o') . ')'; + $sql = "SELECT o.id, o.contenthash + FROM {tool_objectfs_objects} o + LEFT JOIN {files} f ON o.contenthash = f.contenthash + WHERE f.id is null + AND {$notorphaned}"; + return $DB->get_records_sql($sql, [], 0, $config->batchsize); + } + /** * Updates the location of {tool_objectfs_objects} records for files that * have been deleted from the core {files} table. diff --git a/classes/local/object_manipulator/puller.php b/classes/local/object_manipulator/puller.php index 6a5c18ea..1e9243ea 100644 --- a/classes/local/object_manipulator/puller.php +++ b/classes/local/object_manipulator/puller.php @@ -26,11 +26,38 @@ namespace tool_objectfs\local\object_manipulator; use stdClass; +use tool_objectfs\local\location_helper; /** * puller */ class puller extends manipulator { + /** + * Get query name for logging. + * @return string + */ + public static function get_query_name(): string { + return 'get_pull_candidates'; + } + + /** + * Get candidate objects to pull from remote storage. + * @param stdClass $config Plugin config. + * @return array + */ + public static function get_candidates(stdClass $config): array { + global $DB; + $locationconds = location_helper::bits_to_sql_conditions( + OBJECT_LOCATION_IN_MDL_FILES | OBJECT_LOCATION_IN_REMOTE, + OBJECT_LOCATION_IN_FILEDIR + ); + $sql = "SELECT contenthash, filesize + FROM {tool_objectfs_objects} + WHERE {$locationconds} + AND filesize <= :size_ceiling"; + return $DB->get_records_sql($sql, ['size_ceiling' => $config->sizethreshold], 0, $config->batchsize); + } + /** * manipulate_object * @param stdClass $objectrecord diff --git a/classes/local/object_manipulator/pusher.php b/classes/local/object_manipulator/pusher.php index fa301971..eb2668d9 100644 --- a/classes/local/object_manipulator/pusher.php +++ b/classes/local/object_manipulator/pusher.php @@ -26,6 +26,7 @@ namespace tool_objectfs\local\object_manipulator; use stdClass; +use tool_objectfs\local\location_helper; use tool_objectfs\local\store\object_file_system; use tool_objectfs\log\aggregate_logger; @@ -47,6 +48,40 @@ class pusher extends manipulator { */ private $maximumfilesize; + /** + * Get query name for logging. + * @return string + */ + public static function get_query_name(): string { + return 'get_push_candidates'; + } + + /** + * Get candidate objects to push to remote storage. + * @param stdClass $config Plugin config. + * @return array + */ + public static function get_candidates(stdClass $config): array { + global $DB; + $filesystem = new $config->filesystem(); + $locationconds = location_helper::bits_to_sql_conditions( + OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_MDL_FILES, + OBJECT_LOCATION_IN_REMOTE + ); + $sql = "SELECT contenthash, filesize + FROM {tool_objectfs_objects} + WHERE {$locationconds} + AND filesize > :threshold + AND filesize < :max_filesize + AND timeduplicated <= :maxage"; + $params = [ + 'threshold' => $config->sizethreshold, + 'max_filesize' => $filesystem->get_maximum_upload_filesize(), + 'maxage' => time() - $config->minimumage, + ]; + return $DB->get_records_sql($sql, $params, 0, $config->batchsize); + } + /** * pusher constructor. * @param object_file_system $filesystem diff --git a/classes/local/object_manipulator/recoverer.php b/classes/local/object_manipulator/recoverer.php index 93c2d2fe..147bd923 100644 --- a/classes/local/object_manipulator/recoverer.php +++ b/classes/local/object_manipulator/recoverer.php @@ -26,11 +26,37 @@ namespace tool_objectfs\local\object_manipulator; use stdClass; +use tool_objectfs\local\location_helper; /** * recoverer */ class recoverer extends manipulator { + /** + * Get query name for logging. + * @return string + */ + public static function get_query_name(): string { + return 'get_recover_candidates'; + } + + /** + * Get candidate objects to recover from error state. + * @param stdClass $config Plugin config. + * @return array + */ + public static function get_candidates(stdClass $config): array { + global $DB; + $locationconds = location_helper::bits_to_sql_conditions( + OBJECT_LOCATION_IN_MDL_FILES, + OBJECT_LOCATION_IN_FILEDIR | OBJECT_LOCATION_IN_REMOTE + ); + $sql = "SELECT contenthash, filesize + FROM {tool_objectfs_objects} + WHERE {$locationconds}"; + return $DB->get_records_sql($sql, [], 0, $config->batchsize); + } + /** * manipulate_object * @param stdClass $objectrecord diff --git a/classes/tests/testcase.php b/classes/tests/testcase.php index df6f92da..382ef848 100644 --- a/classes/tests/testcase.php +++ b/classes/tests/testcase.php @@ -21,7 +21,6 @@ use stdClass; use stored_file; use tool_objectfs\local\manager; -use tool_objectfs\local\object_manipulator\candidates\candidates_finder; use tool_objectfs\local\store\object_file_system; use tool_objectfs\local\store\signed_url; @@ -408,8 +407,7 @@ private function create_object_record(stored_file $file, $location) { protected function objects_contain_hash($contenthash) { $config = manager::get_objectfs_config(); $config->filesystem = get_class($this->filesystem); - $candidatesfinder = new candidates_finder($this->manipulator, $config); - $candidateobjects = $candidatesfinder->get(); + $candidateobjects = $this->manipulator::get_candidates($config); foreach ($candidateobjects as $candidateobject) { if ($contenthash === $candidateobject->contenthash) { return true; diff --git a/tests/local/object_manipulator/orphaner_test.php b/tests/local/object_manipulator/orphaner_test.php index 1ee000c4..de43625c 100644 --- a/tests/local/object_manipulator/orphaner_test.php +++ b/tests/local/object_manipulator/orphaner_test.php @@ -17,7 +17,6 @@ namespace tool_objectfs\local\object_manipulator; use tool_objectfs\local\manager; -use tool_objectfs\local\object_manipulator\candidates\candidates_finder; /** * Tests for object orphaner. @@ -85,15 +84,14 @@ public function test_orphaner_finds_correct_candidates(): void { // Initialise the candidate finder. $config = manager::get_objectfs_config(); $config->filesystem = get_class($this->filesystem); - $finder = new candidates_finder($this->manipulator, $config); - $objects = $finder->get(); + $objects = $this->manipulator::get_candidates($config); $this->assertCount(0, $objects); // No candidates. // Create an object. $object = $this->create_local_object(); // Still no candidates - object created but nothing is missing from {files} table. - $objects = $finder->get(); + $objects = $this->manipulator::get_candidates($config); $this->assertCount(0, $objects); // Update that object to have a different hash, to mock a non-existent @@ -101,12 +99,12 @@ public function test_orphaner_finds_correct_candidates(): void { $DB->set_field('files', 'contenthash', 'different', ['contenthash' => $object->contenthash]); // Expect one candidate - no matching contenthash in {files}. - $objects = $finder->get(); + $objects = $this->manipulator::get_candidates($config); $this->assertCount(1, $objects); // Ensure it ignores orphaned records during the find. manager::update_object_by_hash($object->contenthash, OBJECT_LOCATION_ORPHANED); - $objects = $finder->get(); + $objects = $this->manipulator::get_candidates($config); $this->assertCount(0, $objects); // No candidates - only candidate has been orphaned. } diff --git a/tests/local/object_manipulator/pusher_test.php b/tests/local/object_manipulator/pusher_test.php index d0fb203a..d23d164b 100644 --- a/tests/local/object_manipulator/pusher_test.php +++ b/tests/local/object_manipulator/pusher_test.php @@ -17,7 +17,6 @@ namespace tool_objectfs\local\object_manipulator; use tool_objectfs\local\manager; -use tool_objectfs\local\object_manipulator\candidates\candidates_finder; /** * Tests for object pusher. @@ -161,8 +160,7 @@ public function test_get_candidate_objects_get_one_object_if_files_have_same_has // Push initial objects so they arnt candidates. $config = manager::get_objectfs_config(); $config->filesystem = get_class($this->filesystem); - $finder = new candidates_finder($this->manipulator, $config); - $objects = $finder->get(); + $objects = $this->manipulator::get_candidates($config); $this->pusher->execute($objects); $object = $this->create_local_object(); @@ -173,7 +171,7 @@ public function test_get_candidate_objects_get_one_object_if_files_have_same_has $file->pathnamehash = '1234'; $DB->insert_record('files', $file); - $objects = $finder->get(); + $objects = $this->manipulator::get_candidates($config); $this->assertEquals(1, count($objects)); } diff --git a/tests/local/object_manipulator/recoverer_test.php b/tests/local/object_manipulator/recoverer_test.php index 7be22a44..fb578ddf 100644 --- a/tests/local/object_manipulator/recoverer_test.php +++ b/tests/local/object_manipulator/recoverer_test.php @@ -17,7 +17,6 @@ namespace tool_objectfs\local\object_manipulator; use tool_objectfs\local\manager; -use tool_objectfs\local\object_manipulator\candidates\candidates_finder; /** * Tests for object recoverer. @@ -28,16 +27,12 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ final class recoverer_test extends \tool_objectfs\tests\testcase { - /** @var candidates_finder Candidates finder object */ - protected $candidatesfinder; - /** @var recoverer Recoverer object */ protected $recoverer; protected function setUp(): void { parent::setUp(); $config = manager::get_objectfs_config(); - $this->candidatesfinder = new candidates_finder(recoverer::class, $config); manager::set_objectfs_config($config); $this->logger = new \tool_objectfs\log\aggregate_logger(); $this->recoverer = new recoverer($this->filesystem, $config, $this->logger); @@ -51,7 +46,7 @@ protected function tearDown(): void { public function test_recoverer_get_candidate_objects_will_get_error_objects(): void { $recovererobject = $this->create_error_object(); - $candidateobjects = $this->candidatesfinder->get(); + $candidateobjects = recoverer::get_candidates(manager::get_objectfs_config()); foreach ($candidateobjects as $candidate) { $this->assertEquals($recovererobject->contenthash, $candidate->contenthash);