Fix resource registry migrations (ansible#119)

ansible_base/resource_registry/apps.py

@@ -31,6 +31,8 @@ def initialize_resources(sender, **kwargs):
     if apps is None:
         from django.apps import apps
 
+    from ansible_base.resource_registry.models import init_resource_from_object
+
     Resource = apps.get_model("dab_resource_registry", "Resource")
     ResourceType = apps.get_model("dab_resource_registry", "ResourceType")
     ContentType = apps.get_model("contenttypes", "ContentType")
@@ -52,25 +54,34 @@ def initialize_resources(sender, **kwargs):
         # Create resources
         for r_type in ResourceType.objects.filter(migrated=False):
             resource_model = apps.get_model(r_type.content_type.app_label, r_type.content_type.model)
+            resource_config = registry.get_config_for_model(resource_model)
 
             logger.info(f"adding unmigrated resources for {r_type.name}")
 
             data = []
             for obj in resource_model.objects.all():
-                data.append(Resource.init_from_object(obj, resource_type=r_type))
+                data.append(init_resource_from_object(obj, resource_model=Resource, resource_type=r_type, resource_config=resource_config))
 
             Resource.objects.bulk_create(data, ignore_conflicts=True)
             r_type.migrated = True
             r_type.save()
 
 
+def connect_signals(*args, **kwargs):
+    from django.db.models.signals import post_delete, post_save
+
+    from ansible_base.resource_registry.signals import handlers
+
+    post_save.connect(handlers.update_resource)
+    post_delete.connect(handlers.remove_resource)
+
+
 class AnsibleAuthConfig(AppConfig):
     default_auto_field = 'django.db.models.BigAutoField'
     name = 'ansible_base.resource_registry'
     label = 'dab_resource_registry'
     verbose_name = 'Service resources API'
 
     def ready(self):
+        post_migrate.connect(connect_signals, sender=self)
         post_migrate.connect(initialize_resources, sender=self)
-
-        from ansible_base.resource_registry.signals import handlers  # noqa: F401 - register signals

ansible_base/resource_registry/models/__init__.py

@@ -1,2 +1,2 @@
-from .resource import Resource, ResourceType  # noqa: 401
+from .resource import Resource, ResourceType, init_resource_from_object  # noqa: 401
 from .service_id import service_id  # noqa: 401

ansible_base/resource_registry/models/resource.py

@@ -87,23 +87,6 @@ def update_from_content_object(self):
                 self.name = name
                 self.save()
 
-    @classmethod
-    def init_from_object(cls, obj, resource_type=None):
-        """
-        Initialize a new Resource object from another model instance.
-        """
-        if resource_type is None:
-            c_type = ContentType.objects.get_for_model(obj)
-            resource_type = c_type.resource_type
-            assert resource_type is not None
-
-        resource = cls(object_id=obj.pk, content_type=resource_type.content_type)
-        resource_config = resource_type.get_resource_config()
-        if hasattr(obj, resource_config.name_field):
-            resource.name = str(getattr(obj, resource_config.name_field))[:512]
-
-        return resource
-
     @classmethod
     def get_resource_for_object(cls, obj):
         """
@@ -158,3 +141,27 @@ def update_resource(self, resource_data: dict, ansible_id=None, partial=False):
             if ansible_id:
                 self.ansible_id = ansible_id
                 self.save()
+
+
+# This is a separate function so that it can work with models from apps in the
+# post migration signal.
+def init_resource_from_object(obj, resource_model=None, resource_type=None, resource_config=None):
+    """
+    Initialize a new Resource object from another model instance.
+    """
+    if resource_type is None:
+        c_type = ContentType.objects.get_for_model(obj)
+        resource_type = c_type.resource_type
+        assert resource_type is not None
+
+    if resource_config is None:
+        resource_config = resource_type.get_resource_config()
+
+    if resource_model is None:
+        resource_model = Resource
+
+    resource = resource_model(object_id=obj.pk, content_type=resource_type.content_type)
+    if hasattr(obj, resource_config.name_field):
+        resource.name = str(getattr(obj, resource_config.name_field))[:512]
+
+    return resource

ansible_base/resource_registry/signals/handlers.py

@@ -1,9 +1,6 @@
 from functools import lru_cache
 
-from django.db.models.signals import post_delete, post_save
-from django.dispatch import receiver
-
-from ansible_base.resource_registry.models import Resource
+from ansible_base.resource_registry.models import Resource, init_resource_from_object
 from ansible_base.resource_registry.registry import get_concrete_model, get_registry
 
 
@@ -18,10 +15,7 @@ def get_resource_models():
     return resource_models
 
 
-@receiver(post_delete)
 def remove_resource(sender, instance, **kwargs):
-    if sender._meta.object_name == 'Migration':
-        return
     model = get_concrete_model(sender)
     if model in get_resource_models():
         try:
@@ -31,14 +25,11 @@ def remove_resource(sender, instance, **kwargs):
             return
 
 
-@receiver(post_save)
 def update_resource(sender, instance, created, **kwargs):
-    if sender._meta.object_name == 'Migration':
-        return
     model = get_concrete_model(sender)
     if model in get_resource_models():
         if created:
-            resource = Resource.init_from_object(instance)
+            resource = init_resource_from_object(instance)
             resource.save()
         else:
             resource = Resource.get_resource_for_object(instance)

test_app/migrations/0002_set_up_resources_test_data.py

@@ -0,0 +1,43 @@
+# Generated by Django 4.2.8 on 2024-01-26 11:03
+
+from django.db import migrations, models
+
+
+def create_test_data(apps, schema_editor):
+    """
+    This migration sets up some resource instances after the resource registry is
+    created. This is to test two things:
+        1. That the post_save and post_delete signals in the resources app do not
+           fire during migrations.
+        2. That the post migration signal to initialize resource instances from
+           existing objects works correctly.
+    """
+    ResourceMigrationTestModel = apps.get_model("test_app", "ResourceMigrationTestModel")
+    Resource = apps.get_model("dab_resource_registry", "Resource")
+
+    ResourceMigrationTestModel.objects.create(name="migration resource")
+    r2 = ResourceMigrationTestModel.objects.create(name="migration resource 2")
+
+    assert Resource.objects.all().count() == 0
+
+    r2.delete()
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('test_app', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='ResourceMigrationTestModel',
+            fields=[
+                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('name', models.CharField(max_length=255)),
+            ],
+        ),
+        migrations.RunPython(
+            code=create_test_data,
+            reverse_code=migrations.RunPython.noop
+        ),
+    ]

test_app/models.py

@@ -27,3 +27,7 @@ def summary_fields(self):
 
 class Team(AbstractTeam):
     pass
+
+
+class ResourceMigrationTestModel(models.Model):
+    name = models.CharField(max_length=255)

test_app/resource_api.py

@@ -3,7 +3,7 @@
 from ansible_base.authentication.models import Authenticator
 from ansible_base.resource_registry.registry import ResourceConfig, ServiceAPIConfig, SharedResource
 from ansible_base.resource_registry.shared_types import OrganizationType, TeamType, UserType
-from test_app.models import Organization, Team
+from test_app.models import Organization, ResourceMigrationTestModel, Team
 
 
 class APIConfig(ServiceAPIConfig):
@@ -24,4 +24,5 @@ class APIConfig(ServiceAPIConfig):
     ),
     # Authenticators won't be a shared resource in production, but it's a convenient model to use for testing.
     ResourceConfig(Authenticator),
+    ResourceConfig(ResourceMigrationTestModel),
 )

test_app/tests/resource_registry/test_migration.py

@@ -0,0 +1,12 @@
+import pytest
+
+from ansible_base.resource_registry.models import Resource
+
+
+@pytest.mark.django_db
+def test_existing_resources_created_in_post_migration():
+    """
+    Test that resources that existed before the registry was added got
+    created successfully.
+    """
+    assert Resource.objects.filter(name="migration resource", content_type__resource_type__name="aap.resourcemigrationtestmodel").exists()

test_app/tests/resource_registry/test_resource_types_api.py

@@ -8,7 +8,9 @@ def test_resource_type_list(admin_api_client):
     url = reverse("resourcetype-list")
     response = admin_api_client.get(url)
     assert response.status_code == 200
-    assert set([x["name"] for x in response.data['results']]) == set(["shared.user", "shared.team", "aap.authenticator", "shared.organization"])
+    assert set([x["name"] for x in response.data['results']]) == set(
+        ["shared.user", "shared.team", "aap.authenticator", "shared.organization", "aap.resourcemigrationtestmodel"]
+    )
 
 
 def test_resource_type_detail(admin_api_client):

test_app/tests/resource_registry/test_resources_api.py

@@ -11,9 +11,12 @@
 def test_resources_list(admin_api_client):
     """Test that the resource list is working."""
     url = reverse("resource-list")
-    resp = admin_api_client.get(url)
+    resp = admin_api_client.get(url + "?content_type__resource_type__name=aap.resourcemigrationtestmodel")
 
     assert resp.status_code == 200
+    assert resp.data['count'] == 1
+    assert resp.data['results'][0]["name"] == "migration resource"
+    assert resp.data['results'][0]["resource_type"] == "aap.resourcemigrationtestmodel"
 
 
 def test_resources_delete(django_user_model):