Bug in Csla.Reflection.Methodcaller method FindMethodUsingFuzzyMatching(..)

Bug in Csla.Reflection.Methodcaller method FindMethodUsingFuzzyMatching(..)

Old forum URL: forums.lhotka.net/forums/t/7243.aspx


rfcdejong posted on Wednesday, July 08, 2009

Situation:
On of our developers is making a small program using only CSLA without our framework, he ran into an situation and asked me for my help. Together we looked at his code and it all seems fine, so i went debugging.

He is using LinqToSql and has one big query in the root business object data access, and passing arrays of the queryresult in FetchChild. The problem resides in an overloaded factory method with two fetch_child methods in the data access, they look alot the same.

Factory methods

internal static MnuItems GetMnuMenuItems(DIT.Data.MNU_GROEP_ITEM[] data)
{
      
return DataPortal.FetchChild<MnuItems>(data);
}

internal static MnuItems GetMnuSubMenuItems(DIT.Data.MNU_ITEM[] data)
{
      
return DataPortal.FetchChild<MnuItems>(data);
}

Data Access

private
void Child_Fetch(DIT.Data.MNU_GROEP_ITEM[] data)
{
...
}

private void Child_Fetch(DIT.Data.MNU_ITEM[] data)
{
...
}


Problem:
The methodcaller has problems finding the correct Child_Fetch method and ends up into returning the first one.
First FindMethod returns null
Second FindMethod returns null
The AmbiguousMatchException is handled and FindMethodUsingFuzzyMatching is called.
But FindMethodUsingFuzzyMatching returns the first one having the same parameter count but wrong array type.

Solution:
On line 423 we added a condition comparing the type of the parameter with the reflected method parameter.

if
(parameters.GetType().FullName == infoParams[0].ParameterType.FullName)

This solved our problem and i don't see a reason that it can go wrong with finding other methods.

sergeyb replied on Wednesday, July 08, 2009

I believe what is confusing the MethodCaller is the fact that your parameter is an array, and it is trying to do matching based on array of parameters.  I think if you change your signatures to List<MNU_ITEM> and List<MNU_GROUP_ITEM> or pass root L2S object instead of array of items, and let your class iterate through children of the root L2S object, that would solve the problem without having to modify the CSLA.  FuzzyMatching is only used if strongly typed match fails, which is really what you would like to rely on.  Although your fix might also be OK, but that is up to Rocky to decide.

 

Sergey Barskiy

Principal Consultant

office: 678.405.0687 | mobile: 404.388.1899

cid:_2_0648EA840648E85C001BBCB886257279
Microsoft Worldwide Partner of the Year | Custom Development Solutions, Technical Innovation

 

From: rfcdejong [mailto:cslanet@lhotka.net]
Sent: Wednesday, July 08, 2009 8:08 AM
To: Sergey Barskiy
Subject: [CSLA .NET] Bug in Csla.Reflection.Methodcaller method FindMethodUsingFuzzyMatching(..)

 

Situation:
On of our developers is making a small program using only CSLA without our framework, he ran into an situation and asked me for my help. Together we looked at his code and it all seems fine, so i went debugging.

He is using LinqToSql and has one big query in the root business object data access, and passing arrays of the queryresult in FetchChild. The problem resides in an overloaded factory method with two fetch_child methods in the data access, they look alot the same.

Factory methods

internal static MnuItems GetMnuMenuItems(DIT.Data.MNU_GROEP_ITEM[] data)
{
      return DataPortal.FetchChild<MnuItems>(data);
}

internal static MnuItems GetMnuSubMenuItems(DIT.Data.MNU_ITEM[] data)
{
      return DataPortal.FetchChild<MnuItems>(data);
}

Data Access

private
void Child_Fetch(DIT.Data.MNU_GROEP_ITEM[] data)
{
...
}

private void Child_Fetch(DIT.Data.MNU_ITEM[] data)
{
...
}


Problem:
The methodcaller has problems finding the correct Child_Fetch method and ends up into returning the first one.
First FindMethod returns null
Second FindMethod returns null
The AmbiguousMatchException is handled and FindMethodUsingFuzzyMatching is called.
But FindMethodUsingFuzzyMatching returns the first one having the same parameter count but wrong array type.

Solution:
On line 423 we added a condition comparing the type of the parameter with the reflected method parameter.

if
(parameters.GetType().FullName == infoParams[0].ParameterType.FullName)

This solved our problem and i don't see a reason that it can go wrong with finding other methods.



sergeyb replied on Wednesday, July 08, 2009

Another approach would be to have single DataPortal_Fetch that takes an object parameter, and have that method test the type and do some branching logic.

 

Sergey Barskiy

Principal Consultant

office: 678.405.0687 | mobile: 404.388.1899

cid:_2_0648EA840648E85C001BBCB886257279
Microsoft Worldwide Partner of the Year | Custom Development Solutions, Technical Innovation

 

From: Sergey Barskiy [mailto:cslanet@lhotka.net]
Sent: Wednesday, July 08, 2009 9:08 AM
To: Sergey Barskiy
Subject: RE: [CSLA .NET] Bug in Csla.Reflection.Methodcaller method FindMethodUsingFuzzyMatching(..)

 

I believe what is confusing the MethodCaller is the fact that your parameter is an array, and it is trying to do matching based on array of parameters.  I think if you change your signatures to List<MNU_ITEM> and List<MNU_GROUP_ITEM> or pass root L2S object instead of array of items, and let your class iterate through children of the root L2S object, that would solve the problem without having to modify the CSLA.  FuzzyMatching is only used if strongly typed match fails, which is really what you would like to rely on.  Although your fix might also be OK, but that is up to Rocky to decide.

 

Sergey Barskiy

Principal Consultant

office: 678.405.0687 | mobile: 404.388.1899

cid:_2_0648EA840648E85C001BBCB886257279
Microsoft Worldwide Partner of the Year | Custom Development Solutions, Technical Innovation

 

From: rfcdejong [mailto:cslanet@lhotka.net]
Sent: Wednesday, July 08, 2009 8:08 AM
To: Sergey Barskiy
Subject: [CSLA .NET] Bug in Csla.Reflection.Methodcaller method FindMethodUsingFuzzyMatching(..)

 

Situation:
On of our developers is making a small program using only CSLA without our framework, he ran into an situation and asked me for my help. Together we looked at his code and it all seems fine, so i went debugging.

He is using LinqToSql and has one big query in the root business object data access, and passing arrays of the queryresult in FetchChild. The problem resides in an overloaded factory method with two fetch_child methods in the data access, they look alot the same.

Factory methods

internal static MnuItems GetMnuMenuItems(DIT.Data.MNU_GROEP_ITEM[] data)
{
      return DataPortal.FetchChild<MnuItems>(data);
}

internal static MnuItems GetMnuSubMenuItems(DIT.Data.MNU_ITEM[] data)
{
      return DataPortal.FetchChild<MnuItems>(data);
}

Data Access

private
void Child_Fetch(DIT.Data.MNU_GROEP_ITEM[] data)
{
...
}

private void Child_Fetch(DIT.Data.MNU_ITEM[] data)
{
...
}


Problem:
The methodcaller has problems finding the correct Child_Fetch method and ends up into returning the first one.
First FindMethod returns null
Second FindMethod returns null
The AmbiguousMatchException is handled and FindMethodUsingFuzzyMatching is called.
But FindMethodUsingFuzzyMatching returns the first one having the same parameter count but wrong array type.

Solution:
On line 423 we added a condition comparing the type of the parameter with the reflected method parameter.

if
(parameters.GetType().FullName == infoParams[0].ParameterType.FullName)

This solved our problem and i don't see a reason that it can go wrong with finding other methods.

 



rfcdejong replied on Thursday, July 09, 2009

Thanks for your feedback. Maybe the problem resides in working with L2S arrays in the examples, that's why the developer went with L2S arrays. Overloading it for the recursive call.

I'll leave the line in the CSLA code and hope we never need to update CSLA for that program. If we we ever update it i think we'll soon enough see it doesn't work anymore.

Anyway i would like to see the fuzzy match works with an overloaded method working with L2S arrays ;)

RockfordLhotka replied on Thursday, July 09, 2009

I'll add this to the bug list.

RockfordLhotka replied on Thursday, July 09, 2009

The whole problem, btw, is that C# doesn't have an equivalent to the VB call by name function. The VB team wrote everything necessary to call methods by name, properly handling overloads and the whole bit. But I can't use that well-engineered and highly tested function, because it would require a reference to the VB runtime DLL - and you know a lot of C# bigots would throw a sh*t fit if I did that...

So instead, I've been forced into creating my own equivalent. Which is kind of fun in a super-geeky sort of way, but turns out to be very complex, especially when trying to support arrays and param arrays (because they look the same, but are actually different).

It did earn me a lot of respect with the C# compiler team though :)  Last year I had a meeting with the sub-team responsible for adding the dynamic keyword (late binding) to C# 4, and when they realized what I was doing they were noticably impressed. Sadly, their work doesn't overlap my work, and it doesn't appear that the dynamic keyword will help me get rid of MethodCaller :(

I do think I'm getting terribly close to having it all right at this point. In fact I thought I had it all right, but obviously not quite.

rfcdejong replied on Thursday, July 09, 2009

RockfordLhotka:
I'll add this to the bug list.


Thanks

RockfordLhotka:
In fact I thought I had it all right, but obviously not quite.



Sorry ;)

RockfordLhotka replied on Friday, July 17, 2009

I thought I'd look at this for 3.7, as it seemed simple enough. But it turns out that a complete fix is much more complex, which means the risk is higher, so it will have to wait for 3.7.1 or something.

The reason the fix is more complex, is that there are numerous variations that must be handled, including:

void MyMethod(params int[] x)

void MyMethod(params string[] x)

void MyMethod(int a, params int[] x)

void MyMethod(int a, params string[] x)

Your code would resolve the first two, but not the last two. And resolving the last two is turning out to be very challenging.

And Sergey is right, this really should be handled by CallMethod(), not by the fuzzy matching routine.

Copyright (c) Marimer LLC