django-seal icon indicating copy to clipboard operation
django-seal copied to clipboard

Errors are not raised on prefetched table with different queries

Open canassa opened this issue 4 years ago • 5 comments

Hello,

I notice that the library is not raising the UnsealedAttributeAccess errors if you try to access a prefetched table with a different query. Take this model for example

class Company(SealableModel):
    users = models.ManyToManyField(settings.AUTH_USER_MODEL, blank=True)

If I try to access the company from the User it raises the error as expected:

User.objects.all().seal().first().company_set.all()

UnsealedAttributeAccess: Attempt to fetch many-to-many field "company" on sealed <User instance>.

Now, if I prefetch the company_set the error disappears as expected:

for u in User.objects.prefetch_related('company_set').all().seal():
    for c in u.company_set.all():
        print(c)  # no errors and 2 queries

The problem is that django-seal cannot detect if the user changes the queryset:

for u in User.objects.prefetch_related('company_set').all().seal():
    for c in u.company_set.order_by('-pk'):
        print(c)  # no errors and 4 queries

canassa avatar Sep 11 '20 22:09 canassa

This is currently done by design. As soon as a queryset clone is created due a queryset operation the queryset is unsealed

https://github.com/charettes/django-seal/blob/75c1697af996f29dd9e3f9571de18efc5436b15f/seal/descriptors.py#L31-L40

I guess we could emit a different kind of warning when this happens (e.g. RelatedQuerysetUnsealed) so users could decide how they want to deal with this scenario?

charettes avatar Sep 11 '20 22:09 charettes

Hi @charettes

I am not sure if it's related, but I was able to "bypass" the seal even without the prefetch:

This one raises the error:

[u.company_set.all() for u in User.objects.all().seal()]

This one doesn't

[u.company_set.order_by('-pk') for u in User.objects.all().seal()]

Personally I would prefer that those cases were at least logged by the library. My use case is that I have a DRF endpoint that displays a nested structure with 3 models deep. I want to make sure that the sealed queryset defined at the view is enough for the whole endpoint, but right now if someone changes any reverse lookup it would invalidate the prefetch related silently.

canassa avatar Sep 11 '20 22:09 canassa

@canassa I spent some time drafting a solution in #66. Would that solve your usecase?

charettes avatar Jun 30 '21 05:06 charettes

@charettes Yes! I think so!

canassa avatar Jun 30 '21 08:06 canassa

I got a question.

When will the warning be emitted exactly? Is it issued when the pretech call is executed or when an unwanted extra query is executed?

canassa avatar Jun 30 '21 10:06 canassa