Jaymon / prom

A PostgreSQL or SQLite orm for Python
MIT License
22 stars 4 forks source link

get_modified sets container fields as modified but depopulate doesn't #43

Closed Jaymon closed 3 years ago

Jaymon commented 7 years ago

similar to the problem with JsonField, let's say you have a custom container that stores in the db as a string, but when pulled out from the db it becomes a set() or something similar, well, if you did something like:

f = FooOrm()
f.myset.add("val")

Then the modified fields wouldn't identify that the field has been changed, so there needs to be a way to account for this.

possible solutions:

  1. pass a value into the Field() creation that basically says to always mark this as modified, something like: myset = Field(str, dirty=True)

  2. when pulling values from the db take hashes of the values and compare those hashes with the the current values to determine what is modified, this would be a wholesale re-write of the current system and would be way slower to pull rows from the db, but would generic and wouldn't need the end user to be aware of anything

  3. when pulling out of the db, if after calling Field.isetter() the value is no longer the same type as the what the field says (ie, myset is stored in the db as a string but after calling myset.issetter() it is now a set) then go ahead and automatically mark that field as modified, this would replace the current checks for ObjectField and JsonField and the like since I think this system would be pretty generic, relatively fast (only one extra isinstance() check) and wouldn't involve end user having to do anything.

Jaymon commented 6 years ago

I left off one, basically just always mark those fields as modified and so any call to save() would include those fields, I think this is the easiest to implement and I had this done in get_modified() but depopulate() doesn't use get_modified() sigh

Anyway, I think the path forward is to just always mark those fields as modified and save them anytime save is called, I think the hassle of resaving those fields is pretty rare and so it's not worth implementing a complicated solution.

Jaymon commented 3 years ago

This issue is semi out-of-date with all the refactoring