tmsmith / Dapper-Extensions

Dapper Extensions is a small library that complements Dapper by adding basic CRUD operations (Get, Insert, Update, Delete) for your POCOs. For more advanced querying scenarios, Dapper Extensions provides a predicate system. The goal of this library is to keep your POCOs pure by not requiring any attributes or base class inheritance.
1.79k stars 585 forks source link

SqlConnection.InsertAsync tries to insert IList<> property, AutoMapper fails to ignore it, Map() throws exception for IList #266

Closed balchen closed 2 years ago

balchen commented 3 years ago

I'm trying to use DapperExtensions to do SqlConnection.InsertAsync on my POCO that looks like this:

  public class Customer 
  {
     public int Id { get; set; }
     public string Name { get; set; }
     ......
     public IList<App> Apps { get; set; }
  }

When I do SqlConnection.InsertAsync(customer), it fails with a System.NotSupportedException: The member of type App cannot be used as a parameter value.

It took me a while to spot the extra space after "member". Obviously there was supposed to be a member name in that exception message, but it's blank.

For one, I can't understand why InsertAsync is trying to map an IList property, but this behaviour is the same with Dapper.Contrib. Since I can't annotate the POCO to ignore the property, I've implemented a custom mapper:

    public class CustomerMap: PluralizedAutoClassMapper<Customer>
    {
        public CustomerMap()
            : base()
        {
            Map(c => c.Apps).Ignore();
        }
    }

and then on startup I do:

            DapperExtensions.DapperAsyncExtensions.DefaultMapper = typeof(PluralizedAutoClassMapper<>);
            DapperExtensions.DapperAsyncExtensions.SetMappingAssemblies(new[]
            {
                this.GetType().Assembly
            });

At runtime, the mapping code is executed, and Map() throws ApplicationException: "Unable to UnMap because mapping does not exist."

That message makes absolutely no sense to me at this point, since the call to base() would/should have caused the AutoClassMapper to AutoMap().

I looked at the code for ClassMapper, and the exception message seems to be a copy-paste mistake:


        /// <summary>
        /// Fluently, maps an entity property to a column
        /// </summary>
        protected virtual MemberMap Map(PropertyInfo propertyInfo, MemberMap parent = null)
        {
            var result = new MemberMap(propertyInfo, this, parent: parent);
            if (GuardForDuplicatePropertyMap(result))
            {
                if (propertyInfo.PropertyType.IsClass)
                {
                    result = (MemberMap)Properties.FirstOrDefault(p => p.Name.Equals(result.Name) && p.ParentProperty == result.ParentProperty);
                }
                else
                {
                    throw new ApplicationException("Unable to UnMap because mapping does not exist.");
                }
            }
            else
            {
                Properties.Add(result);
            }
            return result;
        }

        /// <summary>
        /// Removes a propertymap entry
        /// </summary>
        /// <param name="expression"></param>
        protected virtual void UnMap(Expression<Func<T, object>> expression)
        {
            var propertyInfo = ReflectionHelper.GetProperty(expression) as PropertyInfo;
            var mapping = Properties.SingleOrDefault(w => w.Name == propertyInfo.Name);

            if (mapping == null)
            {
                throw new ApplicationException("Unable to UnMap because mapping does not exist.");
            }

            this.Properties.Remove(mapping);
        }

The exception message is identical to the one from UnMap(), where it makes more sense.

However, even if the exception message made sense, an exception is still thrown by ClassMapper. The exception is thrown if I try to Map() a property -- that is not a class -- for the second time. Map() won't throw exception the first time and happily map the property, but the second time around, only class properties can be mapped.

This logic also makes no sense to me. I'm brand new to this library and code base, so I can't say that this logic is incorrect, but it certainly seems weird.

Seeing this, I changed my IList<> property to List<>, and Map() will now allow me to map it and ignore it.

Still, InsertAsync throw the same System.NotSupportedException: The member of type App cannot be used as a parameter value.

Obviously, InsertAsync is still trying to insert a value based on List, and it isn't ignoring it like the mapping says.

It feels like I'm halfway down a rabbit hole here, and I'm either waaaay off or there are several bugs interacting in unhealthy ways.

Nuget library version: 1.7.0.

balchen commented 3 years ago

Corrected my incorrect issue description: Map() throws exception, not Ignore().

balchen commented 3 years ago

I've cloned the repo and done some more research into why System.NotSupportedException is thrown. The reason is a combination of identity property and properties of unsupported types. When an Insert is performed, the SQL generated contains the correct number of columns and parameters. The issue is not with the SQL itself, but with the list of parameters (column values). The list contains values for ALL properties, including ignored properties. SqlClient evaluates the parameter types even if the parameter is not used in the query, and throws the exception for unsupported types.

The code below contains the problem.

InternalInsert branches if (triggerIdentityColumn != null || identityColumn != null) (identityColumn being "Id" in my case) and calls dynamicParameters = GetDynamicParameters(entity, dynamicParameters, keyColumn, true);, an overload without classMap. This overload of GetDynamicParameters does not evalute the mappings and does not respect the Ignored flag.

This is in contrast when there is no identity column and GetDynamicParameters is called with the classMap, where Ignored is respected.

        protected dynamic InternalInsert<T>(IDbConnection connection, T entity, IDbTransaction transaction, int? commandTimeout,
            IClassMapper classMap, IList<IMemberMap> nonIdentityKeyProperties, IMemberMap identityColumn,
            IMemberMap triggerIdentityColumn, IList<IMemberMap> sequenceIdentityColumn) where T : class
        {
            DynamicParameters dynamicParameters = null;

            foreach (var column in nonIdentityKeyProperties)
            {
                if (column.KeyType == KeyType.Guid && (Guid)column.GetValue(entity) == Guid.Empty)
                {
                    var comb = SqlGenerator.Configuration.GetNextGuid();
                    column.SetValue(entity, comb);
                }
            }

            IDictionary<string, object> keyValues = new ExpandoObject();
            var sql = SqlGenerator.Insert(classMap);
            if (triggerIdentityColumn != null || identityColumn != null)
            {
                var keyColumn = triggerIdentityColumn ?? identityColumn;
                object keyValue;

                dynamicParameters = GetDynamicParameters(entity, dynamicParameters, keyColumn, true);

                if (triggerIdentityColumn != null)
                {
                    keyValue = InsertTriggered(connection, entity, transaction, commandTimeout, sql, triggerIdentityColumn, dynamicParameters);
                }
                else
                {
                    keyValue = InsertIdentity(connection, transaction, commandTimeout, classMap, sql, dynamicParameters);
                }

                keyValues.Add(keyColumn.Name, keyValue);
                keyColumn.SetValue(entity, keyValue);
            }
            else
            {
                dynamicParameters = GetDynamicParameters(classMap, entity, true);

                if (sequenceIdentityColumn.Count > 0)
                {
                    if (sequenceIdentityColumn.Count > 1)
                        throw new ArgumentException("SequenceIdentity generator cannot be used with multi-column keys");

                    AddSequenceParameter(connection, entity, sequenceIdentityColumn[0], dynamicParameters, keyValues);
                }
                else if (nonIdentityKeyProperties != null)
                {
                    AddKeyParameters(entity, nonIdentityKeyProperties, dynamicParameters, true);
                }

                LastExecutedCommand = sql;
                connection.Execute(sql, dynamicParameters, transaction, commandTimeout, CommandType.Text);
            }

            foreach (var column in nonIdentityKeyProperties)
            {
                keyValues.Add(column.Name, column.GetValue(entity));
            }

            if (keyValues.Count == 1)
            {
                return keyValues.First().Value;
            }

            return keyValues;
        }

        .............

        public DynamicParameters GetDynamicParameters<T>(IClassMapper classMap, T entity, bool useColumnAlias = false)
        {
            var sequenceIdentityColumn = classMap.Properties.Where(p => p.KeyType == KeyType.SequenceIdentity)?.ToList();
            var foreignKeys = classMap.Properties.Where(p => p.KeyType == KeyType.ForeignKey).Select(p => p.MemberInfo).ToList();
            var ignored = classMap.Properties.Where(x => x.Ignored).Select(p => p.MemberInfo).ToList();

            if (sequenceIdentityColumn?.Count > 1)
                throw new ArgumentException("SequenceIdentity generator cannot be used with multi-column keys");

            return GetDynamicParameters(entity, classMap, sequenceIdentityColumn, foreignKeys, ignored, useColumnAlias);
        }

        public DynamicParameters GetDynamicParameters<T>(T entity, DynamicParameters dynamicParameters, IMemberMap keyColumn, bool useColumnAlias = false)
        {
            dynamicParameters ??= new DynamicParameters();
            foreach (var prop in entity.GetType().GetProperties(BindingFlags.GetProperty | BindingFlags.Instance | BindingFlags.Public)
                .Where(p => p.Name != keyColumn.Name))
                AddParameter(entity, dynamicParameters, new MemberMap(prop), useColumnAlias);

            return dynamicParameters;
        }
valfrid-ly commented 3 years ago

Hi, I'm going to take a look and reproduce the error. About the GetDynamicParameters without the classmapper, you maybe miss the fact it receives a keyColumn member map and only create parameters for the the properties with the same name.

In your sample code I couldn't see any key assignment and that's why it's really odd.

If you find any other detail please let me know as it can help me finding this bug

balchen commented 3 years ago

Hi @valfrid-ly.

I saw that it receives a IMemberMap keyColumn. It then generates a property value for every other property than the key (Where(p => p.Name != keyColumn.Name)), including ignored properties. This is the problem.

My sample has no explicit key assignment, but AutoClassMapper treats Id as the key.

I've made changes in my fork to correct GetDynamicParameters to that it only returns parameter values for properties that are not ignored (and not the key).

I've also corrected the problem with Map() throwing exception the second time it's called for a property, but like I wrote in the initial description, I am completely unable to see the rationale behind that if() to begin with, so I'm worried I have missed a major point.

I can create PR if you like.

valfrid-ly commented 3 years ago

If you fixed, please, feel free to create the PR. Please, don't forget to update the unit tests including also a case for this situation.

balchen commented 3 years ago

I've created two PRs. Like I said, I don't feel I have a full understanding of this code, so please correct me if this is just wrong.