From 54a6911922bc679f18984f2468b4a6ec26177aec Mon Sep 17 00:00:00 2001 From: Christian Kadluba <10721825+ckadluba@users.noreply.github.com> Date: Sat, 15 Mar 2025 20:45:02 +0100 Subject: [PATCH 1/5] Updated copyright xmldoc headers * Updated copyright year where applicable * Removed copyright header from internal classes --- ...LoggerConfigurationMSSqlServerExtensions.cs | 2 +- ...LoggerConfigurationMSSqlServerExtensions.cs | 2 +- .../Sinks/MSSqlServer/MSSqlServerAuditSink.cs | 2 +- .../Sinks/MSSqlServer/MSSqlServerSink.cs | 2 +- .../Output/IXmlPropertyFormatter.cs | 18 ++---------------- .../Output/JsonLogEventFormatter.cs | 14 -------------- 6 files changed, 6 insertions(+), 34 deletions(-) diff --git a/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Hybrid/LoggerConfigurationMSSqlServerExtensions.cs b/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Hybrid/LoggerConfigurationMSSqlServerExtensions.cs index eefb88af..10cb7531 100644 --- a/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Hybrid/LoggerConfigurationMSSqlServerExtensions.cs +++ b/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Hybrid/LoggerConfigurationMSSqlServerExtensions.cs @@ -1,4 +1,4 @@ -// Copyright 2024 Serilog Contributors +// Copyright 2025 Serilog Contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Microsoft.Extensions.Configuration/LoggerConfigurationMSSqlServerExtensions.cs b/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Microsoft.Extensions.Configuration/LoggerConfigurationMSSqlServerExtensions.cs index 0084b132..153137fa 100644 --- a/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Microsoft.Extensions.Configuration/LoggerConfigurationMSSqlServerExtensions.cs +++ b/src/Serilog.Sinks.MSSqlServer/Configuration/Extensions/Microsoft.Extensions.Configuration/LoggerConfigurationMSSqlServerExtensions.cs @@ -1,4 +1,4 @@ -// Copyright 2024 Serilog Contributors +// Copyright 2025 Serilog Contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs index 035bad25..bd738e04 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs @@ -1,4 +1,4 @@ -// Copyright 2024 Serilog Contributors +// Copyright 2025 Serilog Contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs index d81f674a..b8ac7701 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs @@ -1,4 +1,4 @@ -// Copyright 2024 Serilog Contributors +// Copyright 2025 Serilog Contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/IXmlPropertyFormatter.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/IXmlPropertyFormatter.cs index ee0a7a1d..87de7e4c 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/IXmlPropertyFormatter.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/IXmlPropertyFormatter.cs @@ -1,18 +1,4 @@ -// Copyright 2024 Serilog Contributors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -using Serilog.Events; +using Serilog.Events; namespace Serilog.Sinks.MSSqlServer.Output { @@ -21,4 +7,4 @@ internal interface IXmlPropertyFormatter string GetValidElementName(string name); string Simplify(LogEventPropertyValue value, ColumnOptions.PropertiesColumnOptions options); } -} \ No newline at end of file +} diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/JsonLogEventFormatter.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/JsonLogEventFormatter.cs index 3b475eb4..af64aa6c 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/JsonLogEventFormatter.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/JsonLogEventFormatter.cs @@ -1,17 +1,3 @@ -// Copyright 2024 Serilog Contributors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - using System; using System.Collections.Generic; using System.Data; From 2b3e2fcc4e9a51e8dc1b8773390f3a9f2dadebea Mon Sep 17 00:00:00 2001 From: Christian Kadluba <10721825+ckadluba@users.noreply.github.com> Date: Sat, 15 Mar 2025 20:49:53 +0100 Subject: [PATCH 2/5] Fixed solution It showed perf test project in the samples folder for some reason. --- serilog-sinks-mssqlserver.sln | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serilog-sinks-mssqlserver.sln b/serilog-sinks-mssqlserver.sln index 9768fe57..1f59581d 100644 --- a/serilog-sinks-mssqlserver.sln +++ b/serilog-sinks-mssqlserver.sln @@ -28,13 +28,13 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Directory.Build.props = Directory.Build.props Directory.Packages.props = Directory.Packages.props .github\ISSUE_TEMPLATE.md = .github\ISSUE_TEMPLATE.md + .github\workflows\perftests.yml = .github\workflows\perftests.yml .github\workflows\pr-analysis-codeql.yml = .github\workflows\pr-analysis-codeql.yml .github\workflows\pr-analysis-devskim.yml = .github\workflows\pr-analysis-devskim.yml .github\workflows\pr-validation.yml = .github\workflows\pr-validation.yml README.md = README.md .github\workflows\release.yml = .github\workflows\release.yml RunPerfTests.ps1 = RunPerfTests.ps1 - .github\workflows\perftests.yml = .github\workflows\perftests.yml EndProjectSection EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NetStandardDemoLib", "sample\NetStandardDemo\NetStandardDemoLib\NetStandardDemoLib.csproj", "{8E69E31B-61C7-4175-B886-9C2078FCA477}" From 5cbc40f66bbd4453015a17a898fb41d1d3fc5196 Mon Sep 17 00:00:00 2001 From: Christian Kadluba <10721825+ckadluba@users.noreply.github.com> Date: Sat, 15 Mar 2025 22:05:33 +0100 Subject: [PATCH 3/5] Fixed concurrency issue #607 Do not reuse SqlCommand because apparently it is not threadsafe. Set connection on command leads to a exception in audit sink under heavy load. --- .../Sinks/MSSqlServer/MSSqlServerAuditSink.cs | 31 +----- .../Sinks/MSSqlServer/MSSqlServerSink.cs | 1 - .../Platform/ISqlCommandFactory.cs | 1 - .../Platform/ISqlLogEventWriter.cs | 5 +- .../Platform/SqlBulkBatchWriter.cs | 8 +- .../Platform/SqlClient/ISqlCommandWrapper.cs | 7 -- .../Platform/SqlClient/SqlCommandWrapper.cs | 25 +---- .../MSSqlServer/Platform/SqlCommandFactory.cs | 10 +- .../Platform/SqlInsertStatementWriter.cs | 99 +++++++------------ .../SqlInsertStatementWriterBenchmarks.cs | 10 +- .../MSSqlServer/MSSqlServerAuditSinkTests.cs | 28 +----- .../Sinks/MSSqlServer/MSSqlServerSinkTests.cs | 10 -- .../SqlClient/SqlCommandWrapperTests.cs | 72 +------------- .../Platform/SqlInsertStatementWriterTests.cs | 45 ++------- 14 files changed, 60 insertions(+), 292 deletions(-) diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs index bd738e04..0a793fd3 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs @@ -26,12 +26,10 @@ namespace Serilog.Sinks.MSSqlServer /// Writes log events as rows in a table of MSSqlServer database using Audit logic, meaning that each row is synchronously committed /// and any errors that occur are propagated to the caller. /// - public class MSSqlServerAuditSink : ILogEventSink, IDisposable + public class MSSqlServerAuditSink : ILogEventSink { private readonly ISqlLogEventWriter _sqlLogEventWriter; - private bool _disposedValue; - /// /// Construct a sink posting to the specified database. /// @@ -99,33 +97,6 @@ internal MSSqlServerAuditSink( public void Emit(LogEvent logEvent) => _sqlLogEventWriter.WriteEvent(logEvent); - /// - /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources. - /// - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - /// - /// Releases the unmanaged resources used by the Serilog.Sinks.MSSqlServer.MSSqlServerAuditSink and optionally - /// releases the managed resources. - /// - /// True to release both managed and unmanaged resources; false to release only unmanaged resources. - protected virtual void Dispose(bool disposing) - { - if (!_disposedValue) - { - if (disposing) - { - _sqlLogEventWriter.Dispose(); - } - - _disposedValue = true; - } - } - private static void ValidateParameters(MSSqlServerSinkOptions sinkOptions, ColumnOptions columnOptions) { if (sinkOptions?.TableName == null) diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs index b8ac7701..dc6f58d2 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs @@ -152,7 +152,6 @@ protected virtual void Dispose(bool disposing) if (disposing) { _sqlBulkBatchWriter.Dispose(); - _sqlLogEventWriter.Dispose(); } _disposedValue = true; diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlCommandFactory.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlCommandFactory.cs index 9fe3b891..9e9d8bf8 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlCommandFactory.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlCommandFactory.cs @@ -4,7 +4,6 @@ namespace Serilog.Sinks.MSSqlServer.Platform { internal interface ISqlCommandFactory { - ISqlCommandWrapper CreateCommand(ISqlConnectionWrapper sqlConnection); ISqlCommandWrapper CreateCommand(string cmdText, ISqlConnectionWrapper sqlConnection); } } diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlLogEventWriter.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlLogEventWriter.cs index 675dff0e..1da43251 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlLogEventWriter.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/ISqlLogEventWriter.cs @@ -1,11 +1,10 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; using System.Threading.Tasks; using Serilog.Events; namespace Serilog.Sinks.MSSqlServer.Platform { - internal interface ISqlLogEventWriter : IDisposable + internal interface ISqlLogEventWriter { void WriteEvent(LogEvent logEvent); diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlBulkBatchWriter.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlBulkBatchWriter.cs index 7b799dd3..8cd597d0 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlBulkBatchWriter.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlBulkBatchWriter.cs @@ -1,9 +1,7 @@ using System; using System.Collections.Generic; using System.Data; -using System.Linq; using System.Threading.Tasks; -using Serilog.Debugging; using Serilog.Events; using Serilog.Sinks.MSSqlServer.Output; @@ -45,10 +43,10 @@ public async Task WriteBatch(IEnumerable events) { FillDataTable(events); - using (var cn = _sqlConnectionFactory.Create()) + using (var sqlConnection = _sqlConnectionFactory.Create()) { - await cn.OpenAsync().ConfigureAwait(false); - using (var copy = cn.CreateSqlBulkCopy(_disableTriggers, _schemaAndTableName)) + await sqlConnection.OpenAsync().ConfigureAwait(false); + using (var copy = sqlConnection.CreateSqlBulkCopy(_disableTriggers, _schemaAndTableName)) { for (var i = 0; i < _dataTable.Columns.Count; i++) { diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/ISqlCommandWrapper.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/ISqlCommandWrapper.cs index 76f0896a..3e36c2f0 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/ISqlCommandWrapper.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/ISqlCommandWrapper.cs @@ -1,17 +1,10 @@ using System; -using System.Data; using System.Threading.Tasks; -using Microsoft.Data.SqlClient; namespace Serilog.Sinks.MSSqlServer.Platform.SqlClient { internal interface ISqlCommandWrapper : IDisposable { - CommandType CommandType { get; set; } - string CommandText { get; set; } - - void SetConnection(ISqlConnectionWrapper sqlConnectionWrapper); - void ClearParameters(); void AddParameter(string parameterName, object value); int ExecuteNonQuery(); Task ExecuteNonQueryAsync(); diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapper.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapper.cs index fd844d5a..e08270cb 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapper.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapper.cs @@ -10,32 +10,9 @@ internal class SqlCommandWrapper : ISqlCommandWrapper private readonly SqlCommand _sqlCommand; private bool _disposedValue; - public SqlCommandWrapper(SqlCommand sqlCommand, SqlConnection sqlConnection) + public SqlCommandWrapper(SqlCommand sqlCommand) { _sqlCommand = sqlCommand ?? throw new ArgumentNullException(nameof(sqlCommand)); - _sqlCommand.Connection = sqlConnection ?? throw new ArgumentNullException(nameof(sqlConnection)); - } - - public CommandType CommandType - { - get => _sqlCommand.CommandType; - set => _sqlCommand.CommandType = value; - } - - public string CommandText - { - get => _sqlCommand.CommandText; - set => _sqlCommand.CommandText = value; - } - - public void SetConnection(ISqlConnectionWrapper sqlConnectionWrapper) - { - _sqlCommand.Connection = sqlConnectionWrapper.SqlConnection; - } - - public void ClearParameters() - { - _sqlCommand.Parameters.Clear(); } public void AddParameter(string parameterName, object value) diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlCommandFactory.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlCommandFactory.cs index c61a567e..c6b84519 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlCommandFactory.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlCommandFactory.cs @@ -5,16 +5,10 @@ namespace Serilog.Sinks.MSSqlServer.Platform { internal class SqlCommandFactory : ISqlCommandFactory { - public ISqlCommandWrapper CreateCommand(ISqlConnectionWrapper sqlConnection) - { - var sqlCommand = new SqlCommand(); - return new SqlCommandWrapper(sqlCommand, sqlConnection.SqlConnection); - } - public ISqlCommandWrapper CreateCommand(string cmdText, ISqlConnectionWrapper sqlConnection) { - var sqlCommand = new SqlCommand(cmdText); - return new SqlCommandWrapper(sqlCommand, sqlConnection.SqlConnection); + var sqlCommand = new SqlCommand(cmdText, sqlConnection.SqlConnection); + return new SqlCommandWrapper(sqlCommand); } } } diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlInsertStatementWriter.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlInsertStatementWriter.cs index 3e030ffd..0e2c9d17 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlInsertStatementWriter.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Platform/SqlInsertStatementWriter.cs @@ -1,10 +1,8 @@ using System; using System.Collections.Generic; -using System.Data; using System.Linq; using System.Text; using System.Threading.Tasks; -using Serilog.Debugging; using Serilog.Events; using Serilog.Sinks.MSSqlServer.Output; using Serilog.Sinks.MSSqlServer.Platform.SqlClient; @@ -20,8 +18,7 @@ internal class SqlInsertStatementWriter : ISqlLogEventWriter private readonly ISqlCommandFactory _sqlCommandFactory; private readonly ILogEventDataGenerator _logEventDataGenerator; - private ISqlCommandWrapper _sqlCommand; - private bool _disposedValue; + private string _sqlCommandText; public SqlInsertStatementWriter( string tableName, @@ -43,92 +40,68 @@ public SqlInsertStatementWriter( public async Task WriteEvents(IEnumerable events) { - using (var cn = _sqlConnectionFactory.Create()) + using (var sqlConnection = _sqlConnectionFactory.Create()) { - await cn.OpenAsync().ConfigureAwait(false); + await sqlConnection.OpenAsync().ConfigureAwait(false); foreach (var logEvent in events) { var fields = _logEventDataGenerator.GetColumnsAndValues(logEvent).ToList(); - InitializeSqlCommand(cn, fields); - - var index = 0; - _sqlCommand.ClearParameters(); - foreach (var field in fields) + using (var sqlCommand = InitializeSqlCommand(sqlConnection, fields)) { - _sqlCommand.AddParameter(Invariant($"@P{index}"), field.Value); - index++; + await sqlCommand.ExecuteNonQueryAsync().ConfigureAwait(false); } - - await _sqlCommand.ExecuteNonQueryAsync().ConfigureAwait(false); } } } - /// - /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources. - /// - public void Dispose() + private ISqlCommandWrapper InitializeSqlCommand( + ISqlConnectionWrapper sqlConnection, + IEnumerable> logEventFields) { - Dispose(disposing: true); - GC.SuppressFinalize(this); - } + InitializeSqlCommandText(logEventFields); - /// - /// Releases the unmanaged resources used by the Serilog.Sinks.MSSqlServer.Platform.SqlInsertStatementWriter and optionally - /// releases the managed resources. - /// - /// True to release both managed and unmanaged resources; false to release only unmanaged resources. - protected virtual void Dispose(bool disposing) - { - if (!_disposedValue) + var sqlCommand = _sqlCommandFactory.CreateCommand(_sqlCommandText, sqlConnection); + var index = 0; + foreach (var field in logEventFields) { - if (disposing) - { - _sqlCommand?.Dispose(); - } - - _disposedValue = true; + sqlCommand.AddParameter(Invariant($"@P{index}"), field.Value); + index++; } + + return sqlCommand; } - private void InitializeSqlCommand(ISqlConnectionWrapper sqlConnection, - IEnumerable> logEventFields) + private void InitializeSqlCommandText(IEnumerable> logEventFields) { - // Optimization: generate INSERT statement and SqlCommand only once - // and reuse it with different values and SqlConnections because - // the structure does not change. - if (_sqlCommand == null) + if (_sqlCommandText != null) { - _sqlCommand = _sqlCommandFactory.CreateCommand(sqlConnection); - _sqlCommand.CommandType = CommandType.Text; + return; + } - var fieldList = new StringBuilder(Invariant($"INSERT INTO [{_schemaName}].[{_tableName}] (")); - var parameterList = new StringBuilder(") VALUES ("); + var fieldList = new StringBuilder(Invariant($"INSERT INTO [{_schemaName}].[{_tableName}] (")); + var parameterList = new StringBuilder(") VALUES ("); - var index = 0; - foreach (var field in logEventFields) + var index = 0; + foreach (var field in logEventFields) + { + if (index != 0) { - if (index != 0) - { - fieldList.Append(','); - parameterList.Append(','); - } - - fieldList.Append(Invariant($"[{field.Key}]")); - parameterList.Append("@P"); - parameterList.Append(index); - - index++; + fieldList.Append(','); + parameterList.Append(','); } - parameterList.Append(')'); - fieldList.Append(parameterList); + fieldList.Append(Invariant($"[{field.Key}]")); + parameterList.Append("@P"); + parameterList.Append(index); - _sqlCommand.CommandText = fieldList.ToString(); + index++; } - _sqlCommand.SetConnection(sqlConnection); + parameterList.Append(')'); + fieldList.Append(parameterList); + + _sqlCommandText = fieldList.ToString(); } } } diff --git a/test/Serilog.Sinks.MSSqlServer.PerformanceTests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterBenchmarks.cs b/test/Serilog.Sinks.MSSqlServer.PerformanceTests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterBenchmarks.cs index 65937374..dfc3e4d1 100644 --- a/test/Serilog.Sinks.MSSqlServer.PerformanceTests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterBenchmarks.cs +++ b/test/Serilog.Sinks.MSSqlServer.PerformanceTests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterBenchmarks.cs @@ -13,7 +13,7 @@ namespace Serilog.Sinks.MSSqlServer.PerformanceTests.Platform; [MemoryDiagnoser] [MaxIterationCount(16)] -public class SqlInsertStatementWriterBenchmarks : IDisposable +public class SqlInsertStatementWriterBenchmarks { private const string _tableName = "TestTableName"; private const string _schemaName = "TestSchemaName"; @@ -35,7 +35,7 @@ public void Setup() _sqlCommandWrapperMock = new Mock(); _sqlConnectionFactoryMock.Setup(f => f.Create()).Returns(_sqlConnectionWrapperMock.Object); - _sqlCommandFactoryMock.Setup(f => f.CreateCommand(It.IsAny())) + _sqlCommandFactoryMock.Setup(f => f.CreateCommand(It.IsAny(), It.IsAny())) .Returns(_sqlCommandWrapperMock.Object); CreateLogEvents(); @@ -67,10 +67,4 @@ private void CreateLogEvents() _logEvents.Add(CreateLogEvent()); } } - - public void Dispose() - { - GC.SuppressFinalize(this); - _sut.Dispose(); - } } diff --git a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs index 33389ad4..3d1a82e1 100644 --- a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs +++ b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs @@ -11,7 +11,7 @@ namespace Serilog.Sinks.MSSqlServer.Tests { [Trait(TestCategory.TraitName, TestCategory.Unit)] - public class MSSqlServerAuditSinkTests : IDisposable + public class MSSqlServerAuditSinkTests { private readonly MSSqlServerSinkOptions _sinkOptions; private readonly Serilog.Sinks.MSSqlServer.ColumnOptions _columnOptions; @@ -22,7 +22,6 @@ public class MSSqlServerAuditSinkTests : IDisposable private readonly string _tableName = "tableName"; private readonly string _schemaName = "schemaName"; private MSSqlServerAuditSink _sut; - private bool _disposedValue; public MSSqlServerAuditSinkTests() { @@ -156,36 +155,11 @@ public void EmitCallsSqlLogEventWriter() _sqlLogEventWriter.Verify(w => w.WriteEvent(logEvent), Times.Once); } - [Fact] - public void OnDisposeDisposesSqlLogEventWriterDependency() - { - // Arrange + act - using (new MSSqlServerAuditSink(_sinkOptions, _columnOptions, _sinkDependencies)) { } - - // Assert - _sqlLogEventWriter.Verify(w => w.Dispose(), Times.Once); - } - private void SetupSut(bool autoCreateSqlDatabase = false, bool autoCreateSqlTable = false) { _sinkOptions.AutoCreateSqlDatabase = autoCreateSqlDatabase; _sinkOptions.AutoCreateSqlTable = autoCreateSqlTable; _sut = new MSSqlServerAuditSink(_sinkOptions, _columnOptions, _sinkDependencies); } - - protected virtual void Dispose(bool disposing) - { - if (!_disposedValue) - { - _sut?.Dispose(); - _disposedValue = true; - } - } - - public void Dispose() - { - Dispose(disposing: true); - GC.SuppressFinalize(this); - } } } diff --git a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerSinkTests.cs b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerSinkTests.cs index f6a6b018..cfe49733 100644 --- a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerSinkTests.cs +++ b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerSinkTests.cs @@ -194,16 +194,6 @@ public void OnDisposeDisposesSqlBulkBatchWriterDependency() _sqlBulkBatchWriter.Verify(w => w.Dispose(), Times.Once); } - [Fact] - public void OnDisposeDisposesSqlLogEventWriterDependency() - { - // Arrange + act - using (new MSSqlServerSink(_sinkOptions, _sinkDependencies)) { } - - // Assert - _sqlLogEventWriter.Verify(w => w.Dispose(), Times.Once); - } - private void SetupSut( bool autoCreateSqlDatabase = false, bool autoCreateSqlTable = false, diff --git a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapperTests.cs b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapperTests.cs index ebd57f59..e35501ef 100644 --- a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapperTests.cs +++ b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlClient/SqlCommandWrapperTests.cs @@ -1,6 +1,5 @@ using System; using Microsoft.Data.SqlClient; -using Moq; using Xunit; using Serilog.Sinks.MSSqlServer.Tests.TestUtils; using Serilog.Sinks.MSSqlServer.Platform.SqlClient; @@ -13,23 +12,8 @@ public class SqlCommandWrapperTests [Fact] public void InitializeThrowsIfSqlCommandIsNull() { - // Arrange - using (var sqlConnection = new SqlConnection()) - { - // Act - Assert.Throws(() => new SqlCommandWrapper(null, sqlConnection)); - } - } - - [Fact] - public void InitializeThrowsIfSqlConnectionIsNull() - { - // Arrange - using (var sqlCommand = new SqlCommand()) - { - // Act - Assert.Throws(() => new SqlCommandWrapper(sqlCommand, null)); - } + // Arrange + act + Assert.Throws(() => new SqlCommandWrapper(null)); } [Fact] @@ -38,9 +22,9 @@ public void AddParameterDoesNotThrow() // Arrange using (var sqlConnection = new SqlConnection()) { - using (var sqlCommand = new SqlCommand()) + using (var sqlCommand = new SqlCommand("SELECT * FROM Table WHERE Id = @Parameter", sqlConnection)) { - using (var sut = new SqlCommandWrapper(sqlCommand, sqlConnection)) + using (var sut = new SqlCommandWrapper(sqlCommand)) { // Act (should not throw) sut.AddParameter("Parameter", "Value"); @@ -48,53 +32,5 @@ public void AddParameterDoesNotThrow() } } } - - [Fact] - public void SetConnectionCallsSetConnectionOnSqlCommand() - { - // Arrange - using (var sqlConnection = new SqlConnection()) - { - using (var sqlCommand = new SqlCommand()) - { - using (var sut = new SqlCommandWrapper(sqlCommand, sqlConnection)) - { - using (var sqlConnection2 = new SqlConnection()) - { - var sqlConnectionWrapperMock = new Mock(); - sqlConnectionWrapperMock.SetupGet(c => c.SqlConnection).Returns(sqlConnection2); - - // Act - sut.SetConnection(sqlConnectionWrapperMock.Object); - - // Assert - Assert.Same(sqlConnection2, sqlCommand.Connection); - } - } - } - } - } - - [Fact] - public void ClearParametersCallsClearParametersOnSqlCommand() - { - // Arrange - using (var sqlConnection = new SqlConnection()) - { - using (var sqlCommand = new SqlCommand()) - { - sqlCommand.Parameters.Add(new SqlParameter()); - sqlCommand.Parameters.Add(new SqlParameter()); - using (var sut = new SqlCommandWrapper(sqlCommand, sqlConnection)) - { - // Act - sut.ClearParameters(); - - // Assert - Assert.Empty(sqlCommand.Parameters); - } - } - } - } } } diff --git a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterTests.cs b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterTests.cs index 07f6a040..ee68775a 100644 --- a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterTests.cs +++ b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Platform/SqlInsertStatementWriterTests.cs @@ -12,7 +12,7 @@ namespace Serilog.Sinks.MSSqlServer.Tests.Platform { [Trait(TestCategory.TraitName, TestCategory.Unit)] - public class SqlInsertStatementWriterTests : IDisposable + public class SqlInsertStatementWriterTests { private const string _tableName = "TestTableName"; private const string _schemaName = "TestSchemaName"; @@ -22,7 +22,6 @@ public class SqlInsertStatementWriterTests : IDisposable private readonly Mock _sqlConnectionWrapperMock; private readonly Mock _sqlCommandWrapperMock; private readonly SqlInsertStatementWriter _sut; - private bool _disposedValue; public SqlInsertStatementWriterTests() { @@ -33,7 +32,7 @@ public SqlInsertStatementWriterTests() _sqlCommandWrapperMock = new Mock(); _sqlConnectionFactoryMock.Setup(f => f.Create()).Returns(_sqlConnectionWrapperMock.Object); - _sqlCommandFactoryMock.Setup(c => c.CreateCommand(It.IsAny())) + _sqlCommandFactoryMock.Setup(c => c.CreateCommand(It.IsAny(), It.IsAny())) .Returns(_sqlCommandWrapperMock.Object); _sut = new SqlInsertStatementWriter(_tableName, _schemaName, @@ -102,7 +101,7 @@ public async Task WriteEventsCallsSqlConnectionWrapperOpenAsync() } [Fact] - public async Task WriteEventsCallsSqlConnectionWrappeCreateCommand() + public async Task WriteEventsCallsSqlConnectionWrappeCreateCommandForEachLogEvent() { // Arrange var logEvents = CreateLogEvents(); @@ -111,21 +110,8 @@ public async Task WriteEventsCallsSqlConnectionWrappeCreateCommand() await _sut.WriteEvents(logEvents); // Assert - _sqlCommandFactoryMock.Verify(c => c.CreateCommand(_sqlConnectionWrapperMock.Object), - Times.Once); - } - - [Fact] - public async Task WriteEventsSetsSqlCommandWrapperCommandTypeText() - { - // Arrange - var logEvents = CreateLogEvents(); - - // Act - await _sut.WriteEvents(logEvents); - - // Assert - _sqlCommandWrapperMock.VerifySet(c => c.CommandType = System.Data.CommandType.Text); + _sqlCommandFactoryMock.Verify(c => c.CreateCommand( + It.IsAny(), _sqlConnectionWrapperMock.Object), Times.Exactly(2)); } [Fact] @@ -155,7 +141,7 @@ public async Task WriteEventsCallsSqlCommandWrapperAddParameterForEachField() } [Fact] - public async Task WriteEventsSetsSqlCommandWrapperCommandTextToSqlInsertWithCorrectFieldsAndValues() + public async Task WriteEventsCallsSqlCommandFactoryWithCommandTextToSqlInsertWithCorrectFieldsAndValues() { // Arrange var expectedSqlCommandText = $"INSERT INTO [{_schemaName}].[{_tableName}] ([FieldName1],[FieldName2],[FieldNameThree]) VALUES (@P0,@P1,@P2)"; @@ -173,7 +159,7 @@ public async Task WriteEventsSetsSqlCommandWrapperCommandTextToSqlInsertWithCorr await _sut.WriteEvents(new[] { logEvent }); // Assert - _sqlCommandWrapperMock.VerifySet(c => c.CommandText = expectedSqlCommandText); + _sqlCommandFactoryMock.Verify(f => f.CreateCommand(expectedSqlCommandText, _sqlConnectionWrapperMock.Object)); } [Fact] @@ -218,7 +204,7 @@ public async Task WriteEventsRethrowsIfSqlConnectionFactoryCreateThrows() public async Task WriteEventsRethrowsIfCreateCommandThrows() { // Arrange - _sqlCommandFactoryMock.Setup(c => c.CreateCommand(It.IsAny())) + _sqlCommandFactoryMock.Setup(c => c.CreateCommand(It.IsAny(), It.IsAny())) .Callback(() => throw new InvalidOperationException()); var logEvents = CreateLogEvents(); @@ -271,20 +257,5 @@ private static List CreateLogEvents() }; return logEvents; } - - protected virtual void Dispose(bool disposing) - { - if (!_disposedValue) - { - _sut?.Dispose(); - _disposedValue = true; - } - } - - public void Dispose() - { - Dispose(disposing: true); - GC.SuppressFinalize(this); - } } } From 4c65eb0e48ea7dfa7a4a2ed8664e17780d6c34ed Mon Sep 17 00:00:00 2001 From: Christian Kadluba <10721825+ckadluba@users.noreply.github.com> Date: Sat, 15 Mar 2025 22:08:39 +0100 Subject: [PATCH 4/5] Updated changelog --- CHANGES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index e60df5fc..844fd2f7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,6 @@ +# 8.1.1 +* Fixed concurrency issue #607 in audit sink + # 8.1.0 * Implemented #542: Column option `ResolveHierarchicalPropertyName` to force non-hierarchical handling * Removed unnecessary exception handlers and let Serilog Core do the SelfLog() From e0cb12aac1f05f737d221e1ed45d69eaa0087900 Mon Sep 17 00:00:00 2001 From: Christian Kadluba <10721825+ckadluba@users.noreply.github.com> Date: Sat, 15 Mar 2025 22:37:07 +0100 Subject: [PATCH 5/5] Reverted breaking API change Cannot remove IDisposable from MSSqlServerAuditSink class without bumping major version, so we leave it the same. --- .../Sinks/MSSqlServer/MSSqlServerAuditSink.cs | 21 ++++++++++++++++++- .../MSSqlServer/MSSqlServerAuditSinkTests.cs | 18 +++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs index 0a793fd3..b9e51788 100644 --- a/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs +++ b/src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerAuditSink.cs @@ -26,7 +26,7 @@ namespace Serilog.Sinks.MSSqlServer /// Writes log events as rows in a table of MSSqlServer database using Audit logic, meaning that each row is synchronously committed /// and any errors that occur are propagated to the caller. /// - public class MSSqlServerAuditSink : ILogEventSink + public class MSSqlServerAuditSink : ILogEventSink, IDisposable { private readonly ISqlLogEventWriter _sqlLogEventWriter; @@ -97,6 +97,25 @@ internal MSSqlServerAuditSink( public void Emit(LogEvent logEvent) => _sqlLogEventWriter.WriteEvent(logEvent); + /// + /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources. + /// + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + /// + /// Releases the unmanaged resources used by the Serilog.Sinks.MSSqlServer.MSSqlServerAuditSink and optionally + /// releases the managed resources. + /// + /// True to release both managed and unmanaged resources; false to release only unmanaged resources. + protected virtual void Dispose(bool disposing) + { + // This class needn't to dispose anything. This is just here for sink interface compatibility. + } + private static void ValidateParameters(MSSqlServerSinkOptions sinkOptions, ColumnOptions columnOptions) { if (sinkOptions?.TableName == null) diff --git a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs index 3d1a82e1..741639c6 100644 --- a/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs +++ b/test/Serilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/MSSqlServerAuditSinkTests.cs @@ -11,7 +11,7 @@ namespace Serilog.Sinks.MSSqlServer.Tests { [Trait(TestCategory.TraitName, TestCategory.Unit)] - public class MSSqlServerAuditSinkTests + public class MSSqlServerAuditSinkTests : IDisposable { private readonly MSSqlServerSinkOptions _sinkOptions; private readonly Serilog.Sinks.MSSqlServer.ColumnOptions _columnOptions; @@ -22,6 +22,7 @@ public class MSSqlServerAuditSinkTests private readonly string _tableName = "tableName"; private readonly string _schemaName = "schemaName"; private MSSqlServerAuditSink _sut; + private bool _disposedValue; public MSSqlServerAuditSinkTests() { @@ -161,5 +162,20 @@ private void SetupSut(bool autoCreateSqlDatabase = false, bool autoCreateSqlTabl _sinkOptions.AutoCreateSqlTable = autoCreateSqlTable; _sut = new MSSqlServerAuditSink(_sinkOptions, _columnOptions, _sinkDependencies); } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + _sut?.Dispose(); + _disposedValue = true; + } + } + + public void Dispose() + { + Dispose(disposing: true); + GC.SuppressFinalize(this); + } } }