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

enforce_atomic decorator for commit callbacks to workaround MySQL limitations #106

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Joshun
Copy link
Contributor

@Joshun Joshun commented Sep 8, 2023

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
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Patch coverage: 89.47% and project coverage change: -0.05% ⚠️

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

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

Additional details and impacted files
@@            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 Coverage Δ
hordak/models/core.py 96.88% <88.88%> (-1.35%) ⬇️
hordak/tests/models/test_core.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@autoantwort
Copy link

autoantwort commented Sep 8, 2023

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
Copy link
Contributor Author

Joshun commented Sep 18, 2023

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
Copy link

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

@adamcharnock
Copy link
Owner

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'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants