Opened 2 weeks ago

Closed 82 minutes ago

#36183 closed New feature (wontfix)

Model o2o inheritance with abstract models does not "evaluate" a lazy relationship

Reported by: BeryCZ Owned by:
Component: Database layer (models, ORM) Version: 5.1
Severity: Normal Keywords:
Cc: BeryCZ, Simon Charette Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no
Pull Requests:How to create a pull request

Description

When I define models like this

class AbstractPage(models.Model):
    mtime = models.DateTimeField(
        verbose_name="Last modification at",
        auto_now=True,
    )

    class Meta:
        abstract = True


class AbstractPageType(models.Model):
    page = models.OneToOneField(
        "Page",
        verbose_name="ID",
        primary_key=True,
        on_delete=models.CASCADE,
        related_name="%(class)s",
        parent_link=True,
    )
    title = models.CharField(
        verbose_name="Title",
        max_length=255,
        null=False,
        blank=False,
    )

    class Meta:
        abstract = True


class Page(AbstractPage):
    pass


class ArticlePage(AbstractPageType, Page):
    pass

And then I do

ArticlePage.objects.order_by("-mtime").all()

I get

  File "django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "django/db/models/query.py", line 1701, in order_by
    obj.query.add_ordering(*field_names)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "django/db/models/sql/query.py", line 2249, in add_ordering
    self.names_to_path(item.split(LOOKUP_SEP), self.model._meta)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "django/db/models/sql/query.py", line 1777, in names_to_path
    path_to_parent = opts.get_path_to_parent(model)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "django/db/models/options.py", line 755, in get_path_to_parent
    targets = (final_field.remote_field.get_related_field(),)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "django/db/models/fields/reverse_related.py", line 311, in get_related_field
    field = self.model._meta.get_field(self.field_name)
            ^^^^^^^^^^^^^^^^

Exception Type: AttributeError
Exception Value: 'str' object has no attribute '_meta'

It works when I do

class ArticlePage(AbstractPageType, Page):
    page = models.OneToOneField(
        "Page",
        verbose_name="ID",
        primary_key=True,
        on_delete=models.CASCADE,
        related_name="%(class)s",
        parent_link=True,
    )

but I would prefer not to have to define page field in every PageType.

Change History (15)

comment:1 by Natalia Bidart, 13 days ago

Resolution: invalid
Status: newclosed

Hello BeryCZ, thank you for this report. I've analyzed your models and I fear that the issue is the complex inheritance that you have proposed. Specifically, you cannot have a model (ArticlePage) that both inherits from Page and has a OneToOneField to Page. Without understanding your use case, it feels like a possible solution would be for your "pages" to have a FK to "page types" (instead of the other way around with the 1-1 field).

Your model definitions create an inheritance conflict:

  1. AbstractPageType expects a separate Page instance (this means every model that inherits from AbstractPageType must have a separate Page instance linked via the page field).
  2. ArticlePage is also inheriting Page, which means Django expects ArticlePage itself to be a Page instance.

Django sees two competing ways to resolve ArticlePage:

  • ArticlePage is a Page (due to model inheritance), or
  • ArticlePage has a separate Page instance (due to the page OneToOneField).

The error is caused because Django tries to retrieve the mtime field, which exists on Page (inherited via AbstractPage). But because ArticlePage also has a OneToOneField called page, Django gets confused about how to resolve mtime. Instead of treating page as a relationship to another Page instance, Django interprets it as a string.

Solutions are either to remove the direct inheritance to Page or remove the page field from AbstractPageType.

If you have further questions, there are several user support channels available. Please refer to TicketClosingReasons/UseSupportChannels for ways to get help.

comment:2 by BeryCZ, 13 days ago

Hello Natalia, thanks for your response, but I think you either misunderstood or I'm missing something that's not clear from your answer.

Based on the docs https://docs.djangoproject.com/en/5.1/topics/db/models/#multi-table-inheritance I can override the primary key on ArticlePage so instead of page_ptr I can have just page. So that's what I'm doing, it works when I put it in ArticlePage model, but not when I put it the AbstractPageType.
I've just asked on discord and klove said it should be ok to name the pk page.
So I even tried to remove my custom pk field and checked ArticlePage.page and article_page_object.page and both raise AttributeError, so by default page is not there.

My use case is - I want to have the "parent" model Page from which I can MTI "submodels" like Article, Category, Product, Reference, whatever. The Page model will then be used for references in other models (galeries, urls, tree-maps, ...).

So, am I missing something, or is this a bug? I'm not saying it's an easy-to-fix bug :) but I honestly think it is a bug.
I'm not even sure where the lazy load of to relationship is, or why it doesn't work in abstract models, otherwise I would try to fix it and PR.

Thank you,
Bery

comment:3 by Simon Charette, 12 days ago

I believe your AbstractPageType definition is invalid as it should not allowed to define a parent_link on a model that doesn't directly inherit from the referenced model even when abstract.

The correct way to do this is to have the abstract model defining the parent link subclass the concrete model (abstract MTI). This means you should be able to achieve what you're after by re-organizing your model definition to make sure AbstractPageType does inherit from Page

class AbstractPage(models.Model):
    mtime = models.DateTimeField(
        verbose_name="Last modification at",
        auto_now=True,
    )

    class Meta:
        abstract = True

class Page(AbstractPage):
    pass

class AbstractPageType(Page):
    page = models.OneToOneField(
        "Page",
        verbose_name="ID",
        primary_key=True,
        on_delete=models.CASCADE,
        related_name="%(class)s",
        parent_link=True,
    )

    class Meta:
        abstract = True

class ArticlePage(AbstractPageType):
    pass

class OtherPage(AbstractPageType):
    pass

This is not something Django could be easily adjusted to warn you about as no registry of abstract models is kept and thus system checks cannot be run against them.

Last edited 12 days ago by Simon Charette (previous) (diff)

comment:4 by Simon Charette, 12 days ago

So I tried adapting BaseModel.__new__ to confirm this was effectively never supported with

  • TabularUnified django/db/models/base.py

    diff --git a/django/db/models/base.py b/django/db/models/base.py
    index 575365e11c..5a33f78a98 100644
    a b def __new__(cls, name, bases, attrs, **kwargs):  
    243243            for field in base._meta.local_fields:
    244244                if isinstance(field, OneToOneField) and field.remote_field.parent_link:
    245245                    related = resolve_relation(new_class, field.remote_field.model)
    246                     parent_links[make_model_tuple(related)] = field
     246                    related_model_tuple = make_model_tuple(related)
     247                    if abstract and not any(
     248                        make_model_tuple(parent) == related_model_tuple
     249                        for parent in parents
     250                        if hasattr(parent, "_meta")
     251                    ):
     252                        raise TypeError(
     253                            "Abstract models must directly inherit from parent "
     254                            "links they refer to."
     255                        )
     256                    parent_links[related_model_tuple] = field
    247257
    248258        # Track fields inherited from base models.
    249259        inherited_attributes = set()

And the suited failed on these models which git-blame led me to #20883 (163a34ce4bc1086b346a52c7271f48d2c207f710) which makes me believe this either never fully worked (notice the tests are very minimal) or that the latest changes to abstract model inheritance field attribution in #24305 (85ef98dc6ec565b1add417bd76808664e7318026).

Given we have models in the suite I think we should at least bisect to determine if this was ever properly supported but so minimally tested that #24305 silently broke it. If it never truly worked then I'd suggest we add this TypeError otherwise if it was broken by #24305 we should consider it like a long standing regression.

comment:5 by BeryCZ, 12 days ago

Hello Simon, thank you for your response.
The re-organization of the models isnt really a solution, because in my case the abstracts are in a reusable app and then models are project specific. I dont like to have concrete models in a reusable app and never be able to rewrite them, but I should have the AbstractPageType in the reusable app already.

I would really like if it was possible to define the parent_link's field name in the abstract, so I don't have to define it on all PageType models, because page is much more comfortable to work with than page_ptr.

Version 0, edited 12 days ago by BeryCZ (next)

comment:6 by BeryCZ, 23 hours ago

based on the Simon's comment I think this ticket should be reopened...?

Last edited 23 hours ago by BeryCZ (previous) (diff)

comment:7 by BeryCZ, 23 hours ago

Resolution: invalid
Status: closednew

comment:8 by Sarah Boyce, 18 hours ago

Triage Stage: UnreviewedAccepted

I think we can accept a ticket here.

With the following regression test for the git bisect, I bisected the regression to 513948735b799239f3ef8c89397592445e1a0cd5 (#31426)

  • TabularUnified tests/model_inheritance_regress/test_regress.py

    a b  
     1from django.db import models
     2from django.test import TestCase
     3
     4from .models import Place
     5
     6
     7
     8class ParkingLot4(models.Model):
     9    # Test parent_link connector can be discovered in abstract classes.
     10    parent = models.OneToOneField("Place", models.CASCADE, parent_link=True)
     11
     12    class Meta:
     13        abstract = True
     14
     15
     16class ParkingLot4C(ParkingLot4, Place):
     17    pass
     18
     19
     20class RegressTest(TestCase):
     21    def test_use_explicit_o2o_to_parent_from_abstract_model(self):
     22        self.assertEqual(ParkingLot4C._meta.pk.name, "parent")
     23        ParkingLot4C.objects.create(
     24            name="Parking4A",
     25            address='21 Jump Street',
     26        )
     27        ParkingLot4C.objects.order_by("name").all()

comment:9 by Simon Charette, 17 hours ago

Cc: Simon Charette added

513948735b799239f3ef8c89397592445e1a0cd5 might have altered the way the problem is surfaced when calling order_by but I'd be very surprised if it was at the origin of the actual regression (if it ever worked).

comment:10 by Sarah Boyce, 16 hours ago

Updated the test to call values_list:

        self.assertSequenceEqual(
            ParkingLot4C.objects.order_by("name").values_list("name", flat=True),
            ["Parking4A"]
        )

And tested on 163a34ce4bc1086b346a52c7271f48d2c207f710 the original commit, this also errors with AttributeError: 'str' object has no attribute '_meta'.
In which case, we have never supported the lazy relationship case (note that parent = models.OneToOneField(Place, models.CASCADE, parent_link=True) passes).

comment:11 by Simon Charette, 16 hours ago

Thanks Sarah!

In that case it means that #20883 never truly worked but there has been partial broken support for in main for over 13 years (since 1.7).

I guess that leaves us with a few non-mutually exclusive options

  1. Add proper support for this pattern
  2. Document that he correct way to do this is to have the abstract model defining the parent link subclass the concrete model and add tests for it (abstract MTI) (see comment:3)
  3. Revert tested support for it since it never really worked and the tests were very shallow
  4. Revert uncovered code once 3. is applied

Given this has been flying under the radar for years and that users in the wild might have been using 2 (which works just fine) I'd favor all options but 1.

comment:12 by BeryCZ, 12 hours ago

Honestly I'm in favor of 1.
Not only because it will make my life and the life of my colleagues easier for (probably) many upcoming years - I'm expecting few hundereds, possibly even close to a thousand of use cases where we'll have to define the parent_link in concrete models instead of having it in ONLY one abstract model.

But I think that having support of lazy relationships in all cases except one will still be a bug, even if you document the exception.

comment:13 by Simon Charette, 11 hours ago

I'm expecting few hundereds, possibly even close to a thousand of use cases where we'll have to define the parent_link in concrete models instead of having it in ONLY one abstract model.

As described in comment:3 you can already work around this case by ensuring that your abstract base inherits from the base you are referring to in your parent link. To me your position in comment:5 is more ideological than practical as lazy relationships are designed to help with circular references and references to swappable models not to obscure coupling issues between abstract and concrete entities of your projects.

But I think that having support of lazy relationships in all cases except one will still be a bug, even if you document the exception.

You have to put things in perspective here. This pattern never worked (#20883 apparently failed at implementing proper support for it) and no one considered enough of a limitation to report it since MTI and abstract models were added to Django almost two decades ago. All signs points at this being a niche use case that happens to have a reasonable alternative

If you spent the time trying to implement 1. yourself and gathered consensus on the forum demonstrating a demand for this pattern I could be convinced otherwise but to me this is more of a feature request for a niche use case than a bug report.

comment:14 by BeryCZ, 3 hours ago

As described in comment:3 you can already work around this case by ensuring that your abstract base inherits from the base you are referring to in your parent link. To me your position in comment:5 is more ideological than practical as lazy relationships are designed to help with circular references and references to swappable models not to obscure coupling issues between abstract and concrete entities of your projects.

It's not ideological, it's practical. I'm working on a new cms. We have built over 300 projects on our old cms in the past ~23years. The old cms is completely custom, no framework and it's getting kinda "outdated", so we need a new one. Based on experience I'm expecting few hundereds of projects each containing ~1-10 page types. And putting the base model into the reusable app and never be able to add a project-specific field would be a very bad design.

I suppose it's not an easy-to-fix bug and ofc I understand that you're busy, but imo it's better to leave this ticket open as "accepted - PR welcome", even long-term, than hiding it.
I don't need it fixed right away since there is the workaround by defining parent_link on the concrete submodels. So either I get to it, or you, or whoever else will need it, or whoever will first have the time :) but it's better to leave it open for now.

Thanks

comment:15 by Sarah Boyce, 82 minutes ago

Resolution: wontfix
Status: newclosed
Triage Stage: AcceptedUnreviewed
Type: BugNew feature

IMO AbstractPageType is tightly coupled to Page as it overrides the inheritance behavior of sub-classing Page. Having AbstractPageType be defined in the same place as Page is reasonable. All models that want to inherit this, inherit from Page anyway, so this doesn't cause things like circular imports.


I agree that this is a request to support a niche use case that has never been supported by Django before, hence will categorize this as a new feature request.

The recommended path forward is to now propose and discuss this with the community and gain consensus. To do that, please consider starting a new conversation on the Django Forum, where you'll reach a broader audience and receive additional feedback.

Feature requests are closed until there is an agreement to add the support. Hence, I'll close the ticket for now, but if the community agrees this should be supported, please return to this ticket and reference the forum discussion so we can re-open it. For more information, please refer to the documented guidelines for requesting features.


On options 2-4, all are fine to me and I don't think there's a rush. We could make a decision and create a new ticket for any combination of these

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