Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#30309 closed Uncategorized (invalid)

Remove hasattr reference in One-to-One documentation example

Reported by: David Beitey Owned by: nobody
Component: Documentation Version: 2.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The One-to-One example of using hasattr to avoid exception handling is based on the side effect of hasattr swallowing all exceptions. This was pre-Python 3.2 behaviour and has been changed to only suppress AttributeErrors as explained at https://bugs.python.org/issue9666.

PR: https://github.com/django/django/pull/11159

Change History (7)

comment:1 by Mariusz Felisiak, 6 years ago

Resolution: invalid
Status: newclosed

IMO this doc is still valid and it is not related with pre-Python 3.2 behavior. It's quite straightforward, so if you want to check that p2 doesn’t have an associated restaurant you can catch ObjectDoesNotExist exception or you can use hasattr(p2, 'restaurant') (if you don't want to catch exceptions).

comment:2 by David Beitey, 6 years ago

In my experience today, using hasattr(p2, 'restaurant') is throwing a ObjectDoesNotExist exception with Django 2.1.7 / Python 3.7 and the exception is not being swallowed. This was the reason for me making the contribution. For note, I'm not the only one to making this observation: https://stackoverflow.com/a/22240021/1048705 -- the last paragraph of that answer.

If it's Django-specific behaviour to add hasattr functionality to swallow errors, then perhaps a bug report is necessary instead of a documentation fix. If Django is doing this, I'd suggest changing the behaviour so Django follows Python itself; why Python made the change explained on the mailing list: https://mail.python.org/pipermail/python-dev/2010-August/103178.html

Are we able to re-open this? Happy to do further diagnosis if you can't replicate the exception being raised.

Fwiw, I understand the use case for avoiding exception handling with a OneToOneField - I'd love a clean solution for this myself.

comment:3 by Mariusz Felisiak, 6 years ago

hasattr() is not throwing an exceptions, just in case I checked with Python 3.6/3.7 and Django 2.1/2.2.

Did you check this?

FYI: RelatedObjectDoesNotExist is a subclass of AttributeError.

in reply to:  3 comment:4 by David Beitey, 6 years ago

Replying to felixxm:

hasattr() is not throwing an exceptions, just in case I checked with Python 3.6/3.7 and Django 2.1/2.2.

Did you check this?

FYI: RelatedObjectDoesNotExist is a subclass of AttributeError.

Okay, I dug into the situation and what's happening isn't related to Python version.

RelatedObjectDoesNotExist gets raised when the value of a OneToOneField is None/null. However, if there is data in the field but it isn't present in the related table, [model_identifier].DoesNotExist gets raised. The latter exception doesn't inherit from AttributeError and isn't swallowed with hasattr().

Since the behaviour of hasattr() is to return False when there's no related object, that seems to be what should happen in both situations -- with None as a value or a field value pointing at a non-existant related object. In other words, it feels as though RelatedObjectDoesNotExist should be being raised when accessing the attribute on the model in this way, rather than at the query level (see traceback below).

Here's my example:

from django.db import models

class Person(models.Model):
    login = models.CharField(db_column='login', max_length=100)
    # and more...

   class Meta:
        managed = False
        db_table = 'SYS_PEOPLE'

class Author(models.Model):
    person = models.OneToOneField(Person,
                                  db_column='login',
                                  primary_key=True,
                                  on_delete=models.CASCADE)
    # and more...

    class Meta:
        managed = False
        db_table = 'SYS_AUTHORS'

# manage.py shell
from app.models import Author
author = Author(person_id='fake')
author.person # raises DoesNotExist
hasattr(author, 'person')   # raises DoesNotExist

author = Author(person_id=None)
author.person  # raises RelatedObjectDoesNotExist
hasattr(author, 'person')   # Swallows

Tracebacks:

Traceback (most recent call last):
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 163, in __get__
    rel_obj = self.field.get_cached_value(instance)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/db/models/fields/mixins.py", line 13, in get_cached_value
    return instance._state.fields_cache[cache_name]
KeyError: 'person'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./manage.py", line 17, in <module>
    execute_from_command_line(sys.argv)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/core/management/base.py", line 316, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/core/management/base.py", line 353, in execute
    output = self.handle(*args, **options)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/core/management/commands/shell.py", line 92, in handle
    exec(sys.stdin.read())
  File "<string>", line 4, in <module>
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 177, in __get__
    rel_obj = self.get_object(instance)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 297, in get_object
    return super().get_object(instance)
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 144, in get_object
    return qs.get(self.field.get_reverse_related_filter(instance))
  File "/home/user/local/share/virtualenvs/app-1933afW/lib/python3.7/site-packages/django/db/models/query.py", line 399, in get
    self.model._meta.object_name
app.models.person.DoesNotExist: Person matching query does not exist.

comment:5 by Mariusz Felisiak, 6 years ago

First of all described situation has nothing to do with examples in docs. Secondly I'm sorry but I strongly disagree with you, this is a completely different situation. IMO if related field is assigned but objects does not exists then hasattr() should raise DoesNotExist exception, because attribute is assigned to an object it is simply incorrect, existence != correctness.

You can always start a new thread in the django-developers if you think that this should be changed.

comment:6 by Simon Charette, 6 years ago

FWIW this scenario can also happen when using on_delete=models.DO_NOTHING, db_constraint=False and an attempt is made to access a remote object that has been deleted.

I'd be in favor of making ForwardManyToOneDescriptor.get_object raise self.RelatedObjectDoesNotExist as the current code clearly doesn't take db_constraint=False into account based on the heuristic comment there.

in reply to:  6 comment:7 by David Beitey, 6 years ago

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