search

soynatan / django-easy-audit

Yet another Django audit log app, hopefully the simplest one.
GNU General Public License v3.0
735 stars 182 forks source link

Replace index_together with indexes in CRUDEvent model #258

Closed mschoettle closed 7 months ago

mschoettle commented 1 year ago

CRUDEvent has Meta.index_together specified. The use of index_together is deprecated as of Django 4.2 (see https://docs.djangoproject.com/en/4.2/ref/models/options/#index-together).

It emits the following warning:

django.utils.deprecation.RemovedInDjango51Warning: 'index_together' is deprecated. Use 'Meta.indexes' in 'easyaudit.CRUDEvent' instead.

This PR replaces index_together with indexes. Note that as a result the index becomes named:

Rename unnamed index for ('object_id', 'content_type') on crudevent to easyaudit_c_object__82020b_idx

If desired we can specify a specific name for the index.

mschoettle commented 1 year ago

Hm, looks like RenameIndex was only added in Django 4.1: https://docs.djangoproject.com/en/4.2/ref/migration-operations/#renameindex

On Django 3.2 the following operations are created:

operations = [
        migrations.AlterIndexTogether(
            name='crudevent',
            index_together=set(),
        ),
        migrations.AddIndex(
            model_name='crudevent',
            index=models.Index(fields=['object_id', 'content_type'], name='easyaudit_c_object__82020b_idx'),
        ),
    ]

But this would drop and add the index which is quite an expensive operation.

Are there any plans to drop support for older Django versions for newer versions of this package?

mschoettle commented 1 year ago

@jheld Can we drop Django 3.2 in a future version? It is going to be end of life soon anyway (April 2024).

mschoettle commented 11 months ago

With Django 5 being in beta, is there any chance we can move this forward? I suggest to drop support for older Django versions in a new version which allows this change to be included in a new version. Many Django packages are currently adding support for Django 5.0.

What are your thoughts, @jheld?

jheld commented 11 months ago

@mschoettle

I'm not sure I agree that we'd need need to drop any versions of django just because of this proposed change (by itself). If there are unsupported versions that we allow which add cruft to the codebase, certainly, we'd want to make that happen. Regarding unreleased versions of django, we are under no commitment to make that first-class, though I concur being implicitly friendly toward it is fine in the general sense. I would welcome a PR which would gracefully raise the upper django version limit to allow for that.

Regarding some parts of this PR, can you check on the build failures, and we can go from there regarding plan for merge?

mschoettle commented 11 months ago

@jheld Thanks for your quick response. I understand your view. Now that I checked a few packages, it was a bit harsh to suggest dropping support for Django 3.2 right now.

The issue in this PR is that the migration operation needed (RenameIndex) was only added in Django 4.1. We could use the operations that Django 3.2 generates (see comment above) though it removes the old index and adds a new one which we should probably avoid.

Maybe the best way forward is to hold off with this change. Depending on the philosophy of this package regarding supported Django versions it might be best to wait until Django 3.2 reached end of life and do it then.

I'll create a separate PR to ensure that the pipeline also runs for Python 3.12 and Django 5.0.

mschoettle commented 11 months ago

@jheld See #264 and #265 for Django 5.0 support

I just saw that in the Django 5.0, they recommend third-party app authors to drop support for Django < 4.2: https://docs.djangoproject.com/en/5.0/releases/5.0/#third-party-library-support-for-older-version-of-django

samamorgan commented 9 months ago

@jheld Thanks for your quick response. I understand your view. Now that I checked a few packages, it was a bit harsh to suggest dropping support for Django 3.2 right now.

The issue in this PR is that the migration operation needed (RenameIndex) was only added in Django 4.1. We could use the operations that Django 3.2 generates (see comment above) though it removes the old index and adds a new one which we should probably avoid.

Maybe the best way forward is to hold off with this change. Depending on the philosophy of this package regarding supported Django versions it might be best to wait until Django 3.2 reached end of life and do it then.

I'll create a separate PR to ensure that the pipeline also runs for Python 3.12 and Django 5.0.

Note that I've solved all of this in #267. I don't see any real concern from having to regenerate the index, it'll just take a little bit of time, but that sort of operation is normal and expected.

I created an additional issue for dropping Django <4.2 support, which should be done in April to coincide with Django's schedule. This presents no problem, users on older versions of Django can simply use an older version of easyaudit if they can't upgrade.

mschoettle commented 9 months ago

Thanks for fixing it and creating the extra issue (#269 for reference).

mschoettle commented 7 months ago

Created #277 which this PR is dependant on.

jheld commented 7 months ago

@mschoettle sorry looks like we have some sort of test build failure, can you take a look when you have some time?

mschoettle commented 7 months ago

@jheld Brought this up to date with master which resolved the pipeline failure