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

Multiple fixes for cascade=True save issues #2615

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

Conversation

nickfrev
Copy link

@nickfrev nickfrev commented Jan 7, 2022

Fixes multiple issues when cascade=True in the Document save method:

  • When a ReferenceField is holding an unsaved object you no longer get a ValidationError. The unsaved object is just saved as one would expect.
  • When a ReferenceField is embedded in a ComplexBaseField or any number of nested ComplexBaseField it will be saved.

Modifications:

  • The save method has been edited to be more modular and future friendly.
    • A new BaseField class has been added, SaveableBaseField, which adds a save method to the baseField class.
    • ComplexBaseField now inherits from SaveableBaseField.
    • During a cascade save any SaveableBaseField will run it's coresponding save method. This removes the need to hardcode the saveable field classes as done currently: mongoengine/document.py line 565
    • This allows for developers to easily create their own "saveable" fields.
  • Moved the save logic for ComplexBaseFields out of the Document class and into the ComplexBaseField class to align with the new SaveableBaseField design.
  • Added _save_place_holder method to the Document class which is a helper method for saving that inserts a totally empty (except id) placeholder object into the db. This is what allows that object to get an id before its parent validates.

Below is some code (based on new pytest test_cascade_save_with_cycles) to demonstrate how cascade saves can now work:

class Object1(Document):
    name = StringField()
    oject2_reference = ReferenceField('Object2')
    oject2_list = ListField(ReferenceField('Object2'))

class Object2(Document):
    name = StringField()
    oject1_reference = ReferenceField(Object1)
    oject1_list = ListField(ReferenceField(Object1))

obj_1 = Object1(name="Test Object 1")
obj_2 = Object2(name="Test Object 2")

# Create a cyclic reference nightmare
obj_2.oject1_reference = obj_1
obj_2.oject1_list = [obj_1]

obj_1.oject2_reference = obj_2
obj_1.oject2_list = [obj_2]

# Save only once
obj_1.save(cascade=True)

Personal Notes:

It is my personal opinion that these changes make the code look cleaner as you don't have .save() calls all over the place and you can also avoid the habit of over-saving documents (which I have been guilty of due to a lack of trust in cascade saving). This also removes the burden from the developer to keep track of what has and has not been saved.

I also believe this makes the code look less like ORM code as you only require one save at the end of all data creation/manipulation for complex objects.

I feel a bit shaky about how I implimented the _is_saving flag that is now used during a cascade save. This flag must be set to false when the save method has completed (either regularly or if there was an error thrown). I acomplished this with a wrapper try-catch and the use of a finally clause.

Save the children documents first to avoid the issue where a parent cannot save due to having new children documents.
- Created a new base field (SaveableBaseField) which allows a field to marked as savable during a cascade save.
- Each SaveableBaseField defines a save method which describes how it will deal with a cascade save call this allows lists, dicts, and maps to be effected during a cascade save.
- Added an _is_saving flag during Document save to avoid saving a document that is already in the process of being saved. (Caused if there is a circular reference.)
- Allows for users to cascade save unsaved documents even if a cycle exists.
@kickIDnoah
Copy link

can we get an update, if this will be merged and if so, when?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants