Versioning issues on session.flush #79
Conversation
sqlalchemy_continuum/utils.py
Outdated
| @@ -414,3 +414,20 @@ def changeset(obj): | |||
| if new_value: | |||
| data[prop.key] = [new_value, old_value] | |||
| return data | |||
|
|
|||
|
|
|||
| def commited_identity(obj): | |||
There was a problem hiding this comment.
Judging by the function description maybe we could just use: sa.inspect(obj).identity
There was a problem hiding this comment.
For sure! I knew there was probably a built-in API for this...
|
Good stuff and great unit tests! |
|
Progress has stalled a bit on this... on both items. |
Added tests on behavior when primary keys are modified within the same transaction but after a flush()
Both operations and unit of work internal dicts use PKs to point to stored objects. If the PKs change mid-transaction, keys are now updated to the new correct values.
utils.commited_identity is InstanceState.identity
trasaction Added tests for the create/delete and update/delete within the same transaction cases. Added new operation type STALE_DATA that marks an object's version entry as stale that should be removed. Added method to remove version object in the uow.
Modify the UPDATE trigger to: 1. Leave operation type as INSERT for existing versions whose operation type is INSERT. This fixes the missing INSERT version. 2. Use the OLD primary keys to check if a version already exists. This fixes behavior on primary key updates 3. Added documentation
1. Delete versions that already exist with an operation type of INSERT 2. Added documentation
|
Fixing the INSERT + DELETE within the same transaction took some effort. I decided to add a new I also tried to update the transaction changes plugin but met different problems, so it is left as it was. |
|
I updated the Postgres triggers to have the same behavior on INSERT->UPDATE and INSERT->DELETE within the same transaction as the sql alchemy implementation. I also documented some decisions. Tests pass. You may want to have a look at the implementation. |
| @@ -107,6 +107,7 @@ def after_version_class_built(self, parent_cls, version_cls): | |||
| primaryjoin=( | |||
| self.model_class.transaction_id == transaction_column | |||
| ), | |||
| foreign_keys=[self.model_class.transaction_id] | |||
| foreign_keys=[self.model_class.transaction_id], | |||
| passive_deletes='all' | |||
There was a problem hiding this comment.
This is needed otherwise after deleting a version sql alchemy will by default set the transaction_id of the TransactionChange to NULL, which is never desired.
There was a problem hiding this comment.
Rather than this I think a better one would be:
cascade='all, delete-orphan',
passive_deletes=True
This uses foreign keys for deletion, but at the same time marks the in memory objects as deleted.
There was a problem hiding this comment.
What was happening was that deleting an object of ArticleVersion was setting the transaction_id of the TransactionChanges object to NULL, which was causing a AssertionError: Dependency rule tried to blank-out primary key column 'transaction_changes.transaction_id' on instance '<TransactionChanges at 0x7f7d17a544d0>' error.
Deleting a version object should never affect a TransactionChanges object imo (at least not through foreign key behavior). If for instance you have 2 new ArticleVersions, deleting one of them should not affect the existing TransactionChanges that points to the ArticleVersion (Deleting both of them should, but that's a different story).
With the above in mind, I don't think the cascade='all, delete-orphan' has a place here as it creates a wrong impression about what the motivation here is. The passive_deletes=True does make sense. I set it to passive_deletes='all' because according to documentation, if the changes object is in memory SQL Alchemy will still set the foreign key to NULL, which is not desired. This whole thing can also be avoided if we remove the .changes attribute on the version class; it can still be accessed through the .transaction.changes attribute chain.
What does make sense is to use cascade='all, delete-orphan' on the Transaction->TransactionChanges relation. When a whole transcation is deleted, the changes have to go away with it as well.
There was a problem hiding this comment.
Hmm thanks for pointing that out! I removed the generated changes attr from version classes.
Hey, opening a pull request with some fixes related to mid-transaction flushes. Check the added tests to see what I think the expected behavior should be
Update: