jazzband/django-model-utils

`FieldInstanceTracker` passes `str` instead of `Field` to `DeferredAttributeTracker`

mthuurne opened this issue · 1 comments

Problem

While adding type annotations, mypy flags a typing inconsistency that I think isn't caused by annotations mistakes but rather by the code itself:

$ mypy model_utils/
model_utils/tracker.py:334: error: Argument 1 to "DeferredAttributeTracker" has incompatible type "str"; expected "Field[Any, Any]" 
[arg-type]
                    field_tracker = DeferredAttributeTracker(field)
                                                             ^~~~~

The relevant code reads as follows:

        self.instance._deferred_fields = self.instance.get_deferred_fields()
        for field in self.instance._deferred_fields:
            field_obj = self.instance.__class__.__dict__.get(field)
            if isinstance(field_obj, FileDescriptor):
                field_tracker = FileDescriptorTracker(field_obj.field)
                setattr(self.instance.__class__, field, field_tracker)
            else:
                field_tracker = DeferredAttributeTracker(field)
                setattr(self.instance.__class__, field, field_tracker)

For FileDescriptorTracker, a Field instance is passed to the constructor and this type checks fine. For DeferredAttributeTracker however, a str is passed while a Field instance is expected.

The Django docs confirm that get_deferred_fields() is supposed to return field names, not the actual fields. The fact that field is being used as a key for __dict__ supports this.

I can't find documentation for django.db.models.query_utils.DeferredAttribute, but the implementation uses self.field.attname at some point, which suggests that field is a Field instance rather than a str. That is consistent with django-stubs.

The else clause doesn't seem to have test coverage: all tests still passed after I inserted assert False there.

So all the clues point to this being a bug. I'm not sure how to fix it though: passing field_obj to the DeferredAttributeTracker constructor would satisfy mypy, but without a test I don't know if it would do the right thing at runtime.

Environment

  • Django Model Utils version: current master branch
  • Django version: 4.1.7
  • Python version: 3.10.6
  • Other libraries used, if any: mypy 1.1.1, django-stubs 1.16.0

I think passing field_obj is not the correct solution after all, as field_obj is actually a deferred attribute, not the field itself. So we should be passing field_obj.field to DeferredAttributeTracker, just like is done for FileDescriptorTracker.