external objects retention time by KaterynaLove · Pull Request #1 · KaterynaLove/moodle-tool_objectfs

KaterynaLove · 2023-01-25T21:23:14Z

No description provided.

dvdcastro

Thanks for working on this Kateryna. I've left some minor comments.

The task and the function changes will need unit tests written for them before we can submit to Catalyst.

dvdcastro · 2023-01-26T01:28:21Z

            $currentpath = $this->get_external_path_from_hash($contenthash);
-            $this->externalclient->delete_file($currentpath);
+            $delay_delete_external_object = get_config('tool_objectfs', 'delaydeleteexternalobject');
+            if($force_delay_delete) {


Suggested change

if($force_delay_delete) {

if ($force_delay_delete) {

dvdcastro · 2023-01-26T01:28:35Z

+
+                $delay_objects_delete->id = $DB->insert_record('tool_objectfs_delay_delete', $delay_objects_delete, true);
+            } else {
+


Suggested change

dvdcastro · 2023-01-26T01:31:22Z

 $plugin->maturity  = MATURITY_STABLE;
 $plugin->supported = [310, 401];
+$plugin->dependencies = array(
+    'local_aws' => 2022011300


This is an optional dependency, we should not include it.

This was already in the code. Should I just remove it then?

Their version file does not have it. Yes, let's remove it.
https://github.com/catalyst/moodle-tool_objectfs/blob/MOODLE_310_STABLE/version.php

dvdcastro · 2023-01-26T01:32:12Z

    $settings->add(new admin_setting_configcheckbox('tool_objectfs/preferexternal',
        new lang_string('settings:preferexternal', 'tool_objectfs'), '', ''));
+
+    $settings->add(new admin_settings_aws_region('tool_objectfs/s3_region',


Why is this setting necessary?

This also was already in the code. Not sure..

It should not appear in the diff. I think you need to rebase your changes.

dvdcastro · 2023-01-26T01:32:37Z

 $string['settings:batchsize_help'] = 'Number of files to be transferred in one cron run';
 $string['settings:maxorphanedage'] = 'Max orphaned object age';
 $string['settings:maxorphanedage_help'] = 'If set to zero, this will not delete old orphaned metadata for objects. Otherwise, it will remove these records as they are no longer relevant. An orphaned object is one where the metadata exists on the {tool_objectfs_objects} table but referenced file no longer exists.';
+


Suggested change

This change has not been made.

dvdcastro · 2023-01-26T01:39:26Z

+ * and external file (if delete external enabled) as it is no longer useful/relevant.
+ *
+ * @package   tool_objectfs
+ * @author    Kevin Pham <kevinpham@catalyst-au.net>


Suggested change

* @author Kevin Pham <kevinpham@catalyst-au.net>

* @author Katerina Martinenko <kateryna.martynenko@moodle.com>

dvdcastro · 2023-01-26T01:40:25Z

+
+        $timeperiodforremoval = $this->config->delaydeleteexternalobject;
+        if (empty($timeperiodforremoval)) {
+            mtrace('Skipping delayed deletion of the external object of the delaydeleteexternalobject is set to an empty value.');


What happens if there are files in the table still? Should we purge the table then?

Right now the records are not deleting from the table, only updating. We could delete those directly from the table instead.

Would that be okay keeping always an empty table though?

We can purge the table as a last step. Let's say it purges files from the list which were removed 90 days ago. IDK what would happen if there was a file added that has the same content has and then also deleted. Would we have 2 entries in the table? If that's the case, and they don't clash, I don't see a problem with deleting these entries periodically. Deleting them straight away is also valid, you could get rid of the status field.

Ok, I'll delete them directly then and will remove the status field.

dvdcastro · 2023-01-26T01:41:15Z

+            $objects = $DB->get_recordset_sql($sql, $params);
+            $count = 0;
+            foreach ($objects as $object) {
+


Suggested change

dvdcastro · 2023-01-26T01:41:56Z

+                $deletedexternalobject->id = $object->id;
+                $deletedexternalobject->status = 1;
+                $DB->update_record('tool_objectfs_delay_delete', $deletedexternalobject);
+                $count++;


I think we should short circuit this for loop in case there are many files delayed to be deleted. Having a long list may harm the task execution for the entire site.

Hmm good point! I can add a limit to that, for example, to delete 10 files at a time..?

I think 1000 at a time is a good value.

All right, will put a limit as 1000 :)

dvdcastro · 2023-01-26T01:52:09Z

+            $delay_delete_external_object = get_config('tool_objectfs', 'delaydeleteexternalobject');
+            if($force_delay_delete) {
+                $this->externalclient->delete_file($currentpath);
+            } else if ($delay_delete_external_object > 0)  {
+                $delay_objects_delete = new \stdClass();
+                $delay_objects_delete->contenthash = $contenthash;
+                $delay_objects_delete->timecreated = time();
+
+                $delay_objects_delete->id = $DB->insert_record('tool_objectfs_delay_delete', $delay_objects_delete, true);
+            } else {
+
+                $this->externalclient->delete_file($currentpath);
+            }


Minor suggestion.

Suggested change

$delay_delete_external_object = get_config('tool_objectfs', 'delaydeleteexternalobject');

if($force_delay_delete) {

$this->externalclient->delete_file($currentpath);

} else if ($delay_delete_external_object > 0) {

$delay_objects_delete = new \stdClass();

$delay_objects_delete->contenthash = $contenthash;

$delay_objects_delete->timecreated = time();

$delay_objects_delete->id = $DB->insert_record('tool_objectfs_delay_delete', $delay_objects_delete, true);

} else {

$this->externalclient->delete_file($currentpath);

}

$delaydeleteexternalobject = get_config('tool_objectfs', 'delaydeleteexternalobject');

if($forcedelaydelete || $delaydeleteexternalobject <= 0) {

$this->externalclient->delete_file($currentpath);

} else if ($delaydeleteexternalobject > 0) {

$delay_objects_delete = new \stdClass();

$delay_objects_delete->contenthash = $contenthash;

$delay_objects_delete->timecreated = time();

$delay_objects_delete->id = $DB->insert_record('tool_objectfs_delay_delete', $delay_objects_delete, true);

}

…phaned or errors

dvdcastro · 2023-02-14T12:52:10Z

        }

+        // If location change is 'orphaned' we update timeorphaned. 
+        //Set time orphaned clock is ticking now for delay deletion comparison...


Suggested change

//Set time orphaned clock is ticking now for delay deletion comparison...

// Set time orphaned clock is ticking now for delay deletion comparison...

dvdcastro · 2023-02-14T12:52:36Z

     * Deletes external file depending on deleteexternal settings.
     *
     * @param string $contenthash file to be moved
+     * @param boolean $forcedelete will force to delete the file


Suggested change

* @param boolean $forcedelete will force to delete the file

dvdcastro · 2023-02-14T12:52:59Z

+        global $DB;
+


Suggested change

global $DB;

dvdcastro · 2023-02-14T12:57:09Z

+        'minute'    => '*',
+        'hour'      => '*',


I think we can run this in the same manner as the tool_objectfs\task\orphan_objects and the tool_objectfs\task\delete_orphaned_object_metadata tasks. This is optional, just an opinion.

Suggested change

'minute' => '*',

'hour' => '*',

'minute' => 'R',

'hour' => 'R',

dvdcastro · 2023-02-14T12:57:41Z

 $string['settings:batchsize_help'] = 'Number of files to be transferred in one cron run';
 $string['settings:maxorphanedage'] = 'Max orphaned object age';
 $string['settings:maxorphanedage_help'] = 'If set to zero, this will not delete old orphaned metadata for objects. Otherwise, it will remove these records as they are no longer relevant. An orphaned object is one where the metadata exists on the {tool_objectfs_objects} table but referenced file no longer exists.';
+


This change has not been made.

dvdcastro · 2023-02-14T13:02:51Z

+        $this->assertTrue($this->is_externally_readable_by_hash($filehash));
+    }
+
+    public function presigned_url_should_redirect_provider() {


I think this can be removed.

dvdcastro · 2023-02-14T13:03:06Z

+     *
+     * @return array
+     */
+    public function get_expiration_time_method_if_supported_provider() {


I think this can be removed.

dvdcastro · 2023-02-14T13:03:18Z

+     *
+     * @return array
+     */
+    public function get_valid_http_ranges_provider() {


I think this can be removed.

dvdcastro · 2023-02-14T13:03:34Z

+     *
+     * @return array
+     */
+    public function curl_range_request_to_presigned_url_provider() {


I think this can be removed.

dvdcastro · 2023-02-14T13:08:07Z

+    /**
+     * Execute task
+     */
+    public function execute() {


I don't understand this test. How is it validating that elements are being deleted from the external system? I think we should create mocks for the method $filesystem->delete_external_file_from_hash(...), and check that when the task is run, the method is called under the correct circumstances.

dvdcastro

Thanks for working on this Kateryna. Please review the comments I had left in prior reviews where I suggested removing empty lines or whitespaces, they still have not been addressed.

dvdcastro · 2023-02-22T05:58:51Z

+
+        manager::update_object($objectrecord, OBJECT_LOCATION_ORPHANED);
+
+        $this->assertTrue($this->is_externally_readable_by_hash($filehash));


I suggest we also validate that the objectfs row has the timeorphaned value set.

$objectrecord = $DB->get_record('tool_objectfs_objects', ['contenthash' => $filehash]); $this->assertGreaterThan($objectrecord->timeorphaned, 0, 'Time orphaned should be greater than 0.');

dvdcastro · 2023-02-22T06:00:25Z

+        $this->assertTrue($this->is_externally_readable_by_hash($filehash));
+    }
+
+    public function test_if_external_orphaned_object_is_deleted_after_delaydeleteexternalobject_setting() {


This method is not currently testing something. I suggest we delete it. If you want to test the task execution, I suggest you first create orphaned files with an expired orphanedtime and then run the execute function from the task.

But I think we can get a pass on running tests for the task. They are currently not testing their tasks.

dvdcastro suggested changes Jan 26, 2023

View reviewed changes

issue505 Do not include objects in population task lookup that are or…

b673c6c

…phaned or errors

dvdcastro suggested changes Feb 14, 2023

View reviewed changes

Use full join on table, we don't want null filesize.

1b5bc7e

dvdcastro suggested changes Feb 22, 2023

View reviewed changes

KaterynaLove force-pushed the DEF-475 branch from 426cb24 to 241c6ba Compare March 3, 2023 00:05

Adding support for delayed deletion of orphaned external files.

d0d1d73

KaterynaLove force-pushed the DEF-475 branch from 241c6ba to d0d1d73 Compare March 3, 2023 15:29

merged upgrade changes

bc4a92b


		$delay_objects_delete->id = $DB->insert_record('tool_objectfs_delay_delete', $delay_objects_delete, true);
		} else {

	* @author Kevin Pham <kevinpham@catalyst-au.net>
	* @author Katerina Martinenko <kateryna.martynenko@moodle.com>

	//Set time orphaned clock is ticking now for delay deletion comparison...
	// Set time orphaned clock is ticking now for delay deletion comparison...


		manager::update_object($objectrecord, OBJECT_LOCATION_ORPHANED);

		$this->assertTrue($this->is_externally_readable_by_hash($filehash));

Conversation

KaterynaLove commented Jan 25, 2023

Uh oh!

dvdcastro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdcastro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers