Don't cache query type (#1015)

* Don't cache query type

* Use tableinfo

Author: Charles-Gagnon

src/SqlAsyncCollector.cs

@@ -202,7 +202,7 @@ private async Task UpsertRowsAsync(IList<T> rows, SqlAttribute attribute, IConfi
                 {
                     TelemetryInstance.TrackEvent(TelemetryEventName.TableInfoCacheMiss, props);
                     // set the columnNames for supporting T as JObject since it doesn't have columns in the member info.
-                    tableInfo = TableInformation.RetrieveTableInformation(connection, fullTableName, this._logger, GetColumnNamesFromItem(rows.First()), this._serverProperties);
+                    tableInfo = TableInformation.RetrieveTableInformation(connection, fullTableName, this._logger, this._serverProperties);
                     var policy = new CacheItemPolicy
                     {
                         // Re-look up the primary key(s) after timeout (default timeout is 10 minutes)
@@ -244,7 +244,24 @@ private async Task UpsertRowsAsync(IList<T> rows, SqlAttribute attribute, IConfi
                 }
 
                 var table = new SqlObject(fullTableName);
-                string mergeOrInsertQuery = tableInfo.QueryType == QueryType.Insert ? TableInformation.GetInsertQuery(table, bracketedColumnNamesFromItem) :
+
+                IEnumerable<string> objectColumnNames = GetColumnNamesFromItem(rows.First());
+                IEnumerable<string> primaryKeysFromObject = objectColumnNames.Where(f => tableInfo.PrimaryKeys.Any(k => string.Equals(k.Name, f, StringComparison.Ordinal)));
+                IEnumerable<PrimaryKey> missingPrimaryKeysFromItem = tableInfo.PrimaryKeys
+                    .Where(k => !primaryKeysFromObject.Contains(k.Name));
+                // If none of the primary keys are an identity column or have a default value then we require that all primary keys be present in the POCO so we can
+                // generate the MERGE statement correctly
+                if (!tableInfo.HasIdentityColumnPrimaryKeys && !tableInfo.HasDefaultColumnPrimaryKeys && missingPrimaryKeysFromItem.Any())
+                {
+                    string message = $"All primary keys for SQL table {table} need to be found in '{typeof(T)}.' Missing primary keys: [{string.Join(",", missingPrimaryKeysFromItem)}]";
+                    var ex = new InvalidOperationException(message);
+                    TelemetryInstance.TrackException(TelemetryErrorName.MissingPrimaryKeys, ex, connection.AsConnectionProps(this._serverProperties));
+                    throw ex;
+                }
+                // If any identity columns or columns with default values aren't included in the object then we have to generate a basic insert since the merge statement expects all primary key
+                // columns to exist. (the merge statement can handle nullable columns though if those exist)
+                QueryType queryType = (tableInfo.HasIdentityColumnPrimaryKeys || tableInfo.HasDefaultColumnPrimaryKeys) && missingPrimaryKeysFromItem.Any() ? QueryType.Insert : QueryType.Merge;
+                string mergeOrInsertQuery = queryType == QueryType.Insert ? TableInformation.GetInsertQuery(table, bracketedColumnNamesFromItem) :
                     TableInformation.GetMergeQuery(tableInfo.PrimaryKeys, table, bracketedColumnNamesFromItem);
 
                 var transactionSw = Stopwatch.StartNew();
@@ -415,28 +432,28 @@ public class TableInformation
             public IEnumerable<string> ColumnDefinitions => this.Columns.Select(c => $"{c.Key} {c.Value}");
 
             /// <summary>
-            /// Whether to use an insert query or merge query.
+            /// Whether at least one of the primary keys on this table is an identity column
             /// </summary>
-            public QueryType QueryType { get; }
+            public bool HasIdentityColumnPrimaryKeys { get; }
 
             /// <summary>
-            /// Whether at least one of the primary keys on this table is an identity column
+            /// Whether at least one of the primary keys on this table has a default value
             /// </summary>
-            public bool HasIdentityColumnPrimaryKeys { get; }
+            public bool HasDefaultColumnPrimaryKeys { get; }
+
             /// <summary>
             /// Settings to use when serializing the POCO into SQL.
             /// Only serialize properties and fields that correspond to SQL columns.
             /// </summary>
             public JsonSerializerSettings JsonSerializerSettings { get; }
 
-            public TableInformation(List<PrimaryKey> primaryKeys, IEnumerable<PropertyInfo> primaryKeyProperties, IDictionary<string, string> columns, QueryType queryType, bool hasIdentityColumnPrimaryKeys)
+            public TableInformation(List<PrimaryKey> primaryKeys, IEnumerable<PropertyInfo> primaryKeyProperties, IDictionary<string, string> columns, bool hasIdentityColumnPrimaryKeys, bool hasDefaultColumnPrimaryKeys)
             {
                 this.PrimaryKeys = primaryKeys;
                 this.PrimaryKeyProperties = primaryKeyProperties;
                 this.Columns = columns;
-                this.QueryType = queryType;
                 this.HasIdentityColumnPrimaryKeys = hasIdentityColumnPrimaryKeys;
-
+                this.HasDefaultColumnPrimaryKeys = hasDefaultColumnPrimaryKeys;
                 // Convert datetime strings to ISO 8061 format to avoid potential errors on the server when converting into a datetime. This
                 // is the only format that are an international standard.
                 // https://docs.microsoft.com/previous-versions/sql/sql-server-2008-r2/ms180878(v=sql.105)
@@ -546,10 +563,9 @@ WHEN NOT MATCHED THEN
             /// <param name="sqlConnection">An open connection with which to query SQL against</param>
             /// <param name="fullName">Full name of table, including schema (if exists).</param>
             /// <param name="logger">ILogger used to log any errors or warnings.</param>
-            /// <param name="objectColumnNames">Column names from the object</param>
             /// <param name="serverProperties">EngineEdition and Edition of the target Sql Server.</param>
             /// <returns>TableInformation object containing primary keys, column types, etc.</returns>
-            public static TableInformation RetrieveTableInformation(SqlConnection sqlConnection, string fullName, ILogger logger, IEnumerable<string> objectColumnNames, ServerProperties serverProperties)
+            public static TableInformation RetrieveTableInformation(SqlConnection sqlConnection, string fullName, ILogger logger, ServerProperties serverProperties)
             {
                 Dictionary<TelemetryPropertyName, string> sqlConnProps = sqlConnection.AsConnectionProps(serverProperties);
                 var table = new SqlObject(fullName);
@@ -627,36 +643,20 @@ public static TableInformation RetrieveTableInformation(SqlConnection sqlConnect
 
                 // Match SQL Primary Key column names to POCO property objects. Ensure none are missing.
                 IEnumerable<PropertyInfo> primaryKeyProperties = typeof(T).GetProperties().Where(f => primaryKeys.Any(k => string.Equals(k.Name, f.Name, StringComparison.Ordinal)));
-                IEnumerable<string> primaryKeysFromObject = objectColumnNames.Where(f => primaryKeys.Any(k => string.Equals(k.Name, f, StringComparison.Ordinal)));
-                IEnumerable<PrimaryKey> missingPrimaryKeysFromItem = primaryKeys
-                    .Where(k => !primaryKeysFromObject.Contains(k.Name));
                 bool hasIdentityColumnPrimaryKeys = primaryKeys.Any(k => k.IsIdentity);
                 bool hasDefaultColumnPrimaryKeys = primaryKeys.Any(k => k.HasDefault);
-                // If none of the primary keys are an identity column or have a default value then we require that all primary keys be present in the POCO so we can
-                // generate the MERGE statement correctly
-                if (!hasIdentityColumnPrimaryKeys && !hasDefaultColumnPrimaryKeys && missingPrimaryKeysFromItem.Any())
-                {
-                    string message = $"All primary keys for SQL table {table} need to be found in '{typeof(T)}.' Missing primary keys: [{string.Join(",", missingPrimaryKeysFromItem)}]";
-                    var ex = new InvalidOperationException(message);
-                    TelemetryInstance.TrackException(TelemetryErrorName.MissingPrimaryKeys, ex, sqlConnProps);
-                    throw ex;
-                }
-
-                // If any identity columns or columns with default values aren't included in the object then we have to generate a basic insert since the merge statement expects all primary key
-                // columns to exist. (the merge statement can handle nullable columns though if those exist)
-                QueryType queryType = (hasIdentityColumnPrimaryKeys || hasDefaultColumnPrimaryKeys) && missingPrimaryKeysFromItem.Any() ? QueryType.Insert : QueryType.Merge;
 
                 tableInfoSw.Stop();
                 var durations = new Dictionary<TelemetryMeasureName, double>()
                 {
                     { TelemetryMeasureName.GetColumnDefinitionsDurationMs, columnDefinitionsSw.ElapsedMilliseconds },
                     { TelemetryMeasureName.GetPrimaryKeysDurationMs, primaryKeysSw.ElapsedMilliseconds }
                 };
-                sqlConnProps.Add(TelemetryPropertyName.QueryType, queryType.ToString());
-                sqlConnProps.Add(TelemetryPropertyName.HasIdentityColumn, hasIdentityColumnPrimaryKeys.ToString());
+                sqlConnProps.Add(TelemetryPropertyName.HasIdentityColumnPrimaryKeys, hasIdentityColumnPrimaryKeys.ToString());
+                sqlConnProps.Add(TelemetryPropertyName.HasDefaultColumnPrimaryKeys, hasDefaultColumnPrimaryKeys.ToString());
                 TelemetryInstance.TrackDuration(TelemetryEventName.GetTableInfo, tableInfoSw.ElapsedMilliseconds, sqlConnProps, durations);
-                logger.LogDebug($"RetrieveTableInformation DB and Table: {sqlConnection.Database}.{fullName}. Primary keys: [{string.Join(",", primaryKeys.Select(pk => pk.Name))}].\nSQL Column and Definitions:  [{string.Join(",", columnDefinitionsFromSQL)}]\nObject columns: [{string.Join(",", objectColumnNames)}]");
-                return new TableInformation(primaryKeys, primaryKeyProperties, columnDefinitionsFromSQL, queryType, hasIdentityColumnPrimaryKeys);
+                logger.LogDebug($"RetrieveTableInformation DB and Table: {sqlConnection.Database}.{fullName}. Primary keys: [{string.Join(",", primaryKeys.Select(pk => pk.Name))}].\nSQL Column and Definitions:  [{string.Join(",", columnDefinitionsFromSQL)}]");
+                return new TableInformation(primaryKeys, primaryKeyProperties, columnDefinitionsFromSQL, hasIdentityColumnPrimaryKeys, hasDefaultColumnPrimaryKeys);
             }
         }
 

src/Telemetry/Telemetry.cs

@@ -358,7 +358,8 @@ public enum TelemetryPropertyName
     {
         ErrorCode,
         ErrorName,
-        HasIdentityColumn,
+        HasIdentityColumnPrimaryKeys,
+        HasDefaultColumnPrimaryKeys,
         HasConfiguredMaxBatchSize,
         HasConfiguredMaxChangesPerWorker,
         HasConfiguredPollingInterval,

test/Integration/SqlOutputBindingIntegrationTests.cs

@@ -354,6 +354,32 @@ public void AddProductWithDefaultPKTest(SupportedLanguages lang)
             Assert.Equal(2, this.ExecuteScalar("SELECT COUNT(*) FROM dbo.ProductsWithDefaultPK"));
         }
 
+        /// <summary>
+        /// Regression test for ensuring that the query type isn't cached
+        /// </summary>
+        [Theory]
+        [SqlInlineData()]
+        public void QueryTypeCachingRegressionTest(SupportedLanguages lang)
+        {
+            // Start off by inserting an item into the database, which we'll update later
+            this.ExecuteNonQuery("INSERT INTO Products VALUES (1, 'test', 100)");
+            // Now make a call that is expected to fail. The important part here is that:
+            //      1. This and the function below both target the same table (dbo.Products)
+            //      2. This one will trigger an "insert" query (which ultimately fails due to the incorrect casing, but the table information is still retrieved first)
+            Assert.Throws<AggregateException>(() => this.SendOutputGetRequest("addproduct-incorrectcasing", null, TestUtils.GetPort(lang, true)).Wait());
+            // Ensure that we have the one expected item
+            Assert.True(1 == (int)this.ExecuteScalar("SELECT COUNT(*) FROM dbo.Products"), "There should be one item initially");
+            var productWithPrimaryKey = new Dictionary<string, object>()
+            {
+                { "ProductId", 1 },
+                { "Name", "MyNewProduct" },
+                { "Cost", 100 }
+            };
+            // Now send an output request that we expect to succeed - specifically one that will result in an update so requires the MERGE statement
+            this.SendOutputPostRequest("addproduct", Utils.JsonSerializeObject(productWithPrimaryKey), TestUtils.GetPort(lang)).Wait();
+            Assert.True(1 == (int)this.ExecuteScalar("SELECT COUNT(*) FROM dbo.Products"), "There should be one item at the end");
+        }
+
         /// <summary>
         /// Tests that when using an unsupported database the expected error is thrown
         /// </summary>