wemake-services / django-test-migrations

Test django schema and data migrations, including migrations' order and best practices.
https://pypi.org/project/django-test-migrations/
MIT License
517 stars 31 forks source link

`through_fields` not reflected in test models #418

Open jaredlockhart opened 8 months ago

jaredlockhart commented 8 months ago

Hi,

I just stumbled on a weird little edge case. I am migrating a ManyToMany field over to a through model, and specifying both through and through_fields on the ManyToMany field on the parent model, example:

class ThroughModel(models.Model):
    parent_experiment = models.ForeignKey(
        "NimbusExperiment",
        on_delete=models.CASCADE,
    )
    child_experiment = models.ForeignKey(
        "NimbusExperiment",
        on_delete=models.CASCADE,
    )
    branch_slug = models.SlugField(
        max_length=NimbusConstants.MAX_SLUG_LEN, null=True, blank=True
    )

class NimbusExperiment(models.Model):
    required_experiments = models.ManyToManyField["NimbusExperiment"](
        "NimbusExperiment",
        related_name="required_by",
        blank=True,
        verbose_name="Required Experiments",
        through=ThroughModel,
        through_fields=("parent_experiment", "child_experiment"),
    )

and then using this package to write a test roughly like

    def prepare(self):
        """Prepare some data before the migration."""
        User = self.old_state.apps.get_model("auth", "User")
        NimbusExperiment = self.old_state.apps.get_model(
            "experiments", "NimbusExperiment"
        )

        user = User.objects.create(email="test@example.com")

        parent_experiment = NimbusExperiment.objects.create(
            owner=user,
            name="test parent experiment",
            slug="test-parent-experiment",
            application=NimbusConstants.Application.DESKTOP,
            status=NimbusConstants.Status.DRAFT,
            publish_status=NimbusConstants.PublishStatus.IDLE,
            published_dto="{}",
        )
        required_experiment = NimbusExperiment.objects.create(
            owner=user,
            name="test required experiment",
            slug="test-required-experiment",
            application=NimbusConstants.Application.DESKTOP,
            status=NimbusConstants.Status.DRAFT,
            publish_status=NimbusConstants.PublishStatus.IDLE,
            published_dto="{}",
        )
        parent_experiment.required_experiments.add(required_experiment)

    def test_migration(self):
        """Run the test itself."""
        NimbusExperiment = self.new_state.apps.get_model(
            "experiments", "NimbusExperiment"
        )

        parent_experiment = NimbusExperiment.objects.get(slug="test-parent-experiment")

        self.assertEqual(
            set(
                parent_experiment.required_experiments.all().values_list(
                    "slug", flat=True
                )
            ),
            {"test-required-experiment"},
        )

however this test was failing and I couldn't see any obvious reason why. Digging deeper I found that the generated SQL for the query parent_experiment.required_experiments.all() in the test looked like

...WHERE "experiments_nimbusexperimentbranchthroughrequired"."child_experiment_id" = 1

whereas it should be

...WHERE "experiments_nimbusexperimentbranchthroughrequired"."parent_experiment_id" = 1

and what's interesting is that I get the correct behaviour in ./manage.py shell but not if I pdb inside the test, so something is going wrong inside the test.

Further investigation lead me to find that inside the test I see

ipdb> required_field = NimbusExperiment._meta.many_to_many[-1]
ipdb> required_field
<django.db.models.fields.related.ManyToManyField: required_experiments>
ipdb> required_field.remote_field.through_fields
ipdb> required_field.remote_field.through_fields is None
True

whereas in ./manage.py shell I see

In [9]: required_field.remote_field.through_fields
Out[9]: ('parent_experiment', 'child_experiment')

so somehow the through_fields is None inside the test context and not the tuple it should be, which is causing the relevant method in django here to fall back to the case of looping over fields and finding the first one that matches the model, and finding child_experiment first as we can see here inside a pdb inside the test:

ipdb> NimbusExperimentBranchThroughRequired._meta.fields
(<django.db.models.fields.AutoField: id>, <django.db.models.fields.SlugField: branch_slug>, <django.db.models.fields.related.ForeignKey: child_experiment>, <django.db.models.fields.related.ForeignKey: parent_experiment>)

So I was able to rectify this by explicitly setting through_fields like so in the test:

    def test_migration(self):
        """Run the test itself."""
        NimbusExperiment = self.new_state.apps.get_model(
            "experiments", "NimbusExperiment"
        )
        required_field = next(
            f
            for f in NimbusExperiment._meta.many_to_many
            if f.name == "required_experiments"
        )
        required_field.remote_field.through_fields = (
            "parent_experiment",
            "child_experiment",
        )

        parent_experiment = NimbusExperiment.objects.get(slug="test-parent-experiment")

        self.assertEqual(
            set(
                parent_experiment.required_experiments.all().values_list(
                    "slug", flat=True
                )
            ),
            {"test-required-experiment"},
        )

but I also wanted to file it here. Looking through this repo I couldn't find anything that would clearly cause this behaviour, so I'm not sure why through_fields would be None inside the test context, but hopefully the maintainers of this project will have a better idea.

Aside from that I just want to say thank you for this library and we use it constantly and it's made testing migrations much easier 🙏

Oh and this is on django-test-migrations 1.3.0 and Django 3.2.23 (I know this is old)