Default Sql Server db functions to DBO schema #9303

pmiddleton · 2017-07-31T15:21:16Z

Set the the default schema for db functions in Sql Server to dbo.

Addresses #9214

smitpatel · 2017-07-31T15:30:42Z

src/EFCore.SqlServer/Metadata/Conventions/Internal/SqlServerDbFunctionConvention.cs

+                {  
+                    var dbFunctionBuilder = new InternalDbFunctionBuilder(dbFunction);
+
+                    dbFunctionBuilder.HasSchema("dbo", ConfigurationSource.Convention);


Should this default to "dbo" always or whatever is defined on model?

I'm not understanding your question. With this change if you don't set a schema it will default to dbo.

One thing I just thought of is if we should use the global schema if it is set or go with this default.

@anpete thoughts?

One thing I just thought of is if we should use the global schema if it is set or go with this default.

That's what I meant. If user has done
modelBuilder.HasDefaultSchema("MySchema");
Then should function go there?

Should it be IModelBuiltConvention?
So that once the model is fully built, plug in missing schemas in DbFunction based on default if set on model or "dbo"

Would there every be a reason someone would try to pull the schema back out of a function before finishing building the entire model? That is the only thing I can think of where waiting for IModelBuiltConvention would be different.

Suppose user defines a function without configuring schemas & then define the default schema.

Yea that would be worse. Ok ill refactor it tonight.

Ok I switched it over to IModelBuiltConvention I need to pop a few unit tests to deal with the defaultSchema vs defaulting to dbo still.

pmiddleton · 2017-08-02T05:32:53Z

Updated and unit tests added.

smitpatel · 2017-08-02T05:51:51Z

Can you add a functional test when default schema is specified which is different from dbo and function is defined on that schema and query picks up the schema from model correctly.

pmiddleton · 2017-08-02T15:29:55Z

Let me make sure I am following what you are asking for.

You want the default schema set to "abc" and the function set to "xyz" and then make sure the sql uses "xyz"? By functional test do you want it to actually hit a database and run a query?

smitpatel · 2017-08-02T15:57:44Z

Since the convention does not do anything if model has DefaultSchema set,
What if a function is defined with no schema & model has default schema, does it produce a valid SQL for it?
One of the easiest way to verify SQL validity is to actually hit the database and run the query.

smitpatel · 2017-08-02T16:00:34Z

I believe current functional tests covers when function is defined without schema and we use the "dbo"
We should have functional test for
function defined without schema & with model.defaultschema set and function uses that.
function defined with schema & with model.defaultschema set & function uses the schema from function only.
You can probably add new tests in QueryBugsSqlServerTest

pmiddleton · 2017-08-02T16:12:07Z

If there is no schema set on the function but a default schema is set then the default schema is used. If you don't want the default schema to be used at all then you need to set the schema on the function to an empty string.

For Sql Server that use case would only be for calling built in functions, all udf calls require a schema.

The logic order currently is

Use the schema set directly on the function. This schema can be an empty string.
Use the default schema.
Use dbo

Any function test that hit the db need to be in UdfDbFunctionSqlServerTests. That uses the Northwind database which has the setup script which we can use to create the functions in the database.

smitpatel · 2017-08-02T17:22:22Z

If there is no schema set on the function but a default schema is set then the default schema is used.

That's the scenario I want tested because I have doubts on that.

We added UdfDbFunctionSqlServerTests for UDF testing but we don't modify northwind.sql generally. At that time due to time restrictions we went that way. The issue is northwind is not re-created always.
For AppVeyor/Travis it works fine because we start with new VM which does not have any database at all so always northwind will be created from latest script.

For all local dev machines & CI machines which are not dropping database, this causes mismatch since there will be outdated northwind in sqlserver. And that leads to failure unless northwind database is manually dropped.

smitpatel · 2017-08-02T17:23:03Z

src/EFCore.SqlServer/Metadata/Conventions/Internal/SqlServerDbFunctionConvention.cs

+
+namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal
+{
+    public class SqlServerDbFunctionConvention : IModelBuiltConvention


@AndriySvyryd - Name sounds good to you?

pmiddleton · 2017-08-02T17:43:31Z

I don't see a QueryBugsSqlServerTest in the codebase.

Do you not want me to modify the northwind.sql and find a different approach for creating a new databsae and creating the function for each run?

smitpatel · 2017-08-02T17:54:07Z

Ah, its QueryBugsTest only.
Best would be to have UDF functions test class to use northwind model but use a different database which would get recreated every time so adding more testing around udf in future does not affect northwind database. But that may be more work. If you don't have enough time for it, feel free to put it in QueryBugsTest.

pmiddleton · 2017-08-02T18:15:47Z

I actually have a big family wedding the rest of this week/weekend so I won't be able to get these changes in until early next week.

smitpatel · 2017-08-03T00:27:28Z

@pmiddleton - No rush, we have a lot of time before next release 😄

pmiddleton · 2017-08-08T00:56:13Z

@smitpatel - I added the unit tests you were looking for. Let me know what you think.

smitpatel · 2017-08-08T01:11:49Z

@pmiddleton - Thanks I was looking for precisely that test since I was not able to understand how the default model schema was set on function. After looking more in code today I found out the RelationalDbFunctionConvention which sets name & schema from attribute or defaults.

Should RelationalDbFunctionConvention be ModelBuiltConvention like above rather than annotation changed convention? (it is only applied when the annotation is created & not changed). Then we may override it in SqlServer, to suggest further default to be "dbo". The downside is name & schema will be plugged in once the model is built. Not sure if that is critical to have. Upside lesser convention invocations.

What do you think?
also cc: @AndriySvyryd , @anpete

pmiddleton · 2017-08-08T18:28:43Z

It depends if we want the user to be able to read the name and schema, set via the attribute, during OnModelCreating. As it stands now you can, if we switch to a ModelBuiltConvention you won't be able to.

That would also be a minor change to end user behavior from 2.0 to 2.1.

smitpatel · 2017-08-08T20:38:19Z

The test in QueryBugsTest fail if function is defined using Attribute without specifying schema. The RelationalDbFunctionConvention sets schema to model.DefaultSchema but if the function is found using attribute then at that time OnModelCreating hasn't run so we don't know default schema hence it fails to set any schema.

Further, if user set default schema then define function then set default schema to something else then we are not updating the schema in db function. This is because RelationalDbFunctionConvention would run only when adding annotation for DbFunction first time. Since we follow last one win strategy, all our metadata object goes to schema defined 2nd time but the function goes to first schema causing error. This kind of setting default schema again would be used in scenario like extensible class library using EF (like Identity), when final consumer would want to set default schema to something else)

smitpatel · 2017-08-08T20:39:31Z

Though above is in existing code and out of the scope of PR.
@pmiddleton - Can you rebase on latest dev?

smitpatel · 2017-08-08T20:43:28Z

filed #9360

pmiddleton · 2017-08-08T21:25:27Z

Rebased.

pmiddleton · 2017-08-08T21:33:13Z

Based on your findings I'm leaning towards just applying all of the defaults in a ModelBuiltConvention.

We can make RelationalDbFunctionConvention a ModelBuiltConvention and have it deal with attribute & default schema. We can then have SqlServerDbFunctionConvention derive and override RelationalDbFunctionConvention to deal with dbo.

Thoughts?

smitpatel · 2017-08-08T21:34:20Z

@AndriySvyryd needs to approve it.

pmiddleton · 2017-08-11T14:48:49Z

I added a DbFunction.DefaultSchema and set that in the convention when the function is created. I also added some new unit tests. I think this gets us to where we want to be for #9360 and #9214

smitpatel · 2017-08-11T16:43:22Z

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

        public virtual string Schema
        {
-            get => _schema;
+            get => _schema ?? new RelationalModelAnnotations(_model).DefaultSchema ?? DefaultSchema;


would this be same as _model.Relational().DefaultSchema?

yes - I will switch it to add the abstraction back in.

AndriySvyryd · 2017-08-11T20:31:39Z

src/EFCore.SqlServer/Metadata/Conventions/Internal/SqlServerDbFunctionConvention.cs

+            Check.NotNull(modelBuilder, nameof(modelBuilder));
+            Check.NotNull(name, nameof(name));
+
+            if (name.StartsWith(RelationalAnnotationNames.DbFunction, StringComparison.OrdinalIgnoreCase)


Use StringComparison.Ordinal

Also in RelationalDbFunctionConvention.
Actually you can derive this from RelationalDbFunctionConvention and replace it in SqlServerConventionSetBuilder

pmiddleton · 2017-08-13T17:27:44Z

@AndriySvyryd - Changes made

smitpatel · 2017-08-13T17:44:46Z

src/EFCore.SqlServer/Metadata/Conventions/Internal/SqlServerDbFunctionConvention.cs

+            Check.NotNull(modelBuilder, nameof(modelBuilder));
+            Check.NotNull(name, nameof(name));
+
+            base.Apply(modelBuilder, name, annotation, oldAnnotation);


Calling base this way means the check is happening twice. Instead a helper function to do the task of setting name/schema which can be overridden in derived convention could be more DRY.

pmiddleton · 2017-08-15T16:09:52Z

@smitpatel - switched to a helper method.

smitpatel · 2017-08-15T17:19:18Z

Merged via 7529563

Thanks for contribution @pmiddleton

julielerman · 2018-04-19T18:50:15Z

Hey Paul et alia, reading the two issues on this, my understanding is that dbo is the conventional default schema. But (on EF Core 2.0.1) my queries using a function are failing (sql server returns function not found error) unless I explicitly set the schema to dbo in the function ala

 [DbFunction(Schema ="dbo")]
        public static string EarliestBattleFoughtBySamurai(int samuraiId)
        {
            throw new Exception();
        }

Am I misunderstanding something?
Thanks!

smitpatel · 2018-04-19T18:52:19Z

@julielerman - Yes, that would be expected behavior in 2.0.1. The issue is fixed in 2.1.0-preview1-final

julielerman · 2018-04-19T18:59:34Z

yeah .just found the note in #9663 :)
thanks!

pmiddleton · 2018-04-19T19:59:37Z

@julielerman - Yup I missed that one in 2.0.

pmiddleton · 2018-04-19T20:04:34Z

@julielerman - 2.1.0-preview1-final will also allow you to use instance methods as well as static methods for db functions. See #9755

julielerman · 2018-04-19T20:21:25Z

even with the current requirement for the schema specified, it's such a great feature. I had only read about it up until now but as I'm experimenting with it, I'm so impressed! :)

pmiddleton · 2018-04-19T20:22:53Z

Thanks!

Function quoting logic was changed (for now): all function names are quoted just like any other identifier (the default EF Core behavior is only to quote schema-less functions). This meant changing all expression translators to generate lower-case names (lower() instead of LOWER()), and also to add a hardcoded list of functions which EF Core generates in upper-case but which shouldn't get quoted (e.g. SUM()). See: * dotnet/efcore#8507 * dotnet/efcore#12044 * dotnet/efcore#12757 * dotnet/efcore#9558 * dotnet/efcore#9303

For now, only unquoted function names are supported (i.e. all lower- case, no special chars). Quoting logic for functions is quite complicated, see below issues for details. See: * dotnet/efcore#8507 * dotnet/efcore#12044 * dotnet/efcore#12757 * dotnet/efcore#9558 * dotnet/efcore#9303

dnfclas added the cla-already-signed label Jul 31, 2017

smitpatel reviewed Jul 31, 2017

View reviewed changes

pmiddleton force-pushed the defaultDBO branch 4 times, most recently from 1e29900 to 2e00bb2 Compare August 2, 2017 05:10

smitpatel reviewed Aug 2, 2017

View reviewed changes

pmiddleton force-pushed the defaultDBO branch from 2e00bb2 to 58ee9d1 Compare August 8, 2017 00:55

smitpatel self-assigned this Aug 8, 2017

pmiddleton force-pushed the defaultDBO branch from 58ee9d1 to dc65e9c Compare August 8, 2017 21:24

pmiddleton force-pushed the defaultDBO branch from 64dc9fd to 6f35837 Compare August 11, 2017 05:23

smitpatel requested a review from AndriySvyryd August 11, 2017 16:42

smitpatel reviewed Aug 11, 2017

View reviewed changes

pmiddleton force-pushed the defaultDBO branch from 6f35837 to b58ec40 Compare August 11, 2017 17:57

AndriySvyryd reviewed Aug 11, 2017

View reviewed changes

pmiddleton force-pushed the defaultDBO branch 2 times, most recently from c4aa055 to 9c04bb6 Compare August 13, 2017 17:22

smitpatel reviewed Aug 13, 2017

View reviewed changes

pmiddleton force-pushed the defaultDBO branch from 9c04bb6 to d3d8201 Compare August 15, 2017 15:38

Default Sql Server db functions to DBO schema

dea2ac8

pmiddleton force-pushed the defaultDBO branch from d3d8201 to dea2ac8 Compare August 15, 2017 15:50

smitpatel closed this Aug 15, 2017

pmiddleton deleted the defaultDBO branch August 15, 2017 17:56

roji mentioned this pull request Jul 22, 2018

UDFs and SqlFunction quoting and schema logic #12757

Closed

Default Sql Server db functions to DBO schema #9303

Default Sql Server db functions to DBO schema #9303

Conversation

pmiddleton commented Jul 31, 2017 • edited Loading

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

pmiddleton commented Aug 2, 2017

smitpatel commented Aug 2, 2017

pmiddleton commented Aug 2, 2017

smitpatel commented Aug 2, 2017

smitpatel commented Aug 2, 2017

pmiddleton commented Aug 2, 2017

smitpatel commented Aug 2, 2017

Choose a reason for hiding this comment

pmiddleton commented Aug 2, 2017

smitpatel commented Aug 2, 2017

pmiddleton commented Aug 2, 2017

smitpatel commented Aug 3, 2017

pmiddleton commented Aug 8, 2017

smitpatel commented Aug 8, 2017

pmiddleton commented Aug 8, 2017

smitpatel commented Aug 8, 2017

smitpatel commented Aug 8, 2017

smitpatel commented Aug 8, 2017

pmiddleton commented Aug 8, 2017

pmiddleton commented Aug 8, 2017

smitpatel commented Aug 8, 2017

pmiddleton commented Aug 11, 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

pmiddleton commented Aug 13, 2017

Choose a reason for hiding this comment

pmiddleton commented Aug 15, 2017

smitpatel commented Aug 15, 2017

julielerman commented Apr 19, 2018

smitpatel commented Apr 19, 2018

julielerman commented Apr 19, 2018

pmiddleton commented Apr 19, 2018

pmiddleton commented Apr 19, 2018

julielerman commented Apr 19, 2018 • edited Loading

pmiddleton commented Apr 19, 2018

pmiddleton commented Jul 31, 2017 •

edited

Loading

julielerman commented Apr 19, 2018 •

edited

Loading