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

Meaningful reference errors #28

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

Conversation

thomasst
Copy link
Member

@thomasst thomasst commented Jun 19, 2020

It's been painful trying to figure out which references are broken.

This PR needs more testing, but what do you think? (Feel free to take this PR over)

In [7]: c.pk
Out[7]: 'acti_JYLfSSgIrpHESqDfNZJAzi5eiw9F3q5zqJS7xtFZG3y'

In [8]: c._db_data['transferred_to']
Out[8]: 'acti_nonexistent'

In [9]: c.transferred_to.number
---------------------------------------------------------------------------
DoesNotExist                              Traceback (most recent call last)
/home/closeio/closeio/closeio/exporter/documents/__init__.py in <module>()
----> 1 c.transferred_to.number

/home/closeio/venv/src/mongoengine/build/lib/mongoengine/base/proxy.py in __getattr__(self, name)
     63         if name == '__members__':
     64             return dir(self._get_current_object())
---> 65         return getattr(self._get_current_object(), name)
     66 
     67     def __setitem__(self, key, value):

/home/closeio/venv/src/mongoengine/build/lib/mongoengine/base/proxy.py in _get_current_object(self)
    209                     )
    210                 error += ') has been deleted.'
--> 211                 raise DoesNotExist(error)
    212             document = self.__document_type._from_son(son)
    213             object.__setattr__(self, '_DocumentProxy__document', document)

DoesNotExist: Document (activity acti_nonexistent, referenced by Call acti_JYLfSSgIrpHESqDfNZJAzi5eiw9F3q5zqJS7xtFZG3y in field transferred_to) has been deleted.

@thomasst thomasst requested review from tsx and wojcikstefan June 19, 2020 18:42
@thomasst thomasst self-assigned this Jun 19, 2020
Copy link

@tsx tsx left a comment

Choose a reason for hiding this comment

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

Didn't try it myself but looks reasonable.

self.__parent_ref_field_name,
)
error += ') has been deleted.'
raise DoesNotExist(error)
Copy link

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

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.

@@ -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

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.

Copy link
Member

@wojcikstefan wojcikstefan left a comment

Choose a reason for hiding this comment

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

This is pretty cool, I'm willing to give it a shot. I agree with V that ideally we'd just store the class name/reference and the ID of the parent instance.

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

Successfully merging this pull request may close these issues.

3 participants