netzkolchose / django-computedfields

Provides autogenerated autoupdated database fields for model methods.
MIT License
94 stars 14 forks source link

Use model._base_manager instead of model.objects #120

Closed rob-b closed 1 year ago

rob-b commented 1 year ago

Some applications will have overridden models.objects with a custom manager that excludes some records by default e.g. soft-deleted or draft rows.


class MyCustomManager(models.Manager):

    def get_queryset(self):
        return super().get_queryset().filter(is_draft=False)

class MyModel(models.Model):

    is_draft = models.BooleanField(default=False)
    objects = MyCustomManager()

# queries default to excluding draft records
MyModel.objects.create(is_draft=True)
assert MyModel.objects.all().count() == 0

A side-effect of this is that because computedfields is hardcoded to to call model.objects records like this are inaadvertently excluded from updates

This change uses _base_manager instead to ensure that all db rows are considered

jerch commented 1 year ago

@rob-b Yes the possibility of overloaded query managers is currently an issue for computed fields. I think that using the base manager is the right step for your example. :+1:

Still I think the issue goes further - what if someone explicitly wants to limit the cf logic to be bound to a certain manager (eg. for perf reason mainly)? Maybe we need another keyword argument to set the manager to be used, with falling back to base manager by default? Issue here is - thats quite tricky to express with the API computed fields currently expose, as managers typically work on model and not on field level (so this might need a custom Model._Meta extension, or a model decorator - still unsure about the proper way to handle that).

So any ideas from your side how to deal with that?