DBFunctions - Add support for instance methods. #9755

pmiddleton · 2017-09-11T00:12:06Z

Resolve #9213

Allows DbFunctions to be instance methods as well as static methods.

pmiddleton · 2017-09-13T02:59:50Z

I just thought of a use case which needs addressing before this can be merged.

Currently we allow any static method to be registered via the fluent api regardless of what type it is defined on. However this code change assumes that the instance method is defined on your DbContext.

Do we want to only support instance methods on DbContext, or any instance method from any type?

Either way a change will need to be made to the code.

divega · 2017-09-13T21:09:12Z

However this code change assumes that the instance method is defined on your DbContext.

@pmiddleton @anpete @smitpatel I have created #9811 to track entity instance functions, but reading this comment makes me wonder if there are other interesting scenarios, e.g. a function on a DTO we don't want to block. There is also an interplay between instance methods and extension methods. The latter are static methods, but how do we handle the "this" argument?

BTW, are we assuming that the instance is the DbContext or are we actually checking that if the instance is a DbContext it is the same DbContext we are using to evaluate the query?

pmiddleton · 2017-09-13T22:11:50Z

We don't care what instance the DbContext is when we are translating because there is no state from the context which is used by the translation. We just need to know the type is of DbContext so we can attempt a translation.

Currently there is a if check it the code which is used to avoid attempting to translate instance method calls we know can't be translated. By knowing the type the instance method is attached to is of DbContext we can attempt a translation.

As an alternative we either have to just try translating ever method, or keep a master list of translatable methods (static and instance) which we can look up.

The later would actually help performance a bit. Right now when we try to translate a method the code just loops over all possible translations until it finds one that worked. Having a master list would allow us to bypass a bunch of unneeded processing.

I am not sure off the top of my head what will happen with extension methods. I am thinking they will require a custom translate callback, but I can confirm that tonight.

pmiddleton · 2017-09-14T01:18:51Z

Extension methods will require a custom translation in order to deal with the "this" parameter.

It wouldn't be too hard to detected extension methods for DbContext and throw away the this parameter when the method is registered with the model.

divega · 2017-09-14T05:52:38Z

@pmiddleton thanks. I think it would be ok to create separate Iissues for the outstanding questions that may require additional discussion/work, like extension methods and the “master list”, and move on with this PR. @smitpatel, @anpete, thoughts?

Re the DbContext instance, excuse me if I haven’t dug into the details of how this PR works, and I am rather basing my comments on what you said. IMO, it would be better to narrow the condition down to only translate functions on the same DbContext instance, because in-memory implementations (and in the future also self-bootstrapping functions) may depend on state on the DbContext, which could lead to the results being different.

divega · 2017-09-14T05:56:57Z

Also regarding state on the DbContext, could HasTranslation extract references to state on the provided DbContext instance passed, similar to how HasQueryFilter does?

pmiddleton · 2017-09-14T16:10:49Z

@divega - What rules do you guys follow for introducing changes to interfaces which will affect 3rd party database providers?

divega · 2017-09-14T16:15:55Z

@pmiddleton short answer is we avoid doing that in patches or minor releases. What is the context?

pmiddleton · 2017-09-14T16:22:32Z

I was just thinking about the “master list” idea and how it would help make things cleaner on the backend for instance methods, but it would require adding an interface method which would touch all of the providers. So we are looking at 3.0 before we would be able to do that?

divega · 2017-09-14T18:32:04Z

@pmiddleton without knowing much about the specific context, would introducing a new interface help? I guess it wouldn't help remove the existing code paths that would still need to be there for providers that don't implement it.

pmiddleton · 2017-09-14T20:15:39Z

@divega - so which direction do we want to go with this for now? Instance methods just on Dbcontext for now with a possible expansion for other types down the road? Also make sure the DbContext you are calling the method on is the same as the one that the current query is built off of?

divega · 2017-09-14T22:37:51Z

@pmiddleton I am personally ok with having additional things we need to follow up on accounted for in the issue tracker and take this if @anpete and @smitpatel think it is ready.

pmiddleton · 2017-09-16T23:03:50Z

I've updated the code to allow instance methods to run from any type.

I have been thinking about it and I'm not sure if we want to limit instance methods on DbContext to always be called from the same context instance.

It may help with in memory, but could there also be a valid use case that we would then be preventing?

divega · 2017-09-17T00:05:47Z

FWIW, I wasn’t thinking about preventing calls to instance methods on different context from succeeding, but about evaluating them in memory (as self-bootstrapped functions) so that they can bring their own data, act based on the state of the correct context instance, use their own global query filters, connection string, provider, etc.

BTW, I don’t know for sure from the top of my head how we handle the introduction of a DbSet from a different DbContext instance in a query, but I believe it should work in a similar way.

Also, I don’t think it is at all hard to just use the same DbContext instance when you want as much as possible of the query to be translated and pushed down to the server through the same DbContext.

If in the future anyone figures out how to optimize execution of queries that merge data from multiple context (e.g. reducing the number of round trips or the amount of data transferred over the network) then we would still likely be sending each function evaluation to their own DbContext instances.

pmiddleton · 2017-09-17T18:04:52Z

Ok that makes way more sense than what I had thought you were looking for. That feature will be blocked by #8864 so lets get this in as it stand now.

pmiddleton · 2017-09-22T02:54:00Z

I have cleaned up the last few issues. This is ready for a review now.

smitpatel · 2017-09-22T21:23:56Z

@pmiddleton - We had a discussion about this. Here is brief summary.
Looking at how we want to do #9811 We may want to implicitly pass key value information for entity instance to the server. So that syntax can be even more customer-friendly. But if we allow entity instance method at present then in future the syntax of db function would be different which becomes a breaking change. Till we figure out what exactly we want to do in #9811 , we should not enable translating instance methods on entities.
Another way to think about it is like, if you are writing a db function which does not depend on local state and returns solely based on parameters passed in, you can achieve similar without involving instance states. If that is possible then perhaps such method can be made static easily since it does not depend on instance states. For db context, it does provide us smaller syntax and there are not directly associated state in db instance (like entity instance and key value). So the general usecase would be fine.
cc: @anpete
Can you scope down this translation to only translate methods from db context instance? Implementation looks good.

pmiddleton · 2017-09-25T02:20:04Z

@smitpatel - Done

smitpatel · 2017-09-25T05:02:09Z

src/EFCore.Relational/Metadata/Internal/DbFunction.cs

@@ -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))})";


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.

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

smitpatel · 2017-09-25T05:03:21Z

src/EFCore.Relational/Properties/RelationalStrings.resx

  <data name="DbFunctionGenericMethodNotSupported" xml:space="preserve">
    <value>The DbFunction '{function}' is generic. Generic methods are not supported.</value>
  </data>
  <data name="DbFunctionExpressionIsNotMethodCall" xml:space="preserve">
    <value>The provided DbFunction expression '{expression}' is invalid. The expression should be a lambda expression containing a single method call to the target static method. Default values can be provided as arguments if required. E.g. () =&gt; SomeClass.SomeMethod(null, 0)</value>
  </data>
+  <data name="DbFunctionInvalidInstanceType" xml:space="preserve">
+    <value>DbFunction instance methods are only supported on DbContext.  The method '{function}' is defined on an invalid type '{type}'.</value>


Should re-word this message. Its not invalid type always. Sometimes user may just need to make it a static method.
cc: @divega, @ajcvickers to suggest exception message.

Yes please. I suck at naming things :)

How about "The DbFunction '{function}' defined on type '{type}' must be either a static method or an instance method defined on a DbContext subclass. Instance methods on other types are not supported."

works for me - change made

smitpatel · 2017-09-25T05:03:53Z

LGTM

pmiddleton · 2017-10-03T15:27:28Z

Are we waiting on something from me for this?

ajcvickers · 2017-10-03T17:42:02Z

@pmiddleton I think it's just that @anpete and @smitpatel have been otherwise engaged for the last week or so. Sorry for the delay--one of them will either merge or respond further soon.

pmiddleton · 2017-10-03T17:53:53Z

ah - no rush. I just thought I might owe something yet and missed it.

smitpatel · 2017-10-03T18:49:14Z

src/EFCore.Relational/Infrastructure/RelationalModelCustomizer.cs

-                .Where(
-                    mi => mi.IsStatic
-                          && mi.IsPublic
+            var functions = context.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.FlattenHierarchy)


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

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.

Filed #9956

smitpatel · 2017-10-03T18:53:08Z

            Assert.Equal(

These should be updated to use AssertSql format. You can change in same PR or I will file a new issue to clean-up.

Refers to: test/EFCore.SqlServer.FunctionalTests/Query/UdfDbFunctionSqlServerTests.cs:245 in b4b299c. [](commit_id = b4b299c, deletion_comment = False)

smitpatel · 2017-10-04T00:54:12Z

Merged via 0edbe21
Thanks for contribution @pmiddleton

dnfclas added the cla-already-signed label Sep 11, 2017

pmiddleton force-pushed the nonStatic branch 2 times, most recently from c4fbf7b to 599595a Compare September 11, 2017 01:28

pmiddleton changed the title ~~DBFunctions - Add support for instance methods on DbContext~~ DBFunctions - Add support for instance methods. Sep 11, 2017

pmiddleton force-pushed the nonStatic branch from 599595a to 56d8a92 Compare September 11, 2017 01:29

pmiddleton changed the title ~~DBFunctions - Add support for instance methods.~~ WIP - DBFunctions - Add support for instance methods. Sep 13, 2017

pmiddleton mentioned this pull request Sep 13, 2017

DbFunctions: Consider allowing instance methods #9213

Closed

pmiddleton force-pushed the nonStatic branch from 56d8a92 to bae5058 Compare September 16, 2017 21:22

pmiddleton changed the title ~~WIP - DBFunctions - Add support for instance methods.~~ DBFunctions - Add support for instance methods. Sep 17, 2017

pmiddleton force-pushed the nonStatic branch 2 times, most recently from 6996b51 to 8c16ecd Compare September 22, 2017 01:56

pmiddleton force-pushed the nonStatic branch 2 times, most recently from 3846e36 to 39d924a Compare September 25, 2017 02:19

pmiddleton force-pushed the nonStatic branch from 39d924a to 3d29baa Compare September 25, 2017 02:21

smitpatel reviewed Sep 25, 2017

View reviewed changes

smitpatel assigned anpete Sep 25, 2017

smitpatel requested a review from anpete September 25, 2017 05:03

ajcvickers approved these changes Sep 25, 2017

View reviewed changes

DBFunctions - Add support for instance methods.

b4b299c

pmiddleton force-pushed the nonStatic branch from 3d29baa to b4b299c Compare September 25, 2017 18:53

smitpatel reviewed Oct 3, 2017

View reviewed changes

smitpatel approved these changes Oct 3, 2017

View reviewed changes

pmiddleton closed this Oct 4, 2017

pmiddleton deleted the nonStatic branch October 4, 2017 23:50

pmiddleton mentioned this pull request Apr 19, 2018

Default Sql Server db functions to DBO schema #9303

Closed

anpete added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels Apr 26, 2018

jaredpar unassigned anpete Jan 7, 2020

rowanmiller mentioned this pull request Feb 6, 2020

Query: Additional function mapping capabilities #4319

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBFunctions - Add support for instance methods. #9755

DBFunctions - Add support for instance methods. #9755

pmiddleton commented Sep 11, 2017

pmiddleton commented Sep 13, 2017

divega commented Sep 13, 2017 •

edited

Loading

pmiddleton commented Sep 13, 2017

pmiddleton commented Sep 14, 2017 •

edited

Loading

divega commented Sep 14, 2017

divega commented Sep 14, 2017

pmiddleton commented Sep 14, 2017

divega commented Sep 14, 2017

pmiddleton commented Sep 14, 2017

divega commented Sep 14, 2017

pmiddleton commented Sep 14, 2017

divega commented Sep 14, 2017 •

edited

Loading

pmiddleton commented Sep 16, 2017

divega commented Sep 17, 2017 •

edited

Loading

pmiddleton commented Sep 17, 2017

pmiddleton commented Sep 22, 2017

smitpatel commented Sep 22, 2017

pmiddleton commented Sep 25, 2017

smitpatel Sep 25, 2017

pmiddleton Sep 25, 2017

smitpatel Sep 25, 2017

pmiddleton Sep 25, 2017

ajcvickers Sep 25, 2017

pmiddleton Sep 25, 2017

smitpatel commented Sep 25, 2017

pmiddleton commented Oct 3, 2017

ajcvickers commented Oct 3, 2017

pmiddleton commented Oct 3, 2017

smitpatel Oct 3, 2017

pmiddleton Oct 3, 2017

smitpatel Oct 3, 2017

smitpatel commented Oct 3, 2017

smitpatel commented Oct 4, 2017

DBFunctions - Add support for instance methods. #9755

DBFunctions - Add support for instance methods. #9755

Conversation

pmiddleton commented Sep 11, 2017

pmiddleton commented Sep 13, 2017

divega commented Sep 13, 2017 • edited Loading

pmiddleton commented Sep 13, 2017

pmiddleton commented Sep 14, 2017 • edited Loading

divega commented Sep 14, 2017

divega commented Sep 14, 2017

pmiddleton commented Sep 14, 2017

divega commented Sep 14, 2017

pmiddleton commented Sep 14, 2017

divega commented Sep 14, 2017

pmiddleton commented Sep 14, 2017

divega commented Sep 14, 2017 • edited Loading

pmiddleton commented Sep 16, 2017

divega commented Sep 17, 2017 • edited Loading

pmiddleton commented Sep 17, 2017

pmiddleton commented Sep 22, 2017

smitpatel commented Sep 22, 2017

pmiddleton commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Sep 25, 2017

pmiddleton commented Oct 3, 2017

ajcvickers commented Oct 3, 2017

pmiddleton commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Oct 3, 2017

smitpatel commented Oct 4, 2017

divega commented Sep 13, 2017 •

edited

Loading

pmiddleton commented Sep 14, 2017 •

edited

Loading

divega commented Sep 14, 2017 •

edited

Loading

divega commented Sep 17, 2017 •

edited

Loading