charettes / django-seal Goto Github PK

I really like the functionality of this library and want to subclass a number of my existing models with SealableModel. However, I don't want to update each of their respective queries with the seal() call because there are a lot of callsites, and I'm also worried that the extra call will be too easily omitted in future callsites.

Is there a recommended approach for sealing all queries by default, so that I can get the sealing functionality for free with a standard Blog.objects.filter/get call? Thank you so much!

For example:

instance = SeaLion.objects.select_related(
    'location',
).prefetch_related(
    'location__climates',
).seal().get()

instance.location.climates.all()  # boom!

Where location is a FK and location__climates is a M2M.

They should already be working. We just need tests to assert it's the case.

Would it be possible to use a mixin that could implement the SealableModel, seal=True functionality? In particular I'm inheriting from django extensions' TimeStampedModel for a few models that I'd like to try django-seal on.

Thanks for maintaining this repo!

Providing error messages detailing from which sealed object the exceptions was caused and for which field would make it easier to figure out what needs to be prefetched.

There might be cases where a queryset could be sealed only partially as they are fields that are rarely accessed and thus not worth immediately fetching all the time.

Being able to provide a list of fields and relations to QuerySet.seal to allow by-passes would provide a nice interface to do so.

Thanks for maintaining this and thanks so much for #76 !!

My linter is complaining about the seal=True directive:

Unexpected keyword argument "seal" for "init_subclass" of "object"

Another option might be to allow using a class attribute (or even add it to Meta):

class MyModel(SealableModel):
    seal=True
...

Making this compatible with MyPy would be very helpful. Thank you for considering it!

The lib doesn't seal the {foreign key}_id of ForeignKey relationship. When I access that attribute, it triggers a query to database, even though the queryset has .only('pk').seal() .

When I check the class attribute, normal attribute and foreign key attribute like this are different:
one is <django.db.models.fields.related_descriptors.ForeignKeyDeferredAttribute object at 0x7fffeff3b8d0>
other is <seal.descriptors.SealableDeferredAttribute object at 0x7fffeff2ac10>

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

Thanks for this very helpful tool! For now we're able to work around this by changing SealableModel back to models.Model on that one particular model and not sealing those queries. This is so useful though that I'm happy to help if there's any additional debugging I can do.

• Python 3.8.6 (via Docker)
• Django 3.1.3
• django-seal 1.2.3

# models.py
from seal.models import SealableModel

class SymmetricalModel(SealableModel):
    symmetrical = models.ManyToManyField("self")

If I run any manage command, even python manage.py --help, I get the following stack trace:

Traceback (most recent call last):
  File "manage.py", line 8, in <module>
    django.setup()
  File "/usr/local/lib/python3.8/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 114, in populate
    app_config.import_models()
  File "/usr/local/lib/python3.8/site-packages/django/apps/config.py", line 211, in import_models
    self.models_module = import_module(models_module_name)
  File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/usr/src/herbie/contacts/models.py", line 2281, in <module>
    class TestModel(SealableModel):
  File "/usr/local/lib/python3.8/site-packages/django/db/models/base.py", line 321, in __new__
    new_class._meta.apps.register_model(new_class._meta.app_label, new_class)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 232, in register_model
    self.do_pending_operations(model)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 425, in do_pending_operations
    function(model)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 404, in apply_next_model
    self.lazy_model_operation(next_function, *more_models)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 416, in lazy_model_operation
    apply_next_model(model_class)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 404, in apply_next_model
    self.lazy_model_operation(next_function, *more_models)
  File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 390, in lazy_model_operation
    function()
  File "/usr/local/lib/python3.8/site-packages/seal/models.py", line 75, in make_remote_field_descriptor_sealable
    make_descriptor_sealable(related_model, accessor_name)
  File "/usr/local/lib/python3.8/site-packages/seal/models.py", line 59, in make_descriptor_sealable
    descriptor = getattr(model, attname)
TypeError: getattr(): attribute name must be string

The docs on symmetrical many-to-many relationships have this note:

If you do not want symmetry in many-to-many relationships with self, set symmetrical to False. This will force Django to add the descriptor for the reverse relationship, allowing ManyToManyField relationships to be non-symmetrical.

Based on the stack trace ending in make_descriptor_sealable, perhaps it's assuming the existence of the opposite descriptor that Django isn't generating?

A 'first__second' prefetch should be converted to Prefetch('first', First.objects.prefetch_related('second')) on queryset sealing.

I turned on Django query debug and found out that when I query like this:

models = list(Parent.objects.seal().prefetch_related("children").all()[0:10])

It outputs one duplicated query for the prefetch. If I remove seal(), it only outputs one query, which is as expected.
Could you check this?

IE: if one side of the relation isn't sealable (IE: SealableModel subclass) -- it at least shouldn't 💥 . This may not be a problem -- but should confirm with some tests.

There's no point in raising a violation for unfetched many-to-many relationship when explicitly fetching a single object .get(), .first(), [index] since it would result in the same number of queries.

class Library(Model):
    pass
class Book(Model):
    library = ForeignKey(Library)

libraries = Library.objects.prefetch_related('book_set').seal()

Fails with FieldDoesNotExist on book_set. If you set the related_name of library to book_set it works fine. (All relationships in the test data have related names set explicitly.)

Is there any way to achieve this behavior?

For example:

book = Book.objects.seal().get(pk=book_id)
book.author
>> None

They are not part of attrs on __new__ so we can alter their related descriptor. We should inject an appropriately adjusted OneToOneField(parent_link=True) into attrs when we detect MTI takes place and no parent_link is explicitly specified.

Given a many to many field with a through relationship where one model is sealed:

class AQuerySet(SealableQuerySet):
    ...
class A(SealableModel):
    objects = AQuerySet.as_manager()
    bs = models.ManyToManyField(
        B,
        blank=True,
        related_name='a_b',
        through=AB,
    )
class B(Model):
    ...
class AB(Model):
    a = models.ForeignKey(
        'A',
        related_name='+',
        on_delete=models.CASCADE,
    )
    b = models.ForeignKey(
        'B',
        related_name='+',
        on_delete=models.CASCADE,
    )

Then using the through relationship will produce incorrect results where a query directly on the model will produce correct results:

# before new relation added
a_instance.bs.count() # 0
AB.objects.filter(a=a_instance).count() # 0

AB.objects.create(
 a=a_instance,
 b=b_instance,
)

a_instance.bs.count() # 0
AB.objects.filter(a=a_instance).count() # 1

This behavior does not happen when AQuerySet is a regular QuerySet class.

test = MyModel.objects.all().prefetch_related('a', 'a__b')
for x in test:
    pass

Results in ValueError: 'a' lookup was already seen with a different queryset. You may need to adjust the ordering of your lookups.

I believe, from reading another issue, this is due to prefetches being resolved into explicit Prefetch objects, so it should just involved some de-duping.

Note that I believe that in the absence of django-seal, both prefetches are necessary if you want to avoid extra queries when looking up information on b.

Right now if you do Seal.objects.select_related('location') and then seal.location won't be sealed when it should be the case.

There's a similar issue with prefetched objects. I think we should seal queryset generated for string prefetched_related lookups but avoid doing it for explicitly specified Prefecth(queryset) to allow an opt-out if desired.

A few things to test.

1. Tests for seal().select_related() and select_related().seal().
2. Ditto for prefetch_related().
3. Interaction with non-sealable models.

diff --git a/tests/test_query.py b/tests/test_query.py
index d5dfef2..7b0fb8e 100644
--- a/tests/test_query.py
+++ b/tests/test_query.py
@@ -252,6 +252,8 @@ class SealableQuerySetTests(TestCase):
         message = 'Attempt to fetch many-to-many field "visitors" on sealed <Location instance>'
         with self.assertRaisesMessage(UnsealedAttributeAccess, message):
             list(instance.visitors.all())
+        with self.assertRaisesMessage(UnsealedAttributeAccess, message):
+            instance.visitors.all()[0]
 
     def test_not_sealed_reverse_foreign_key(self):
         instance = Location.objects.get()

ERROR: test_sealed_reverse_foreign_key (tests.test_query.SealableQuerySetTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_query.py", line 256, in test_sealed_reverse_foreign_key
    instance.visitors.all()[0]
  File "/home/fish/Documents/programming/django-seal/.tox/py27-1.11/lib/python2.7/site-packages/django/db/models/query.py", line 289, in __getitem__
    return list(qs)[0]
  File "/home/fish/Documents/programming/django-seal/.tox/py27-1.11/lib/python2.7/site-packages/django/db/models/query.py", line 250, in __iter__
    self._fetch_all()
  File "seal/descriptors.py", line 43, in _fetch_all
    warnings.warn(self._sealed_warning, category=UnsealedAttributeAccess, stacklevel=3)
AttributeError: 'SealedRelatedQuerySet' object has no attribute '_sealed_warning'

I believe this is because __getitem__ clones the queryset in order to apply a limit for the index. I haven't fully understood what goes on, but to save you a bit of time I did notice that when we get to clone, self.__class__.__mro__ starts

(seal.descriptors.SealedSealableQuerySet, descriptors.SealedRelatedQuerySet, query.SealableQuerySet, ...)

So the line which strips off the outermost sealed queryset leaves you with another sealed queryset. I guess the fault lies in the code which is constructing the queryset class because it should see that the class is already sealed and not try to seal it again.

While looking into this issue I noticed that SealableQueryset defines sealed = False but SealedRelatedQuerySet does not override this. Nothing seems to use this parameter.

Hi - I just wanted to flag that after hours of debugging we pinned down a regression today to an unpinned dep upgrade.

I will dig into tomorrow, but 1.4.2 breaks some of our code that previously worked perfectly on 1.4.1

Hi!

I wonder if it's possible to implement support for pickle. Currently dynamically created "Sealed" classes do not allow queryset to be pickled. Pickle does not support dynamically generated types in general. Although there are workarounds for it, implementing them may be quite difficult depending on the situation.

I'm getting a traceback that is related to a OneToOneField:

  File "/app/lib/python2.7/site-packages/django/core/paginator.py", line 145, in __getitem__
    self.object_list = list(self.object_list)
  File "/app/lib/python2.7/site-packages/django/db/models/query.py", line 250, in __iter__
    self._fetch_all()
  File "/app/lib/python2.7/site-packages/django/db/models/query.py", line 1123, in _fetch_all
    self._prefetch_related_objects()
  File "/app/lib/python2.7/site-packages/django/db/models/query.py", line 678, in _prefetch_related_objects
    prefetch_related_objects(self._result_cache, *self._prefetch_related_lookups)
  File "/app/lib/python2.7/site-packages/django/db/models/query.py", line 1472, in prefetch_related_objects
    obj_list, additional_lookups = prefetch_one_level(obj_list, prefetcher, lookup, level)
  File "/app/lib/python2.7/site-packages/django/db/models/query.py", line 1585, in prefetch_one_level
    prefetcher.get_prefetch_queryset(instances, lookup.get_current_queryset(level)))
  File "/app/lib/python2.7/site-packages/django/db/models/fields/related_descriptors.py", line 346, in get_prefetch_queryset
    queryset = self.get_queryset()

TypeError: get_queryset() takes exactly 2 arguments (1 given)

When comparing SealableReverseOneToOneDescriptor with ReverseOneToOneDescriptor (Django's original implementation) I seemed to find something that may be an inconsistency?

The seal's one takes an instance:

While in Django, it doesn't:

https://github.com/django/django/blob/837ffcfa681d0f65f444d881ee3d69aec23770be/django/db/models/fields/related_descriptors.py#L353-L354

I'm not sure what's the solution for this, but with some guidance I can provide one

We should allow assignment and filtering to take place on sealed unfetched many-to-many.

e.g.

author = Author.objects.seal().get().
author.books.add(book)  # Should be allowed.
author.books.filter(title='test')  # Should be allowed.