Merge pull request #330 from girder/registration

Author: brianhelba

s3_file_field/_registry.py

@@ -1,4 +1,5 @@
 from typing import TYPE_CHECKING, Iterator
+import warnings
 from weakref import WeakValueDictionary
 
 from django.core.files.storage import Storage
@@ -17,9 +18,15 @@
 
 def register_field(field: "S3FileField") -> None:
     field_id = field.id
-    if field_id in _fields and not (_fields[field_id] is field):
+    if field_id in _fields and _fields[field_id] is not field:
         # This might be called multiple times, but it should always be consistent
-        raise RuntimeError(f"Cannot overwrite existing S3FileField declaration for {field_id}")
+        warnings.warn(
+            f"Overwriting existing S3FileField declaration for {field_id}",
+            RuntimeWarning,
+            # This should attribute to the re-defining class (instead of S3FF or Django internals),
+            # but it was determined empirically and could break if Django is restructured.
+            stacklevel=5,
+        )
     _fields[field_id] = field
 
     storage = field.storage

tests/test_registry.py

@@ -1,6 +1,9 @@
-from typing import cast
+import inspect
+import sys
+from typing import Generator, cast
 
 from django.core.files.storage import default_storage
+from django.db import models
 import pytest
 
 from s3_file_field import _registry
@@ -15,6 +18,21 @@ def s3ff_field() -> S3FileField:
     return cast(S3FileField, Resource._meta.get_field("blob"))
 
 
+@pytest.fixture()
+def ephemeral_s3ff_field() -> Generator[S3FileField, None, None]:
+    # Declaring this will implicitly register the field
+    class EphemeralResource(models.Model):
+        class Meta:
+            app_label = "test_app"
+
+        blob = S3FileField()
+
+    field = cast(S3FileField, EphemeralResource._meta.get_field("blob"))
+    yield field
+    # The registry state is global to the process, so attempt to clean up
+    del _registry._fields[field.id]
+
+
 def test_field_id(s3ff_field: S3FileField) -> None:
     assert s3ff_field.id == "test_app.Resource.blob"
 
@@ -41,3 +59,28 @@ def test_registry_iter_storages() -> None:
 
     assert len(fields) == 1
     assert fields[0] is default_storage
+
+
+@pytest.mark.skipif(sys.version_info < (3, 9), reason="Bug bpo-35113")
+def test_registry_register_field_multiple(ephemeral_s3ff_field: S3FileField) -> None:
+    with pytest.warns(
+        RuntimeWarning, match=r"Overwriting existing S3FileField declaration"
+    ) as records:
+        # Try to create a field with the same id
+        class EphemeralResource(models.Model):
+            class Meta:
+                app_label = "test_app"
+
+            blob = S3FileField()
+
+    # Ensure the warning is attributed to the re-defined model, since stacklevel is set empirically
+    warning = next(record for record in records if str(record.message).startswith("Overwriting"))
+    assert warning.filename == inspect.getsourcefile(EphemeralResource)
+    assert warning.lineno == inspect.getsourcelines(EphemeralResource)[1]
+
+    duplicate_field = cast(S3FileField, EphemeralResource._meta.get_field("blob"))
+    # Sanity check
+    assert duplicate_field.id == ephemeral_s3ff_field.id
+    assert duplicate_field is not ephemeral_s3ff_field
+    # The most recently registered field should by stored
+    assert _registry.get_field(duplicate_field.id) is duplicate_field