adamcharnock / django-hordak

Double entry accounting in Django
http://django-hordak.readthedocs.io
MIT License
228 stars 55 forks source link

enforce_atomic decorator for commit callbacks to workaround MySQL limitations #106

Open Joshun opened 10 months ago

Joshun commented 10 months ago

Possible solution to an issue noticed here with the MySQL implementation of the Account and Leg triggers We were using transaction.on_commit to call stored procedures after the transaction had been committed, which successfully raise an error but do not probably rollback changes since the commit has already taken place.

Mostly an experiment, there are almost certainly reasons why this approach may be unsuitable.

Since MySQL doesn't have deferred triggers, the only way I can see of us enforcing these triggers is to call them from Python code.

As you can see some of the tests fail, this is because this approach currently only works if functions are decorated with @enforce_atomic

codecov-commenter commented 10 months ago

Codecov Report

Patch coverage: 89.47% and project coverage change: -0.05% :warning:

Comparison is base (322c264) 92.95% compared to head (213d807) 92.90%. Report is 5 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #106 +/- ## ========================================== - Coverage 92.95% 92.90% -0.05% ========================================== Files 59 59 Lines 3817 3848 +31 Branches 245 254 +9 ========================================== + Hits 3548 3575 +27 - Misses 224 226 +2 - Partials 45 47 +2 ``` | [Files Changed](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/106?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock) | Coverage Δ | | |---|---|---| | [hordak/models/core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/106?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL21vZGVscy9jb3JlLnB5) | `96.88% <88.88%> (-1.35%)` | :arrow_down: | | [hordak/tests/models/test\_core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/106?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL21vZGVscy90ZXN0X2NvcmUucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.

:loudspeaker: Have feedback on the report? Share it here.

autoantwort commented 10 months ago

Maybe one could create a feature request at the django project for a pre_commit callback. Edit: Ok that would not help since there are no django transactions in auto commit mode.

Joshun commented 9 months ago

Maybe one could create a feature request at the django project for a pre_commit callback. Edit: Ok that would not help since there are no django transactions in auto commit mode.

I was thinking if the change was made in django-hordak, you could throw an exception when attempting to save a Leg object outside of an enforce_commit transaction (I could add a check to the overriden .save() method - preventing autocommit etc. and someone doing Leg.save() without doing a transaction. But I appreciate this might seem a bit too hacky?

autoantwort commented 9 months ago

And that does not cover all cases. You can change an object without calling save. For example by calling update

adamcharnock commented 1 week ago

FWIW: I'm not keen on adding complexity to the core Hordak codebase in order to support MySQL's limitations (which includes the current use of transaction.on_commit). My (perhaps hard-nosed) attitude is that if people deploying Hordak want advanced database features then I would prefer they work towards using a database that supports these features.

Adding complexity and workarounds to Hordak's core feels like pushing in the wrong direction to me.

I'm open to conversation on this, but I'm currently leaning towards Hordak having 'MySQL support with caveats'.