New Gerrit review created for change d25b6e4: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5790

Michael Bayer (zzzeek) wrote:

OK this is the first I am looking at this as well as at the issue that was posted and I'd like to make significant changes.

When we change a behavior in SQLAlchemy, the path is:

1. add deprecation warnings. These warnings emit when the user is making use of a behavior that we'd like to remove at some point
2. add new APIs that provide a new, modern way to achieve the use case the user is using, which also provide for a means to fully disable the old behavior.
3. If the user does not want the deprecation warnings to emit, their options are 1. use the Python warnings filter or 2. switch to the new API.

So at the moment this patch needs all three of these things added.

I propose the API be this:

1. a new parameter is added to the MS dialect "legacy_identity_insert", defaulting to True. When this parameter is True, the old behavior that kicks in when an INSERT runs, and then looks to see if the row has primary key values present for a table that has autoincrement, runs as it always did
2. when the above "look for an insert with pk cols on a table that has autoincrement" in fact finds a case that it needs to emit "SET IDENTITY INSERT" for the statement to succeed, it emits a deprecation warning.
3. when legacy_identity_insert is set to False (engine/dialect level parameter), none of the above happens. INSERT statements simply run as given.
4. a new feature is added - the "mssql_enable_identity_insert=True" execution option. Usage looks like:

    with engine.begin() as conn:
         conn.execute(
              table.insert(), [... values ...],
              execution_options={"mssql_enable_identity_insert": True}
         )

Note that execution options can be established at several different scopes, including per-connection, per-engine, per statement, per-execute. To keep things simple, since MSExecutionContext receives the union of these dictionaries as a single dictionary and there is not precedent for treating differently-scoped execution options differently, I propose we treat the use of this option identically at all of these levels.

So when the above option is present in MSExecutionContext.execution_options, the "set identity insert" logic takes effect in the exact same way as it does for "legacy_identity_insert" - it intercepts statements, looks for insert() that includes PK col values where those cols have identity, and turns on identity_insert for the table.

I think the entire patch in base.py can be summed up as below:

diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py
index 24425fc81..96b99feb1 100644
--- a/lib/sqlalchemy/dialects/mssql/base.py
+++ b/lib/sqlalchemy/dialects/mssql/base.py
@@ -1867,12 +1867,35 @@ class MSExecutionContext(default.DefaultExecutionContext):
            ):
                insert_has_identity = True
                compile_state = self.compiled.dml_compile_state
-                self._enable_identity_insert = (
-                    id_column.key in self.compiled_parameters[0]
-                ) or (
-                    compile_state._dict_parameters
-                    and (id_column.key in compile_state._insert_col_keys)
-                )
+
+                if (
+                    self.dialect.legacy_identity_insert
+                    or self.execution_options.get(
+                        "mssql_enable_identity_insert", False
+                    )
+                ):
+
+                    self._enable_identity_insert = set_identity_insert = (
+                        id_column.key in self.compiled_parameters[0]
+                    ) or (
+                        compile_state._dict_parameters
+                        and (id_column.key in compile_state._insert_col_keys)
+                    )
+
+                    if (
+                        set_identity_insert
+                        and not self.execution_options.get(
+                            "mssql_enable_identity_insert", False
+                        )
+                    ):
+                        util.warn_deprecated(
+                            "Automatic identity insert is deprecated; "
+                            "please use the mssql_enable_identity_insert "
+                            "execution option"
+                        )
+
+                else:
+                    self._enable_identity_insert = False

            else:
                insert_has_identity = False

thoughts?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5790

I'm not sure we need a new way of doing things. The change was just because sqlalchemy does by default a kinda surprising thing, so the idea was to let users to control that, opting into it or not explicitly.

That said if you prefer to migrate to an execution option, that's fine too for me, but it's kind of optional in this case.