Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

select_related() from a plain model with a ForeignKey to a polymorphic model #244

Open
Bartvds opened this issue Sep 28, 2016 · 13 comments
Open

Comments

@Bartvds
Copy link

Bartvds commented Sep 28, 2016

If you have a regular model that has a ForeignKey to a polymorphic model, and then use select_related() from the regular model''s manager/queryset to select the related polymorphic instances we only get these downcasted as the base class of the polymorphic model.

Illustration:

class ParentModel(PolymorphicModel):
    pass

class ChildModel(ParentModel):
    # some fields etc
    pass   

class PlainModel(models.Model):
    relation = models.ForeignKey(ParentModel)


# if we let it do 1+n
for obj in PlainModel.objects.all():
    # these obj.relation values will have their proper sub type
    print(type(obj.relation))

# if we select_related()
for obj in PlainModel.objects.select_related('relation'):
    # these obj.relation values will all be ParentModel's
    print(type(obj.relation))

My quess is the PlainModel's manager is not aware it has to apply the polymorphic magic? Is this expected behaviour? I see the note on select_related() here http://django-polymorphic.readthedocs.io/en/stable/advanced.html but I think it doesn't apply.

@lorinkoz
Copy link

lorinkoz commented Feb 6, 2017

Same issue here.

@WhyNotHugo
Copy link
Member

The linked note seems to refer to #198. This sounds like a separate (although similar-sounding) issue.

@WhyNotHugo
Copy link
Member

WhyNotHugo commented Jul 9, 2017

I've been looking into this the last couple of hours. Looks like it's a lot harder than it seems.

select_related fetches from multiple tables in a single query, and this is constructed here. This is then constructed into instances using from_db here.

A hackier approach may work, since is that it's possible to do a weird select_related call that fetches subchildren, so it's also possible to produce a hacky workaround:

qs = PlainModel.objects.select_related('relation__childmodel')

for obj in qs:
    try:
        obj.relation = obj.relation.childmodel
    except AttributeError:
        pass
    # You can also try other subclasses, just add them to the select_related() call above.

Note that this raises AttributeError: it doesn't try to query the DB, is just fails an attribute lookup (which is a lot cheaper!).

You can also specify nested lookups too, suppose that only childA has an address, and childB has a phone:

orders = Order.objects.select_related('child__childA__address', 'child__childB__phone')
for obj in qs:
    try:
        obj.child = obj.child.childA
    except AttributeError:
        pass
    try:
        obj.child = obj.child.childB
    except AttributeError:
        pass

orders[0].child.phone  # No lookup (assuming this particular child was class ChildB

It's simple to write a reusable function to wrap around this, so we end up with a select_related function that takes a specific queryset, but I don't know how to patch this into existing querysets due to the change of chained calls.

If somebody can come up with changes for a custom QuerySet class to inject this behaviour, the last bit (the try/except part), this approach would be perfect (albeit, hacky underneath). (Classes which related to this model would have to use a custom QuerySet provided by us, but that hardly seems like an issue). An alternative to that is to simply monkey-patch django's QuerySet.

--- Update ---

Looks like monkey-patching django.db.models.query.ModelIterable with the above try/except trick should do it. I'm not sure if we want to:

  1. Alter the default behaviour for select_related (and make it polymorphic).
  2. Force being explicit with the syntax of the above examples.
  3. Monkey-path django.db.models.query.Queryset, and add a select_related_polymorphic method. This method would have do an explosion that replaces 'child' with 'child__childA', 'child__childB'.

I strongly opt for the first choice. It's the simplest to develop, most explicit/pythonic, and doesn't have any pitfalls compared to others. It also provides a clear syntax for deeper-level selections in a single query.

@WhyNotHugo
Copy link
Member

This worked for me. If it looks clean enough to the maintainers, I can write a proper PR:

def polymorphic_iterator(*fields):
    class PolyMorphicModelIterable:
        def __init__(self, *args, **kwargs):
            self.iterable = ModelIterable(*args, **kwargs)

        def __iter__(self):
            for obj in self.iterable:
                for field in fields:
                    instance = getattr(obj, field)
                    real_instance_name = instance.polymorphic_ctype.model
                    real_instance = getattr(obj.shipment, real_instance_name)
                    setattr(obj, field, real_instance)
                yield obj

    return PolyMorphicModelIterable


class PolymorphicRelatedQuerySet(models.QuerySet):
   """A class with a relationship to a polymorphic model should use this queryset"""
    def select_polymorphic_related(self, *fields):
        """
        Specify fields that should be cast to the real polymorphic class.

        Note that you still need to pass each field and the proper subclasses
        as `select_related`. You might want an auxiliary method in your
        queryset to do this if it's a common scenario.
        """
        obj = self.select_related(*fields)
        obj._iterable_class = polymorphic_iterator(*fields)
        return obj

In my scenario, Shipment has two subclasses: ShipmentA and ShipmentB. This is the usage for the above:

qs = Order.objects.select_related(
    'shipment__shipmentA__address',
    'shipment__shipmentB',
).select_polymorphic_related(
    'shipment',
)

qs.last().shipment  # this returns a ShipmentA instance

Hopefully though, someone will come up with was to improve this and provide a better API than what it currently is.

@AndySun25
Copy link

The above code snippet fixed the instantiation issue for polymorphic models instances, however it ends up incurring additional db queries since django-polymorphic ends up re-fetching the instances without resorting to the selected cache.

This is a fairly painful bug for us since it's breaking optimizations in many areas of our application requiring undesirable workarounds, is there any chance for this to be fixed in the near future?

@AndySun25
Copy link

AndySun25 commented Mar 13, 2018

I dug a bit deeper into the django RelatedPopulator code and discovered that the related instances are in fact being instantiated correctly, i.e. the child models are being created and set in field caches

# Fetch the instance with appropriate select_related.
instance = PlainModel.objects.select_related('relation__childmodel').first()
# You can access the ChildModel by the following, this will NOT incurr additional queries
instance.relation._state.fields_cache['childmodel']

The reason why the trying to fetch the childmodel instance via instance.relation.childmodel causes additional queries can be seen here https://github.com/django-polymorphic/django-polymorphic/blob/master/polymorphic/models.py#L174

I've taken @WhyNotHugo 's code snippet above and made the following change that retains the optimizations of select_related whilst also instantiating the instances properly.

def polymorphic_iterator(*fields):
    class PolyMorphicModelIterable:
        def __init__(self, *args, **kwargs):
            self.iterable = ModelIterable(*args, **kwargs)

        def __iter__(self):
            for obj in self.iterable:
                for field in fields:
                    instance = getattr(obj, field)
                    real_instance_name = instance.polymorphic_ctype.model
                    # Here we fetch instance from cache instead of calling .get_real_instance()
                    real_instance = instance._state.fields_cache[real_instance_name]
                    setattr(obj, field, real_instance)
                yield obj

    return PolyMorphicModelIterable


class PolymorphicRelatedQuerySet(models.QuerySet):
   """A class with a relationship to a polymorphic model should use this queryset"""
    def select_polymorphic_related(self, *fields):
        """
        Specify fields that should be cast to the real polymorphic class.

        Note that you still need to pass each field and the proper subclasses
        as `select_related`. You might want an auxiliary method in your
        queryset to do this if it's a common scenario.
        """
        obj = self.select_related(*fields)
        obj._iterable_class = polymorphic_iterator(*fields)
        return obj

Please let me know if this is of any help in solving the issue.

@vdboor
Copy link
Collaborator

vdboor commented Mar 14, 2018

Sweet! This is clearly getting somewhere! I actually hope it can be done without changing non-polymorphic models though I'm not sure if that's possible. For now, free free to include this snippet into your projects. Hopefully I can find a generic solution for polymorphic next month.

@AndySun25
Copy link

AndySun25 commented Mar 14, 2018

I've gone about and improved some things further.
See https://gist.github.com/AndySun25/157da4195c139a38ec3d609a277773cb for example code.

The above fixes a few things:

# No need to manually do select_related on all polymorphic subtypes
# The below will handle all select_related on all subtypes automatically
PlainClass.objects.select_polymorphic_related('polymorphic_relation')

# You can now do select_related through layers of plain relations
PlainClass.objects.select_polymorphic_related('plain_relation__polymorphic_relation').first()

# _state cache is preserved across nested relations
# i.e. plain_instance.polymorphic_relation.plain_relation will not incur additional queries
PlainClass.objects.select_related('polymorphic_relation__plain_relation').select_polymorphic_related('polymorphic_relation')

The fix is provided as a mixin so you can use it with other custom queryset classes.
There's plenty of improvements to be made in there, especially since it requires all models that have some form of relation (even nested) to use a custom queryset; though I consider it stable enough for now to use in our own project.

@BrendanSimon
Copy link

I have a model design where ModelA has a set of ModelB, and ModelB has a set of ModelC, and ModelC has a set of ModelD.
i.e. ModelD has an FK to ModelC, ModelC has an FK to ModelB and ModelB has an FK to ModelA.

ModelA, ModelB, ModelC, ModelD are polymorhphic sub-classes of ModelBase.

When iterating over all the models (nested outer to inner) to show in a web dashboard, it causes many many DB hits. So I figured prefetch_related() is probably the answer.

Does normal prefetch_releated() work as in the Django docs with polymorphic sub-classes or are there issues similar to selected_related() ?

What is the best way to get all the model hierarchy data in one DB hit?

Does the above mixin help for this? If so, how do I get it and apply it? Is it integrated into django-polymorhipic yet? It's now Dec 2020.

Thanks, for any assistance. Is this better discussed on Stack Overflow (or here)?

@Finndersen
Copy link

@AndySun25 nice one, I made some changes to support Polymorphic Proxy models:
https://gist.github.com/Finndersen/3a9647718b35d6776b0dc3808a501519

@WhyNotHugo
Copy link
Member

That looks pretty sweet. Are you planning on creating a PR based on this?

@MeRuslan
Copy link

MeRuslan commented May 1, 2021

Not a lot of activity here, huh?

@Finndersen
Copy link

PolymorphicRelatedQuerySetMixin could probably be modified to integrate select_polymorphic_related() functionality seamlessly into normal select_related()? Note my update above also supports chaining of select_polymorphic_related() (previous version would override fields in PolymorphicModelIterable with each call)

pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Jun 26, 2023
implement support for a single query for select related base fetches across
polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch
related models.

Fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
Possible Fixes:
  jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
Related: jazzband#531
pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Jun 26, 2023
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
andriilahuta pushed a commit to andriilahuta/django-polymorphic that referenced this issue Dec 19, 2023
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Apr 4, 2024
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Apr 4, 2024
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants