Opened 4 years ago

Closed 4 years ago

#32676 closed Bug (fixed)

Migration autodetector changes related_name for self-referential ManyToManyField.

Reported by: Mariusz Felisiak Owned by: David Wobrock
Component: Migrations Version: 4.0
Severity: Release blocker Keywords:
Cc: David Wobrock, Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Migration autodetector no longer adds a model name to the related_name attribute for self-referential ManyToManyField, e.g. for a field

class MyModel2(models.Model):
    field_3 = models.ManyToManyField('self')

it creates a migration with related_name='field_3_rel_+' instead of related_name='_mymodel2_field_3_+'.

Regression in aa4acc164d1247c0de515c959f7b09648b57dc42 (see #29899).

Change History (9)

comment:1 by Mariusz Felisiak, 4 years ago

Component: Database layer (models, ORM)Migrations

comment:2 by David Wobrock, 4 years ago

Hi,

Similarly to #32675, on a first and very quick look, the ManyToManyField has some logic at django.db.models.fields.related.ManyToManyField.contribute_to_class that looks very much like the names described in the ticket itself:

    def contribute_to_class(self, cls, name, **kwargs):
        # To support multiple relations to self, it's useful to have a non-None
        # related name on symmetrical relations for internal reasons. The
        # concept doesn't make a lot of sense externally ("you want me to
        # specify *what* on my non-reversible relation?!"), so we set it up
        # automatically. The funky name reduces the chance of an accidental
        # clash.
        if self.remote_field.symmetrical and (
            self.remote_field.model == RECURSIVE_RELATIONSHIP_CONSTANT or
            self.remote_field.model == cls._meta.object_name
        ):
            self.remote_field.related_name = "%s_rel_+" % name
        elif self.remote_field.is_hidden():
            # If the backwards relation is disabled, replace the original
            # related_name with one generated from the m2m field name. Django
            # still uses backwards relations internally and we need to avoid
            # clashes between multiple m2m fields with related_name == '+'.
            self.remote_field.related_name = '_%s_%s_%s_+' % (
                cls._meta.app_label,
                cls.__name__.lower(),
                name,
            )

Since _meta should not be available anymore, I expect that the result of the self.remote_field.model == cls._meta.object_name condition changed, because we should not be able to use cls._meta.app_label.

I'm open to help if we have an idea about how we can fix this :)

comment:3 by Simon Charette, 4 years ago

Triage Stage: UnreviewedAccepted

That seems like a flaw of RelatedField.deconstruct tbh, it should not include related_name and friends if it was not specified at initialization.

I suggest we make RelatedField.__init__ store related_name and related_query_name as self._related_name and self._related_query_name and use them instead of relying on self.remote_field in deconstruct. That's the approach we've taken for post-initialization alterable attributes in Field for example.

comment:4 by David Wobrock, 4 years ago

Has patch: set
Needs tests: set
Owner: changed from nobody to David Wobrock
Patch needs improvement: set
Status: newassigned

Started working on a patch https://github.com/django/django/pull/14324
but still Work In Progress

comment:5 by David Wobrock, 4 years ago

Needs tests: unset
Patch needs improvement: unset

Some discussions occurred on the Pull Request: https://github.com/django/django/pull/14324#issuecomment-843038110

Added tests and updated release note, patch is ready for review :) Thanks!

comment:6 by Mariusz Felisiak, 4 years ago

Needs tests: set

comment:7 by David Wobrock, 4 years ago

Needs tests: unset

comment:8 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In b9df2b7:

Fixed #32676 -- Prevented migrations from rendering related field attributes when not passed during initialization.

Thanks Simon Charette for the implementation idea.

Note: See TracTickets for help on using tickets.
Back to Top