Skip to content

Adapt _process_response for runtime database selection#827

Merged
albertyw merged 2 commits intojazzband:masterfrom
izabala033:master
Sep 27, 2025
Merged

Adapt _process_response for runtime database selection#827
albertyw merged 2 commits intojazzband:masterfrom
izabala033:master

Conversation

@izabala033
Copy link
Copy Markdown
Contributor

Previously, _process_response used a @transaction.atomic decorator, which evaluated the database at import time. Since the database can change at runtime, this caused connection errors in certain scenarios (in my case, multitenant server that uses request host to identify database).

This PR updates _process_response to:

Use a runtime-evaluated database alias inside a with transaction.atomic(using=...) block.

Impact:

Fixes runtime database connection issues.

Ensures profiling and response processing continue to work as intended.

Copy link
Copy Markdown
Member

@albertyw albertyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that there are no other transaction.atomic decorators, though there are 3 other with transaction.atomics in models.py

Comment thread silk/middleware.py
collector = DataCollector()
collector.stop_python_profiler()
silk_request = collector.request
with transaction.atomic(using=router.db_for_write(models.SQLQuery)):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to add a comment here describing why this is a with block instead of a decorator. This was actually the subject of a discussion in #801 but it did not go over the possibility of the database mutating at runtime.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment added, ty :)

@albertyw albertyw merged commit ce68a39 into jazzband:master Sep 27, 2025
91 checks passed
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.

3 participants