Merge pull request #1630 from umap-project/datalayer-uuids

chore: replace datalayer ids with uuids

umap/fields.py

@@ -4,6 +4,8 @@ import six
 from django.db import models
 from django.utils.encoding import smart_str
 
+from .utils import json_dumps
+
 
 class DictField(models.TextField):
     """
@@ -14,7 +16,7 @@ class DictField(models.TextField):
         if not value:
             value = {}
         if not isinstance(value, six.string_types):
-            value = json.dumps(value)
+            value = json_dumps(value)
         return value
 
     def from_db_value(self, value, expression, connection):

umap/migrations/0018_datalayer_uuid.py

@@ -0,0 +1,62 @@
+# Generated by Django 4.2.8 on 2024-02-19 17:40
+
+import uuid
+
+from django.db import migrations, models
+
+drop_index = """DO $$
+BEGIN
+    EXECUTE 'ALTER TABLE umap_datalayer DROP CONSTRAINT ' || (
+        SELECT indexname
+        FROM pg_indexes
+        WHERE tablename = 'umap_datalayer' AND indexname LIKE '%pk%'
+    );
+END $$;
+"""
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("umap", "0017_migrate_to_openstreetmap_oauth2"),
+    ]
+
+    operations = [
+        # Add the new uuid field
+        migrations.AddField(
+            model_name="datalayer",
+            name="uuid",
+            field=models.UUIDField(
+                default=uuid.uuid4, editable=False, null=True, serialize=False
+            ),
+        ),
+        # Generate UUIDs for existing records
+        migrations.RunSQL(
+            "UPDATE umap_datalayer SET uuid = gen_random_uuid()",
+            reverse_sql=migrations.RunSQL.noop,
+        ),
+        # Remove the primary key constraint
+        migrations.RunSQL(drop_index, reverse_sql=migrations.RunSQL.noop),
+        # Drop the "id" primary key…
+        migrations.AlterField(
+            "datalayer", name="id", field=models.IntegerField(null=True, blank=True)
+        ),
+        # Rename "id" to "old id"
+        migrations.RenameField(
+            model_name="datalayer", old_name="id", new_name="old_id"
+        ),
+        # … to put it back on the "uuid"
+        migrations.AlterField(
+            model_name="datalayer",
+            name="uuid",
+            field=models.UUIDField(
+                default=uuid.uuid4,
+                editable=False,
+                unique=True,
+                primary_key=True,
+                serialize=False,
+            ),
+        ),
+        # When applying the migration backwards, we need to drop the pk index
+        # Before addding a new one.
+        migrations.RunSQL(migrations.RunSQL.noop, reverse_sql=drop_index),
+    ]

umap/migrations/0019_migrate_internal_remote_datalayers.py

@@ -0,0 +1,52 @@
+# Generated by Django 4.2 on 2024-02-26 14:09
+
+import re
+
+from django.conf import settings
+from django.db import migrations
+from django.urls import NoReverseMatch, reverse
+
+# Some users hacked uMap to use another map datalayer as a remote data source.
+# This script gently handles the migration for them.
+
+
+def migrate_datalayers(apps, schema_editor):
+    DataLayer = apps.get_model("umap", "DataLayer")
+
+    datalayers = DataLayer.objects.filter(
+        settings__remoteData__url__icontains="datalayer"
+    )
+
+    for item in datalayers:
+        old_url = item.settings["remoteData"]["url"]
+        match = re.search(
+            rf"{settings.SITE_URL}/datalayer/(?P<map_id>\d+)/(?P<datalayer_id>\d+)",
+            old_url,
+        )
+        if match:
+            remote_id = match.group("datalayer_id")
+            map_id = match.group("map_id")
+            try:
+                remote_uuid = DataLayer.objects.get(old_id=remote_id).uuid
+            except DataLayer.DoesNotExist:
+                pass
+            else:
+                try:
+                    new_url = settings.SITE_URL + reverse(
+                        "datalayer_view", args=[map_id, remote_uuid]
+                    )
+                except NoReverseMatch:
+                    pass
+                else:
+                    item.settings["remoteData"]["url"] = new_url
+                    item.save()
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("umap", "0018_datalayer_uuid"),
+    ]
+
+    operations = [
+        migrations.RunPython(migrate_datalayers, reverse_code=migrations.RunPython.noop)
+    ]

umap/models.py

@@ -1,6 +1,7 @@
 import json
 import os
 import time
+import uuid
 
 from django.conf import settings
 from django.contrib.auth.models import User
@@ -373,7 +374,10 @@ class DataLayer(NamedModel):
         (EDITORS, _("Editors only")),
         (OWNER, _("Owner only")),
     )
-
+    uuid = models.UUIDField(
+        unique=True, primary_key=True, default=uuid.uuid4, editable=False
+    )
+    old_id = models.IntegerField(null=True, blank=True)
     map = models.ForeignKey(Map, on_delete=models.CASCADE)
     description = models.TextField(blank=True, null=True, verbose_name=_("description"))
     geojson = models.FileField(upload_to=upload_to, blank=True, null=True)
@@ -436,6 +440,7 @@ class DataLayer(NamedModel):
 
     def clone(self, map_inst=None):
         new = self.__class__.objects.get(pk=self.pk)
+        new._state.adding = True
         new.pk = None
         if map_inst:
             new.map = map_inst
@@ -444,7 +449,10 @@ class DataLayer(NamedModel):
         return new
 
     def is_valid_version(self, name):
-        return name.startswith("%s_" % self.pk) and name.endswith(".geojson")
+        valid_prefixes = [name.startswith("%s_" % self.pk)]
+        if self.old_id:
+            valid_prefixes.append(name.startswith("%s_" % self.old_id))
+        return any(valid_prefixes) and name.endswith(".geojson")
 
     def version_metadata(self, name):
         els = name.split(".")[0].split("_")

umap/templatetags/umap_tags.py

@@ -1,9 +1,10 @@
-import json
 from copy import copy
 
 from django import template
 from django.conf import settings
 
+from umap.utils import json_dumps
+
 register = template.Library()
 
 
@@ -25,7 +26,7 @@ def map_fragment(map_instance, **kwargs):
     page = kwargs.pop("page", None) or ""
     unique_id = prefix + str(page) + "_" + str(map_instance.pk)
     return {
-        "map_settings": json.dumps(map_settings),
+        "map_settings": json_dumps(map_settings),
         "map": map_instance,
         "unique_id": unique_id,
     }

umap/tests/integration/test_collaborative_editing.py

@@ -98,7 +98,7 @@ def test_collaborative_editing_create_markers(context, live_server, tilelayer):
         "name": "test datalayer",
         "inCaption": True,
         "editMode": "advanced",
-        "id": datalayer.pk,
+        "id": str(datalayer.pk),
         "permissions": {"edit_status": 1},
     }
 
@@ -116,7 +116,7 @@ def test_collaborative_editing_create_markers(context, live_server, tilelayer):
         "name": "test datalayer",
         "inCaption": True,
         "editMode": "advanced",
-        "id": datalayer.pk,
+        "id": str(datalayer.pk),
         "permissions": {"edit_status": 1},
     }
     expect(marker_pane_p1).to_have_count(4)
@@ -136,7 +136,7 @@ def test_collaborative_editing_create_markers(context, live_server, tilelayer):
         "name": "test datalayer",
         "inCaption": True,
         "editMode": "advanced",
-        "id": datalayer.pk,
+        "id": str(datalayer.pk),
         "permissions": {"edit_status": 1},
     }
     expect(marker_pane_p2).to_have_count(5)

umap/tests/test_datalayer_views.py

@@ -111,7 +111,7 @@ def test_update(client, datalayer, map, post_data):
     # Test response is a json
     j = json.loads(response.content.decode())
     assert "id" in j
-    assert datalayer.pk == j["id"]
+    assert str(datalayer.pk) == j["id"]
     assert j["browsable"] is True
     assert Path(modified_datalayer.geojson.path).exists()
 
@@ -225,6 +225,41 @@ def test_versions_should_return_versions(client, datalayer, map, settings):
     assert version in versions["versions"]
 
 
+def test_versions_can_return_old_format(client, datalayer, map, settings):
+    map.share_status = Map.PUBLIC
+    map.save()
+    root = datalayer.storage_root()
+    datalayer.old_id = 123  # old datalayer id (now replaced by uuid)
+    datalayer.save()
+
+    datalayer.geojson.storage.save(
+        "%s/%s_1440924889.geojson" % (root, datalayer.pk), ContentFile("{}")
+    )
+    datalayer.geojson.storage.save(
+        "%s/%s_1440923687.geojson" % (root, datalayer.pk), ContentFile("{}")
+    )
+
+    # store with the id prefix (rather than the uuid)
+    old_format_version = "%s_1440918637.geojson" % datalayer.old_id
+    datalayer.geojson.storage.save(
+        ("%s/" % root) + old_format_version, ContentFile("{}")
+    )
+
+    url = reverse("datalayer_versions", args=(map.pk, datalayer.pk))
+    versions = json.loads(client.get(url).content.decode())
+    assert len(versions["versions"]) == 4
+    version = {
+        "name": old_format_version,
+        "size": 2,
+        "at": "1440918637",
+    }
+    assert version in versions["versions"]
+
+    client.get(
+        reverse("datalayer_version", args=(map.pk, datalayer.pk, old_format_version))
+    )
+
+
 def test_version_should_return_one_version_geojson(client, datalayer, map):
     map.share_status = Map.PUBLIC
     map.save()

umap/urls.py

@@ -72,18 +72,18 @@ i18n_urls = [
 ]
 i18n_urls += decorated_patterns(
     [can_view_map, cache_control(must_revalidate=True)],
-    re_path(
-        r"^datalayer/(?P<map_id>\d+)/(?P<pk>[\d]+)/$",
+    path(
+        "datalayer/<int:map_id>/<uuid:pk>/",
         views.DataLayerView.as_view(),
         name="datalayer_view",
     ),
-    re_path(
-        r"^datalayer/(?P<map_id>\d+)/(?P<pk>[\d]+)/versions/$",
+    path(
+        "datalayer/<int:map_id>/<uuid:pk>/versions/",
         views.DataLayerVersions.as_view(),
         name="datalayer_versions",
     ),
-    re_path(
-        r"^datalayer/(?P<map_id>\d+)/(?P<pk>[\d]+)/(?P<name>[_\w]+.geojson)$",
+    path(
+        "datalayer/<int:map_id>/<uuid:pk>/<str:name>",
         views.DataLayerVersion.as_view(),
         name="datalayer_version",
     ),
@@ -145,13 +145,13 @@ map_urls = [
         views.DataLayerCreate.as_view(),
         name="datalayer_create",
     ),
-    re_path(
-        r"^map/(?P<map_id>[\d]+)/datalayer/delete/(?P<pk>\d+)/$",
+    path(
+        "map/<int:map_id>/datalayer/delete/<uuid:pk>/",
         views.DataLayerDelete.as_view(),
         name="datalayer_delete",
     ),
-    re_path(
-        r"^map/(?P<map_id>[\d]+)/datalayer/permissions/(?P<pk>\d+)/$",
+    path(
+        "map/<int:map_id>/datalayer/permissions/<uuid:pk>/",
         views.UpdateDataLayerPermissions.as_view(),
         name="datalayer_permissions",
     ),
@@ -165,8 +165,8 @@ if settings.DEFAULT_FROM_EMAIL:
         )
     )
 datalayer_urls = [
-    re_path(
-        r"^map/(?P<map_id>[\d]+)/datalayer/update/(?P<pk>\d+)/$",
+    path(
+        "map/<int:map_id>/datalayer/update/<uuid:pk>/",
         views.DataLayerUpdate.as_view(),
         name="datalayer_update",
     ),

umap/utils.py

@@ -1,7 +1,9 @@
 import gzip
+import json
 import os
 
 from django.conf import settings
+from django.core.serializers.json import DjangoJSONEncoder
 from django.urls import URLPattern, URLResolver, get_resolver
 
 
@@ -162,3 +164,8 @@ def merge_features(reference: list, latest: list, incoming: list):
         merged.append(item)
 
     return merged
+
+
+def json_dumps(obj, **kwargs):
+    """Utility using the Django JSON Encoder when dumping objects"""
+    return json.dumps(obj, cls=DjangoJSONEncoder, **kwargs)

umap/views.py

@@ -67,7 +67,14 @@ from .forms import (
     UserProfileForm,
 )
 from .models import DataLayer, Licence, Map, Pictogram, Star, TileLayer
-from .utils import ConflictError, _urls_for_js, gzip_file, is_ajax, merge_features
+from .utils import (
+    ConflictError,
+    _urls_for_js,
+    gzip_file,
+    is_ajax,
+    json_dumps,
+    merge_features,
+)
 
 User = get_user_model()
 
@@ -315,7 +322,7 @@ class UserDownload(DetailView, SearchMixin):
         with zipfile.ZipFile(zip_buffer, "a", zipfile.ZIP_DEFLATED, False) as zip_file:
             for map_ in self.get_maps():
                 umapjson = map_.generate_umapjson(self.request)
-                geojson_file = io.StringIO(json.dumps(umapjson))
+                geojson_file = io.StringIO(json_dumps(umapjson))
                 file_name = f"umap_backup_{map_.slug}_{map_.pk}.umap"
                 zip_file.writestr(file_name, geojson_file.getvalue())
 
@@ -354,7 +361,7 @@ class MapsShowCase(View):
             }
 
         geojson = {"type": "FeatureCollection", "features": [make(m) for m in maps]}
-        return HttpResponse(smart_bytes(json.dumps(geojson)))
+        return HttpResponse(smart_bytes(json_dumps(geojson)))
 
 
 showcase = MapsShowCase.as_view()
@@ -441,7 +448,7 @@ ajax_proxy = AjaxProxy.as_view()
 
 
 def simple_json_response(**kwargs):
-    return HttpResponse(json.dumps(kwargs), content_type="application/json")
+    return HttpResponse(json_dumps(kwargs), content_type="application/json")
 
 
 # ############## #
@@ -537,7 +544,7 @@ class MapDetailMixin:
             geojson["properties"] = {}
         geojson["properties"].update(properties)
         geojson["properties"]["datalayers"] = self.get_datalayers()
-        context["map_settings"] = json.dumps(geojson, indent=settings.DEBUG)
+        context["map_settings"] = json_dumps(geojson, indent=settings.DEBUG)
         self.set_preconnect(geojson["properties"], context)
         return context
 
@@ -1106,7 +1113,7 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
 
             # Replace the uploaded file by the merged version.
             self.request.FILES["geojson"].file = BytesIO(
-                json.dumps(merged).encode("utf-8")
+                json_dumps(merged).encode("utf-8")
             )
 
             # Mark the data to be reloaded by form_valid