feat(django-spanner)!: support Django 5.2 and deprecate Django 3.2/4.2#16624
feat(django-spanner)!: support Django 5.2 and deprecate Django 3.2/4.2#16624sinhasubham wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Spanner database backend to support Django 5.2 and Python 3.10+, while removing legacy support for Django 3.2 and 4.2. Key changes include the implementation of a global client cache to prevent initialization crashes, support for JSONArray, GeneratedField, and db_default, and a retry mechanism for flushing tables with foreign key constraints. Feedback highlights several critical issues: the use of a 32-bit mask for primary key generation significantly reduces the key space, and making GOOGLE_CLOUD_PROJECT mandatory is a breaking change for local development. Additionally, the package version was incorrectly decremented, and a minor version bump was recommended for this release. Finally, using WHERE 1=1 was suggested as a more idiomatic alternative to WHERE true for flush operations.
2b71c2a to
f6098f1
Compare
…bsolete workflows
f6098f1 to
4fc923b
Compare
|
|
||
| # Global cache for Spanner client to prevent multiple initializations | ||
| # which can cause OpenTelemetry 'MeterProvider override' crashes. | ||
| _SPANNER_CLIENT_CACHE = None |
There was a problem hiding this comment.
I dont understand why do we need this, ideally our Spanner client library should handle the override crash ?
There was a problem hiding this comment.
Added this global cache to fix a crash observed during testing: OpenTelemetry MeterProvider override crashes. In Django, DatabaseWrapper instances are created frequently (e.g., per thread or request), leading to multiple spanner.Client initializations. While it would be ideal for the client library to handle this, this cache effectively prevents the crash and resource exhaustion in the meantime.
There was a problem hiding this comment.
Got it. Please create a bug in Python Spanner Client and add a todo here to clean this up once the bug is fixed
| "project": os.environ.get("GOOGLE_CLOUD_PROJECT") | ||
| or self.settings_dict.get("project") | ||
| or self.settings_dict.get("PROJECT") | ||
| or "test-project", |
There was a problem hiding this comment.
This is being done in multiple places, better to extract in "init" method once and use it everywhere.
Also why do we support self.settings_dict.get("project") and self.settings_dict.get("PROJECT") now ?
There was a problem hiding this comment.
Addressed. I made these changes for some debugging purpose earlier, missed to revert in the final commit.
| conn_params.pop("instance", None) | ||
| conn_params.pop("instance_id", None) | ||
| conn_params.pop("client", None) |
There was a problem hiding this comment.
Why this change is required ?
We are not using instance_id or client anywhere. We are only using instance.instance_id or instance._client
There was a problem hiding this comment.
Because get_connection_params puts instance_id into the conn_params dictionary by default, passing **conn_params will try to pass instance_id again. Python rejects this because it is receiving the same argument twice.
Popping instance_id and client from conn_params is required to support the global client cache fix. Removing this crashes the adapter on connection.
| # This method copies the complete code of this overridden method from | ||
| # Django core and modify it for Spanner by adding one line |
There was a problem hiding this comment.
This is duplicate comment
| # https://developers.google.com/open-source/licenses/bsd | ||
|
|
||
| __version__ = "4.0.3" | ||
| __version__ = "4.1.0" |
There was a problem hiding this comment.
Why are we modifying this version manually ? shouldn't this be updated automatically once we do the release ?
There was a problem hiding this comment.
Makes sense, addressed
| self.assertEqual( | ||
| sql_compiled, | ||
| "SELECT tests_report.name FROM tests_report ORDER BY " | ||
| "SELECT tests_report.name AS name FROM tests_report ORDER BY " |
There was a problem hiding this comment.
For my knowledge why this change is required ?
|
|
||
| UNIT_TEST_PYTHON_VERSIONS = [ | ||
| "3.8", | ||
| "3.9", |
There was a problem hiding this comment.
I thought we received 3.9 support as well, as we removed 3.9 integration test suite file
There was a problem hiding this comment.
The unit 3.9 workflow step fails without version 3.9 in this list. And we cannot remove 3.9 from github workflows as it is a common workflow file for monorepo now. I will check on this with owning team how we are planning to handle this
| if session.python == "3.9": | ||
| session.skip("Python 3.9 is not supported for Django 5.2 tests") |
There was a problem hiding this comment.
Lets remove 3.9 from the list then ?
There was a problem hiding this comment.
Since unittest.yml is a shared github workflow for all packages in the monorepo, I cannot remove Python 3.9 from it without affecting other libraries.
| # https://developers.google.com/open-source/licenses/bsd | ||
|
|
||
| __version__ = "4.0.3" | ||
| __version__ = "4.1.0" |
There was a problem hiding this comment.
Also this should go as a Major version release, since we are deprecating older vesion of Django.
PR title should be feat! to trigger a major version
There was a problem hiding this comment.
Sure, will update the PR title
| from django.db import DEFAULT_DB_ALIAS | ||
| from django.db.models.fields import ( | ||
|
|
||
| from django.db import DEFAULT_DB_ALIAS # noqa: E402 |
There was a problem hiding this comment.
why are you suppressing linting errors?
|
|
||
| def gen_rand_int64(): | ||
| # Credit to https://stackoverflow.com/a/3530326. | ||
| # Use 32-bit integer for Emulator compatibility (High-bit issues observed). |
There was a problem hiding this comment.
Is this a valid comment?
There was a problem hiding this comment.
I had removed the 32-bit integer changes here but missed to remove this comment earlier. Removed now
| method_name, | ||
| skip("unsupported by Spanner")(method), | ||
| ) | ||
| try: |
There was a problem hiding this comment.
Make you sure you clean up the method. Not suppress the error.
There was a problem hiding this comment.
Since we need to support a range of Django versions, a test that is non-existent in 5.2 might still need to be skipped in 4.2. Suppressing the AttributeError is the best way to handle this scenario in long term according to me.
| "constraint": self.sql_check_constraint % {"check": check}, | ||
| } | ||
|
|
||
| def _unique_sql( |
There was a problem hiding this comment.
why are we removing it?
There was a problem hiding this comment.
The method was initially removed in an attempt to rely on the Django 5.2 base class implementation. But i agree, the custom implementation is required to prevent Django from generating inline UNIQUE constraints in CREATE TABLE statements, which base class implementation does not support in the same way. I have restored the method and updated it to support Django 5.2's nulls_distinct feature.
| @@ -28,7 +28,6 @@ | |||
| DEFAULT_PYTHON_VERSION = "3.14" | |||
|
|
|||
| UNIT_TEST_PYTHON_VERSIONS = [ | |||
There was a problem hiding this comment.
Remove support for 3.8 and 3.9
There was a problem hiding this comment.
The unit 3.9 workflow step fails without version 3.9 in this list. And we cannot remove 3.9 from github workflows as it is a common workflow file for monorepo now. I will check on this with owning team how we are planning to handle this
| else: | ||
| return [] | ||
|
|
||
| def execute_sql_flush(self, sql_list): |
There was a problem hiding this comment.
this looks so bruteforce. is there any other better approach?
There was a problem hiding this comment.
Earlier, we were relying on Django's default execute_sql_flush, which fails on Spanner when foreign key constraints are present. This retry logic was introduced in this PR specifically to handle Spanner's inability to disable foreign key checks during test cleanup.
I agree that the retry loop in execute_sql_flush is a brute-force approach. But to avoid the complexity of implementing a full topological sort of the tables based on their foreign key dependencies to determine the exact deletion order (child tables first, then parents), i used this multi-pass approach.
| # Spanner does not support expression indexes | ||
| # example: CREATE INDEX index_name ON table (LOWER(column_name)) | ||
| supports_expression_indexes = False | ||
| supports_stored_generated_columns = True | ||
|
|
||
| # Django tests that aren't supported by Spanner. | ||
| skip_tests = ( |
There was a problem hiding this comment.
We need to group them by cause. Otherwise we will not know if we are doing it correctly or not.
| try: | ||
| cursor.execute(sql) | ||
| progress = True | ||
| except Exception as e: |
There was a problem hiding this comment.
catch more specific exception ?
There was a problem hiding this comment.
Spanner might raise various errors (such as Aborted, FailedPrecondition, or IntegrityError) depending on which constraint is violated first. I felt adding generic exception catching was better in this case instead of maintaining list of exceptions for same block.
01d798c to
941fe99
Compare
This PR migrates the
django-google-spannerpackage to support Django 5.2 and deprecates support for older Django versions (3.2 and 4.2). Due to the deprecation of older versions, this is a breaking change and requires a major version release.💥 BREAKING CHANGES: