Efcore: Decimal parameter may not work with always-encrypted

[ajcvickers]

Issue came through support channels; I have full details.

Basic idea--note that the column type is explicitly configured:
```C#
modelBuilder.Entity(entity =>
{
entity.Property(e => e.Bar).HasColumnType("decimal(18, 0)");
});

```C#
var foo = new Foo()
{
    Bar = 123,
};

context.Add(bonus);
context.SaveChanges();

Exception:

Microsoft.EntityFrameworkCore.DbUpdateException: An error occurred while updating the entries. See the inner exception for details.

 ---> Microsoft.Data.SqlClient.SqlException (0x80131904): Operand type clash: decimal(5,0) encrypted with (encryption_type = 'DETERMINISTIC', encryption_algorithm_name = 'AEAD_AES_256_CBC_HMAC_SHA_256', column_encryption_key_name = 'CEK_Auto1', column_encryption_key_database_name = 'XxxxDB') is incompatible with decimal(18,0) encrypted with (encryption_type = 'DETERMINISTIC', encryption_algorithm_name = 'AEAD_AES_256_CBC_HMAC_SHA_256', column_encryption_key_name = 'CEK_Auto1', column_encryption_key_database_name = 'XxxxDB')

Statement(s) could not be prepared.

   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlDataReader.TryConsumeMetaData()
   at Microsoft.Data.SqlClient.SqlDataReader.get_MetaData()
   at Microsoft.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString, Boolean isInternal, Boolean forDescribeParameterEncryption, Boolean shouldCacheForAlwaysEncrypted)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, Boolean isAsync, Int32 timeout, Task& task, Boolean asyncWrite, Boolean inRetry, SqlDataReader ds, Boolean describeParameterEncryptionRequest)
   at Microsoft.Data.SqlClient.SqlCommand.TryFetchInputParameterEncryptionInfo(Int32 timeout, Boolean isAsync, Boolean asyncWrite, Boolean& inputParameterEncryptionNeeded, Task& task, ReadOnlyDictionary`2& describeParameterEncryptionRpcOriginalRpcMap)
   at Microsoft.Data.SqlClient.SqlCommand.PrepareForTransparentEncryption(CommandBehavior cmdBehavior, Boolean returnStream, Boolean isAsync, Int32 timeout, TaskCompletionSource`1 completion, Task& returnTask, Boolean asyncWrite, Boolean& usedCache, Boolean inRetry)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String method)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteDbDataReader(CommandBehavior behavior)
   at System.Data.Common.DbCommand.ExecuteReader()
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReader(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.Execute(IRelationalConnection connection)

[maumar]

Problem here is that when we create parameters for the insert command we don't copy facets (precision scale) from the column that the parameter value is going to be assigned to. The reason for that is to not accidentally lose data via truncation (e.g. sending 123.17 with scale = 1 will be persisted as 123.1, but if we ideally want it to be persisted as 123.2 - setting scale to 0 achieves that)

Workaround for always encrypted is to use command interceptor to modify the parameter facets accordingly:

        public class AlwaysEncryptedInsertInterceptor : DbCommandInterceptor
        {

            public override InterceptionResult<DbDataReader> ReaderExecuting(
                DbCommand command, 
                CommandEventData eventData, 
                InterceptionResult<DbDataReader> result)
            {
                command.Parameters[0].Precision = 18;
                command.Parameters[0].Value = Math.Round((decimal)command.Parameters[0].Value);

                return base.ReaderExecuting(command, eventData, result);
            }
        }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer("Server=.;Database=TestAlwaysEncWithEF3;Column Encryption Setting=Enabled;Trusted_Connection=True;")
                .AddInterceptors(new AlwaysEncryptedInsertInterceptor());
            base.OnConfiguring(optionsBuilder);
        }

Interceptor will modify every command, so either those inserts need to be separated or the interceptor logic needs to be more sophisticated so that only correct commands have their parameters modified - interceptor has access to CommandText so decision can be made based on that and parameter collection.

[ajcvickers]

Thanks @maumar. Removing from milestone to discuss in triage how to approach an actual fix here.

[ajcvickers]

It appears that the change for precision and scaled (#20468) fixed this, even though that was not the intent of that change. Being able to set precision and scale was not also supposed to change the way we construct parameters, since this would have re-introduced the truncation bug described above. The truncation issue has now been fixed in SqlClient. However, the changes to way we create parameters is still a breaking change because SqlClient will now round where before it would not do anything. This is fine if the precision and scale are set correctly in the model, but not if they are different. We should also consider the changes to other types, such as DateTime. See https://github.com/dotnet/efcore/pull/20468/files#diff-f4b95de72d29ea3af9c1d7795fbea349

[ajcvickers]

The team agreed that we're okay with the change made. Co-assigning with @maumar to add tests for always-encrypted, or at least these cases.

[ajcvickers]

Closing this in favor of #19293. #3380.

[smitpatel]

@ajcvickers - We have circular reference here.