From 3f488280d2703f781b493c3ae2f27e909ca3bf55 Mon Sep 17 00:00:00 2001 From: Ramon Smits Date: Wed, 15 Nov 2023 09:25:38 +0100 Subject: [PATCH] Stored json gets corrupted on SqlServer after an update with a smaller sized payload (#1331) * Fix issue where sql parameter optimalisation was applied for INSERT/UPDATE. Now only applied for `ExecuteReaderAsync` and `ExecuteReaderAsync`. * Fix failing CI tests due to new test using incorrect table for testing * Updated the test name to better express its intent --- .../When_updating_saga_with_smaller_state.cs | 93 +++++++++++++++++++ src/SqlPersistence/CommandWrapper.cs | 2 + src/SqlPersistence/Config/SqlDialect.cs | 4 + .../Config/SqlDialect_MsSqlServer.cs | 33 ++++--- 4 files changed, 120 insertions(+), 12 deletions(-) create mode 100644 src/SqlPersistence.PersistenceTests/When_updating_saga_with_smaller_state.cs diff --git a/src/SqlPersistence.PersistenceTests/When_updating_saga_with_smaller_state.cs b/src/SqlPersistence.PersistenceTests/When_updating_saga_with_smaller_state.cs new file mode 100644 index 000000000..887d18657 --- /dev/null +++ b/src/SqlPersistence.PersistenceTests/When_updating_saga_with_smaller_state.cs @@ -0,0 +1,93 @@ +namespace NServiceBus.PersistenceTesting.Sagas +{ + using System; + using System.Linq; + using System.Threading.Tasks; + using NUnit.Framework; + + public class When_updating_saga_with_smaller_state : SagaPersisterTests + { + [Test] + public async Task It_should_truncate_the_stored_state() + { + // When updating an existing saga where the serialized state is smaller in length than the previous the column value should not have any left over data from the previous value. + // The deserializer ignores any trailing + + var sqlVariant = (SqlTestVariant)param.Values[0]; + + if (sqlVariant.Dialect is not SqlDialect.MsSqlServer) + { + Assert.Ignore("Only relevant for SQL Server"); + return; // Satisfy compiler + } + + var sagaData = new SagaWithCorrelationPropertyData + { + CorrelatedProperty = Guid.NewGuid().ToString(), + Payload = "very long state" + }; + + await SaveSaga(sagaData); + + SagaWithCorrelationPropertyData retrieved; + var context = configuration.GetContextBagForSagaStorage(); + var persister = configuration.SagaStorage; + + using (var completeSession = configuration.CreateStorageSession()) + { + await completeSession.Open(context); + + retrieved = await persister.Get(nameof(sagaData.CorrelatedProperty), sagaData.CorrelatedProperty, completeSession, context); + + retrieved.Payload = "short"; + + await persister.Update(retrieved, completeSession, context); + await completeSession.CompleteAsync(); + } + + var retrieved2 = await GetById(sagaData.Id); + + Assert.LessOrEqual(retrieved.Payload, sagaData.Payload); // No real need, but here to prevent accidental updates + Assert.AreEqual(retrieved.Payload, retrieved2.Payload); + + await using var con = sqlVariant.Open(); + await con.OpenAsync(); + var cmd = con.CreateCommand(); + cmd.CommandText = $"SELECT Data FROM [PersistenceTests_SWCP] WHERE Id = '{retrieved.Id}'"; + var data = (string)await cmd.ExecuteScalarAsync(); + + // Payload should only have a single closing bracket, if there are more that means there is trailing data + var countClosingBrackets = data.ToCharArray().Count(x => x == '}'); + + Assert.AreEqual(1, countClosingBrackets); + } + + public class SagaWithCorrelationProperty : Saga, IAmStartedByMessages + { + public Task Handle(StartMessage message, IMessageHandlerContext context) + { + throw new NotImplementedException(); + } + + protected override void ConfigureHowToFindSaga(SagaPropertyMapper mapper) + { + mapper.ConfigureMapping(msg => msg.SomeId).ToSaga(saga => saga.CorrelatedProperty); + } + } + + public class SagaWithCorrelationPropertyData : ContainSagaData + { + public string CorrelatedProperty { get; set; } + public string Payload { get; set; } + } + + public class StartMessage + { + public string SomeId { get; set; } + } + + public When_updating_saga_with_smaller_state(TestVariant param) : base(param) + { + } + } +} \ No newline at end of file diff --git a/src/SqlPersistence/CommandWrapper.cs b/src/SqlPersistence/CommandWrapper.cs index f31acda2d..b6ac231b9 100644 --- a/src/SqlPersistence/CommandWrapper.cs +++ b/src/SqlPersistence/CommandWrapper.cs @@ -64,12 +64,14 @@ public Task ExecuteNonQueryAsync(CancellationToken cancellationToken = defa public Task ExecuteReaderAsync(CancellationToken cancellationToken = default) { + dialect.OptimizeForReads(command); return command.ExecuteReaderAsync(cancellationToken); } public Task ExecuteReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken = default) { var resultingBehavior = dialect.ModifyBehavior(command.Connection, behavior); + dialect.OptimizeForReads(command); return command.ExecuteReaderAsync(resultingBehavior, cancellationToken); } diff --git a/src/SqlPersistence/Config/SqlDialect.cs b/src/SqlPersistence/Config/SqlDialect.cs index b2d839236..463268b41 100644 --- a/src/SqlPersistence/Config/SqlDialect.cs +++ b/src/SqlPersistence/Config/SqlDialect.cs @@ -62,5 +62,9 @@ internal virtual void ValidateTablePrefix(string tablePrefix) } internal abstract object GetCustomDialectDiagnosticsInfo(); + + internal virtual void OptimizeForReads(DbCommand command) + { + } } } \ No newline at end of file diff --git a/src/SqlPersistence/Config/SqlDialect_MsSqlServer.cs b/src/SqlPersistence/Config/SqlDialect_MsSqlServer.cs index 4c1b53e08..29d6fbdd0 100644 --- a/src/SqlPersistence/Config/SqlDialect_MsSqlServer.cs +++ b/src/SqlPersistence/Config/SqlDialect_MsSqlServer.cs @@ -34,18 +34,7 @@ internal override void SetParameterValue(DbParameter parameter, object value) if (value is ArraySegment charSegment) { parameter.Value = charSegment.Array; - - // Set to 4000 or -1 to improve query execution plan reuse - // Must be set when exceeding 4000 characters for nvarchar(max) https://stackoverflow.com/a/973269/199551 - parameter.Size = charSegment.Count > 4000 ? -1 : 4000; - } - else if (value is string stringValue) - { - parameter.Value = stringValue; - - // Set to 4000 or -1 to improve query execution plan reuse - // Must be set when exceeding 4000 characters for nvarchar(max) https://stackoverflow.com/a/973269/199551 - parameter.Size = stringValue.Length > 4000 ? -1 : 4000; + parameter.Size = charSegment.Count; } else { @@ -84,9 +73,29 @@ internal override object GetCustomDialectDiagnosticsInfo() }; } + internal override void OptimizeForReads(DbCommand command) + { + foreach (DbParameter parameter in command.Parameters) + { + if (parameter.Value is ArraySegment charSegment) + { + // Set to 4000 or -1 to improve query execution plan reuse + // Must be set when exceeding 4000 characters for nvarchar(max) https://stackoverflow.com/a/973269/199551 + parameter.Size = charSegment.Count > 4000 ? -1 : 4000; + } + else if (parameter.Value is string stringValue) + { + // Set to 4000 or -1 to improve query execution plan reuse + // Must be set when exceeding 4000 characters for nvarchar(max) https://stackoverflow.com/a/973269/199551 + parameter.Size = stringValue.Length > 4000 ? -1 : 4000; + } + } + } + internal string Schema { get; set; } bool hasConnectionBeenInspectedForEncryption; bool isConnectionEncrypted; } + } } \ No newline at end of file