Merge Django Subatomic
- Dominant language
- No language data
- Stars
- 188
- Forks
- 7
- PR merge metrics
- No merged PRs in 30d
Description
### Code of Conduct
- [x] I agree to follow Django's Code of Conduct
### Package Information
https://github.com/kraken-tech/django-subatomic
### Problem
Django's transactions API comes with some hidden costs:
- Without looking at callers, it's impossible to know if `atomic` will create a transaction or create a savepoint. This can lead to subtle bugs which Django offers no way to prevent.
- There is no (test-aware) way to require an existing transaction. This is useful for code which knows it must be run in a transaction, but should not be responsible for determining the scope of a transaction (e.g. a model method that does two writes).
- There is no (test-aware) way for a function to ensure a transaction is not already open other than creating a transaction with `durable=True`.
- After-commit callbacks are not run in tests by default. The standard solutions to enabling them are either slow, or have order-of-execution issues.
- Savepoints can be created as decorators. Savepoints are intrinsically linked to error handling. They are only required to provide a safe place to continue from after a failure within a transaction. When using `atomic` as a decorator, savepoint creation gets separated from the error handling logic it is linked to.
- When no arguments are passed to `atomic` (and a transaction is already open), it defaults to creating a savepoint. This is wasteful if no roll back handling is required.
- When `on_commit` is called without an active transaction, it will execute the callback immediately. This can lead to test behaviour that differs from how code runs in production, and is surprising: code which apparently defers work does not. (Note: this could be addressed in Django before introducing the other proposed changes from Django Subatomic.)
For further discussion of these points, see the following docs from Django Subatomic:
- https://kraken-tech.github.io/django-subatomic/v1.0.0/django-atomic/
- https://kraken-tech.github.io/django-subatomic/v1.0.0/features/
- https://kraken-tech.github.io/django-subatomic/v1.0.0/testing-after-commit-callbacks/
### Rationale
Bringing these tools into Django makes them more available to Django's users. It will allow us to document this safer, more expressive API as the preferred approach. It will also mean we can use and benefit from these APIs within Django, and in third party packages, without fear of introducing extra dependencies.
Using these tools will help to eliminate whole classes of bugs which are currently enabled by Django, and so make Django a more reliable choice.
Integrating Subatomic's testing features will allow Django projects to test transactional behaviour (especially on-commit callbacks) with greater confidence that their tests match production behaviour.
Note: this is _not_ a proposal to deprecate `atomic`.
### Additional Details
There are some known challenges to integrating Subatomic's API into Django.
- The name `savepoint` is already taken (see #138).
- Subatomic's `run_after_commit` serves the same purpose as `on_commit` except it raises an error if there is no transaction open. It's not clear how to integrate them.
- Many of Subatomic's guard-rails can be disabled with settings, which would mean Django needs to introduce a number of new settings. It's not clear if/how they should be enabled by default.
- Documenting this new API will not be trivial. Subatomic's existing documentation can act as inspiration here.
### Implementation Details
The API we believe should be moved are:
- `transaction`
- `transaction_required`
- `durable`
- `savepoint`
- `in_transaction`: this higher level query accounts for transactions created by the test suite (unlike Django's existing API, which should remain).
- `run_after_commit`: at least in spirit -- `on_commit` could be changed instead.
- `part_of_a_transaction`: for tests to call "transaction required" code without doing transaction-completion simulation.
We may also want to introduce:
- `transaction_if_not_already`: it's not yet clear if this is required.
- `dbs_with_open_transactions`: this is not required, but can be helpful.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.