Add DbFunctionParameter support for DbFunctions #14990

pmiddleton · 2019-03-12T03:39:57Z

Adds the ability to map properties for DbFunction parameters. See #13752

smitpatel · 2019-03-15T18:17:48Z

@ajcvickers - Should DbFunction's parameter have ValueConverter also?
One weird issue which can come in the absence of it, is if you are passing a column vs a constant. They both would have different types.

pmiddleton · 2019-03-15T18:53:26Z

I need to review this PR this weekend to make sure I didn't miss anything.

I was not planning implementing the code which is going to use the new properties. Since the query pipeline is being rewritten it doesn't make sense to update the old one.

@smitpatel - Does the new query work live in the smit/query branch?

smitpatel · 2019-03-15T19:11:42Z

@pmiddleton - Yes, that is the branch. I was thinking the same that we can commit & merge API to master and new query pipeline will make use of it when rebased. It would be good if we have tests to verify the behavior but skipped in old pipeline.

ajcvickers · 2019-03-19T19:18:58Z

@smitpatel I think it needs to be possible to specify the actual type mapping to use (which could have a value converter on it) rather than only allowing the database type name. The database type name could be nice sugar for getting to the type mapping in common cases.

smitpatel · 2019-03-19T19:34:01Z

@pmiddleton - I discussed with @ajcvickers about ValueConverters area. Essentially query needs TypeMapping for parameters so that it can generate literal/parameter correctly in SQL. So IDbFunctionParameter should store RelationalTypeMapping (& also IDbFunction should store RelationalTypeMapping as return type).

Currently, there is no way to specify type mapping directly in fluent API. So for now we can have sugar method to specify database type and run a convention to assign type mapping whenever database type is set. TypeMappingConvention does the same for columns.

pmiddleton · 2019-03-19T20:32:58Z

@smitpatel

Does it make more sense to look up the mappings at query translation time vs storing it? I am not completely familiar with the RelationalTypeMapping but is is possible for the mappings to be changed at runtime after model creation? If so then we shouldn't be precomputing and storing them.

smitpatel · 2019-03-19T20:36:14Z

@pmiddleton - I believe we can differ that for now since IProperty also does not change TypeMapping at runtime.

pmiddleton · 2019-03-19T20:37:56Z

@smitpatel - Ok I will cache the values for parameters and return type, we can change to a realtime lookup later if needed.

pmiddleton · 2019-03-25T00:57:45Z

@smitpatel - Take a look now and let me know if this is what you had in mind for RelationalTypeMapping. If so I still need to add unit tests.

smitpatel · 2019-03-26T23:56:42Z

Requesting review from @ajcvickers for what to store in function parameter for type mapping.

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

ajcvickers · 2019-04-02T17:58:27Z

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

+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used 
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual string DatabaseType { get; set; }


Assuming this is the store type name, it probably doesn't need to be here in addition to the type mapping.

It is still appearing for me in codeflow review.

In reply to: 272869029 [](ancestors = 272869029)

pmiddleton · 2019-04-23T18:46:50Z

@smitpatel @ajcvickers - Rebased on latest master. Was there something else that needed changing for this PR before merging?

Sorry I'm in process of trying to buy a house and move this spring so I haven't had as much time as I would like to work on things.

ajcvickers · 2019-04-25T19:57:31Z

@smitpatel Are we still waiting for anything here?

smitpatel · 2019-04-25T21:10:54Z

src/EFCore/EFCore.csproj

@@ -1,11 +1,12 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <Description>Entity Framework Core is a lightweight and extensible version of the popular Entity Framework data access technology.
+    <Description>


Seems like unrelated change

smitpatel · 2019-04-25T21:13:38Z

                if (RelationalDependencies.TypeMappingSource.FindMapping(methodInfo.ReturnType) == null)

This should be outside of Translation == null check. Even if they are providing translation, they cannot pass in TypeMapping for the translated expression.
Also see below for parameter

Refers to: src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs:84 in a99735f. [](commit_id = a99735f, deletion_comment = False)

smitpatel · 2019-04-25T21:14:06Z

                    if (RelationalDependencies.TypeMappingSource.FindMapping(parameter.ParameterType) == null)

This should go away. All parameters on method info should be inside dbFunction.Parameters check below. And that loop should handle parameters

Refers to: src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs:94 in a99735f. [](commit_id = a99735f, deletion_comment = False)

smitpatel · 2019-04-25T21:14:57Z

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

+
+                    foreach (var parameter in dbFunction.Parameters)
+                    {
+                        if(RelationalDependencies.TypeMappingSource.FindMapping(parameter.FunctionType) == null)


We should first check if a typemapping is specified which can be mapped if not then only we should go and find mapping based on ClrType. That is applicable for both parameters and return value of actual function

@smitpatel - Based on what you have said across these comments there won't be anything inside of the Translation == null check. Is this appropriate for the new query pipeline? Every parameter should have a type mapping, even if the translation is done manually?

That is interesting question. Likely.

smitpatel · 2019-04-25T21:15:07Z

src/EFCore.Relational/Metadata/IDbFunction.cs

@@ -13,8 +14,7 @@ namespace Microsoft.EntityFrameworkCore.Metadata
    /// </summary>
    public interface IDbFunction
    {
-        /// <summary>
-        ///     The name of the function in the database.
+        /// <summary>,abase.


smitpatel · 2019-04-25T21:15:46Z

src/EFCore.Relational/Metadata/IDbFunctionParameter.cs

+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used 
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        Type FunctionType { get; }


Should just call this ClrType

src/EFCore.Relational/Metadata/IMutableDbFunctionParameter.cs

smitpatel · 2019-04-25T21:17:51Z

src/EFCore.Relational/Metadata/Builders/DbFunctionParameterBuilder.cs

+        /// <summary>
+        ///     Sets the name of the parameter
+        /// </summary>
+        public virtual DbFunctionParameterBuilder HasName([NotNull] string name)


Can parameter name be mutated? I would assume it would be same as parameter name you specify on your function.

This was leftover from when I originally wrote this a few years ago before I was auto pulling the values from the metadata.

That being said Oracle does support calling functions with named parameters. If for some reason the db function parameter name didn't match up with the code name then you could reset it. That assumes the backend Oracle provider would support this.

That can be an enhancement down the road and I will remove this.

smitpatel · 2019-04-25T21:18:27Z

src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs

@@ -74,6 +74,8 @@ public override ConventionSet CreateConventionSet()
            conventionSet.PropertyAddedConventions.Add(relationalColumnAttributeConvention);

            var sharedTableConvention = new SharedTableConvention(logger);
+            var dbFunctionTypeMappingConvention = new DbFunctionTypeMappingConvention(RelationalDependencies.TypeMappingSource);


Initialize closer to usage

smitpatel · 2019-04-25T21:18:53Z

src/EFCore.Relational/Metadata/Conventions/Internal/DbFunctionTypeMappingConvention.cs

+        /// </summary>
+        public virtual InternalModelBuilder Apply(InternalModelBuilder modelBuilder)
+        {
+            foreach (var dbFunc in modelBuilder.Metadata.Relational().DbFunctions.Cast<IMutableDbFunction>())


nit:Prefer full name like dbFunction rather than Func

smitpatel · 2019-04-25T21:19:32Z

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

@@ -81,7 +84,7 @@ public class DbFunction : IMutableDbFunction, IMethodCallTranslator
                && !typeof(DbContext).IsAssignableFrom(methodInfo.DeclaringType))
            {
                throw new ArgumentException(
-                    RelationalStrings.DbFunctionInvalidInstanceType(methodInfo.DisplayName(), methodInfo.DeclaringType.ShortDisplayName()));
+                    RelationalStrings.DbFunctionInvalidInstanceType(methodInfo.DisplayName(), methodInfo.DeclaringType?.ShortDisplayName()));


When would declaring type would be null?

That's what I thought as well, however Resharper was giving me a "possible null assignment warning". I added the ? to shut it up.

Add R# disable instead.

smitpatel · 2019-04-25T21:21:55Z

src/EFCore.Relational/Metadata/Builders/DbFunctionBuilder.cs

+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used 
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual DbFunctionParameterBuilder HasParameter([NotNull] string name)


@AndriySvyryd - Parameters are already defined looking at methodInfo. Here we want to get particular parameter to configure it. What should be right fluent way to do it? Calling HasParameter with name which is not any parameter in function throws

smitpatel

🕐

smitpatel · 2019-04-25T21:24:24Z

This actually does not add any API to configure the parameter & functions return type's database type. Let me discuss with team and get back to you in terms of API shape and data storage necessary.
Columns remember the annotation which specify string format of the type on database. If we don't store that info, I am not sure how would we get type mapping source to use that info to set the type mapping accurately.

ajcvickers · 2019-05-11T22:44:17Z

When merging, check if #13782 is still needed.

pmiddleton · 2019-05-14T03:21:06Z

@smitpatel Did you confirm with the team that a return type parameter mapping on dbfunction is required? If so I will get it added in.

smitpatel · 2019-05-14T15:37:55Z

@pmiddleton - I haven't got a chance yet. Give me about 2 more weeks to sort it out. Presently I am dealing with bigger chunks missing in new pipeline since it is blocking AspNetCore Identity to take new EF packages.

pmiddleton · 2019-05-14T19:31:33Z

@smitpatel - No problem. I am moving in 2 weeks so I don't have a ton of time right now myself. We will get this done in June.

smitpatel · 2019-06-14T00:50:53Z

Design meeting decision:
There will be API taking string type name only. Which would map to TypeMapping.
HasType is the name of API for parameters
HasReturnType for function.

smitpatel · 2019-06-14T00:57:36Z

@pmiddleton - Would you have chance to complete this in next 1 week? If not I can take over and make modifications to merge. Release deadlines to meet.

pmiddleton · 2019-06-17T18:29:48Z

@smitpatel - sorry to do this to you, but I'm not going to be able to get to this in that time frame. Too much stuff to deal with from my move. We are still trying to stop living out of boxes :)

smitpatel · 2019-06-17T19:00:31Z

@pmiddleton - No worries. I will wrap up the pending work.

smitpatel · 2019-06-21T00:22:50Z

@pmiddleton - I have created #16182
Thanks for contribution.

pmiddleton · 2019-06-21T01:34:39Z

@smitpatel Thanks for finishing this up for me. Sorry I wasn't able to get this done.

pmiddleton force-pushed the dbparameter branch from 1e6a57e to 9027514 Compare March 12, 2019 04:00

ajcvickers assigned smitpatel Mar 15, 2019

pmiddleton force-pushed the dbparameter branch from 9027514 to ab2f8ce Compare March 25, 2019 00:56

smitpatel requested a review from ajcvickers March 26, 2019 23:55

ajcvickers reviewed Apr 2, 2019

View reviewed changes

src/EFCore.Relational/Metadata/Internal/DbFunctionParameter.cs Show resolved Hide resolved

ajcvickers reviewed Apr 2, 2019

View reviewed changes

pmiddleton force-pushed the dbparameter branch from ab2f8ce to 570aa8b Compare April 8, 2019 02:23

pmiddleton changed the title ~~WIP - Add DbFunctionParameter support for DbFunctions~~ Add DbFunctionParameter support for DbFunctions Apr 8, 2019

pmiddleton force-pushed the dbparameter branch 4 times, most recently from 6159baa to 52746ef Compare April 8, 2019 03:05

pmiddleton force-pushed the dbparameter branch from 52746ef to 66062f6 Compare April 23, 2019 18:45

pmiddleton force-pushed the dbparameter branch from 66062f6 to a99735f Compare April 24, 2019 15:57

smitpatel reviewed Apr 25, 2019

View reviewed changes

src/EFCore.Relational/Metadata/IMutableDbFunctionParameter.cs Show resolved Hide resolved

smitpatel reviewed Apr 25, 2019

View reviewed changes

smitpatel suggested changes Apr 25, 2019

View reviewed changes

pmiddleton force-pushed the dbparameter branch from a99735f to 2644086 Compare May 14, 2019 03:22

Add DbFunctionParameter support for DbFunctions

835ef92

pmiddleton force-pushed the dbparameter branch from 2644086 to 835ef92 Compare May 14, 2019 03:52

smitpatel mentioned this pull request Jun 21, 2019

Add DbFunctionParameter support for DbFunctions #16182

Merged

smitpatel closed this Jun 21, 2019

Add DbFunctionParameter support for DbFunctions #14990

Add DbFunctionParameter support for DbFunctions #14990

Conversation

pmiddleton commented Mar 12, 2019

smitpatel commented Mar 15, 2019

pmiddleton commented Mar 15, 2019

smitpatel commented Mar 15, 2019

ajcvickers commented Mar 19, 2019

smitpatel commented Mar 19, 2019

pmiddleton commented Mar 19, 2019

smitpatel commented Mar 19, 2019

pmiddleton commented Mar 19, 2019

pmiddleton commented Mar 25, 2019

smitpatel commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmiddleton commented Apr 23, 2019

ajcvickers commented Apr 25, 2019

Choose a reason for hiding this comment

smitpatel commented Apr 25, 2019

smitpatel commented Apr 25, 2019

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

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

Choose a reason for hiding this comment

smitpatel left a comment

Choose a reason for hiding this comment

smitpatel commented Apr 25, 2019

ajcvickers commented May 11, 2019 • edited Loading

pmiddleton commented May 14, 2019

smitpatel commented May 14, 2019

pmiddleton commented May 14, 2019

smitpatel commented Jun 14, 2019 • edited Loading

smitpatel commented Jun 14, 2019

pmiddleton commented Jun 17, 2019

smitpatel commented Jun 17, 2019

smitpatel commented Jun 21, 2019

pmiddleton commented Jun 21, 2019

ajcvickers commented May 11, 2019 •

edited

Loading

smitpatel commented Jun 14, 2019 •

edited

Loading