-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
DBFunctions - Add support for instance methods. #9755
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,9 +68,11 @@ private DbFunction( | |
throw new ArgumentException(RelationalStrings.DbFunctionGenericMethodNotSupported(methodInfo.DisplayName())); | ||
} | ||
|
||
if (!methodInfo.IsStatic) | ||
if (!methodInfo.IsStatic | ||
&& !typeof(DbContext).IsAssignableFrom(methodInfo.DeclaringType)) | ||
{ | ||
throw new ArgumentException(RelationalStrings.DbFunctionMethodMustBeStatic(methodInfo.DisplayName())); | ||
throw new ArgumentException( | ||
RelationalStrings.DbFunctionInvalidInstanceType(methodInfo.DisplayName(), methodInfo.DeclaringType.ShortDisplayName())); | ||
} | ||
|
||
if (methodInfo.ReturnType == null | ||
|
@@ -102,7 +104,7 @@ public static IEnumerable<IDbFunction> GetDbFunctions([NotNull] IModel model, [N | |
} | ||
|
||
private static string BuildAnnotationName(string annotationPrefix, MethodBase methodBase) | ||
=> $@"{annotationPrefix}{methodBase.Name}({string.Join(",", methodBase.GetParameters().Select(p => p.ParameterType.Name))})"; | ||
=> $@"{annotationPrefix}{methodBase.DeclaringType.ShortDisplayName()}{methodBase.Name}({string.Join(",", methodBase.GetParameters().Select(p => p.ParameterType.Name))})"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is changing annotation name for DbFunction. Since everything is runtime, it would not break anything particular. But hand-crafted model can break may be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is actually fixing a bug I ran into. If you declare two static methods on different types with the same name then the annotation name was colliding. We need the type name to differentiate the methods. See the new unit test DbFunctions_with_duplicate_names_and_parameters_on_different_types_dont_collide |
||
|
||
/// <summary> | ||
/// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Do we have logic for selecting only this set of
BindingFlags
? It includes static protected methods from base types in search but not instance protected methods.Also should we only support public methods or protected methods too. @anpete @divega
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.
The
BindingFlags.Public
will filter out the protected static methods as well as protected instance methods.I think we should only bring in public methods since users shouldn't be putting their queries directly in their DbContext class - imho. Therefore there is no need for protected methods.
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.
Filed #9956