fix: Make transactions target the DB alias selected by the router #801
Conversation
…g the same DB alias as selected by the db router
for more information, see https://pre-commit.ci
| @@ -142,28 +143,29 @@ def process_request(self, request): | |||
| request_model = RequestModelFactory(request).construct_request_model() | |||
| DataCollector().configure(request_model, should_profile=should_profile) | |||
|
|
|||
| @transaction.atomic() | |||
There was a problem hiding this comment.
Looking at the transaction.atomic implementation in https://github.com/albertyw/django/blob/master/django/db/transaction.py#L316, this looks like it can be modified into transaction.atomic(using=router.db_for_write(models.SQLQuery)) instead of converting into a with block
There was a problem hiding this comment.
I agree that I prefer it using the annotation to avoid a level of indentation. I made the suggested change successfully for SilkyMiddleware's _process_response, but unfortunately for the other cases there are issues with scoping...
In SQLQueryManager when I reference SQLQuery in the annotation, if I order SQLQueryManager before SQLQuery, it's not in scope in the annotation.
If I order SQLQueryManager after SQLQuery, then SQLQueryManager is not in scope of the SQLQuery class:
Additionally, in the save and delete functions I run into similar issues, I guess because neither self or SQLQuery are in scope in the annotation:
In both cases I could also solve this by referencing some arbitrary other silk model class that is defined earlier, but it feels weird to reference a different models in SQLQuery's save/delete/Query manager. It might mislead someone that there is some relationship between the models that doesn't actually exist.
In these cases I decided to leave it as-is, please let me know if you have any thoughts/disagreements.
…ntext for transaction where possible
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #801 +/- ##
==========================================
- Coverage 87.26% 87.13% -0.14%
==========================================
Files 53 53
Lines 2160 2161 +1
==========================================
- Hits 1885 1883 -2
- Misses 275 278 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the help with this and quick reviews! |
In #800 I identified a mistake in the transaction handling in the package. This meant that transactions were always started for the
defaultDB alias, even if theDATABASE_ROUTERused by the project would actually result in silk models being written to a different DB alias.In my scenario, it caused my application to crash as I do not have a configuration set for the
defaultalias.The fix I have applied is similar to one I found in jazzband/django-axes#1109, where same same bug was once present in
django-axes(this is another package I use in my project where I do not encounter this bug).Assumptions
I have used
router.db_for_write(models.SQLQuery)to determine the DB alias to use for the transaction. I pickedmodels.SQLQueryat random.This makes an assumption that all of silk's models are routed to the same DB alias. I think this is a fair assumption, but technically someone could make a weird setup where some silk models are routed to one DB alias, and other models to another DB alias. This approach does not handle this.
Testing
I have performed manual testing with these changes in my project that was broken prior to this change. It solves the issue.
I spent far too long trying to write some test cases for this change, but it was just far too difficult. Django didn't seem to permit using
override_settingsto override theDATABASESconfig for a specific test class/case. In the end it proved too challenging, so I have not produced any tests.I think the change is sufficiently low risk that perhaps we can accept this omission.