vvangelovski / django-audit-log

Audit log for your Django models
Other
232 stars 93 forks source link

Django 1.8 compatibility #21

Open mxsasha opened 9 years ago

mxsasha commented 9 years ago

On Django 1.8, the presence of an AuditLog on a model which has foreign keys to any other model, appears to cause Django check failures regarding reverse name clashes, e.g.:

letter.Letter.sender: (fields.E304) Reverse accessor for 'Letter.sender' clashes with reverse accessor for 'Letter.sender'.
        HINT: Add or change a related_name argument to the definition for 'Letter.sender' or 'Letter.sender'.

Removing the audit log removes this error. I'm not entirely sure where to look for this bug.

sagrawal-code commented 9 years ago

Is Letter.sender a 'User'?

mxsasha commented 9 years ago

Nope, it's another model in another app (also managed by migrations, and also has an AuditLog attached, in case that matters). I also see the error for other models where the FK does refer to a (standard) django.contrib.auth.models.User.

Also, I realised that I omitted something: the exact same error occurs for the LetterAuditLogEntry model.

wkang0 commented 9 years ago

Any plan to fix the Django 1.8 compatibility issue?

richardjmarini commented 9 years ago

having the same problem:

users.UserAllergy.users_with_allergy: (fields.E304) Reverse accessor for 'UserAllergy.users_with_allergy' clashes with reverse accessor for 'UserAllergy.users_with_allergy'. HINT: Add or change a related_name argument to the definition for 'UserAllergy.users_with_allergy' or 'UserAllergy.users_with_allergy'. users.UserAllergyAuditLogEntry.users_with_allergy: (fields.E304) Reverse accessor for 'UserAllergyAuditLogEntry.users_with_allergy' clashes with reverse accessor for 'UserAllergy.users_with_allergy'. HINT: Add or change a related_name argument to the definition for 'UserAllergyAuditLogEntry.users_with_allergy' or 'UserAllergy.users_with_allergy'.

brianmay commented 9 years ago

I filled a bug report about this against Django. https://code.djangoproject.com/ticket/24768 - it was somewhat confusing because the reverse accessor produced results of the wrong type, and I didn't realize at first there was a clash.

I also have test code available to reproduce the issue: https://github.com/brianmay/django-24768

wkang0 commented 9 years ago

It seems that the ticket 24768 has been closed. Now what? Can anybody figure out a solution for this?

richardjmarini commented 9 years ago

I was able to resolve this error by making sure I gave ALL of my Foreign-Key relationships a distinct 'related_name': https://docs.djangoproject.com/en/1.8/ref/models/fields/

brianmay commented 9 years ago

@richardjmarini There is the following code in the copy_fields method:

                if field.rel and field.rel.related_name:
                    field.rel.related_name = '_auditlog_%s' % field.rel.related_name

My tests sure that normally this doesn't get executed. Am wondering if what you have done is provide an initial value for related_name, so that does get executed.

There are a number of nasty bugs open against this project, and I don't see any responses from the project owner. Am seriously thinking it might be better to switch to something better supported, e.g. https://github.com/etianen/django-reversion.

wkang0 commented 9 years ago

The issue can be solved by updating the code mentioned on brianmay's comments.

            if field.rel and field.rel.get_accessor_name():
                field.rel.related_name = '_auditlog_%s' % field.rel.get_accessor_name()
brianmay commented 9 years ago

This change seems to have broken Django 1.7 support :-(

Traceback (most recent call last):
  File "./manage.py", line 24, in <module>
    management.execute_from_command_line()
  File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 354, in execute
    django.setup()
  File "/usr/lib/python2.7/dist-packages/django/__init__.py", line 21, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/usr/lib/python2.7/dist-packages/django/apps/registry.py", line 108, in populate
    app_config.import_models(all_models)
  File "/usr/lib/python2.7/dist-packages/django/apps/config.py", line 202, in import_models
    self.models_module = import_module(models_module_name)
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/home/brian/tree/django/karaage/karaage/karaage/models.py", line 21, in <module>
    import karaage.people.models  # NOQA
  File "/home/brian/tree/django/karaage/karaage/karaage/people/models.py", line 48, in <module>
    class Person(AbstractBaseUser):
  File "/usr/lib/python2.7/dist-packages/django/db/models/base.py", line 282, in __new__
    new_class._prepare()
  File "/usr/lib/python2.7/dist-packages/django/db/models/base.py", line 342, in _prepare
    signals.class_prepared.send(sender=cls)
  File "/usr/lib/python2.7/dist-packages/django/dispatch/dispatcher.py", line 198, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/home/brian/tmp/django-audit-log/audit_log/models/managers.py", line 120, in finalize
    log_entry_model = self.create_log_entry_model(sender)
  File "/home/brian/tmp/django-audit-log/audit_log/models/managers.py", line 247, in create_log_entry_model
    attrs = self.copy_fields(model)
  File "/home/brian/tmp/django-audit-log/audit_log/models/managers.py", line 169, in copy_fields
    if field.rel and field.rel.get_accessor_name():
AttributeError: 'ManyToOneRel' object has no attribute 'get_accessor_name'
brianmay commented 9 years ago

Of course, you could argue that Django 1.7 support was already broken... None of these related_name values were getting set. It seems under Django 1.7 the defaults were sane values that didn't conflict; under Django 1.8 it uses the values from the original class that do conflict.

Django < 1.8 support isn't absolutely required for me (at least not in this package), however might be required for other people if they still have Django South migrations pending (which requires Django 1.6).

wkang0 commented 9 years ago

Then the following change will fix the 1.7 issue:

           if field.rel and field.rel.related_name:
                field.rel.related_name = '_auditlog_%s' % field.rel.related_name                
            elif field.rel: 
                try:
                    if field.rel.get_accessor_name():
                        field.rel.related_name = '_auditlog_%s' % field.rel.get_accessor_name()
                except:
                    pass
brianmay commented 9 years ago

@wkang0 No, that still won't work (as desired) under Django 1.7, for exactly the same reasons that the original code didn't work properly under Django 1.7 (it didn't crash, but it didn't work either).

Under Django 1.7: field.rel.related_name == None

I suspect this might be different if you pass a related_name field to the constructor of the original constructor.

richardhi commented 9 years ago

Are there any plans to make this fix part of a future release version?

vvangelovski commented 9 years ago

Yes, I plan to work on a fix this week.