From 3eb2b780e3fd805b8f765d3f15272ba67b238ca7 Mon Sep 17 00:00:00 2001 From: David Newswanger Date: Mon, 5 Feb 2024 09:53:59 -0600 Subject: [PATCH] Fix resource registry migrations (#119) --- ansible_base/resource_registry/apps.py | 17 ++++++-- .../resource_registry/models/__init__.py | 2 +- .../resource_registry/models/resource.py | 41 ++++++++++-------- .../resource_registry/signals/handlers.py | 13 +----- .../0002_set_up_resources_test_data.py | 43 +++++++++++++++++++ test_app/models.py | 4 ++ test_app/resource_api.py | 3 +- .../tests/resource_registry/test_migration.py | 12 ++++++ .../test_resource_types_api.py | 4 +- .../resource_registry/test_resources_api.py | 5 ++- 10 files changed, 109 insertions(+), 35 deletions(-) create mode 100644 test_app/migrations/0002_set_up_resources_test_data.py create mode 100644 test_app/tests/resource_registry/test_migration.py diff --git a/ansible_base/resource_registry/apps.py b/ansible_base/resource_registry/apps.py index ec2fbdd43..66808bdb4 100644 --- a/ansible_base/resource_registry/apps.py +++ b/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,18 +54,28 @@ 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' @@ -71,6 +83,5 @@ class AnsibleAuthConfig(AppConfig): 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 diff --git a/ansible_base/resource_registry/models/__init__.py b/ansible_base/resource_registry/models/__init__.py index e6c9196cb..5024ab4d1 100644 --- a/ansible_base/resource_registry/models/__init__.py +++ b/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 diff --git a/ansible_base/resource_registry/models/resource.py b/ansible_base/resource_registry/models/resource.py index 3e2ee4b80..d554f7961 100644 --- a/ansible_base/resource_registry/models/resource.py +++ b/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 diff --git a/ansible_base/resource_registry/signals/handlers.py b/ansible_base/resource_registry/signals/handlers.py index f4695aa65..c1860e37e 100644 --- a/ansible_base/resource_registry/signals/handlers.py +++ b/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) diff --git a/test_app/migrations/0002_set_up_resources_test_data.py b/test_app/migrations/0002_set_up_resources_test_data.py new file mode 100644 index 000000000..aab8ad46d --- /dev/null +++ b/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 + ), + ] diff --git a/test_app/models.py b/test_app/models.py index 4a2516fb5..8d1888db6 100644 --- a/test_app/models.py +++ b/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) diff --git a/test_app/resource_api.py b/test_app/resource_api.py index fd0fde09d..ccb2ebe38 100644 --- a/test_app/resource_api.py +++ b/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), ) diff --git a/test_app/tests/resource_registry/test_migration.py b/test_app/tests/resource_registry/test_migration.py new file mode 100644 index 000000000..35e770b98 --- /dev/null +++ b/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() diff --git a/test_app/tests/resource_registry/test_resource_types_api.py b/test_app/tests/resource_registry/test_resource_types_api.py index 69eef1b59..7a5abbe77 100644 --- a/test_app/tests/resource_registry/test_resource_types_api.py +++ b/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): diff --git a/test_app/tests/resource_registry/test_resources_api.py b/test_app/tests/resource_registry/test_resources_api.py index cc4edc558..78321f5a2 100644 --- a/test_app/tests/resource_registry/test_resources_api.py +++ b/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):