Sync to 5.0.0-alpha.1.20073.3 #1234

roji · 2020-01-28T14:46:05Z

No description provided.

YohDeadfall

Partly reviewed the PR and will continue tomorrow.

YohDeadfall · 2020-01-29T20:29:37Z

src/EFCore.PG/Extensions/NpgsqlDbContextOptionsBuilderExtensions.cs

@@ -66,16 +98,39 @@ public static class NpgsqlDbContextOptionsExtensions
            var extension = (NpgsqlOptionsExtension)GetOrCreateExtension(optionsBuilder).WithConnection(connection);
            ((IDbContextOptionsBuilderInfrastructure)optionsBuilder).AddOrUpdateExtension(extension);

+            ConfigureWarnings(optionsBuilder);
+
            npgsqlOptionsAction?.Invoke(new NpgsqlDbContextOptionsBuilder(optionsBuilder));


These lines are repeated multiple times, so it's worth to make a private method for them.

You're right. But since this class is basically a copy-paste from EF Core, I prefer to just leave things like they are upstream... And there's not huge value here in the private method.

src/EFCore.PG/Extensions/NpgsqlDbContextOptionsBuilderExtensions.cs

src/EFCore.PG/Internal/NpgsqlLoggerExtensions.cs

YohDeadfall · 2020-01-29T20:36:18Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

-                    .Append(" INCLUDE (")
-                    .Append(ColumnList(includeColumns))
-                    .Append(")");
+                if (operation[NpgsqlAnnotationNames.IndexInclude] is string[] includeColumns &&


The nested condition can be safely merged with the outer since there are no returns on else blocks.

Done.

Note that I wasn't totally sure here about something - would be good to have your opinion (also @bricelam's).

In most cases, if something isn't supported, I prefer throwing an exception to make it clear to the user. In the case of index-included properties, for old versions which don't support this, for now I've decided to simply ignore - creating the index without the included properties. This is because included properties are an optional feature which only improves perf, and this makes the same model usable on any version of PG: with newer versions we get the speedup, with older versions we don't.

Does this make sense? Or is it better to throw on older versions?

Throwing or ignoring both seem fine to me. Did you consider logging a warning?

I keep forgetting those :) Ignoring and logging does sound like the way to go.

src/EFCore.PG/Scaffolding/Internal/DbDataReaderExtension.cs

YohDeadfall · 2020-01-29T20:40:13Z

src/EFCore.PG/Scaffolding/Internal/DbDataReaderExtension.cs

    {
        [DebuggerStepThrough]
-        public static T GetValueOrDefault<T>([NotNull] this DbDataReader reader, [NotNull] string name)
+        [return: MaybeNull]
+        internal static T GetValueOrDefault<T>(this DbDataReader reader, string name)


In which case a reader is not NpgsqlDataReader?

In our code I think it's always NpgsqlDataReader - but it doesn't really make a difference.. Any reason to change it?

Since NpgsqlDataReader is a final class the JIT could inline calls if they meet requirements. Another reason is that it's an internal extension method which is for Npgsql only, no need in an additional abstraction.

YohDeadfall · 2020-01-29T20:43:47Z

src/EFCore.PG/Scaffolding/Internal/NpgsqlDatabaseModelFactory.cs

@@ -215,15 +207,16 @@ public override DatabaseModel Create(DbConnection dbConnection, DatabaseModelFac
            {
                while (reader.Read())
                {
-                    var table = new DatabaseTable
+                    var schema = reader.GetValueOrDefault<string>("nspname");


So you're using extensions here. I have fixed all methods on the command to return an NpgsqlDataReader.

Yeah. Any reason to do any change?

See dotnet/efcore#16200 Follows 690ebc4540334c1d0938660a5960a25d4338acd4 in EF Core

roji · 2020-01-29T21:30:06Z

Partly reviewed the PR and will continue tomorrow.

No problem and thanks! Have addressed the comments you've already made.

YohDeadfall

Hope reviewed it carefully...

src/EFCore.PG/Scaffolding/Internal/NpgsqlDatabaseModelFactory.cs

YohDeadfall · 2020-01-31T21:44:49Z

src/EFCore.PG/Scaffolding/Internal/NpgsqlDatabaseModelFactory.cs

@@ -1083,6 +1033,7 @@ static SequenceInfo ReadSequenceInfo(DbDataRecord record, Version postgresVersio

        class SequenceInfo
        {
+            public SequenceInfo(string storeType) => StoreType = storeType;
            public string StoreType { get; set; }


Remove setter?

Just leaving it since everything else has a setter... It's just a temporarily values holder...

YohDeadfall · 2020-01-31T21:46:28Z

test/EFCore.PG.FunctionalTests/MigrationsInfrastructureNpgsqlTest.cs

+//            {
+//                DiffSnapshot(new BloggingContextModelSnapshot22(), context);
+//            }
+//


Why is it commented?

It was this way before too - I don't remember really. One day someone will look at it :)

YohDeadfall · 2020-01-31T21:51:08Z

test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs

+            AssertSql(
+                @"ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" TYPE text;
+ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" SET NOT NULL;
+ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" DROP DEFAULT;");


Why is this style of assertion used in some places, but in others you put different commands in multiple strings?

Unless I'm mistaken, when you see multiple strings with a comment separating them, this means the migrations generator is generating multiple ADO commands. In other cases (like here), multiple statements are batched in the same command. In general it doesn't make much of a difference (we don't care about network roundtrips in migrations), but it could be an issue where we need to suppress transactions, since that holds for the entire command.

YohDeadfall · 2020-01-31T21:51:26Z

test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs

+            AssertSql(
+                @"ALTER TABLE ""People"" DROP COLUMN ""Sum"";",
+                //
+                @"ALTER TABLE ""People"" ADD ""Sum"" integer GENERATED ALWAYS AS (""X"" + ""Y"") STORED;");


For example, there are two strings.

YohDeadfall · 2020-01-31T21:57:36Z

test/EFCore.PG.FunctionalTests/Query/NorthwindMiscellaneousQueryNpgsqlTest.cs

-
-        #region Helpers
+        protected override void ClearLog()
+            => Fixture.TestSqlLoggerFactory.Clear();


Why does EF implement this method as no op?

This is because TestSqlLoggerFactory is a relational-only thing (SQL), but the test base class isn't relational - it is used also for non-relational databases. So you can think about ClearLog as a general, "clear all logs" that isn't SQL specific, which providers must then override to clear the log in their particular way (for relational databases that means clearing SQL).

For test base classes which are relational only this can and should be implemented at the base class.

YohDeadfall · 2020-01-31T22:00:21Z

test/EFCore.PG.FunctionalTests/Query/SpatialQueryNpgsqlFixture.cs

        protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
        {
            base.OnModelCreating(modelBuilder, context);

            modelBuilder.HasPostgresExtension("postgis");
        }
+
+        //protected override bool CanExecuteQueryString => true;


Because we haven't implemented this yet, see #1232

YohDeadfall · 2020-01-31T22:01:06Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlDbContextOptionsBuilderExtensions.cs

    {
        public static NpgsqlDbContextOptionsBuilder ApplyConfiguration(this NpgsqlDbContextOptionsBuilder optionsBuilder)
        {
+            //optionsBuilder.ExecutionStrategy(d => new TestNpgsqlRetryingExecutionStrategy(d));


Why commented?

Because this introduces a lot of test breakage which I didn't want to deal with at this point. We should do this at some point.

Opened #1244 to track this.

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlDbContextOptionsBuilderExtensions.cs

YohDeadfall · 2020-01-31T22:03:00Z

test/EFCore.PG.Tests/Migrations/MigrationSqlGeneratorTestBase.cs

+                .Generate(operation, modelBuilder.Model);
+
+            Sql = string.Join(
+                "GO" + EOL + EOL,


Is see an SQL Server directive!

This is just a "delimiter" introduced in the tests to indicate a batch boundary (I've added a comment) - yes it's "inspired" by SQL Server :) Don't forget that these are just unit tests on the produced migrations DDL - nothing is executed on the database. Also note that this base class is copied as-is from EF Core, since there's no nuget package for the non-functional tests.

See dotnet/efcore#19321

Following dotnet/efcore#18786

…ontext Closes npgsql#1174 Following dotnet/efcore#8427

Redo migration tests to actually execute DDL against the database. Follows #19039

roji · 2020-02-01T11:09:57Z

Thanks for the review @YohDeadfall! Have answered all the comments above, if there's something you want more info on let me know.

roji requested review from austindrenski and YohDeadfall as code owners January 28, 2020 14:46

roji force-pushed the 5.0-sync branch 6 times, most recently from d37adcf to f95a1ec Compare January 28, 2020 16:53

roji mentioned this pull request Jan 28, 2020

Translate bytea length #1237

Merged

Depend on EF Core 5.0.0-alpha.1.20073.3

83af9be

roji force-pushed the 5.0-sync branch from f95a1ec to 43da308 Compare January 28, 2020 21:11

YohDeadfall reviewed Jan 29, 2020

View reviewed changes

Simple logging changes

d54d234

See dotnet/efcore#16200 Follows 690ebc4540334c1d0938660a5960a25d4338acd4 in EF Core

roji force-pushed the 5.0-sync branch from 43da308 to 5f30310 Compare January 29, 2020 21:28

YohDeadfall approved these changes Jan 31, 2020

View reviewed changes

roji mentioned this pull request Feb 1, 2020

Fixes around adding identity/serial columns #1242

Merged

roji added 5 commits February 1, 2020 12:04

NRT-related changes in scaffolding

60801c9

See dotnet/efcore#19321

Reorganize simple function tests

46fb7d8

Following dotnet/efcore#18786

Allow DbConnection or connection string to be changed on existing DbC…

4e24c89

…ontext Closes npgsql#1174 Following dotnet/efcore#8427

Various minor test changes

2127c86

Functional migration tests

039e86b

Redo migration tests to actually execute DDL against the database. Follows #19039

roji force-pushed the 5.0-sync branch from 5f30310 to 039e86b Compare February 1, 2020 11:09

roji merged commit 1fe05c6 into npgsql:dev Feb 1, 2020

roji deleted the 5.0-sync branch February 1, 2020 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync to 5.0.0-alpha.1.20073.3 #1234

Sync to 5.0.0-alpha.1.20073.3 #1234

roji commented Jan 28, 2020

YohDeadfall left a comment

YohDeadfall Jan 29, 2020

roji Jan 29, 2020

YohDeadfall Jan 29, 2020

roji Jan 29, 2020

bricelam Jan 30, 2020

roji Jan 30, 2020

YohDeadfall Jan 29, 2020

roji Jan 29, 2020 •

edited

Loading

YohDeadfall Jan 31, 2020

YohDeadfall Jan 29, 2020

roji Jan 29, 2020

YohDeadfall Jan 31, 2020

roji commented Jan 29, 2020

YohDeadfall left a comment

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

YohDeadfall Jan 31, 2020

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

roji Feb 1, 2020

YohDeadfall Jan 31, 2020

roji Feb 1, 2020

roji commented Feb 1, 2020

Sync to 5.0.0-alpha.1.20073.3 #1234

Sync to 5.0.0-alpha.1.20073.3 #1234

Conversation

roji commented Jan 28, 2020

YohDeadfall left a 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

roji Jan 29, 2020 • 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

roji commented Jan 29, 2020

YohDeadfall left a 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

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

roji commented Feb 1, 2020

roji Jan 29, 2020 •

edited

Loading