Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IColumnModification and IModificationCommand to allow the implementations to be replaced easily #12169

Closed
dmitry-lipetsk opened this issue May 30, 2018 · 9 comments · Fixed by #25327
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported providers-beware type-enhancement
Milestone

Comments

@dmitry-lipetsk
Copy link
Contributor

EF Core version: dev

My implementation of AppendInsertOperation generates following SQL text:

INSERT INTO "DECI" ("D_6_1") VALUES (:p0) RETURNING "TEST_ID" INTO :p1;

Where
"p0" - (input) parameter from writeOperations
"p1" - (output) parameter from readOperations

 public override ResultSetMapping AppendInsertOperation
                                           (StringBuilder       commandStringBuilder,
                                            ModificationCommand command,
                                            int                 commandPosition)
 {
  var operations       = command.ColumnModifications;
  var writeOperations  = operations.Where(o => o.IsWrite).ToList();
  var readOperations   = operations.Where(o => o.IsRead).ToList();
  
  // ............

  return ResultSetMapping.NoResultSet;
 }//AppendInsertOperation

My data provider returns values of OUT-parameters through parameters object, not through DataReader.

So, For this query ExecuteReader returns "empty" DataReader, without columns.

EntityFrameworkCore returns the error

Message: System.InvalidOperationException : The property 'TEST_ID' on entity type 'TEST_RECORD' has a temporary value while attempting to change the entity's state to 'Unchanged'. Either set a permanent value explicitly or ensure that the database is configured to generate values for this property.

Question: In theory, it is possible override EFCore classes for support OUT-parameters?

@dmitry-lipetsk
Copy link
Contributor Author

What I have right now:

I override RelationalCommand::CreateCommand method and build all parameters of command.

sealed class LcpiOleDb_RelationalCommand:RelationalCommand
{
 public LcpiOleDb_RelationalCommand(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger,
                                    string commandText,
                                    IReadOnlyList<IRelationalParameter> parameters)
  :base(logger,
        commandText,
        parameters)
 {
 }

 //-----------------------------------------------------------------------
 protected override DbCommand CreateCommand
                               (IRelationalConnection               connection,
                                IReadOnlyDictionary<string, object> parameterValues)
 {
  var xcommand=new xdb.OleDbCommand();

  DbCommand command=xcommand;

  command.Connection=connection.DbConnection;

  command.CommandText=AdjustCommandText(CommandText);

  this.ConfigureCommand(command);

  if(!Object.ReferenceEquals(connection.CurrentTransaction,null))
  {
   command.Transaction=connection.CurrentTransaction.GetDbTransaction();
  }

  xcommand.Parameters.Refresh(); // <------- GENERATION OF ALL COMMAND PARAMETERS

  if(connection.CommandTimeout.HasValue)
  {
   command.CommandTimeout=(int)connection.CommandTimeout;
  }

  if(Parameters.Count>0)
  {
   if(Object.ReferenceEquals(parameterValues,null))
   {
    //! \todo
    //!  ERROR - no values

    Debug.Assert(false);

    //throw new InvalidOperationException(
    //    RelationalStrings.MissingParameterValue(
    //        Parameters[0].InvariantName));
   }//if

   foreach(var p in Parameters)
   {
    object v;

    if(!parameterValues.TryGetValue(p.InvariantName,out v))
    {
     //! \todo
     //!  ERROR - parameter without values

     Debug.Assert(false);
    }//if

    //! \todo
    //!  HACK: Replace ':'+p.InvariantName to normal code

    xcommand[':'+p.InvariantName].Value=v;
   }//for
  }//if Parameters.Count>0

  return command;
 }//CreateCommand
};//class LcpiOleDb_RelationalCommand

So generation of OUT-parameters it is not problem.

The problem in the OUT-parameter value processing after executing the command.

Could you point me - what I should override for processing OUT-parameters values?

@dmitry-lipetsk
Copy link
Contributor Author

@ralmsdeveloper

Seem, you are right.

I think, need:

  1. Create the subclass of SingularModificationCommandBatch (instance is created in IModificationCommandBatchFactory::Create)

  2. Override the (as minimum) ConsumeResultSetWithPropagation:

https://github.com/aspnet/EntityFrameworkCore/blob/d8b7ebbfabff3d2e8560c24b1ff14d1f4244ca6a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs#L172-L201

One moment.

ConsumeResultSetWithPropagation obtaines the RelationalDataReader. This class contains the reference to command object, but not allow to access it from outside.

@dmitry-lipetsk
Copy link
Contributor Author

dmitry-lipetsk commented May 31, 2018

I rewrited ConsumeResultSetWithPropagation and get expected behaviour.

Prototype:

 protected override int ConsumeResultSetWithPropagation
                                           (int                  commandIndex,
                                            RelationalDataReader reader)
 {
  Debug.Assert(!Object.ReferenceEquals(reader,null));
  Debug.Assert(!Object.ReferenceEquals(reader.DbCommand,null));
  Debug.Assert(!Object.ReferenceEquals(reader.DbDataReader,null));
  Debug.Assert(!Object.ReferenceEquals(this.CommandResultSet,null));
  Debug.Assert(!Object.ReferenceEquals(this.ModificationCommands,null));
  Debug.Assert(!Object.ReferenceEquals(this.SqlGenerationHelper,null));

  Debug.Assert(commandIndex<this.CommandResultSet.Count);
  Debug.Assert(commandIndex<this.ModificationCommands.Count);

  //----------------------------------------------------------------------
  if(this.CommandResultSet[commandIndex]!=ResultSetMapping.LastInResultSet)
  {
   //! \todo
   //!  ERROR - unexpected commandResult

   Debug.Assert(false);
  }//if

  //----------------------------------------------------------------------
  if(reader.DbDataReader.FieldCount!=0)
  {
   //! \todo
   //!  ERROR - we expected result in output parameters

   Debug.Assert(false);
  }//if

  //----------------------------------------------------------------------
  if(reader.DbDataReader.RecordsAffected!=1)
  {
   //! \todo
   //!  Use own code for generation of exception

   this.ThrowAggregateUpdateConcurrencyException
    (commandIndex,
     1,
     reader.DbDataReader.RecordsAffected);
  }//if

  //----------------------------------------------------------------------
  var tableModification=this.ModificationCommands[commandIndex];

  Debug.Assert(tableModification.RequiresResultPropagation);

  var operations=tableModification.ColumnModifications;

  //! \todo
  //!  Rewrite 
  var xcommand=(xdb.OleDbCommand)reader.DbCommand;

  for(int iOp=0,_cOp=operations.Count;iOp!=_cOp;++iOp)
  {
   var op=operations[iOp];

   Debug.Assert(!Object.ReferenceEquals(op,null));

   if(!op.IsRead)
    continue;

   var cmdParam=xcommand[this.SqlGenerationHelper.GenerateParameterName(op.ParameterName)];

   switch(cmdParam.Direction)
   {
    case System.Data.ParameterDirection.Output:
    case System.Data.ParameterDirection.InputOutput:
    {
     object v=cmdParam.Value;

     if(DBNull.Value.Equals(v))
      v=null;

     op.Value=v;
     continue;
    }//case
   }//switch cmdParam.Direction

   //! \todo
   //!  ERROR - incorrect parameter direction
   Debug.Assert(false);
  }//for iOp

  return commandIndex+1;
 }//ConsumeResultSetWithPropagation

I think, this ticket can be closed.

One small suggestion for EFCore - add to RelationalDataReader the public property:

        /// <summary>
        ///     Gets the underlying command for the result set.
        /// </summary>
        public virtual DbCommand DbCommand => _command;

Or I should create my own class based on RelationalDataReader?

@dmitry-lipetsk
Copy link
Contributor Author

@ralmsdeveloper

I seem understood why your CoreEF provider uses EXECUTE BLOCK in AppendUpdateOperation and AppendDeleteOperation :)

@ralmsdeveloper
Copy link
Contributor

I'm going to an event ... when I return I can pass on some information and experiences that I already had with Efcore + Firebird

@dmitry-lipetsk
Copy link
Contributor Author

Original idea - usage OUT-parameters instead DataReader is good from perfomance point of view.

But has the some problems.

ColumnModification::set_Value requires exact type of data.

This type defined in ColumnModification::Property::ClrType, if I understood correctly.

I should transform DbParameter.Value to this type.

Otherwise I get an error like "Message: System.InvalidCastException : Unable to cast object of type 'System.Int16' to type 'System.Nullable`1[System.Int32]'."

Stack:

Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.Metadata.Internal.ClrPropertySetter<ef_core__lcpi_oledb__ntests.Work.DataTypes.Group002.Work_DataTypes__Test_001__SMALLINT__Int32.MyContext.TEST_RECORD, long?>.SetClrValue(object instance, object value) Line 34 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.WritePropertyValue(Microsoft.EntityFrameworkCore.Metadata.IPropertyBase propertyBase, object value) Line 616 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetProperty(Microsoft.EntityFrameworkCore.Metadata.IPropertyBase propertyBase, object value, bool setModified) Line 889 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.this[Microsoft.EntityFrameworkCore.Metadata.IPropertyBase].set(Microsoft.EntityFrameworkCore.Metadata.IPropertyBase propertyBase, object value) Line 822 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.AcceptChanges() Line 932 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.AcceptAllChanges(System.Collections.Generic.IReadOnlyList<Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry> changedEntries) Line 971 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(bool acceptAllChangesOnSuccess) Line 786 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.DbContext.SaveChanges(bool acceptAllChangesOnSuccess) Line 436 C#
Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.DbContext.SaveChanges() Line 401 C#

Need upgrade ColumnModification::set_Value (or added new Method) for transformation of "generic" value to required ClrType.

Or need create this code byself in my data provider :(

@ajcvickers ajcvickers added this to the 2.2.0 milestone Jun 1, 2018
dmitry-lipetsk added a commit to dmitry-lipetsk/EntityFrameworkCore that referenced this issue Jun 15, 2018
New property RelationalDataReader::DbCommand allow to read OUTPUT-parameters of modification commands (INSERT, UPDATE).

It used from ConsumeResultSetWithPropagation method.
AndriySvyryd pushed a commit that referenced this issue Jun 21, 2018
New property RelationalDataReader::DbCommand allow to read OUTPUT-parameters of modification commands (INSERT, UPDATE).

It used from ConsumeResultSetWithPropagation method.
@AndriySvyryd
Copy link
Member

@dmitry-lipetsk For full support for update commands with output parameters in EF you'd need to add something like GetValue to IRelationalParameter and perform the necessary conversion in the implementations. Also add ParametersModificationCommandBatch derived from ModificationCommandBatch that uses the parameters from the RelationalCommand.
Additional refactoring will probably be needed to make the provider implementation simpler.

@AndriySvyryd
Copy link
Member

Remaining work tracked in #21433

@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 23, 2021
@AndriySvyryd AndriySvyryd removed their assignment Jul 23, 2021
AndriySvyryd added a commit that referenced this issue Jul 23, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 23, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 24, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 26, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 26, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 26, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 26, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 26, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
AndriySvyryd added a commit that referenced this issue Jul 27, 2021
…ntations to be replaced easily

Fixes #23027
Fixes #12169
Fixes #17946

Co-authored-by: Kovalenko Dmitry <dmitry.lipetsk@gmail.com>
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc1 Aug 12, 2021
@ajcvickers ajcvickers changed the title AppendInsertOperation and output parameters Add IColumnModification and IModificationCommand to allow the implementations to be replaced easily Sep 16, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc1, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported providers-beware type-enhancement
Projects
None yet
4 participants