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 586 forks source link

Issue in ReflectionHelper.cs #281

Open sstoenner opened 2 years ago

sstoenner commented 2 years ago

On line 326 in function GetParameter of file ReflectionHelper.cs you have this code: var entityPropertyName = propertyName.Split('_').Last(); and it is this variable that is used to search for the mapped property - WHY? This causes an exception if any field-name of the entity contains an underscore... Please fix by just using the property name.

ALMMa commented 2 years ago

Confirmed this as a breaking change on my side as well.
I had several legacy classes with properties which contain _ as part of their names and this caused a major refactor on the entire system.

Here's the core that triggers this https://github.com/tmsmith/Dapper-Extensions/blame/master/DapperExtensions/ReflectionHelper.cs#L326

Here's a previous PR that got closed - though not sure if it would help or make things worse: https://github.com/tmsmith/Dapper-Extensions/pull/261/files

This was introduced 6 months ago on the latest changes. Previous code works fine. Although I get the idea for this change, I believe that just splitting through the name is not safe. You can't expect a string property to have a child object there. Maybe we should remove types like int, string and others and only apply this to custom types, then? Or have an empty interface on the type we want this split/dive to be applied?

valfrid-ly commented 2 years ago

I accept suggestions when you have reference maps if this is changed. Basically, when you have a joint table the properties will be like Class1_PropertyName.

The change in the closed PR it would try to find "Class1_PropertyName" classmapper and it wouldn't find it. But try to get the classmapper of PropertyName works.

Other chars can create SQL issues also.

For 1 level property, changing last to first would fix but what about multilevel?

Let's take this example:

Client -> Orders -> Items

When getting Client I have a list of Orders. So the SQL will have the client columns, them the orders columns like "Orders_?" where ? will be the column name.

Adding the items to it we would have "OrdersItems?".

I invite you guys to please help me and provide a solution to this issue without forgeting the possible reference maps.

ALMMa commented 2 years ago

IMO, the thing was that this is a major breaking change and we were not aware of it 🤣

TBH, so far, the feeling was that this was not intentionally introduced into DapperExtensions. I don't have strong feeling for either keeping the split on _ or changing it to something else. I would try to keep it simple and rely on the _ then.

Requires refactor, but doable. I just applied to an entire system by the time I was checking these. Has been working fine ever since.

@sstoenner your thoughts?

hatuan commented 1 year ago

Hello. I am using PostgreSQL, all tables and columns contain _ Current I have changed ReflectionHelper.cs to

string[] entityPropertyNames = propertyName.Split('_');

var entityPropertyName = entityPropertyNames[entityPropertyNames.Length - 1];

IMemberMap propertyMap = map.Properties.SingleOrDefault(p => p.Name == entityPropertyName);

for (int i = entityPropertyNames.Length - 2; i>=0 && propertyMap == null; i--)
{
    entityPropertyName = entityPropertyNames[i] + "_" + entityPropertyName;
    propertyMap = map.Properties.SingleOrDefault(p => p.Name == entityPropertyName);
}

if (propertyMap == null)
    throw new NullReferenceException(String.Format("{0} was not found for {1}", entityPropertyName, entityType));

Please review it.