Fix DefaultMethodInfoLocator to handle overloaded methods#3315
Fix DefaultMethodInfoLocator to handle overloaded methods#3315
Conversation
Co-authored-by: liliankasem <2198905+liliankasem@users.noreply.github.com>
Co-authored-by: liliankasem <2198905+liliankasem@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in DefaultMethodInfoLocator.GetMethod() that threw AmbiguousMatchException when a class contained overloaded methods with the same name, even when only one had the [Function] attribute.
Changes:
- Modified method lookup logic to handle overloaded methods by filtering for the method with
[Function]attribute - Added project reference from DotNetWorker.Core to Worker.Extensions.Abstractions for FunctionAttribute access
- Added test case for overloaded methods scenario
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/DotNetWorker.Core/Invocation/DefaultMethodInfoLocator.cs | Replaced Type.GetMethod() with GetMethods().Where() and added logic to select method with [Function] attribute when multiple overloads exist |
| src/DotNetWorker.Core/DotNetWorker.Core.csproj | Added project reference to Worker.Extensions.Abstractions for FunctionAttribute access |
| test/DotNetWorker.Tests/GrpcFunctionDefinitionTests.cs | Added test case with overloaded methods where only one has [Function] attribute |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MethodInfo? methodInfo = methods?.Length switch | ||
| { | ||
| 1 => methods[0], | ||
| > 1 => methods.FirstOrDefault(m => m.GetCustomAttribute<FunctionAttribute>() is not null), |
There was a problem hiding this comment.
Using FirstOrDefault instead of SingleOrDefault could silently return the wrong method when multiple overloads are decorated with [Function] attribute. This would be a configuration error by the developer, but FirstOrDefault will silently pick the first match instead of throwing an exception to alert them to the problem. Consider using SingleOrDefault to ensure that only one method has the [Function] attribute when there are multiple overloads.
| > 1 => methods.FirstOrDefault(m => m.GetCustomAttribute<FunctionAttribute>() is not null), | |
| > 1 => methods.SingleOrDefault(m => m.GetCustomAttribute<FunctionAttribute>() is not null), |
There was a problem hiding this comment.
Changed to SingleOrDefault in commit 9c1bfba. This will now throw an InvalidOperationException if multiple overloads have the [Function] attribute, alerting developers to the configuration error.
| Type? functionType = assembly.GetType(typeName); | ||
|
|
||
| MethodInfo? methodInfo = functionType?.GetMethod(methodName); | ||
| var methods = functionType?.GetMethods().Where(m => m.Name == methodName).ToArray(); |
There was a problem hiding this comment.
GetMethods() without BindingFlags will include all public methods including inherited methods from base classes. This could lead to unexpected behavior if a base class has a method with the same name. Consider using BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly to only search methods declared on the type itself, which would be more consistent with the original Type.GetMethod() behavior.
| var methods = functionType?.GetMethods().Where(m => m.Name == methodName).ToArray(); | |
| var methods = functionType?.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly).Where(m => m.Name == methodName).ToArray(); |
There was a problem hiding this comment.
Added BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly in commit 9c1bfba. This ensures we only search methods declared on the type itself, matching the original Type.GetMethod() behavior.
| [Fact] | ||
| public void GrpcFunctionDefinition_OverloadedMethod_Creates() | ||
| { | ||
| using var testVariables = new TestScopedEnvironmentVariable("FUNCTIONS_WORKER_DIRECTORY", "."); | ||
|
|
||
| var bindingInfoProvider = new DefaultOutputBindingsInfoProvider(); | ||
| var methodInfoLocator = new DefaultMethodInfoLocator(); | ||
|
|
||
| string fullPathToThisAssembly = GetType().Assembly.Location; | ||
| var functionLoadRequest = new FunctionLoadRequest | ||
| { | ||
| FunctionId = "abc", | ||
| Metadata = new RpcFunctionMetadata | ||
| { | ||
| EntryPoint = $"Microsoft.Azure.Functions.Worker.Tests.{nameof(GrpcFunctionDefinitionTests)}+{nameof(MyOverloadedFunctionClass)}.{nameof(MyOverloadedFunctionClass.Run)}", | ||
| ScriptFile = Path.GetFileName(fullPathToThisAssembly), | ||
| Name = "myfunction" | ||
| } | ||
| }; | ||
|
|
||
| // We base this on the request exclusively, not the binding attributes. | ||
| functionLoadRequest.Metadata.Bindings.Add("req", new BindingInfo { Type = "HttpTrigger", Direction = Direction.In }); | ||
| functionLoadRequest.Metadata.Bindings.Add("$return", new BindingInfo { Type = "Http", Direction = Direction.Out }); | ||
|
|
||
| FunctionDefinition definition = functionLoadRequest.ToFunctionDefinition(methodInfoLocator); | ||
|
|
||
| Assert.Equal(functionLoadRequest.FunctionId, definition.Id); | ||
| Assert.Equal(functionLoadRequest.Metadata.EntryPoint, definition.EntryPoint); | ||
| Assert.Equal(functionLoadRequest.Metadata.Name, definition.Name); | ||
| Assert.Equal(fullPathToThisAssembly, definition.PathToAssembly); | ||
|
|
||
| // Parameters - should match the overload with [Function] attribute | ||
| Assert.Collection(definition.Parameters, | ||
| p => | ||
| { | ||
| Assert.Equal("req", p.Name); | ||
| Assert.Equal(typeof(HttpRequestData), p.Type); | ||
| }); | ||
|
|
||
| // InputBindings | ||
| Assert.Collection(definition.InputBindings, | ||
| p => | ||
| { | ||
| Assert.Equal("req", p.Key); | ||
| Assert.Equal(BindingDirection.In, p.Value.Direction); | ||
| Assert.Equal("HttpTrigger", p.Value.Type); | ||
| }); | ||
|
|
||
| // OutputBindings | ||
| Assert.Collection(definition.OutputBindings, | ||
| p => | ||
| { | ||
| Assert.Equal("$return", p.Key); | ||
| Assert.Equal(BindingDirection.Out, p.Value.Direction); | ||
| Assert.Equal("Http", p.Value.Type); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The test only covers the happy path where one overload has [Function] attribute and others don't. Consider adding additional test cases for edge cases: 1) Multiple overloads where none have [Function] attribute (should fail with a clear error), and 2) Multiple overloads where more than one has [Function] attribute (should fail or handle deterministically). These edge cases would help ensure the implementation handles configuration errors appropriately.
There was a problem hiding this comment.
Added two edge case tests in commit 9c1bfba:
GrpcFunctionDefinition_OverloadedMethod_NoFunctionAttribute_ThrowsException- Tests multiple overloads with no[Function]attributeGrpcFunctionDefinition_OverloadedMethod_MultipleFunctionAttributes_ThrowsException- Tests multiple overloads with multiple[Function]attributes
Both tests verify appropriate error handling.
| { | ||
| using var testVariables = new TestScopedEnvironmentVariable("FUNCTIONS_WORKER_DIRECTORY", "."); | ||
|
|
||
| var bindingInfoProvider = new DefaultOutputBindingsInfoProvider(); |
There was a problem hiding this comment.
This assignment to bindingInfoProvider is useless, since its value is never read.
| var bindingInfoProvider = new DefaultOutputBindingsInfoProvider(); |
There was a problem hiding this comment.
Removed the unused bindingInfoProvider variable in commit 9c1bfba.
kshyju
left a comment
There was a problem hiding this comment.
Are we even using the DefaultMethodInfoLocator? Are we not using the source generated executor version which does not rely on this class?
I forgot we did the work to move to source gen. But is DefaultMethodInfoLocator 100% unused? Asked copilot to search for this: DefaultMethodInfoLocator IS still used in these scenarios:
So yes, the fix is still needed for users with:
|
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\extensions\Worker.Extensions.Abstractions\src\Worker.Extensions.Abstractions.csproj" /> |
There was a problem hiding this comment.
This dependency is concerning. Why is now the first time we are introducing this? Will this be problematic to add?
There was a problem hiding this comment.
Yeah I don't understand why this would even be added?
@copilot can you remove this dependency? Why did you add in the first place?
There was a problem hiding this comment.
Removed the project reference in commit 9c1bfba.
The dependency was initially added because I used GetCustomAttribute<FunctionAttribute>() which required a compile-time reference. However, this was unnecessary - the attribute can be checked using reflection by name: m.GetCustomAttributes().Any(a => a.GetType().Name == "FunctionAttribute"). This works because the attribute is loaded from the assembly at runtime, so no compile-time reference is needed.
There was a problem hiding this comment.
m.GetCustomAttributes().Any(a => a.GetType().Name == "FunctionAttribute").
This is not equivalent. Assembly or namespace is not being validated.
…dge case tests Co-authored-by: liliankasem <2198905+liliankasem@users.noreply.github.com>
Co-authored-by: liliankasem <2198905+liliankasem@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@liliankasem I think copilot got it a bit wrong with this being a slow fallback path. I thought they were mutually exclusive. The source gen executor completely replaces the default executor. No fallback mechanism is in place. I question the priority of this issue/fix given the source gen route is the better option. Customers would be better served to investigate why they can't use source gen than us investing in this executor. I am also cautious because of the |
I agree - I don't think its worth the effort here. I was hoping this was a quick fix when I assigned to copilot but didn't have all of this context. I think let's close this and point folks at source gen. |
Issue describing the changes in this PR
DefaultMethodInfoLocator.GetMethod()throwsAmbiguousMatchExceptionwhen a class contains overloaded methods with the same name, even when only one has the[Function]attribute.Changes
Core Fix
Type.GetMethod(methodName)withGetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly).Where(m => m.Name == methodName)SingleOrDefaultto select the one decorated with[Function]attribute (throws exception if multiple have the attribute)BindingFlags.DeclaredOnlyto match only methods declared on the type itself, consistent with originalType.GetMethod()behaviorImplementation Details
FunctionAttributeby name:m.GetCustomAttributes().Any(a => a.GetType().Name == "FunctionAttribute")SingleOrDefaultensures configuration errors (multiple methods with[Function]attribute) are caught and reportedTest Coverage
GrpcFunctionDefinition_OverloadedMethod_Creates- Happy path: overloaded methods where only one has[Function]attributeGrpcFunctionDefinition_OverloadedMethod_NoFunctionAttribute_ThrowsException- Edge case: multiple overloads with no[Function]attributeGrpcFunctionDefinition_OverloadedMethod_MultipleFunctionAttributes_ThrowsException- Edge case: multiple overloads with multiple[Function]attributesExample Scenario
Previously: Throws
AmbiguousMatchExceptionNow: Correctly identifies the method with
[Function]attributePull request checklist
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.