Merge pull request #270 from ckadluba/refactor-SinkTraits

Refactor sink traits

Author: ckadluba

sample/AppConfigDemo/Program.cs

@@ -2,7 +2,6 @@
 using System.Threading;
 using Serilog;
 using Serilog.Events;
-using Serilog.Sinks.MSSqlServer;
 using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Options;
 
 namespace AppConfigDemo

src/Serilog.Sinks.MSSqlServer/GlobalSuppressions.cs

@@ -7,12 +7,10 @@
 
 [assembly: SuppressMessage("Globalization", "CA1303:Do not pass literals as localized parameters", Justification = "Supplying string literals and not using resources is accepted within this project.", Scope = "namespaceanddescendants", Target = "Serilog.Sinks.MSSqlServer")]
 [assembly: SuppressMessage("Security", "CA2100:Review SQL queries for security vulnerabilities", Justification = "Too hard to change. Accepted for now.", Scope = "namespaceanddescendants", Target = "Serilog.Sinks.MSSqlServer")]
-[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.MSSqlServerSinkTraits.TryChangeType(System.Object,System.Type,System.Object@)~System.Boolean")]
-[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.MSSqlServerSink.EmitBatchAsync(System.Collections.Generic.IEnumerable{Serilog.Events.LogEvent})~System.Threading.Tasks.Task")]
-[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.MSSqlServerSinkTraits.ConvertPropertiesToXmlStructure(System.Collections.Generic.IEnumerable{System.Collections.Generic.KeyValuePair{System.String,Serilog.Events.LogEventPropertyValue}})~System.String")]
-[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.MSSqlServerSinkTraits.#ctor(System.String,System.String,Serilog.Sinks.MSSqlServer.ColumnOptions,System.IFormatProvider,System.Boolean,Serilog.Formatting.ITextFormatter,Serilog.Sinks.MSSqlServer.Platform.ISqlTableCreator)")]
-[assembly: SuppressMessage("Design", "CA1062:Validate arguments of public methods", Justification = "Serilog core guarantees to call Emit() with non-null logEvent parameter.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.MSSqlServerAuditSink.Emit(Serilog.Events.LogEvent)")]
-[assembly: SuppressMessage("Design", "CA1062:Validate arguments of public methods", Justification = "Serilog core guarantees to call EmitBatchAsync() with non-null logEvent parameter.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.MSSqlServerSink.EmitBatchAsync(System.Collections.Generic.IEnumerable{Serilog.Events.LogEvent})~System.Threading.Tasks.Task")]
+[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.Platform.SqlTableCreator.CreateTable(System.String,System.String,System.Data.DataTable,Serilog.Sinks.MSSqlServer.ColumnOptions)")]
+[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Platform.SqlBulkBatchWriter.WriteBatch(System.Collections.Generic.IEnumerable{Serilog.Events.LogEvent},System.Data.DataTable)~System.Threading.Tasks.Task")]
+[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.Output.StandardColumnDataGenerator.ConvertPropertiesToXmlStructure(System.Collections.Generic.IEnumerable{System.Collections.Generic.KeyValuePair{System.String,Serilog.Events.LogEventPropertyValue}})~System.String")]
+[assembly: SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Too hard to change. Accepted for now.", Scope = "member", Target = "~M:Serilog.Sinks.MSSqlServer.Output.PropertiesColumnDataGenerator.TryChangeType(System.Object,System.Type,System.Object@)~System.Boolean")]
 [assembly: SuppressMessage("Design", "CA1034:Nested types should not be visible", Justification = "Cannot be changed on public classes for backward compatibility reasons.", Scope = "namespaceanddescendants", Target = "Serilog.Sinks.MSSqlServer")]
 [assembly: SuppressMessage("Usage", "CA2227:Collection properties should be read only", Justification = "Cannot be changed on public classes for backward compatibility reasons.", Scope = "type", Target = "~T:Serilog.Sinks.MSSqlServer.ColumnOptions")]
 [assembly: SuppressMessage("Design", "CA1010:Collections should implement generic interface", Justification = "Cannot be changed on public classes for backward compatibility reasons.", Scope = "namespaceanddescendants", Target = "Serilog.Sinks.MSSqlServer")]

src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Dependencies/SinkDependencies.cs

@@ -0,0 +1,13 @@
+using Serilog.Sinks.MSSqlServer.Platform;
+using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Platform;
+
+namespace Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Dependencies
+{
+    internal class SinkDependencies
+    {
+        public IDataTableCreator DataTableCreator { get; set; }
+        public ISqlTableCreator SqlTableCreator { get; set; }
+        public ISqlBulkBatchWriter SqlBulkBatchWriter { get; set; }
+        public ISqlLogEventWriter SqlLogEventWriter { get; set; }
+    }
+}

src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Dependencies/SinkDependenciesFactory.cs

@@ -0,0 +1,47 @@
+using System;
+using Serilog.Formatting;
+using Serilog.Sinks.MSSqlServer.Output;
+using Serilog.Sinks.MSSqlServer.Platform;
+using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Options;
+using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Platform;
+
+namespace Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Dependencies
+{
+    internal static class SinkDependenciesFactory
+    {
+        internal static SinkDependencies Create(
+            string connectionString,
+            SinkOptions sinkOptions,
+            IFormatProvider formatProvider,
+            ColumnOptions columnOptions,
+            ITextFormatter logEventFormatter)
+        {
+            columnOptions = columnOptions ?? new ColumnOptions();
+            columnOptions.FinalizeConfigurationForSinkConstructor();
+
+            var sqlConnectionFactory =
+                new SqlConnectionFactory(connectionString,
+                    new AzureManagedServiceAuthenticator(
+                        sinkOptions?.UseAzureManagedIdentity ?? default,
+                        sinkOptions.AzureServiceTokenProviderResource));
+            var logEventDataGenerator =
+                new LogEventDataGenerator(columnOptions,
+                    new StandardColumnDataGenerator(columnOptions, formatProvider, logEventFormatter),
+                    new PropertiesColumnDataGenerator(columnOptions));
+
+            var sinkDependencies = new SinkDependencies
+            {
+                SqlTableCreator = new SqlTableCreator(new SqlCreateTableWriter(), sqlConnectionFactory),
+                DataTableCreator = new DataTableCreator(),
+                SqlBulkBatchWriter = new SqlBulkBatchWriter(
+                    sinkOptions.TableName, sinkOptions.SchemaName,
+                    columnOptions.DisableTriggers, sqlConnectionFactory, logEventDataGenerator),
+                SqlLogEventWriter = new SqlLogEventWriter(
+                    sinkOptions.TableName, sinkOptions.SchemaName,
+                    sqlConnectionFactory, logEventDataGenerator)
+            };
+
+            return sinkDependencies;
+        }
+    }
+}

src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs

@@ -13,13 +13,10 @@
 // limitations under the License.
 
 using System;
-using System.Data;
-using System.Data.SqlClient;
-using System.Text;
 using Serilog.Core;
-using Serilog.Debugging;
 using Serilog.Events;
 using Serilog.Formatting;
+using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Dependencies;
 using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Options;
 using Serilog.Sinks.MSSqlServer.Sinks.MSSqlServer.Platform;
 
@@ -31,8 +28,7 @@ namespace Serilog.Sinks.MSSqlServer
     /// </summary>
     public class MSSqlServerAuditSink : ILogEventSink, IDisposable
     {
-        private readonly ISqlConnectionFactory _sqlConnectionFactory;
-        private readonly MSSqlServerSinkTraits _traits;
+        private readonly ISqlLogEventWriter _sqlLogEventWriter;
 
         /// <summary>
         /// Construct a sink posting to the specified database.
@@ -76,99 +72,60 @@ public MSSqlServerAuditSink(
             IFormatProvider formatProvider = null,
             ColumnOptions columnOptions = null,
             ITextFormatter logEventFormatter = null)
+            : this(sinkOptions, columnOptions,
+                  SinkDependenciesFactory.Create(connectionString, sinkOptions, formatProvider, columnOptions, logEventFormatter))
+        {
+        }
+
+        // Internal constructor with injectable dependencies for better testability
+        internal MSSqlServerAuditSink(
+            SinkOptions sinkOptions,
+            ColumnOptions columnOptions,
+            SinkDependencies sinkDependencies)
         {
             if (sinkOptions?.TableName == null)
             {
                 throw new InvalidOperationException("Table name must be specified!");
             }
 
-            columnOptions?.FinalizeConfigurationForSinkConstructor();
+            columnOptions = columnOptions ?? new ColumnOptions();
+            columnOptions.FinalizeConfigurationForSinkConstructor();
+            if (columnOptions.DisableTriggers)
+                throw new NotSupportedException($"The {nameof(ColumnOptions.DisableTriggers)} option is not supported for auditing.");
 
-            if (columnOptions != null)
+            if (sinkDependencies == null)
             {
-                if (columnOptions.DisableTriggers)
-                    throw new NotSupportedException($"The {nameof(ColumnOptions.DisableTriggers)} option is not supported for auditing.");
+                throw new ArgumentNullException(nameof(sinkDependencies));
             }
+            _sqlLogEventWriter = sinkDependencies?.SqlLogEventWriter ?? throw new InvalidOperationException($"SqlLogEventWriter is not initialized!");
 
-            var azureManagedServiceAuthenticator = new AzureManagedServiceAuthenticator(sinkOptions.UseAzureManagedIdentity,
-                sinkOptions.AzureServiceTokenProviderResource);
-            _sqlConnectionFactory = new SqlConnectionFactory(connectionString, azureManagedServiceAuthenticator);
-
-            _traits = new MSSqlServerSinkTraits(_sqlConnectionFactory, sinkOptions.TableName, sinkOptions.SchemaName,
-                columnOptions, formatProvider, sinkOptions.AutoCreateSqlTable, logEventFormatter);
-        }
-
-        /// <summary>Emit the provided log event to the sink.</summary>
-        /// <param name="logEvent">The log event to write.</param>
-        public void Emit(LogEvent logEvent)
-        {
-            try
+            if (sinkOptions.AutoCreateSqlTable)
             {
-                using (var connection = _sqlConnectionFactory.Create())
+                if (sinkDependencies?.DataTableCreator == null)
                 {
-                    connection.Open();
-                    using (SqlCommand command = connection.CreateCommand())
-                    {
-                        command.CommandType = CommandType.Text;
-
-                        var fieldList = new StringBuilder($"INSERT INTO [{_traits.SchemaName}].[{_traits.TableName}] (");
-                        var parameterList = new StringBuilder(") VALUES (");
-
-                        var index = 0;
-                        foreach (var field in _traits.GetColumnsAndValues(logEvent))
-                        {
-                            if (index != 0)
-                            {
-                                fieldList.Append(',');
-                                parameterList.Append(',');
-                            }
-
-                            fieldList.Append(field.Key);
-                            parameterList.Append("@P");
-                            parameterList.Append(index);
-
-                            var parameter = new SqlParameter($"@P{index}", field.Value ?? DBNull.Value);
-
-                            // The default is SqlDbType.DateTime, which will truncate the DateTime value if the actual
-                            // type in the database table is datetime2. So we explicitly set it to DateTime2, which will
-                            // work both if the field in the table is datetime and datetime2, which is also consistent with 
-                            // the behavior of the non-audit sink.
-                            if (field.Value is DateTime)
-                                parameter.SqlDbType = SqlDbType.DateTime2;
-
-                            command.Parameters.Add(parameter);
-
-                            index++;
-                        }
-
-                        parameterList.Append(')');
-                        fieldList.Append(parameterList.ToString());
-
-                        command.CommandText = fieldList.ToString();
+                    throw new InvalidOperationException($"DataTableCreator is not initialized!");
+                }
 
-                        command.ExecuteNonQuery();
-                    }
+                using (var eventTable = sinkDependencies.DataTableCreator.CreateDataTable(sinkOptions.TableName, columnOptions))
+                {
+                    sinkDependencies.SqlTableCreator.CreateTable(sinkOptions.SchemaName, sinkOptions.TableName, eventTable, columnOptions);
                 }
             }
-            catch (Exception ex)
-            {
-                SelfLog.WriteLine("Unable to write log event to the database due to following error: {1}", ex.Message);
-                throw;
-            }
         }
 
+        /// <summary>Emit the provided log event to the sink.</summary>
+        /// <param name="logEvent">The log event to write.</param>
+        public void Emit(LogEvent logEvent) =>
+            _sqlLogEventWriter.WriteEvent(logEvent);
+
         /// <summary>
         /// Releases the unmanaged resources used by the Serilog.Sinks.MSSqlServer.MSSqlServerAuditSink and optionally
         /// releases the managed resources.
         /// </summary>
-        /// <param name="disposing">True to release both managed and unmanaged resources; false to release only unmanaged
-        ///                         resources.</param>
+        /// <param name="disposing">True to release both managed and unmanaged resources; false to release only unmanaged resources.</param>
         protected virtual void Dispose(bool disposing)
         {
-            if (disposing)
-            {
-                _traits.Dispose();
-            }
+            // This class needn't to dispose anything. This is just here for sink interface compatibility.
         }
 
         /// <summary>