#21563 closed Cleanup/optimization (fixed)

calling hasattr(model_instance, fieldname) raises DoesNotExist when False

Reported by: monkut Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: charettes Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using python 3.3 and django 1.6.

I've got a model (created from an AbstractBaseClass) and creating an instance of the model from some JSON, via serializers.deserialize(), then checking if the resulting parsed JSON model has the fields I'm expecting.

(thinking about it now, this seems like the wrong thing to do, since it may be expected that the resulting Model instance has the field, but takes the default value? but anyway...)

When I check to see if the parsed instance.object has the given attribute, via hasattr(), it's raising DoesNotExist, instead of returning the expected False.

Again, maybe it's ok that it doesn't return False... but I don't think hasattr() should raise an exception.

from django.contrib.auth.models import User

class BaseTask(models.Model):
    assigned_to = models.ForiegnKey(User)

    class Meta:
        abstract = True

class Task(BaseTask):
    pass
Python 3.3.2 (v3.3.2:d047928ae3f6, May 16 2013, 00:06:53) [MSC v.1600 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from myproject.myapp.models import Task
>>> from django.contrib.auth.models import User
>>> t = Task()
>>> hasattr(t, "assigned_to")
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "C:\Python33\lib\site-packages\django\db\models\fields\related.py", line 314, in __get__
    "%s has no %s." % (self.field.model.__name__, self.field.name))
django.contrib.auth.models.DoesNotExist: Task has no assigned_to.
>>> u = User.objects.get(pk=1)
>>>
>>> t.assigned_to = u
>>> hasattr(t, "assigned_to")
True
>>>

Change History (6)

comment:1 follow-up: Changed 19 months ago by aaugustin

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

This is a bit tricky. Python's documentation for hasattr notes that it's "implemented by calling getattr(object, name) and seeing whether it raises an AttributeError or not". However, when no related object exists, Django raises DoesNotExist, not AttributeError.

Could we raise a RelatedObjectDoesNotExist exception inheriting both from DoesNotExist (for backwards compatibility) and AttributeError (to change the behavior according to your suggestion)?

I'm not entirely sure it's a good idea, but it's worth considering.

comment:2 in reply to: ↑ 1 Changed 19 months ago by charettes

Replying to aaugustin:

Could we raise a RelatedObjectDoesNotExist exception inheriting both from DoesNotExist (for backwards compatibility) and AttributeError (to change the behavior according to your suggestion)?

Gave it a try and it seems worth considering. The full test suite passes on Python2.7 SQLite3.

comment:3 follow-up: Changed 19 months ago by aaugustin

The patch looks good to me.

It adds tests for both forwards and backwards relations.

I don't see a reason to make this new exception a public API. It's just making hasattr behave as expected. Therefore, no documentation is needed at this time.

It could be preferrable to define RelatedObjectDoesNotExist similarly for the forwards and backwards relations. Is there a downside to using a cached property for both? I value consistency a lot in this area of the code base, for reasons that must be obvious to you by now ;-) Once you've considered this question, ship it!

comment:4 in reply to: ↑ 3 Changed 19 months ago by charettes

Replying to aaugustin:

It could be preferrable to define RelatedObjectDoesNotExist similarly for the forwards and backwards relations. Is there a downside to using a cached property for both? I value consistency a lot in this area of the code base, for reasons that must be obvious to you by now ;-) Once you've considered this question, ship it!

No objection to use a cached_property in both cases.

I kept the one created at initialization time since I felt like the cached_property approach was kind of hackish, hence the ReverseSingleRelatedObjectDescriptor comment.

I'll add a comment to explain SingleRelatedObjectDescriptor.RelatedObjectDoesNotExist is also a cached_property for the sake of consistency.

comment:5 Changed 19 months ago by charettes

  • Cc charettes added
  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from 1.6 to master

Opened a PR with the changes, I'll ship it if no objections are raised in the next days.

comment:6 Changed 19 months ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 75924cfa6dca95aa1f02e38802df285271dc7c14:

Fixed #21563 -- Single related object descriptors should work with hasattr.

Thanks to Aymeric Augustin for the review and Trac alias monkut for the report.

Note: See TracTickets for help on using tickets.
Back to Top