Opened 5 years ago

Closed 34 hours ago

#17002 closed Bug (fixed)

ManyToManyField through a model which extends some other model

Reported by: Mitar Owned by: InvalidInterrupt
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: mmitar@…, gm.lawr@… 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

It seems that currently it is impossible to create a ManyToManyField through a model which extends some other model. Django assumes that all fields are accessible in the given model and does not traverse the model hierarchy. This means that it is impossible to use inheritance to prevent data (and code) duplication and have a normalized database.

Change History (14)

comment:1 Changed 5 years ago by Sebastian Goll

Triage Stage: UnreviewedDesign decision needed
Type: New featureBug
Version: 1.3SVN

Django expects the foreign key attributes to be present in the intermediary model given as the through= argument (and not some non-abstract parent) when constructing the related queries. On the other hand, model validation and table creation works even when the foreign key attributes are in the base class, or split over base and derived class as in e.g.

class A(models.Model):
    pass
class B(models.Model):
    a_s = models.ManyToManyField(A, through='Derived')

class Base(models.Model):
    a = models.ForeignKey(A)
class Derived(Base):
    b = models.ForeignKey(B)

The example validates just fine and even generates the appropriate database structure, but related queries fail during runtime, e.g. B.objects.get(pk=1).a_s.all() throws "DatabaseError: no such column: app_derived.a_id".

Can you give an example where foreign keys in the base class of the intermediary model (as opposed to arbitrary attributes, or methods) are necessary or would be useful?

In any case, I think the behavior here should be consistent. If the query cannot be created (because attributes are expected to be present in the derived table), then the validation of the model should not succeed. Or otherwise, the query should take attributes imported from base classes into account and insert the necessary joins.

comment:2 Changed 5 years ago by Mitar

The concrete example where we discovered this is when we wanted to make a schema other users could extend. For exmaple, we have defined something like;

class Node(models.Model):
    children = models.ManyToManyField(Child, through='Link')

class Child(models.Model):
    pass

class Link(models.Model):
    source = models.ForeignKey(Node)
    destination = models.ForeignKey(Child)

And then we wanted to provide users a way to extend our schema with their own applications/modules. So that they can instead derive their own schema, like:

class BetterNode(Node):
    better_children = models.ManyToManyField(Child, through='BetterLink')

class BetterLink(Link):
    weight = models.IntegerField()

And then old applications could still use the old fields, while users' extended applications could know how to handle this additional fields.

comment:3 Changed 4 years ago by Alex Gaynor

Triage Stage: Design decision neededAccepted

Yeah this is a bug (at the very minimum a docs one, although I think it's definitely a "true" bug), I'm not sure how much of a pain it'll be to fix off hand.

comment:4 Changed 4 months ago by InvalidInterrupt

Cc: gm.lawr@… added
Version: master1.9

I recently encountered this bug on a project I am working on.

I think that we may be able to fix the issue by changing django.db.models.fields.related.ManyToManyField._get_path_info to use some logic from django.db.models.sql.query.Query.names_to_path (lines 1334-1349 in commit 698be78). To keep things maintainable, I'd like to split that logic out into a separate function (e.g. get_path_to_parent_model(model, parent)). Could someone with more familiarity with the codebase please suggest an appropriate place for this function to live?

comment:5 Changed 4 months ago by Tim Graham

Possibly django/db/models/query_utils.py, but it'll be easier to tell upon seeing the pull request so don't worry too much about it now.

comment:6 Changed 4 months ago by InvalidInterrupt

Owner: changed from nobody to InvalidInterrupt
Status: newassigned

comment:7 Changed 4 months ago by InvalidInterrupt

Has patch: set
Patch needs improvement: set
Version: 1.9master

Patch work is occurring on my topic branch at https://github.com/InvalidInterrupt/django/tree/ticket_17002. I need to add tests to confirm proxy models are handled correctly.

comment:8 Changed 4 months ago by InvalidInterrupt

Patch needs improvement: unset

I've run the test suite for the py3, py2, and py3-posgtres environments successfully.

comment:9 Changed 4 months ago by InvalidInterrupt

Patch needs improvement: set

Found that prefetch_related is also broken.

comment:10 Changed 4 months ago by InvalidInterrupt

Patch needs improvement: unset

comment:11 Changed 3 months ago by Tim Graham

Patch needs improvement: set

I left some comments for improvement. Please uncheck "Patch needs improvement" after updating the pull request.

comment:12 Changed 2 months ago by InvalidInterrupt

Patch needs improvement: unset

comment:13 Changed 42 hours ago by Tim Graham

Triage Stage: AcceptedReady for checkin

I've made a few cosmetic edits to the PR and plan to merge this after #27579 as I've used part of that patch .

comment:14 Changed 34 hours ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 9835910:

Fixed #17002 -- Allowed using a ManyToManyField through model that inherits another.

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