Several bug fixes for reverse sync

- Don't instance.resource
- Don't sync an update of something owned by the local service
- Load ansible_id from gateway on create

Signed-off-by: Rick Elrod <[email protected]>

ansible_base/resource_registry/signals/handlers.py

@@ -50,14 +50,12 @@ def decide_to_sync_update(sender, instance, raw, using, update_fields, **kwargs)
         return
 
     try:
-        if not getattr(instance, 'resource', None) or not instance.resource.ansible_id:
-            # We can't sync here, but we want to log that, so let sync_to_resource_server() discard it.
-            return
+        resource = Resource.get_resource_for_object(instance)
     except Resource.DoesNotExist:
-        # The getattr() will raise a Resource.DoesNotExist if the resource doesn't exist.
+        # We can't sync here, but we want to log that, so let sync_to_resource_server() discard it.
         return
 
-    fields_that_sync = instance.resource.content_type.resource_type.serializer_class().get_fields().keys()
+    fields_that_sync = resource.content_type.resource_type.serializer_class().get_fields().keys()
 
     if update_fields is None:
         # If we're not given a useful update_fields, manually calculate the changed fields

ansible_base/resource_registry/utils/sync_to_resource_server.py

@@ -6,7 +6,7 @@
 from django.utils.translation import gettext_lazy as _
 from rest_framework.exceptions import ValidationError
 
-from ansible_base.resource_registry.models import Resource
+from ansible_base.resource_registry.models import Resource, service_id
 from ansible_base.resource_registry.rest_client import ResourceRequestBody, get_resource_server_client
 
 logger = logging.getLogger('ansible_base.resource_registry.utils.sync_to_resource_server')
@@ -39,29 +39,32 @@ def sync_to_resource_server(instance, action, ansible_id=None):
         logger.info(f"Skipping sync of resource {instance}")
         return
 
+    if action != "delete" and ansible_id is not None:
+        raise Exception("ansible_id should not be provided for create/update actions")
+    elif action == "delete" and ansible_id is None:
+        raise Exception("ansible_id should be provided for delete actions")
+
     try:
-        if action != "delete" and ansible_id is not None:
-            raise Exception("ansible_id should not be provided for create/update actions")
-        elif action == "delete" and ansible_id is None:
-            raise Exception("ansible_id should be provided for delete actions")
-        elif not getattr(instance, 'resource', None) or not instance.resource.ansible_id:
-            # We can't sync if we don't have a resource and an ansible_id.
-            logger.error(f"Resource {instance} does not have a resource or ansible_id")
-            return
+        resource = Resource.get_resource_for_object(instance)
     except Resource.DoesNotExist:
-        # The getattr() will raise a Resource.DoesNotExist if the resource doesn't exist.
         logger.error(f"Resource {instance} does not have a resource")
         return
 
+    if str(resource.service_id) == service_id() and action == "update":
+        # Don't sync if we're updating a resource that isn't owned by the resource server yet.
+        logger.info(f"Skipping sync of resource {instance} because its service_id is local")
+        return
+
     user_ansible_id = None
     user = get_current_user()
     if user:
         # If we have a user, try to get their ansible_id and sync as them.
         # If they don't have one some how, or if we don't have a user, sync with None and
         # let the resource server decide what to do.
         try:
-            user_ansible_id = user.resource.ansible_id
-        except AttributeError:
+            user_resource = Resource.get_resource_for_object(user)
+            user_ansible_id = user_resource.ansible_id
+        except (Resource.DoesNotExist, AttributeError):
             logger.error(f"User {user} does not have a resource")
             pass
     else:
@@ -74,9 +77,9 @@ def sync_to_resource_server(instance, action, ansible_id=None):
     )
 
     if action != "delete":
-        ansible_id = instance.resource.ansible_id
+        ansible_id = resource.ansible_id
 
-    resource_type = instance.resource.content_type.resource_type
+    resource_type = resource.content_type.resource_type
     data = resource_type.serializer_class(instance).data
     body = ResourceRequestBody(
         resource_type=resource_type.name,
@@ -89,8 +92,9 @@ def sync_to_resource_server(instance, action, ansible_id=None):
             response = client.create_resource(body)
             json = response.json()
             if isinstance(json, dict):  # Mainly for tests... to avoid getting here with mock
-                instance.resource.service_id = json['service_id']
-                instance.resource.save()
+                resource.service_id = json['service_id']
+                resource.ansible_id = json['ansible_id']
+                resource.save()
         elif action == "update":
             client.update_resource(ansible_id, body)
         elif action == "delete":

test_app/tests/resource_registry/test_utils.py

@@ -1,4 +1,5 @@
 import os
+import uuid
 from contextlib import nullcontext
 from unittest import mock
 
@@ -49,6 +50,9 @@ def test_sync_to_resource_server_happy_path(self, user, action, enable_reverse_s
                             if action == 'create':
                                 org = Organization.objects.create(name='Hello')
                             elif action == 'update':
+                                # Fake the service_id to not be local
+                                org.resource.service_id = uuid.uuid4()
+                                org.resource.save()
                                 org.name = 'World'
                                 org.save()
                             elif action == 'delete':
@@ -102,6 +106,20 @@ def test_sync_to_resource_server_no_resource(self, user, nullify_resource, enabl
         # We bail out if we don't have a resource
         get_resource_server_client.assert_not_called()
 
+    @pytest.mark.django_db
+    def test_sync_to_resource_server_update_with_local_service_id(self, user, organization, enable_reverse_sync, expected_log):
+        """
+        If the resource's service_id is the local service_id, we should not attempt to sync it on update.
+        """
+        with enable_reverse_sync():
+            with mock.patch(f'{utils_path}.get_resource_server_client') as get_resource_server_client:
+                with impersonate(user):
+                    organization.name = 'World'
+                    with expected_log(f'{utils_path}.logger', 'info', 'service_id is local'):
+                        organization.save()
+
+        get_resource_server_client.assert_not_called()
+
     @pytest.mark.django_db(transaction=True)
     @pytest.mark.parametrize(
         'ansible_reverse_resource_sync, should_sync',