From 99207638d9a692b879feccca9fa4e2c51f4d69eb Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 25 Mar 2024 19:46:02 +0100 Subject: [PATCH] fix: make sure to order datalayer versions by time When we changed from id to uuid, we broke the sorting, which supposed to have a constant id as string prefix from version to the other. --- umap/models.py | 29 +++++++++++++--------- umap/tests/test_datalayer.py | 48 +++++++++++++++++++++++------------- umap/views.py | 3 ++- 3 files changed, 51 insertions(+), 29 deletions(-) diff --git a/umap/models.py b/umap/models.py index c61401ff..31c1b01a 100644 --- a/umap/models.py +++ b/umap/models.py @@ -1,4 +1,5 @@ import json +import operator import os import time import uuid @@ -471,17 +472,14 @@ class DataLayer(NamedModel): "size": self.geojson.storage.size(self.get_version_path(name)), } - def get_versions(self): + @property + def versions(self): root = self.storage_root() names = self.geojson.storage.listdir(root)[1] names = [name for name in names if self.is_valid_version(name)] - names.sort(reverse=True) # Recent first. - return names - - @property - def versions(self): - names = self.get_versions() - return [self.version_metadata(name) for name in names] + versions = [self.version_metadata(name) for name in names] + versions.sort(reverse=True, key=operator.itemgetter("at")) + return versions def get_version(self, name): path = self.get_version_path(name) @@ -493,8 +491,13 @@ class DataLayer(NamedModel): def purge_old_versions(self): root = self.storage_root() - names = self.get_versions()[settings.UMAP_KEEP_VERSIONS :] - for name in names: + versions = self.versions[settings.UMAP_KEEP_VERSIONS :] + for version in versions: + name = version["name"] + # Should not be in the list, but ensure to not delete the file + # currently used in database + if self.geojson.name.endswith(name): + continue try: self.geojson.storage.delete(os.path.join(root, name)) except FileNotFoundError: @@ -503,8 +506,12 @@ class DataLayer(NamedModel): def purge_gzip(self): root = self.storage_root() names = self.geojson.storage.listdir(root)[1] + prefixes = [f"{self.pk}_"] + if self.old_id: + prefixes.append(f"{self.old_id}_") + prefixes = tuple(prefixes) for name in names: - if name.startswith(f"{self.pk}_") and name.endswith(".gz"): + if name.startswith(prefixes) and name.endswith(".gz"): self.geojson.storage.delete(os.path.join(root, name)) def can_edit(self, user=None, request=None): diff --git a/umap/tests/test_datalayer.py b/umap/tests/test_datalayer.py index c5c12f89..ee564bc8 100644 --- a/umap/tests/test_datalayer.py +++ b/umap/tests/test_datalayer.py @@ -1,4 +1,5 @@ import os +from pathlib import Path import pytest from django.core.files.base import ContentFile @@ -60,30 +61,43 @@ def test_clone_should_clone_geojson_too(datalayer): assert clone.geojson.path != datalayer.geojson.path -def test_should_remove_old_versions_on_save(datalayer, map, settings): +def test_should_remove_old_versions_on_save(map, settings): + datalayer = DataLayerFactory(uuid="0f1161c0-c07f-4ba4-86c5-8d8981d8a813", old_id=17) settings.UMAP_KEEP_VERSIONS = 3 - root = datalayer.storage_root() + root = Path(datalayer.storage_root()) before = len(datalayer.geojson.storage.listdir(root)[1]) - newer = f"{root}/{datalayer.pk}_1440924889.geojson" - medium = f"{root}/{datalayer.pk}_1440923687.geojson" - older = f"{root}/{datalayer.pk}_1440918637.geojson" - other = f"{root}/123456_1440918637.geojson" - for path in [medium, newer, older, other]: - datalayer.geojson.storage.save(path, ContentFile("{}")) - datalayer.geojson.storage.save(path + ".gz", ContentFile("{}")) - assert len(datalayer.geojson.storage.listdir(root)[1]) == 8 + before + newer = f"{datalayer.pk}_1440924889.geojson" + medium = f"{datalayer.pk}_1440923687.geojson" + older = f"{datalayer.pk}_1440918637.geojson" + with_old_id = f"{datalayer.old_id}_1440918537.geojson" + other = "123456_1440918637.geojson" + for path in [medium, newer, older, with_old_id, other]: + datalayer.geojson.storage.save(root / path, ContentFile("{}")) + datalayer.geojson.storage.save(root / f"{path}.gz", ContentFile("{}")) + assert len(datalayer.geojson.storage.listdir(root)[1]) == 10 + before + files = datalayer.geojson.storage.listdir(root)[1] + # Those files should be present before save, which will purge them + assert older in files + assert older + ".gz" in files + assert with_old_id in files + assert with_old_id + ".gz" in files datalayer.save() files = datalayer.geojson.storage.listdir(root)[1] # Flat + gz files, but not latest gz, which is created at first datalayer read. + # older and with_old_id should have been removed assert len(files) == 5 - assert os.path.basename(newer) in files - assert os.path.basename(medium) in files - assert os.path.basename(datalayer.geojson.path) in files + assert newer in files + assert medium in files + assert Path(datalayer.geojson.path).name in files # File from another datalayer, purge should have impacted it. - assert os.path.basename(other) in files - assert os.path.basename(other + ".gz") in files - assert os.path.basename(older) not in files - assert os.path.basename(older + ".gz") not in files + assert other in files + assert other + ".gz" in files + assert older not in files + assert older + ".gz" not in files + assert with_old_id not in files + assert with_old_id + ".gz" not in files + names = [v["name"] for v in datalayer.versions] + assert names == [Path(datalayer.geojson.name).name, newer, medium] def test_anonymous_cannot_edit_in_editors_mode(datalayer): diff --git a/umap/views.py b/umap/views.py index 3a8d1743..c8806b1c 100644 --- a/umap/views.py +++ b/umap/views.py @@ -1073,7 +1073,8 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView): """ # Use the provided info to find the correct version in our storage. - for name in self.object.get_versions(): + for version in self.object.versions: + name = version["name"] path = Path(settings.MEDIA_ROOT) / self.object.get_version_path(name) if reference_version == self.read_version(path): with open(path) as f: