mathesar-foundation / mathesar

Web application providing an intuitive user experience to databases.
https://mathesar.org/
GNU General Public License v3.0
2.35k stars 323 forks source link

`makemigrations` always creates a new migration #1935

Closed dmos62 closed 1 year ago

dmos62 commented 1 year ago

Multiple people reported that running docker exec mathesar_service sh -c "python manage.py makemigrations" always creates a new migration, even if you run it multiple times in sequence, that looks like this:

# Generated by Django 3.1.14 on 2022-11-15 13:18

from django.db import migrations, models
import mathesar.models.query

class Migration(migrations.Migration):

    dependencies = [
        ('mathesar', '0007_auto_20221115_1318'),
    ]

    operations = [
        migrations.AlterField(
            model_name='uiquery',
            name='display_options',
            field=models.JSONField(blank=True, null=True, validators=[mathesar.models.query._get_validator_for_dict]),
        ),
        migrations.AlterField(
            model_name='uiquery',
            name='initial_columns',
            field=models.JSONField(validators=[mathesar.models.query._get_validator_for_list_of_dicts, mathesar.models.query._get_validator_for_initial_columns]),
        ),
        migrations.AlterField(
            model_name='uiquery',
            name='transformations',
            field=models.JSONField(blank=True, null=True, validators=[mathesar.models.query._get_validator_for_list_of_dicts, mathesar.models.query._get_validator_for_transformations]),
        ),
    ]

Subsequent runs create migrations that have the previous migration as its dependency. That's the only difference between these redundant migrations.

The STDOUT is:

Migrations for 'mathesar':
  mathesar/migrations/0008_auto_20221115_1318.py
    - Alter field display_options on uiquery
    - Alter field initial_columns on uiquery
    - Alter field transformations on uiquery
dmos62 commented 1 year ago

My untested theory is that this has to do with validations. Some fields on UIQuery declare validators and they are the fields mentioned in the generated migration. We use validator factories to create those validators, which might be messing up Django's introspection.

I'll note that it's unclear to me why migrations should care about field validators.

Anish9901 commented 1 year ago

Here is a link for a SO post similar to this issue: https://stackoverflow.com/questions/55089705/django-keeps-creating-new-migrations-even-when-model-has-not-changed

dmos62 commented 1 year ago

Update with comments by me and Mukesh:

The problem is the validation functions for some of the fields of UIQuery are being generated via a factory.

@silentninja adds:

The migrations bug is same as https://github.com/centerofci/mathesar/pull/2082. There is no way for Django to verify if it is the same function/

That implies that if we move the generation of validation functions out of the declaration, so that a given field will always have the same function referenced as its validator, that will fix endless migrations.

aagmanbhatt commented 1 year ago

@dmos62 why are we using wraps decoration in validators?

dmos62 commented 1 year ago

@aagman945 I don't recall the details, but I left a code comment next to their usages saying that it's needed to interop with Django's migrations.

A solution to this might be to instantiate each of the validator functions globally (top of the file) and then pass them to UIQuery fields (instead of instantiating them inside the field declarations). What's annoying about that is that you then redundantly hardcode field names outside their definition. Which is why I used factories (which apparently broke migration detection).

aagmanbhatt commented 1 year ago

@dmos I tried instantiating validator globally it didn't worked. We to use class based validator, and __eq__ method will take care of duplicate migrations. I tried this approach and it fixes our problem.

dmos62 commented 1 year ago

@aagman945 I wonder why that didn't work. Not sure what you mean about __eq__. If you submit a PR and we can discuss implementation there.

aagmanbhatt commented 1 year ago

@dmos62 I've created a PR with changes.