-
Notifications
You must be signed in to change notification settings - Fork 63
UserfunctionId change and integration test. #1091
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| var methodInfo = (MethodInfo)this._parameter.Member; | ||
| string functionName = $"{methodInfo.DeclaringType.FullName}.{methodInfo.Name}"; | ||
| string functionName = $"{methodInfo.Name}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone has two functions in their app that share a name this means they'll share a lease table now.
What is special about the scale controller that you can't get the FullName of the type it's declared in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build will fail if user tries to create different functions with same name in an app. So that's covered. But if the user creates same function name across different apps - with this change, they could end up using the same lease table.
@AmeyaRele said he can only access the function name on the Scale controller PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have multiple methods with the same name being used for functions as long as they :
- Are in different classes
- Have different FunctionName attribute values
(note this is for C# - other languages may not allow this but since C# does it doesn't matter if others do or not)
e.g.
namespace Company.Function
{
public static class AddProduct
{
[FunctionName("AddProduct")]
public static IActionResult Run(
[HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "addproduct")]
[FromBody] Product prod,
[Sql("dbo.Products", "SqlConnectionString")] out Product product)
{
product = prod;
return new CreatedResult($"/api/addproduct", product);
}
}
}and
namespace Company.Function
{
public static class AddProduct2
{
[FunctionName("AddProduct2")]
public static IActionResult Run(
[HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "addproduct2")]
[FromBody] Product prod,
[Sql("dbo.Products", "SqlConnectionString")] out Product product)
{
product = prod;
return new CreatedResult($"/api/addproduct", product);
}
}
}Note that both of these have the attribute on the "Run" method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of our samples all have the same method name Run - and I'm going to guess this is pretty common for users too so would be pretty impactful to make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methodInfo.Name gives me AddProduct for the first one and AddProduct2 for the second one. I'll have to test for non dotnet languages and confirm the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that's odd - can you check if it's pulling it from the class name or the FunctionName?
Might be worth asking the core team where this value is populated from. Because for non-C# languages I'm curious how they would even get a "MethodInfo" given that that's specifically for reflection. But I suppose it would make sense if they decided to shim in their own version of it based on other stuff...
If it using the FunctionName then that should be fine though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't debug and see what's the value returned for methodInfo.DeclaringType.Name on javascript functions (not able to debug even after replacing the local dll in the bundle path), but I did run two different functions under the same app and it created two different leases table so definitely not taking the method name which is same for both the functions.
And methodInfo.Name is Run so probably methodInfo.DeclaringType is something added by the functions core.
alrod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with minor comment
|
|
||
| var methodInfo = (MethodInfo)this._parameter.Member; | ||
| string functionName = $"{methodInfo.DeclaringType.FullName}.{methodInfo.Name}"; | ||
| string functionName = $"{methodInfo.DeclaringType.Name}.{methodInfo.Name}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the discussion in other comments, shouldn't this be just methodInfo.DeclaringType.Name. Why're we appending methodInfo.Name as well now? I think the ScaleController wouldn't have the info around methodInfo.Name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methodInfo.DeclaringType.Name is the className of the function and not the function name as I assumed, so if a class has two functions against the same table then that would only trigger one function (one of our tests was set up that way and that's how I caught it). @AmeyaRele Could you confirm if you can access method name on ScaleController end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vivekjilla I just synced with @AmeyaRele and updated the code to read the function name from FunctionName attribute. Will merge mu changes in once the tests pass and get on with the releasing the preview package tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
| displayName: 'Component Detection' | ||
| inputs: | ||
| failOnAlert: true | ||
| failOnAlert: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest doing this as a separate PR so we can get this fixed asap
* add integrayion test and use just functionName in userFunctionId * make websitename required * add docs * use MethodInfo.Name * update error msg * methodInfo.DeclaringType.Name * update doc * set WEBSITE_SITE_NAME for tests * add method name to UserFunctionId * remove method name and replace with functionName * update to sdk with latest security patch * do not fail on PRs * argument excepion fix * undo Component Detection change * undo sdk bump * fix FunctionName for non .Net
For enabling the SQL on Scale controller, Ameya has a PR which requires further changes in userfunctionId since the current namespace+functionName is not available on scale controller.
I updated the userfucntionId to just use functionName. The implications of doing that is: For local functions if the user doesn't provide a WEBSITE_SITE_NAME setting and uses the same functionName across apps, they use the same leases table. Deployed functions read the WEBSITE_SITE_NAME which is a read-only environment variable and does not run into this issue. Below are the changes addressed in this PR: