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

FieldInstanceTracker passes str instead of Field to DeferredAttributeTracker #559

Open
mthuurne opened this issue Mar 17, 2023 · 1 comment

Comments

@mthuurne
Copy link
Contributor

mthuurne commented Mar 17, 2023

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
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 17, 2023
This makes it pass mypy inspection, but there is no test coverage,
so I'm not certain this solution is correct. However, as the code
was already broken, it's unlikely to make things worse.

jazzband#559
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 20, 2023
This makes it pass mypy inspection, but there is no test coverage,
so I'm not certain this solution is correct. However, as the code
was already broken, it's unlikely to make things worse.

jazzband#559
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 21, 2023
This makes it pass mypy inspection, but there is no test coverage,
so I'm not certain this solution is correct. However, as the code
was already broken, it's unlikely to make things worse.

jazzband#559
@mthuurne
Copy link
Contributor Author

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.

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

1 participant