mongoengine icon indicating copy to clipboard operation
mongoengine copied to clipboard

Multiple fixes for cascade=True save issues

Open nickfrev opened this issue 2 years ago • 1 comments

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.

nickfrev avatar Jan 07 '22 18:01 nickfrev

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

kickIDnoah avatar Nov 15 '22 13:11 kickIDnoah