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

Improve Reverse Delete Rules #2765

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

combscCode
Copy link

@combscCode combscCode commented Aug 4, 2023

Fixes issues #2764 and #2308, implements lazy delete rules and delete rules on GenericReferenceFields.

To implement lazy delete rules:
When delete rules are assigned to a Document that doesn't exist yet, I store them in a global dict. Whenever a new Document class is defined, it checks said global dict for any outstanding delete rules that it needs to assign on itself.

To implement delete rules on GenericReferenceFields:
GenericReferenceField is now smart enough to register delete rules on many documents when its asked to. It does this by employing a helper class which provides a register_delete_rule function that actually can register delete rules on many documents, not just one. It exposes this class by defining a document_type property, which is kind of lying in a sense since this really is only intended to expose register_delete_rule and not handle any other functionality expected of document_type.

This is a little janky but I can't think of a way to do it "right" without changing which class is responsible for register_delete_rule and thus making a breaking change.

@combscCode
Copy link
Author

mm i guess its not working : ) will come back to this later.

@combscCode combscCode changed the title Implement lazy delete rules Improve Reverse Delete Rules Aug 5, 2023
Comment on lines +213 to +214
f.document_type.register_delete_rule(
new_class, field.name, delete_rule
Copy link
Author

Choose a reason for hiding this comment

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

In the ideal world I would cut out document_type here and just make fields responsible for implementing register_delete_rule.

However, when I tried this I realized that it would break the interface for how to register delete rules and thus any users dynamically setting delete rules would be upset.

Comment on lines +1482 to +1506
Use the `reverse_delete_rule` to handle what should happen if the document
the field is referencing is deleted. EmbeddedDocuments, DictFields and
MapFields does not support reverse_delete_rule and an `InvalidDocumentError`
will be raised if trying to set on one of these Document / Field types.

The options are:

* DO_NOTHING (0) - don't do anything (default).
* NULLIFY (1) - Updates the reference to null.
* CASCADE (2) - Deletes the documents associated with the reference.
* DENY (3) - Prevent the deletion of the reference object.
* PULL (4) - Pull the reference from a :class:`~mongoengine.fields.ListField` of references

Alternative syntax for registering delete rules

.. code-block:: python

class Org(Document):
owner = ReferenceField('User')

class User(Document):
org = ReferenceField('Org', reverse_delete_rule=CASCADE)

User.register_delete_rule(Org, 'owner', DENY)

Copy link
Author

Choose a reason for hiding this comment

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

This wasn't showing up in sphinx when I ran make html-readthedocs, not sure why?

@@ -1453,8 +1512,11 @@ class GenericReferenceField(BaseField):
* You can use the choices param to limit the acceptable Document types
"""

def __init__(self, *args, **kwargs):
def __init__(self, *args, reverse_delete_rule=DO_NOTHING, **kwargs):
Copy link
Author

Choose a reason for hiding this comment

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

Would like to call this param out in the GenericReferenceField docs but I couldn't seem to get it to show up properly, I'd make a docstring for the init but it wouldn't populate to the docs 🤷

@combscCode
Copy link
Author

@bagerard I know you're busy, anything more I need to do to get this merged in? Fixed a couple issues - just need a bit of help with the sphinx formatting.

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.

1 participant