chore(django-spanner): disable savepoints and remove can_rollback_tests#16866
chore(django-spanner): disable savepoints and remove can_rollback_tests#16866sinhasubham merged 1 commit intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the DatabaseFeatures class in django_spanner by setting uses_savepoints to False and removing the can_rollback_tests attribute. The reviewer identified that removing can_rollback_tests is a breaking change for supported Django versions (4.2, 5.0, and 5.1), as it would cause the test suite to incorrectly attempt transaction-based isolation instead of the required flush method. A suggestion was provided to retain the attribute and add explanatory comments to both flags for better compatibility and clarity.
sinhasubham
force-pushed
the
django-savepoints-fix
branch
from
04e6af6 to
adf7b2f
Compare
sinhasubham
changed the title
sinhasubham
force-pushed
the
django-savepoints-fix
branch
from
adf7b2f to
9e838d0
Compare
| "fixtures_model_package.tests.FixtureTestCase.test_loaddata", | ||
| "sessions_tests.tests.CacheDBSessionTests.test_session_asave_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.CacheDBSessionTests.test_session_save_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.CacheDBSessionWithTimeZoneTests.test_session_asave_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.CacheDBSessionWithTimeZoneTests.test_session_save_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.CustomDatabaseSessionTests.test_session_asave_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.CustomDatabaseSessionTests.test_session_save_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.DatabaseSessionTests.test_session_asave_does_not_resurrect_session_logged_out_in_other_context", | ||
| "sessions_tests.tests.DatabaseSessionTests.test_session_save_does_not_resurrect_session_logged_out_in_other_context", |
There was a problem hiding this comment.
These tests are designed to simulate expected failures within a transaction and rely on savepoints to recover and continue execution. Because we have disabled savepoints (uses_savepoints = False) for production safety, and Cloud Spanner breaks the entire transaction upon encountering any error, these tests are unable to recover and fail with TransactionManagementError. They have been skipped on the emulator as they are fundamentally incompatible with Spanner's transaction model without native savepoint support.
These were passing previously because when uses_savepoints was set to True, our adapter emulated savepoints as dummy no-op statements. This allowed the test flow to proceed without Django immediately detecting that the transaction was broken.
|
Fixed as of 72f0438 |
sinhasubham
force-pushed
the
django-savepoints-fix
branch
from
9e838d0 to
72f0438
Compare
| supports_subqueries_in_group_by = False | ||
| uses_savepoints = True | ||
| uses_savepoints = False # Spanner does not support savepoints. | ||
| can_rollback_tests = False # Spanner savepoints are no-ops; rely on flush. |
There was a problem hiding this comment.
Is this value still correct? The comment is at least now outdated, as we explicitly turn off savepoints
There was a problem hiding this comment.
We can remove this line if we plan to stop supporting versions 5.0/5.1. This is dead code for Django >=5.2.
3 participants
uses_savepoints = Falseinfeatures.pyto prevent silent rollback failures in production, as Cloud Spanner does not support savepoints natively.Additionally, 9 tests (primarily from
sessions_tests) were added to the emulator skip list. These tests are designed to simulate expected failures within a transaction and rely on savepoints to recover and continue execution. Because we have disabled savepoints (uses_savepoints = False) for production safety, and Cloud Spanner breaks the entire transaction upon encountering any error, these tests are unable to recover and fail withTransactionManagementError. They have been skipped on the emulator as they are fundamentally incompatible with Spanner's transaction model without native savepoint support.Fixes #16839