Fix | Default SqlCommand's transaction to owning SqlConnection's local transaction

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs

@@ -261,9 +261,6 @@ private CachedAsyncState cachedAsyncState
 
         private SqlNotificationRequest _notification;
 
-        // transaction support
-        private SqlTransaction _transaction;
-
         private StatementCompletedEventHandler _statementCompletedEventHandler;
 
         private TdsParserStateObject _stateObj; // this is the TDS session we're using.
@@ -376,14 +373,6 @@ private SqlCommand(SqlCommand from) : this()
                     }
                 }
 
-                // Check to see if the currently set transaction has completed.  If so,
-                // null out our local reference.
-                if (null != _transaction && _transaction.Connection == null)
-                {
-                    _transaction = null;
-                }
-
-
                 // Command is no longer prepared on new connection, cleanup prepare status
                 if (IsPrepared)
                 {
@@ -467,25 +456,55 @@ internal SqlStatistics Statistics
         {
             get
             {
-                // if the transaction object has been zombied, just return null
-                if ((null != _transaction) && (null == _transaction.Connection))
-                {
-                    _transaction = null;
-                }
-                return _transaction;
+                // there is no real reason to read this value except when creating a new command based on this command
+                // so we just use the Connection's CurrentTransaction since that it the only valid value anyway
+                SqlTransaction transaction = (_activeConnection?.InnerConnection as SqlInternalConnectionTds)?.CurrentTransaction?.Parent;
+                return transaction?.IsZombied == true ? default(SqlTransaction) : transaction;
             }
             set
             {
                 // Don't allow the transaction to be changed while in an async operation.
-                if (_transaction != value && _activeConnection != null)
+                if (Transaction != value && _activeConnection != null)
                 { // If new value...
                     if (cachedAsyncState.PendingAsyncOperation)
                     { // If in pending async state, throw
                         throw SQL.CannotModifyPropertyAsyncOperationInProgress();
                     }
                 }
 
-                _transaction = value;
+                // on null transaction just move on
+                if (value is null)
+                {
+                    return;
+                }
+
+                // on zombied transaction throw and warn the user
+                // or just fail silently?
+                if (value.IsZombied)
+                {
+                    throw ADP.TransactionZombied(value);
+                }
+
+                // if connection is not already set-up, grab it from the transaction
+                if (null == _activeConnection)
+                {
+                    Connection = value.Connection;
+                }
+
+                // if connection differ's from the transaction's connection, throw
+                if (_activeConnection != value.Connection)
+                {
+                    throw ADP.TransactionConnectionMismatch();
+                }
+
+                // check to see if transaction
+                if (value != Transaction)
+                {
+                    throw ADP.TransactionConnectionMismatch();
+                }
+
+                // we don't store the transaction in any way as the connection has its own value
+                // all above code are just validations for the user's input
             }
         }
 
@@ -4617,21 +4636,6 @@ private void ValidateCommand(bool isAsync, [CallerMemberName] string method = ""
             // close any non MARS dead readers, if applicable, and then throw if still busy.
             // Throw if we have a live reader on this command
             _activeConnection.ValidateConnectionForExecute(method, this);
-            // Check to see if the currently set transaction has completed.  If so,
-            // null out our local reference.
-            if (null != _transaction && _transaction.Connection == null)
-                _transaction = null;
-
-            // throw if the connection is in a transaction but there is no
-            // locally assigned transaction object
-            if (_activeConnection.HasLocalTransactionFromAPI && (null == _transaction))
-                throw ADP.TransactionRequired(method);
-
-            // if we have a transaction, check to ensure that the active
-            // connection property matches the connection associated with
-            // the transaction
-            if (null != _transaction && _activeConnection != _transaction.Connection)
-                throw ADP.TransactionConnectionMismatch();
 
             if (string.IsNullOrEmpty(this.CommandText))
                 throw ADP.CommandTextRequired(method);

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalTransaction.cs

@@ -190,14 +190,7 @@ internal SqlTransaction Parent
         {
             get
             {
-                SqlTransaction result = null;
-                // Should we protect against this, since this probably is an invalid state?
-                Debug.Assert(null != _parent, "Why are we calling Parent with no parent?");
-                if (null != _parent)
-                {
-                    result = (SqlTransaction)_parent.Target;
-                }
-                return result;
+                return _parent?.Target as SqlTransaction;
             }
         }
 

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs

@@ -275,9 +275,6 @@ private CachedAsyncState cachedAsyncState
         private SqlNotificationRequest _notification;
         private bool _notificationAutoEnlist = true;            // Notifications auto enlistment is turned on by default
 
-        // transaction support
-        private SqlTransaction _transaction;
-
         private StatementCompletedEventHandler _statementCompletedEventHandler;
 
         private TdsParserStateObject _stateObj; // this is the TDS session we're using.
@@ -512,13 +509,6 @@ private SqlCommand(SqlCommand from) : this()
                     }
                 }
 
-                // Check to see if the currently set transaction has completed.  If so,
-                // null out our local reference.
-                if (null != _transaction && _transaction.Connection == null)
-                {
-                    _transaction = null;
-                }
-
                 // If the connection has changes, then the request context may have changed as well
                 _smiRequestContext = null;
 
@@ -687,27 +677,58 @@ internal SqlStatistics Statistics
         {
             get
             {
-                // if the transaction object has been zombied, just return null
-                if ((null != _transaction) && (null == _transaction.Connection))
-                { // MDAC 72720
-                    _transaction = null;
-                }
-                return _transaction;
+                // there is no real reason to read this value except when creating a new command based on this command
+                // so we just use the Connection's CurrentTransaction since that it the only valid value anyway
+                SqlTransaction transaction = (_activeConnection?.InnerConnection as SqlInternalConnectionTds)?.CurrentTransaction?.Parent;
+                return transaction?.IsZombied == true ? default(SqlTransaction) : transaction;
             }
             set
             {
                 // Don't allow the transaction to be changed while in a async opperation.
-                if (_transaction != value && _activeConnection != null)
+                if (Transaction != value && _activeConnection != null)
                 { // If new value...
                     if (cachedAsyncState.PendingAsyncOperation)
                     { // If in pending async state, throw
                         throw SQL.CannotModifyPropertyAsyncOperationInProgress(SQL.Transaction);
                     }
                 }
 
+                // on null transaction just move on
+                if (value is null)
+                {
+                    return;
+                }
+
+                // on zombied transaction throw and warn the user
+                // or just fail silently?
+                if (value.IsZombied)
+                {
+                    throw ADP.TransactionZombied(value);
+                }
+
+                // if connection is not already set-up, grab it from the transaction
+                if (null == _activeConnection)
+                {
+                    Connection = value.Connection;
+                }
+
+                // if connection differ's from the transaction's connection, throw
+                if (_activeConnection != value.Connection)
+                {
+                    throw ADP.TransactionConnectionMismatch();
+                }
+
+                // check to see if transaction
+                if (value != Transaction)
+                {
+                    throw ADP.TransactionConnectionMismatch();
+                }
+
                 // TODO: Add objid here
                 Bid.Trace("<sc.SqlCommand.set_Transaction|API> %d#\n", ObjectID);
-                _transaction = value;
+
+                // we don't store the transaction in any way as the connection has its own value
+                // all above code are just validations for the user's input
             }
         }
 
@@ -5776,21 +5797,6 @@ private void ValidateCommand(string method, bool async)
                 SqlInternalConnection.BestEffortCleanup(bestEffortCleanupTarget);
                 throw;
             }
-            // Check to see if the currently set transaction has completed.  If so,
-            // null out our local reference.
-            if (null != _transaction && _transaction.Connection == null)
-                _transaction = null;
-
-            // throw if the connection is in a transaction but there is no
-            // locally assigned transaction object
-            if (_activeConnection.HasLocalTransactionFromAPI && (null == _transaction))
-                throw ADP.TransactionRequired(method);
-
-            // if we have a transaction, check to ensure that the active
-            // connection property matches the connection associated with
-            // the transaction
-            if (null != _transaction && _activeConnection != _transaction.Connection)
-                throw ADP.TransactionConnectionMismatch();
 
             if (ADP.IsEmpty(this.CommandText))
                 throw ADP.CommandTextRequired(method);

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/sqlinternaltransaction.cs

@@ -215,14 +215,7 @@ internal SqlTransaction Parent
         {
             get
             {
-                SqlTransaction result = null;
-                // Should we protect against this, since this probably is an invalid state?
-                Debug.Assert(null != _parent, "Why are we calling Parent with no parent?");
-                if (null != _parent)
-                {
-                    result = (SqlTransaction)_parent.Target;
-                }
-                return result;
+                return _parent?.Target as SqlTransaction;
             }
         }
 

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs

@@ -113,7 +113,8 @@ public void TestSqlTransactionRollbackToSavePoint(string connection)
                     sqlTransaction.Rollback();
 
                     // Now even row no:1 should not be available.
-                    VerifyRecordAbsent(sqlConnection, customer1, sqlTransaction);
+                    // Removed transaction argument as it's zombied.
+                    VerifyRecordAbsent(sqlConnection, customer1);
                 }
             }
         }

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ConversionTests.cs

@@ -444,7 +444,10 @@ List<ValueErrorTuple> GenerateOutOfRangeValuesForType(SqlDbType type, int length
                 case SqlDbType.Date:
                     // These values are out of range and should fail.
                     list.Add(new ValueErrorTuple("10000/1/1", true));
-                    list.Add(new ValueErrorTuple("0/12/31", true));
+                    // Former value of "0/12/31" passed insert even if test expected failure.
+                    // Debugging noticed a coerced value that turned into 2000/12/31.
+                    // Changed date to a clear invalid day value.
+                    list.Add(new ValueErrorTuple("0/12/32", true));
                     break;
                 case SqlDbType.Time:
                 case SqlDbType.DateTime2:

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/CertificateUtility.cs

@@ -261,7 +261,8 @@ internal static byte[] DecryptDataUsingAED(byte[] encryptedCellBlob, byte[] key,
 
         internal static SqlConnection GetOpenConnection(bool fTceEnabled, SqlConnectionStringBuilder sb, bool fSuppressAttestation = false)
         {
-            SqlConnection conn = new SqlConnection(GetConnectionString(fTceEnabled, sb, fSuppressAttestation));
+            var cs = GetConnectionString(fTceEnabled, sb, fSuppressAttestation);
+            SqlConnection conn = new SqlConnection(cs);
             try
             {
                 conn.Open();
@@ -284,8 +285,17 @@ public static string GetConnectionString(bool fTceEnabled, SqlConnectionStringBu
             SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder();
             builder.DataSource = sb.DataSource;
             builder.InitialCatalog = sb.InitialCatalog;
-            builder.UserID = sb.UserID;
-            builder.Password = sb.Password;
+            // if connection string has integrated security, respect that
+            if (!sb.IntegratedSecurity)
+            {
+                builder.UserID = sb.UserID;
+                builder.Password = sb.Password;
+                builder.IntegratedSecurity = false;
+            }
+            else
+            {
+                builder.IntegratedSecurity = true;
+            }
             if (fTceEnabled)
             {
                 builder.ColumnEncryptionSetting = SqlConnectionColumnEncryptionSetting.Enabled;

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/CertificateUtilityWin.cs

@@ -34,6 +34,7 @@ internal static void CreateCertificate(string certificateName, string certificat
             Process process = new Process();
             process.StartInfo = processStartInfo;
             process.StartInfo.UseShellExecute = true;
+            process.StartInfo.WindowStyle = ProcessWindowStyle.Hidden;
             process.StartInfo.CreateNoWindow = true;
             process.Start();
             process.WaitForExit();

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj

@@ -165,6 +165,7 @@
     <Compile Include="SQL\RandomStressTest\RandomStressTest.cs" />
     <Compile Include="SQL\SplitPacketTest\SplitPacketTest.cs" />
     <Compile Include="SQL\SqlNotificationTest\SqlNotificationTest.cs" />
+    <Compile Include="SQL\TransactionTest\TransactionInferenceTest.cs" />
     <Compile Include="SQL\TransactionTest\TransactionTest.cs" />
     <Compile Include="SQL\TransactionTest\TransactionEnlistmentTest.cs" />
     <Compile Include="SQL\UdtTest\SqlServerTypesTest.cs" />

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Transaction3.cs

@@ -41,9 +41,11 @@ public static void Test(string srcConstr, string dstConstr, string dstTable)
 
                                 SqlCommand myCmd = dstConn.CreateCommand();
                                 myCmd.CommandText = "select * from " + dstTable;
-                                myCmd.Transaction = myTrans;
-
-                                DataTestUtility.AssertThrowsWrapper<InvalidOperationException>(() => myCmd.ExecuteReader(), exceptionMessage: exceptionMsg);
+                                // ///// The following 2-lines test can no longer work
+                                // myCmd.Transaction = myTrans;
+                                // DataTestUtility.AssertThrowsWrapper<InvalidOperationException>(() => myCmd.ExecuteReader(), exceptionMessage: exceptionMsg);
+                                // ///// So it is replaced with the following as now validation is done on assignment
+                                DataTestUtility.AssertThrowsWrapper<InvalidOperationException>(() => myCmd.Transaction = myTrans, exceptionMessage: exceptionMsg);
                             }
                         }
                     }