Merge pull request #80 from jheld/bugfix/content-type-models-transaction

soynatan · web-flow · commit 5249423f9e5d · 2019-04-04T00:25:41.000-03:00
split user db constraint, content type retrieve and create fix
diff --git a/README.md b/README.md
@@ -59,6 +59,23 @@ Below are some of the settings you may want to use. These should be defined in y
   will be matched against the URL path.
   [Check our wiki](https://github.com/soynatan/django-easy-audit/wiki/Settings#request-auditing)
   for more details on how to use it.
+  
+* `DJANGO_EASY_AUDIT_CRUD_DIFFERENCE_CALLBACKS`
+
+  May point to a list of callables/string-paths-to-functions-classes in which the application code can determine
+  on a per CRUDEvent whether or not the application chooses to create the CRUDEvent or not. This is different
+  from the registered/unregistered settings (e.g. `DJANGO_EASY_AUDIT_UNREGISTERED_CLASSES_EXTRA`). 
+  This is meant to be for dynamic configurations where the application
+  may inspect the current save/create/delete and choose whether or not to save that into the database or ignore it.
+  
+* `DJANGO_EASY_AUDIT_USER_DB_CONSTRAINT`
+
+  Default is `True`. This is reserved for future use (does not do anything yet). The functionality provided by the 
+  setting (whether enabled or disabled) could be handled more explicitly in certain 
+  code paths (or even internally as custom model managers). For projects that separate the easyaudit database, such
+  that the tables are not on the same database as the user table, this could help with making certain queries easier.
+  Again, this doesn't do anything yet, and if it ever does, the version will be increased and the README will be
+  updated accordingly. If you keep your database together (the standard usage), you have nothing to worry about.
 
 ## What does it do
 
diff --git a/easyaudit/migrations/0009_auto_20180314_2225.py b/easyaudit/migrations/0009_auto_20180314_2225.py
@@ -20,6 +20,6 @@ class Migration(migrations.Migration):
         migrations.AlterField(
             model_name='requestevent',
             name='url',
-            field=models.TextField(db_index=True),
+            field=models.TextField(db_index=False),
         ),
     ]
diff --git a/easyaudit/migrations/0011_auto_20181101_1339.py b/easyaudit/migrations/0011_auto_20181101_1339.py
@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11 on 2018-11-01 13:39
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('easyaudit', '0010_repr_text'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='requestevent',
+            name='url',
+            field=models.TextField(),
+        ),
+    ]
diff --git a/easyaudit/migrations/0012_auto_20181018_0012.py b/easyaudit/migrations/0012_auto_20181018_0012.py
@@ -0,0 +1,42 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.12 on 2018-10-18 00:12
+from __future__ import unicode_literals
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('easyaudit', '0011_auto_20181101_1339'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='crudevent',
+            name='user',
+            field=models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL),
+        ),
+        migrations.AlterField(
+            model_name='crudevent',
+            name='content_type',
+            field=models.ForeignKey(to='contenttypes.ContentType', db_constraint=False, on_delete=models.CASCADE),
+        ),
+        migrations.AlterField(
+            model_name='loginevent',
+            name='user',
+            field=models.ForeignKey(blank=True, db_constraint=False, null=True,
+                                    on_delete=django.db.models.deletion.SET_NULL,
+                                    to=settings.AUTH_USER_MODEL),
+        ),
+        migrations.AlterField(
+            model_name='requestevent',
+            name='user',
+            field=models.ForeignKey(blank=True, db_constraint=False, null=True,
+                                    on_delete=django.db.models.deletion.SET_NULL,
+                                    to=settings.AUTH_USER_MODEL),
+        ),
+
+    ]
diff --git a/easyaudit/models.py b/easyaudit/models.py
@@ -21,14 +21,15 @@ class CRUDEvent(models.Model):
 
     event_type = models.SmallIntegerField(choices=TYPES)
     object_id = models.IntegerField()  # we should try to allow other ID types
-    content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE)
+    content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, db_constraint=False)
     object_repr = models.TextField(null=True, blank=True)
     object_json_repr = models.TextField(null=True, blank=True)
     changed_fields = models.TextField(null=True, blank=True)
     user = models.ForeignKey(settings.AUTH_USER_MODEL, null=True,
-                             blank=True, on_delete=models.SET_NULL)
+                             blank=True, on_delete=models.SET_NULL,
+                             db_constraint=False)
     user_pk_as_string = models.CharField(max_length=255, null=True, blank=True,
-                                     help_text='String version of the user pk')
+                                         help_text='String version of the user pk')
     datetime = models.DateTimeField(auto_now_add=True)
 
     def is_create(self):
@@ -59,7 +60,7 @@ class LoginEvent(models.Model):
     login_type = models.SmallIntegerField(choices=TYPES)
     username = models.CharField(max_length=255, null=True, blank=True)
     user = models.ForeignKey(settings.AUTH_USER_MODEL, null=True, blank=True,
-                             on_delete=models.SET_NULL)
+                             on_delete=models.SET_NULL, db_constraint=False)
     remote_ip = models.CharField(max_length=50, null=True, db_index=True)
     datetime = models.DateTimeField(auto_now_add=True)
 
@@ -70,11 +71,11 @@ class Meta:
 
 
 class RequestEvent(models.Model):
-    url = models.TextField(null=False, db_index=True)
+    url = models.TextField(null=False, db_index=False)
     method = models.CharField(max_length=20, null=False, db_index=True)
     query_string = models.TextField(null=True)
     user = models.ForeignKey(settings.AUTH_USER_MODEL, null=True, blank=True,
-                             on_delete=models.SET_NULL)
+                             on_delete=models.SET_NULL, db_constraint=False)
     remote_ip = models.CharField(max_length=50, null=True, db_index=True)
     datetime = models.DateTimeField(auto_now_add=True)
 
diff --git a/easyaudit/settings.py b/easyaudit/settings.py
@@ -30,6 +30,8 @@ def get_model_list(class_list):
 WATCH_REQUEST_EVENTS = getattr(settings, 'DJANGO_EASY_AUDIT_WATCH_REQUEST_EVENTS', True)
 REMOTE_ADDR_HEADER = getattr(settings, 'DJANGO_EASY_AUDIT_REMOTE_ADDR_HEADER', 'REMOTE_ADDR')
 
+USER_DB_CONSTRAINT = bool(getattr(settings, 'DJANGO_EASY_AUDIT_USER_DB_CONSTRAINT', True))
+
 
 # Models which Django Easy Audit will not log.
 # By default, all but some models will be audited.
diff --git a/easyaudit/signals/auth_signals.py b/easyaudit/signals/auth_signals.py
@@ -10,7 +10,7 @@ def user_logged_in(sender, request, user, **kwargs):
         with transaction.atomic():
             login_event = LoginEvent.objects.create(login_type=LoginEvent.LOGIN,
                                      username=getattr(user, user.USERNAME_FIELD),
-                                     user=user,
+                                     user_id=getattr(user, 'id', None),
                                      remote_ip=request.META[REMOTE_ADDR_HEADER])
     except:
         pass
@@ -20,8 +20,8 @@ def user_logged_out(sender, request, user, **kwargs):
     try:
         with transaction.atomic():
             login_event = LoginEvent.objects.create(login_type=LoginEvent.LOGOUT,
-                                                    username=getattr(user, user.USERNAME_FIELD),
-                                                    user=user,
+                                                    username=getattr(user, user.USERNAME_FIELD, None),
+                                                    user_id=getattr(user, 'id', None),
                                                     remote_ip=request.META[REMOTE_ADDR_HEADER])
     except:
         pass
diff --git a/easyaudit/signals/model_signals.py b/easyaudit/signals/model_signals.py
@@ -10,14 +10,13 @@
 from django.utils import timezone
 from django.utils.encoding import force_text
 
-from easyaudit.middleware.easyaudit import get_current_request,\
-                                           get_current_user
+from easyaudit.middleware.easyaudit import get_current_request, \
+    get_current_user
 from easyaudit.models import CRUDEvent
-from easyaudit.settings import REGISTERED_CLASSES, UNREGISTERED_CLASSES,\
-                               WATCH_MODEL_EVENTS, CRUD_DIFFERENCE_CALLBACKS
+from easyaudit.settings import REGISTERED_CLASSES, UNREGISTERED_CLASSES, \
+    WATCH_MODEL_EVENTS, CRUD_DIFFERENCE_CALLBACKS
 from easyaudit.utils import model_delta
 
-
 logger = logging.getLogger(__name__)
 
 
@@ -46,14 +45,18 @@ def should_audit(instance):
 def pre_save(sender, instance, raw, using, update_fields, **kwargs):
     """https://docs.djangoproject.com/es/1.10/ref/signals/#post-save"""
     if raw:
-      # Return if loading Fixtures      
-      return
-    
+        # Return if loading Fixtures
+        return
+
     try:
         with transaction.atomic():
             if not should_audit(instance):
                 return False
-            object_json_repr = serializers.serialize("json", [instance])
+            try:
+                object_json_repr = serializers.serialize("json", [instance])
+            except Exception:
+                # We need a better way for this to work. ManyToMany will fail on pre_save on create
+                return None
 
             if instance.pk is None:
                 created = True
@@ -80,32 +83,41 @@ def pre_save(sender, instance, raw, using, update_fields, **kwargs):
 
             # callbacks
             kwargs['request'] = get_current_request()  # make request available for callbacks
-            create_crud_event = all(callback(instance, object_json_repr, created, raw, using, update_fields, **kwargs)
-                                    for callback in CRUD_DIFFERENCE_CALLBACKS if callable(callback))
-
+            create_crud_event = all(
+                callback(instance, object_json_repr, created, raw, using, update_fields, **kwargs)
+                for callback in CRUD_DIFFERENCE_CALLBACKS if callable(callback))
             # create crud event only if all callbacks returned True
             if create_crud_event and not created:
-                crud_event = CRUDEvent.objects.create(
-                    event_type=event_type,
-                    object_repr=str(instance),
-                    object_json_repr=object_json_repr,
-                    changed_fields=changed_fields,
-                    content_type=ContentType.objects.get_for_model(instance),
-                    object_id=instance.pk,
-                    user=user,
-                    datetime=timezone.now(),
-                    user_pk_as_string=str(user.pk) if user else user
-                )
+                c_t = ContentType.objects.get_for_model(instance)
+                sid = transaction.savepoint()
+                try:
+                    with transaction.atomic():
+                        crud_event = CRUDEvent.objects.create(
+                            event_type=event_type,
+                            object_repr=str(instance),
+                            object_json_repr=object_json_repr,
+                            changed_fields=changed_fields,
+                            content_type_id=c_t.id,
+                            object_id=instance.pk,
+                            user_id=getattr(user, 'id', None),
+                            datetime=timezone.now(),
+                            user_pk_as_string=str(user.pk) if user else user
+                        )
+                except Exception as e:
+                    logger.exception(
+                        "easy audit had a pre-save exception on CRUDEvent creation. instance: {}, instance pk: {}".format(
+                            instance, instance.pk))
+                    transaction.savepoint_rollback(sid)
     except Exception:
         logger.exception('easy audit had a pre-save exception.')
 
 
 def post_save(sender, instance, created, raw, using, update_fields, **kwargs):
     """https://docs.djangoproject.com/es/1.10/ref/signals/#post-save"""
     if raw:
-      # Return if loading Fixtures      
-      return
-    
+        # Return if loading Fixtures
+        return
+
     try:
         with transaction.atomic():
             if not should_audit(instance):
@@ -130,22 +142,31 @@ def post_save(sender, instance, created, raw, using, update_fields, **kwargs):
             # callbacks
             kwargs['request'] = get_current_request()  # make request available for callbacks
             create_crud_event = all(callback(instance, object_json_repr,
-                                    created, raw, using, update_fields, **kwargs)
+                                             created, raw, using, update_fields, **kwargs)
                                     for callback in CRUD_DIFFERENCE_CALLBACKS
                                     if callable(callback))
 
             # create crud event only if all callbacks returned True
             if create_crud_event and created:
-                crud_event = CRUDEvent.objects.create(
-                    event_type=event_type,
-                    object_repr=str(instance),
-                    object_json_repr=object_json_repr,
-                    content_type=ContentType.objects.get_for_model(instance),
-                    object_id=instance.pk,
-                    user=user,
-                    datetime=timezone.now(),
-                    user_pk_as_string=str(user.pk) if user else user
-                )
+                c_t = ContentType.objects.get_for_model(instance)
+                sid = transaction.savepoint()
+                try:
+                    with transaction.atomic():
+                        crud_event = CRUDEvent.objects.create(
+                            event_type=event_type,
+                            object_repr=str(instance),
+                            object_json_repr=object_json_repr,
+                            content_type_id=c_t.id,
+                            object_id=instance.pk,
+                            user_id=getattr(user, 'id', None),
+                            datetime=timezone.now(),
+                            user_pk_as_string=str(user.pk) if user else user
+                        )
+                except Exception as e:
+                    logger.exception(
+                        "easy audit had a pre-save exception on CRUDEvent creation. instance: {}, instance pk: {}".format(
+                            instance, instance.pk))
+                    transaction.savepoint_rollback(sid)
     except Exception:
         logger.exception('easy audit had a post-save exception.')
 
@@ -161,8 +182,8 @@ def _m2m_rev_field_name(model1, model2):
     m2m_field_names = [
         rel.get_accessor_name() for rel in model1._meta.get_fields()
         if rel.many_to_many
-        and rel.auto_created
-        and rel.related_model == model2
+           and rel.auto_created
+           and rel.related_model == model2
     ]
     return m2m_field_names[0]
 
@@ -206,17 +227,26 @@ def m2m_changed(sender, instance, action, reverse, model, pk_set, using, **kwarg
 
             if isinstance(user, AnonymousUser):
                 user = None
+            c_t = ContentType.objects.get_for_model(instance)
+            sid = transaction.savepoint()
 
-            crud_event = CRUDEvent.objects.create(
-                event_type=event_type,
-                object_repr=str(instance),
-                object_json_repr=object_json_repr,
-                content_type=ContentType.objects.get_for_model(instance),
-                object_id=instance.pk,
-                user=user,
-                datetime=timezone.now(),
-                user_pk_as_string=str(user.pk) if user else user
-            )
+            try:
+                with transaction.atomic():
+                    crud_event = CRUDEvent.objects.create(
+                        event_type=event_type,
+                        object_repr=str(instance),
+                        object_json_repr=object_json_repr,
+                        content_type_id=c_t.id,
+                        object_id=instance.pk,
+                        user_id=getattr(user, 'id', None),
+                        datetime=timezone.now(),
+                        user_pk_as_string=str(user.pk) if user else user
+                    )
+            except Exception as e:
+                logger.exception(
+                    "easy audit had a pre-save exception on CRUDEvent creation. instance: {}, instance pk: {}".format(
+                        instance, instance.pk))
+                transaction.savepoint_rollback(sid)
     except Exception:
         logger.exception('easy audit had an m2m-changed exception.')
 
@@ -240,18 +270,27 @@ def post_delete(sender, instance, using, **kwargs):
 
             if isinstance(user, AnonymousUser):
                 user = None
-
-            # crud event
-            crud_event = CRUDEvent.objects.create(
-                event_type=CRUDEvent.DELETE,
-                object_repr=str(instance),
-                object_json_repr=object_json_repr,
-                content_type=ContentType.objects.get_for_model(instance),
-                object_id=instance.pk,
-                user=user,
-                datetime=timezone.now(),
-                user_pk_as_string=str(user.pk) if user else user
-            )
+            c_t = ContentType.objects.get_for_model(instance)
+            sid = transaction.savepoint()
+            try:
+                with transaction.atomic():
+                    # crud event
+                    crud_event = CRUDEvent.objects.create(
+                        event_type=CRUDEvent.DELETE,
+                        object_repr=str(instance),
+                        object_json_repr=object_json_repr,
+                        content_type_id=c_t.id,
+                        object_id=instance.pk,
+                        user_id=getattr(user, 'id', None),
+                        datetime=timezone.now(),
+                        user_pk_as_string=str(user.pk) if user else user
+                    )
+
+            except Exception as e:
+                logger.exception(
+                    "easy audit had a pre-save exception on CRUDEvent creation. instance: {}, instance pk: {}".format(
+                        instance, instance.pk))
+                transaction.savepoint_rollback(sid)
     except Exception:
         logger.exception('easy audit had a post-delete exception.')
 
diff --git a/easyaudit/signals/request_signals.py b/easyaudit/signals/request_signals.py

Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,6 @@ class Migration(migrations.Migration):`
`20`	`20`	`migrations.AlterField(`
`21`	`21`	`model_name='requestevent',`
`22`	`22`	`name='url',`
`23`		`- field=models.TextField(db_index=True),`
	`23`	`+ field=models.TextField(db_index=False),`
`24`	`24`	`),`
`25`	`25`	`]`