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

Handle transaction rollbacks #148

Open
lukebryant opened this issue Jan 17, 2020 · 4 comments
Open

Handle transaction rollbacks #148

lukebryant opened this issue Jan 17, 2020 · 4 comments
Labels

Comments

@lukebryant
Copy link

lukebryant commented Jan 17, 2020

Currently, if we do:

tm.field = 'new field value'
tm.get_dirty_fields() #returns {'field': 'original field value'}
try:
    with transaction.atomic(): 
        tm.save()
        raise Exception #force a rollback
except Exception:
     pass
tm.get_dirty_fields() #returns {}

This behaviour is problematic, because field in-memory and database values are still different.

It seems like this will be tricky to solve.

We could have a property uncommitted_dirty_fields, which we could clear in an on_commit() signal, but I can't think of a way of setting dirty_fields = uncommitted_dirty_fields in the case of a rollback, since there is no on_rollback() signal.

edit: If we had some way of uniquely identifying the current transaction (I can't see one in current django api), we might be able set dirty_fields = uncommitted_dirty_fields inside get_dirty_fields() if transaction_at_time_of_last_save != current_transaction. Then there'd be some additional complexity from rollbacks inside nested transactions.

@romgar
Copy link
Owner

romgar commented Jan 17, 2020

Hey @lukebryant!
Interesting problem...
We could create a custom decorator / context manager that would wrap the transaction atomic and add this missing logic, but that's not really ideal as people would have to know that they should use this custom feature from dirtyfields instead of the default Django API...

@lukebryant
Copy link
Author

lukebryant commented Jan 17, 2020

Hey @lukebryant!
Interesting problem...
We could create a custom decorator / context manager that would wrap the transaction atomic and add this missing logic, but that's not really ideal as people would have to know that they should use this custom feature from dirtyfields instead of the default Django API...

I was thinking this wouldn't work because the context manager would have no way of getting references to all the models that are changed within its scope. Am I missing something, or did you mean that the context manager would take a list of potentially mutating models as arguments?

edit: thanks for quick response btw :)

@romgar
Copy link
Owner

romgar commented Jan 21, 2020

Yeah you would need to pass these as arguments. Far from ideal for sure...

@romgar
Copy link
Owner

romgar commented Jan 21, 2020

Right now, I would at least document the issue and ask people to manually save and restore dirty fields for their affected model in case of rollback.

LincolnPuzey added a commit that referenced this issue Jan 23, 2022
Document Issue #148 when using transactions and dirtyfields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants