Fix NullReferenceException on bulk operations on entities with no keys #355
Conversation
Failure to check for HasIdentity on an entity would lead to NullReferenceException when indexing tableInfo.PropertyColumnNamesDict with tableInfo.IdentityColumnName
|
@knalinne I also included a fix for entities which do not have an identity column going through an irrelevant branch in |
If you are running a BulkInsert from a method which accepts AbstractBaseClass, and pass in a set of entities which are DerivedClass, SQL server operations correctly use the derived class entity configuration (ToTable, etc.). However when using Sqlite, it attempts to use the properties of the base class in the type accessor, which will not work if you have extra properties on your derived class.
| @@ -483,6 +486,7 @@ public static async Task MergeAsync(DbContext context, Type type, IList<object> | |||
| { | |||
| var command = GetSqliteCommand(context, type, entities, tableInfo, connection, transaction); | |||
|
|
|||
| type = tableInfo.HasAbstractList ? entities[0].GetType() : type; | |||
There was a problem hiding this comment.
The type that is passed in is always the abstract base class if the entity being passed in is covariant. So for instance BulkInsert<T> at compile time recognizes T as AbstractBaseClass, but at run time you may be passing in a type of DerivedClass.
| @@ -17,7 +17,7 @@ public static string InsertIntoTable(TableInfo tableInfo, OperationType operatio | |||
| List<string> columnsList = tableInfo.PropertyColumnNamesDict.Values.ToList(); | |||
|
|
|||
| bool keepIdentity = tableInfo.BulkConfig.SqlBulkCopyOptions.HasFlag(SqlBulkCopyOptions.KeepIdentity); | |||
| if(operationType == OperationType.Insert && !keepIdentity) | |||
| if(operationType == OperationType.Insert && !keepIdentity && tableInfo.HasIdentity) | |||
There was a problem hiding this comment.
In the absence of this check, on line 22, tableInfo.IdentityColumnName will be null, leading to a NullReferenceException when indexing tableInfo.PropertyColumnNamesDict.
| @@ -123,7 +123,7 @@ private void LoadData<T>(DbContext context, Type type, IList<T> entities, bool l | |||
| bool AreSpecifiedUpdateByProperties = BulkConfig.UpdateByProperties?.Count() > 0; | |||
| var primaryKeys = entityType.FindPrimaryKey()?.Properties?.Select(a => a.Name)?.ToList(); | |||
|
|
|||
| HasSinglePrimaryKey = primaryKeys.Count == 1; | |||
| HasSinglePrimaryKey = primaryKeys?.Count == 1; | |||
There was a problem hiding this comment.
The previous updates fixed line 124 with conditional null access, but this line, which is also susceptible to a potential null value on HasNoKey() entities, was omitted. This is fixed here.
| foreach (var derived in entityType.GetDirectlyDerivedTypes()) | ||
| { | ||
| extendedAllProperties.AddRange(dervied.GetProperties()); | ||
| extendedAllProperties.AddRange(derived.GetProperties()); |
There was a problem hiding this comment.
Merely fixing a typo.
|
Merged and 3.1.4 published |
@borisdj
I tested this with an
InsertAsyncoperation on an entity that was configured withHasNoKey(). It appears to work successfully. That said, I don't know the full context of this code, but it seems in line with the recent changes made in 121d2d5.