Add SeedData #9772

AndriySvyryd · 2017-09-12T06:54:10Z

Fixes #629

Further improvements will be done in separate PRs

smitpatel · 2017-09-12T18:44:29Z

Rebase needed.

AndriySvyryd · 2017-09-12T18:46:20Z

I'll rebase when the PR is approved. Incremental rebases are time-consuming as I cannot rebase the original commit.

ajcvickers · 2017-09-12T21:52:38Z

src/EFCore/ChangeTracking/Internal/StateManager.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 IReadOnlyList<InternalEntityEntry> GetEntriesToSave()


public virtual IReadOnlyList GetEntriesToSave() [](start = 8, length = 68)

Should this really be exposed publicly? What ends up using the InternalEntityEntries? Is it provider code at all?

IStateManager is an Internal service

I was just concerned that it might be consumed by code that could be overridden by a provider--the same reason we introduced IUpdateEntry for the update pipeline. Looks like this isn't the case, so no problem. However, did we consider returning IUpdateEntrys instead?

I'll change it to return IReadOnlyList<IUpdateEntry>

ajcvickers · 2017-09-12T21:55:33Z

src/EFCore/ChangeTracking/Internal/StateManager.cs

+
+            entry.ToEntityEntry().CurrentValues.SetValues(values);
+            return entry;
+        }


This method seems like it needs some work. There is duplicated logic here with other GetOrCreate methods, and this could make better use of the materializer.

ajcvickers · 2017-09-12T21:59:15Z

src/EFCore/ChangeTracking/Internal/KeyPropagator.cs

+                        // Remove when issue #749 is fixed
+                        var principal = foreignKey.DependentToPrincipal?.IsShadowProperty ?? true
+                            ? null
+                            : foreignKey.DependentToPrincipal.GetGetter().GetClrValue(entry.Entity);
                        InternalEntityEntry principalEntry = null;


What's the consequence of just skipping propagation here?

ajcvickers · 2017-09-12T22:02:50Z

src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs

+                return propertyBase is INavigation && propertyBase.IsShadowProperty // Remove when issue #749 is fixed
+                    ? null
+                    : _storeGeneratedValues.TryGetValue(propertyBase, out var value)
+                        ? value


I'm concerned in general about the hacks for shadow nav props--for example, how they may influence other code paths rather than just seeding. Do we have a plan to either fix #749 in 2.1, or make the hacks less hacky?

I don't think that these hacks have any functional impact on non-seeding scenarios. If we have time to do #749 in 2.1 I would prefer it instead of trying to improve the hacks further.

ajcvickers · 2017-09-12T22:11:09Z

src/EFCore.Relational/breakingchanges.netcore.json

+      "TypeId": "public class Microsoft.EntityFrameworkCore.Migrations.Operations.UpdateDataOperation : Microsoft.EntityFrameworkCore.Migrations.Operations.MigrationOperation",
+      "Kind": "Removal"
+    },
+    {


These are here because we added a base class? So, not really breaks?

Yeah, seems to be a bug in the API tool

ajcvickers · 2017-09-12T22:13:38Z

src/EFCore.Relational/Update/ColumnModification.cs

@@ -127,15 +163,26 @@ public virtual string OriginalParameterName
        /// <summary>
        ///     The original value of the property mapped to this column.
        /// </summary>
-        public virtual object OriginalValue => Entry.GetOriginalValue(Property);
+        public virtual object OriginalValue => Entry == null ? _originalValue : Entry.GetOriginalValue(Property);


Entry == null [](start = 47, length = 13)

What is it that allows Entry to be null now?

The new constructor.
This is needed because when generating SQL when applying a migration we might not have an IModel anymore

ajcvickers

but make sure we follow up on the state manager changes stuff and the shadow nav stuff before we ship. Maybe create issues if needed?

… dev

AndriySvyryd · 2017-09-13T01:05:47Z

I've added a todo list to #629 to track the outsanding issues roughly in decreasing priority order. Let me know if I missed anything.

bricelam

Sorry for the late review.

bricelam · 2017-09-13T16:27:58Z

src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs

+                            Table = c.TableName,
+                            KeyColumns = c.ColumnModifications.Select(col => col.ColumnName).ToArray(),
+                            KeyValues = ToMultidimensionalArray(c.ColumnModifications.Select(col => col.Value).ToArray())
+                        };


We should look into accumulating these. Instead of creating one per row, if the last operation was the same type for the same table, we should just append the values.

Yes, it's on the ToDo list: #629

bricelam · 2017-09-13T16:29:34Z

src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs

+            if (source.EntityTypes.Count != 1 || target.EntityTypes.Count != 1)
+            {
+                // Table splitting not supported
+                return;


Should it throw or is is made impossible at a higher level?

It's on the ToDo list: #629

bricelam · 2017-09-13T16:32:02Z

src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs

-            => Diff(source, target, new DiffContext(source, target)).Any();
+        {
+            var result = Diff(source, target, new DiffContext(source, target)).Any();
+            StateManager.ResetState();


Could you hit issues if you call HasDifferences during SaveChanges? This seems like a dangerous side affect...

You mean concurrently? We definitely don't support that.

I imagined something like this:

db.Add(customer); if (differ.HasDiferences(modelSnapshot, db.Model) Log.Warn("Someone forgot to migrate. This might fail."); db.SaveChanges();

Still doesn't look like an important enough scenario, but I am open to hearing opinions from others

cc @divega @ajcvickers

Coming very late to this comment, but I need more context. It seems it would be good to discuss it when @bricelam is present. @AndriySvyryd can you please file something so that we remember?

I already fixed this in the new PR that's currently open

bricelam · 2017-09-13T16:37:01Z

src/EFCore.Relational/Migrations/Operations/ModificationOperation.cs

+        ///     The commands that correspond to this operation.
+        /// </summary>
+        public virtual IEnumerable<ModificationCommand> ModificationCommands
+            => _modificationCommands ?? (_modificationCommands = GenerateModificationCommands());


This doesn't feel like a property to me. Lets discuss during API review.

No need to discuss, I'll just make it a method

bricelam · 2017-09-13T16:42:07Z

src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs


            builder
-                .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator)


So the last statement is in a separate batch now?

Appears so. I'll revert this when I add batching back

bricelam · 2017-09-13T16:45:26Z

test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs

+                        x.ToTable("Firefly", "dbo");
+                        x.Property<int>("Id");
+                        x.HasKey("Id");
+                    }),


This looks like a bad merge.

Looks more like a bad PR differ

No, the old code didn't have HasKey() and used chaining (not a closure) on the target model.

bricelam · 2017-09-13T16:47:03Z

test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs

+                    {
+                        x.ToTable("Firefly", "dbo");
+                        x.Property<int>("Id");
+                        x.HasKey("Id");


Unnecessary HasKey() calls were removed from these tests.

Squashing the Data Seed work for easier rebasing

6d9007d

AndriySvyryd requested review from bricelam and ajcvickers September 12, 2017 06:54

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

ajcvickers reviewed Sep 12, 2017

View reviewed changes

ajcvickers approved these changes Sep 12, 2017

View reviewed changes

Merge branch 'dev' of https://github.com/tinchou/EntityFramework into…

6187ade

… dev

AndriySvyryd force-pushed the Seed_squashed branch from b0fb0e4 to 6187ade Compare September 13, 2017 00:36

AndriySvyryd merged commit 6187ade into dev Sep 13, 2017

AndriySvyryd deleted the Seed_squashed branch September 13, 2017 01:09

AndriySvyryd mentioned this pull request Sep 13, 2017

Migrations: Leverage IUpdateSqlGenerator for CUD operations #8569

Closed

bricelam reviewed Sep 13, 2017

View reviewed changes


		builder
		.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator)

Add SeedData #9772

Add SeedData #9772

Conversation

AndriySvyryd commented Sep 12, 2017

smitpatel commented Sep 12, 2017

AndriySvyryd commented Sep 12, 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

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

ajcvickers left a comment

Choose a reason for hiding this comment

AndriySvyryd commented Sep 13, 2017

bricelam 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

bricelam Sep 13, 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

bricelam Sep 13, 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

bricelam Sep 13, 2017 •

edited

Loading

bricelam Sep 13, 2017 •

edited

Loading