From 80224b791d29e2155fc6d90f2a6752ae9575592e Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Wed, 12 Jul 2017 16:38:05 -0400 Subject: [PATCH 1/2] avoid a race condition in recording deletions in the activity stream 1. You delete something. 2. A signal is generated to record an activity stream deletion. 3. The process of deleting that activity stream deletion attempts to look up a related field which has been deleted (in the meantime) via a cascade. see: #6721 see: #7022 --- awx/main/utils/common.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index 473683a668..392ebaa961 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -21,6 +21,7 @@ import tempfile from decorator import decorator # Django +from django.core.exceptions import ObjectDoesNotExist from django.utils.translation import ugettext_lazy as _ from django.db.models.fields.related import ForeignObjectRel, ManyToManyField @@ -354,7 +355,10 @@ def model_to_dict(obj, serializer_mapping=None): for field in obj._meta.fields: if field.name not in allowed_fields: continue - attr_d[field.name] = _convert_model_field_for_display(obj, field.name, password_fields=password_fields) + try: + attr_d[field.name] = _convert_model_field_for_display(obj, field.name, password_fields=password_fields) + except ObjectDoesNotExist: + logger.warn(_('%s no longer exists for %s') % (field.name, obj)) return attr_d From 0a6329feff3237dcb6a0b07cecd4cad25207861f Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 13 Jul 2017 07:53:33 -0400 Subject: [PATCH 2/2] deleted related in activity stream tests/surfacing This adds a test to replicate the scenario reported about bugs in activity stream entry generation in cascade delete chains. Also puts a new string in the entry that uses the deleted objects's primary key. --- .../functional/models/test_activity_stream.py | 19 ++++++++++++++++++- awx/main/utils/common.py | 10 +++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/awx/main/tests/functional/models/test_activity_stream.py b/awx/main/tests/functional/models/test_activity_stream.py index a8e0d4ef87..bdee4b80c1 100644 --- a/awx/main/tests/functional/models/test_activity_stream.py +++ b/awx/main/tests/functional/models/test_activity_stream.py @@ -8,9 +8,18 @@ from awx.main.models import ( Organization, JobTemplate, Credential, - CredentialType + CredentialType, + InventorySource ) +# other AWX +from awx.main.utils import model_to_dict +from awx.api.serializers import InventorySourceSerializer + + +model_serializer_mapping = { + InventorySource: InventorySourceSerializer +} class TestImplicitRolesOmitted: @@ -140,3 +149,11 @@ class TestUserModels: entry = ActivityStream.objects.filter(user=alice)[0] assert entry.operation == 'create' assert json.loads(entry.changes)['password'] == 'hidden' + + +@pytest.mark.django_db +def test_missing_related_on_delete(inventory_source): + old_is = InventorySource.objects.get(name=inventory_source.name) + inventory_source.inventory.delete() + d = model_to_dict(old_is, serializer_mapping=model_serializer_mapping) + assert d['inventory'] == '-{}'.format(old_is.inventory_id) diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index 392ebaa961..5aedf8b216 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -289,7 +289,10 @@ def get_allowed_fields(obj, serializer_mapping): def _convert_model_field_for_display(obj, field_name, password_fields=None): # NOTE: Careful modifying the value of field_val, as it could modify # underlying model object field value also. - field_val = getattr(obj, field_name, None) + try: + field_val = getattr(obj, field_name, None) + except ObjectDoesNotExist: + return '-{}'.format(obj._meta.verbose_name, getattr(obj, '{}_id'.format(field_name))) if password_fields is None: password_fields = set(getattr(type(obj), 'PASSWORD_FIELDS', [])) | set(['password']) if field_name in password_fields: @@ -355,10 +358,7 @@ def model_to_dict(obj, serializer_mapping=None): for field in obj._meta.fields: if field.name not in allowed_fields: continue - try: - attr_d[field.name] = _convert_model_field_for_display(obj, field.name, password_fields=password_fields) - except ObjectDoesNotExist: - logger.warn(_('%s no longer exists for %s') % (field.name, obj)) + attr_d[field.name] = _convert_model_field_for_display(obj, field.name, password_fields=password_fields) return attr_d