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

ColumnModification.ParameterName for OUTPUT parameter [readValue] #23027

Closed
dmitry-lipetsk opened this issue Oct 17, 2020 · 10 comments · Fixed by #25327
Closed

ColumnModification.ParameterName for OUTPUT parameter [readValue] #23027

dmitry-lipetsk opened this issue Oct 17, 2020 · 10 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 type-enhancement
Milestone

Comments

@dmitry-lipetsk
Copy link
Contributor

Hello,

Reference question: #12169

EF Core version: master
Database provider: my own
Target framework: (e.g. .NET 5.0)

I am trying to restore my project for EFCore, but I have problems :)

My implementation of UpdateSqlGenerator generates INSERT (and UPDATE) statement with RETURNING clause.

For example:
INSERT INTO "TEST_MODIFY_ROW_WD" ("COL_TEXT_BLOB") VALUES (:p0) RETURNING "TEST_ID" INTO :p1;

My UpdateSqlGenerator obtains two ColumnModification objects:

  1. For "COL_TEXT_BLOB" [IsWrite=true, IsRead=false, _useParameters=true]
  2. For "TEST_ID" [IsWrite=false, IsRead=true, _useParameters=true]

ColumnModification.ParameterName for "COL_TEXT_BLOB" returns generated name.

ColumnModification.ParameterName for "TEST_ID" returns null.

Current implementation of ColumnModification.ParameterName:

/// <summary>
/// Indicates whether the current value of the property must be passed as a parameter to the SQL
/// </summary>
public virtual bool UseCurrentValueParameter
=> _useParameters && IsWrite;
/// <summary>
/// The parameter name to use for the current value parameter (<see cref="UseCurrentValueParameter" />), if needed.
/// </summary>
public virtual string ParameterName
=> _parameterName ??= UseCurrentValueParameter ? _generateParameterName() : null;

Old implementation code:

        public virtual string ParameterName
            => _parameterName ?? (_parameterName = _generateParameterName());

ColumnModification.ParameterName is virtual, but I not see the way for override this method.

Could you pointed me - how I can resolve this problem?

@ajcvickers
Copy link
Contributor

@dmitry-lipetsk What are you reporting here that isn't covered by #12169?

@dmitry-lipetsk
Copy link
Contributor Author

About an issue that came up after improving ColumnModification::get_ParameterName :)

@ajcvickers
Copy link
Contributor

@dmitry-lipetsk We read through this issue in triage. It wasn't clear to anyone there what issue you are reporting that wasn't already discussed in #12169. Please try to explain more clearly.

@dmitry-lipetsk
Copy link
Contributor Author

For #12169 I found a working solution. That issue can be closed.

This issue about ColumnModification::get_ParameterName.

New implementation (with UseCurrentValueParameter condition) does not allow usage OUTPUT-parameters.

ColumnModification::get_ParameterName declared as virtual.

But I don't see an easy way to create a new child class from ColumnModification and integrate this new class into the existing EF framework.

@ajcvickers
Copy link
Contributor

@dmitry-lipetsk The team looked at this in triage, but unfortunately we still don't know what you're attempting to do.

@dmitry-lipetsk
Copy link
Contributor Author

Hello,

////////////////////////////////////////////////////////////////////////////////
//EF.Core Provider for LCPI OLE DB.
//                                                 Kovalenko Dmitry. 22.05.2018.
using System;
using System.Diagnostics;
using System.Text;
using System.Linq;
using System.Collections.Generic;
using Microsoft.EntityFrameworkCore.Update;

namespace EntityFrameworkCore.DataProvider.Lcpi.OleDb.Dbms.Firebird.Common.Update{
////////////////////////////////////////////////////////////////////////////////
//class FB_Common__UpdateSqlGenerator

/*public*/ sealed class FB_Common__UpdateSqlGenerator:UpdateSqlGenerator
{
 private const ErrSourceID c_ErrSrcID
  =ErrSourceID.FB_Common__UpdateSqlGenerator;

 //-----------------------------------------------------------------------
 public FB_Common__UpdateSqlGenerator(UpdateSqlGeneratorDependencies  dependencies)
  :base(dependencies)
 {
#if TRACE
  Core.Core_Trace.Method
   ("FB_Common__UpdateSqlGenerator::FB_Common__UpdateSqlGenerator(...)");
#endif
 }//FB_UpdateSqlGenerator

 //-----------------------------------------------------------------------
 protected override void AppendIdentityWhereCondition
                                           (StringBuilder      commandStringBuilder,
                                            ColumnModification columnModification)
 {
#if TRACE
  Core.Core_Trace.Method
   ("FB_Common__UpdateSqlGenerator::AppendIdentityWhereCondition(...) - invalid operation");
#endif

  //----------------------------------------------------------------------
  throw new InvalidOperationException();
 }//AppendIdentityWhereCondition

 //-----------------------------------------------------------------------
 protected override void AppendRowsAffectedWhereCondition
                                           (StringBuilder commandStringBuilder,
                                            int           expectedRowsAffected)
 {
#if TRACE
  Core.Core_Trace.Method
   ("FB_Common__UpdateSqlGenerator::AppendRowsAffectedWhereCondition(...) - invalid operation");
#endif

  //----------------------------------------------------------------------
  throw new InvalidOperationException();
 }//AppendRowsAffectedWhereCondition

 //-----------------------------------------------------------------------
 public override ResultSetMapping AppendInsertOperation
                                           (StringBuilder       commandStringBuilder,
                                            ModificationCommand command,
                                            int                 commandPosition)
 {
#if TRACE
  Core.Core_Trace.Method_Enter
   ("FB_Common__UpdateSqlGenerator::AppendInsertOperation(...)");
#endif

  //----------------------------------------------------------------------
  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(AppendInsertOperation),
    nameof(commandStringBuilder),
    commandStringBuilder);

  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(AppendInsertOperation),
    nameof(command),
    command);

  //----------------------------------------------------------------------
  var operations       = command.ColumnModifications;
  var writeOperations  = operations.Where(o => o.IsWrite).ToList();
  var readOperations   = operations.Where(o => o.IsRead).ToList();

  this.AppendInsertCommandHeader
   (commandStringBuilder,
    command.TableName,
    command.Schema,
    writeOperations);

  this.AppendValuesHeader
   (commandStringBuilder,
    writeOperations);

  var tableName  = command.TableName;
  var schemaName = command.Schema;

#if TRACE
  Core.Core_Trace.Send
   ("tableName : {0}",tableName);

  Core.Core_Trace.Send
   ("schemaName: {0}",schemaName);
#endif

  this.AppendValues
   (commandStringBuilder,
    tableName,
    schemaName,
    writeOperations);

  this.Helper__AppendReturningInto
   (commandStringBuilder,
    readOperations);

  commandStringBuilder
   .Append(SqlGenerationHelper.StatementTerminator)
   .AppendLine();

  //----------------------------------------------------------------------
#if TRACE
  Core.Core_Trace.Method_Exit
   ("FB_Common__UpdateSqlGenerator::AppendInsertOperation(...)");
#endif

  //! \attention
  //!  RESEARCH: ALWAYS CHECK AFFECTED ROWS COUNT.

  //if(readOperations.Count>0)
   return ResultSetMapping.LastInResultSet;

  //return ResultSetMapping.NoResultSet;
 }//AppendInsertOperation

 //-----------------------------------------------------------------------
 public override ResultSetMapping AppendUpdateOperation
                                           (StringBuilder       commandStringBuilder,
                                            ModificationCommand command,
                                            int                 commandPosition)
 {
#if TRACE
  Core.Core_Trace.Method_Enter
   ("FB_Common__UpdateSqlGenerator::AppendUpdateOperation(...)");
#endif

  //----------------------------------------------------------------------
  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(AppendUpdateOperation),
    nameof(commandStringBuilder),
    commandStringBuilder);

  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(AppendUpdateOperation),
    nameof(command),
    command);

  //----------------------------------------------------------------------
  var operations          = command.ColumnModifications;

  var writeOperations     = operations.Where(o=>o.IsWrite).ToList();
  var conditionOperations = operations.Where(o=>o.IsCondition).ToList();
  var readOperations      = operations.Where(o=>o.IsRead).ToList();

  this.AppendUpdateCommandHeader
   (commandStringBuilder,
    command.TableName,
    command.Schema,
    writeOperations);

  this.AppendWhereClause
   (commandStringBuilder,
    conditionOperations);

  this.Helper__AppendReturningInto
   (commandStringBuilder,
    readOperations);

  commandStringBuilder
   .Append(SqlGenerationHelper.StatementTerminator)
   .AppendLine();

  //----------------------------------------------------------------------
#if TRACE
  Core.Core_Trace.Method_Exit
   ("FB_Common__UpdateSqlGenerator::AppendUpdateOperation(...)");
#endif

  //! \attention
  //!  RESEARCH: ALWAYS CHECK AFFECTED ROWS COUNT.

  //if(readOperations.Count>0)
   return ResultSetMapping.LastInResultSet;

  //return ResultSetMapping.NoResultSet;
 }//AppendUpdateOperation

 //-----------------------------------------------------------------------
 public override ResultSetMapping AppendDeleteOperation
                                           (StringBuilder       commandStringBuilder,
                                            ModificationCommand command,
                                            int                 commandPosition)
 {
#if TRACE
  Core.Core_Trace.Method_Enter
   ("FB_Common__UpdateSqlGenerator::AppendDeleteOperation(...)");
#endif

  //----------------------------------------------------------------------
  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(AppendUpdateOperation),
    nameof(commandStringBuilder),
    commandStringBuilder);

  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(AppendUpdateOperation),
    nameof(command),
    command);

  //----------------------------------------------------------------------
  var operations          = command.ColumnModifications;

  var conditionOperations = operations.Where(o=>o.IsCondition).ToList();
  var readOperations      = operations.Where(o=>o.IsRead).ToList();

  this.AppendDeleteCommandHeader
   (commandStringBuilder,
    command.TableName,
    command.Schema);

  this.AppendWhereClause
   (commandStringBuilder,
    conditionOperations);

  this.Helper__AppendReturningInto
   (commandStringBuilder,
    readOperations);

  commandStringBuilder
   .Append(SqlGenerationHelper.StatementTerminator)
   .AppendLine();

  //----------------------------------------------------------------------
#if TRACE
  Core.Core_Trace.Method_Exit
   ("FB_Common__UpdateSqlGenerator::AppendDeleteOperation(...)");
#endif

  //! \attention
  //!  RESEARCH: ALWAYS CHECK AFFECTED ROWS COUNT.

  //if(readOperations.Count>0)
   return ResultSetMapping.LastInResultSet;

  //return ResultSetMapping.NoResultSet;
 }//AppendDeleteOperation

 //-----------------------------------------------------------------------
 private void Helper__AppendReturningInto(StringBuilder             commandStringBuilder,
                                          IList<ColumnModification> readOperations)
 {
  Debug.Assert(!Object.ReferenceEquals(commandStringBuilder,null));
  Debug.Assert(!Object.ReferenceEquals(readOperations,null));

  if(readOperations.Count==0)
   return;

  commandStringBuilder.AppendLine();
  commandStringBuilder.Append("RETURNING ");

  bool f=true;

  foreach(var op in readOperations)
  {
   if(f)
   {
    f=false;
   }
   else
   {
    commandStringBuilder.Append(',');
   }//else

   this.SqlGenerationHelper.DelimitIdentifier
    (commandStringBuilder,
     op.ColumnName);
  }//foreach e

  commandStringBuilder.AppendLine();
  commandStringBuilder.Append("INTO ");

  f=true;

  foreach(var e in readOperations)
  {
   if(f)
   {
    f=false;
   }
   else
   {
    commandStringBuilder.Append(',');
   }//else

   SqlGenerationHelper.GenerateParameterNamePlaceholder
    (commandStringBuilder,
     e.ParameterName); // <------------------------------------------- HERE
  }//foreach e
 }//Helper__AppendReturningInto
};//class FB_Common__UpdateSqlGenerator

////////////////////////////////////////////////////////////////////////////////
}//namespace EntityFrameworkCore.DataProvider.Lcpi.OleDb.Dbms.Firebird.Common.Update

Problem in Helper__AppendReturningInto (called from AppendInsertOperation, AppendUpdateOperation, AppendDeleteOperation):

   SqlGenerationHelper.GenerateParameterNamePlaceholder
    (commandStringBuilder,
     e.ParameterName); // <------------------------------------------- HERE

Your current implementation of get_ParameterName returns null.

I temporary changed your code to:

        public virtual string ParameterName
            => _parameterName ??= _generateParameterName();

@ajcvickers ajcvickers added this to the Backlog milestone Oct 26, 2020
@AndriySvyryd
Copy link
Member

@dmitry-lipetsk It should be possible, though you'd need to copy over some code from EF.

You'd need to create a class derived from ModificationCommand and override ColumnModifications property calling your GenerateColumnModifications implementation that creates your implementation of ColumnModification.

Then in your implementation of CommandBatchPreparer override CreateModificationCommands creating instances of the derived ModificationCommand.

@dmitry-lipetsk
Copy link
Contributor Author

dmitry-lipetsk commented Nov 1, 2020

Hi Andriy,

Thanks for this research.

The first glance says that this is not enough.

I'll try do it later. After porting my existing code to the new EFCore codebase.

Thanks again.

@dmitry-lipetsk
Copy link
Contributor Author

dmitry-lipetsk commented Nov 24, 2020

Hello @AndriySvyryd,

I created a patch, that resolves this issue through IColumnModificationFactory.

But unable to push it to GitHub - I got the problem with your TestCosmos.yaml :)

git -c diff.mnemonicprefix=false -c core.quotepath=false --no-optional-locks push -v --set-upstream origin Issue23027_ColumnModificationFactory:Issue23027_ColumnModificationFactory
POST git-receive-pack (7250 bytes)
Pushing to https://github.com/dmitry-lipetsk/EntityFrameworkCore.git
To https://github.com/dmitry-lipetsk/EntityFrameworkCore.git
! [remote rejected] Issue23027_ColumnModificationFactory -> Issue23027_ColumnModificationFactory (refusing to allow an OAuth App to create or update workflow .github/workflows/TestCosmos.yaml without workflow scope)
error: failed to push some refs to 'https://github.com/dmitry-lipetsk/EntityFrameworkCore.git'

Could you pointed me - how I can resolve this problem?

Thanks.

UPD. Fixed. Problem was in SSH.

@ajcvickers
Copy link
Contributor

See also #17946

@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 6.0.0 Jul 23, 2021
@AndriySvyryd AndriySvyryd removed their assignment Jul 23, 2021
@AndriySvyryd AndriySvyryd added area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement labels 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc1 Aug 12, 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 type-enhancement
Projects
None yet
3 participants