Skip to content

Meaningful reference errors #28

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Meaningful reference errors #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mongoengine/base/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

from mongoengine.base.common import ALLOW_INHERITANCE
from mongoengine.base.datastructures import BaseDict, BaseList
from mongoengine.base.proxy import DocumentProxy

__all__ = ("BaseField", "ComplexBaseField", "ObjectIdField", "GeoJsonBaseField")

Expand Down Expand Up @@ -103,9 +104,12 @@ def __get__(self, instance, owner):
value = self.default() if callable(self.default) else self.default
else:
value = self.to_python(db_value)
if isinstance(value, DocumentProxy):
value._set_parent_ref(instance, name)
Copy link

@tsx tsx Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to avoid holding onto the parent instance as this might cause memory leaks. Store just the class (name or reference), and the instance pk.


if hasattr(self, 'value_for_instance'):
value = self.value_for_instance(value, instance)

data[name] = value

return data[name]
Expand Down
26 changes: 24 additions & 2 deletions mongoengine/base/proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,19 @@ def __delslice__(self, i, j):


class DocumentProxy(LocalProxy):
__slots__ = ('__document_type', '__document', '__pk')
__slots__ = (
'__document_type',
'__document',
'__pk',
'__parent_ref_instance',
'__parent_ref_field_name',
)

def _set_parent_ref(self, instance, field_name):
object.__setattr__(self, '_DocumentProxy__parent_ref_instance',
instance)
object.__setattr__(self, '_DocumentProxy__parent_ref_field_name',
field_name)

def __init__(self, document_type, pk):
object.__setattr__(self, '_DocumentProxy__document_type', document_type)
Expand Down Expand Up @@ -186,7 +198,17 @@ def _get_current_object(self):
collection = self.__document_type._get_collection()
son = collection.find_one({'_id': self.__pk})
if son is None:
raise DoesNotExist('Document has been deleted.')
error = 'Document ({} {}'.format(
collection.name, self.__pk
)
if self.__parent_ref_instance:
error += ', referenced by {} {} in field {}'.format(
self.__parent_ref_instance.__class__.__name__,
self.__parent_ref_instance.pk,
self.__parent_ref_field_name,
)
error += ') has been deleted.'
raise DoesNotExist(error)
Copy link

@tsx tsx Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add the extra data as exception attributes? At some point we would want to inspect the parent object and parsing error message is not a good idea.

Copy link
Member

@wojcikstefan wojcikstefan Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, we should start with doing it in the upstream version so that we have a consistent interface. A more informative message on the same exception is good. Raising what would essentially amount to a different exception is not good.

document = self.__document_type._from_son(son)
object.__setattr__(self, '_DocumentProxy__document', document)
return self.__document
Expand Down