[sakthivelmanii]

Scope autocommit-disabled DDL batching in DatabaseSchemaEditor to DatabaseCreation.create_test_db() so test database schema setup sends queued CREATE TABLE, index, and foreign key statements in batched UpdateDatabaseDdl requests while keeping autocommit enabled during test execution.

[gemini-code-assist]

Code Review

This pull request introduces DDL batching during test database creation for the Django Google Spanner backend. It sets a _batch_ddl_in_schema_editor flag on the connection during create_test_db and overrides __enter__ and __exit__ in DatabaseSchemaEditor to temporarily disable autocommit, batching DDL statements together and restoring autocommit on exit. Comprehensive unit and mock server tests are added to verify this behavior. A critical issue was identified in DatabaseSchemaEditor.__exit__ where an exception raised during super().__exit__ (such as during deferred DDL execution) would not trigger a rollback because exc_type is initially None. A code suggestion is provided to track exceptions occurring during the parent exit call to ensure proper rollback.

[gemini-code-assist]

If an exception occurs during super().__exit__ (for example, when executing deferred DDL statements), exc_type passed to __exit__ is initially None. As a result, exc_type is not None evaluates to False, and self.connection.rollback() is not called, leaving the connection in an inconsistent state. We should track whether an exception occurred either inside the with block or during super().__exit__ to ensure we roll back correctly.

    def __exit__(self, exc_type, exc_value, traceback):
        has_exception = exc_type is not None
        try:
            return super().__exit__(exc_type, exc_value, traceback)
        except Exception:
            has_exception = True
            raise
        finally:
            if getattr(self, "_restore_autocommit", False):
                try:
                    if has_exception:
                        self.connection.rollback()
                finally:
                    self.connection.set_autocommit(True)

[sakthivelmanii]

Updated __exit__ to track clean exit across super().__exit__() and separate explicit commit()/rollback() from set_autocommit(True) so autocommit is always restored even when deferred or server-side DDL execution fails.

[olavloite]

set_autocommit(True) only triggers a flush of the buffered DDL statements in google-cloud-spanner >= 3.72.0. So we should probably also update the minimum version required for Django.

[olavloite]

This is a bit of a code smell. It introduces a private flag that is currently only used by tests, but that someone who inspects the code could find and set, and then it will also be used in production. Can we try to make a design that makes a clear choice between one of the following:

1. A test-only fix that makes sure that our tests use DDL batching
2. A feature that makes DDL batching a public and supported feature for Spanner Django

A test-only solution could look like this:

# In django_spanner/creation.py
from django_spanner.schema import DatabaseSchemaEditor


class BatchingDatabaseSchemaEditor(DatabaseSchemaEditor):
    """Schema editor that batches DDL statements by toggling autocommit."""

    def __enter__(self):
        super().__enter__()
        self._restore_autocommit = False
        if (
            not self.collect_sql
            and not self.connection.in_atomic_block
            and self.connection.get_autocommit()
        ):
            self.connection.set_autocommit(False)
            self._restore_autocommit = True
        return self

    def __exit__(self, exc_type, exc_value, traceback):
        has_exception = exc_type is not None
        try:
            super().__exit__(exc_type, exc_value, traceback)
        except Exception:
            has_exception = True
            raise
        finally:
            if getattr(self, "_restore_autocommit", False):
                try:
                    if has_exception:
                        self.connection.rollback()
                finally:
                    self.connection.set_autocommit(True)


class DatabaseCreation(BaseDatabaseCreation):
    def create_test_db(self, *args, **kwargs):
        if os.environ.get("RUNNING_SPANNER_BACKEND_TESTS") == "1":
            self.mark_skips()
        
        # Temporarily use the batching schema editor during test database creation
        original_schema_editor_class = self.connection.SchemaEditorClass
        self.connection.SchemaEditorClass = BatchingDatabaseSchemaEditor
        try:
            return super().create_test_db(*args, **kwargs)
        finally:
            self.connection.SchemaEditorClass = original_schema_editor_class

Or if we want to support this publicly for both tests and applications, then we could add an option to DATABASES:

# In settings.py
DATABASES = {
    "default": {
        "ENGINE": "django_spanner",
        "OPTIONS": {
            "batch_ddl": True,  # Defaults to True or False depending on what we find the safest
        },
    }
}

# In DatabaseSchemaEditor.__enter__
batch_ddl = self.connection.settings_dict.get("OPTIONS", {}).get("batch_ddl", True)
if batch_ddl and not self.collect_sql and not self.connection.in_atomic_block and self.connection.get_autocommit():
    ...

I would personally lean towards making it a public option, and maybe disable it by default in the first release, and consider making it the default in a follow-up release.