Create explicit index object for Keys & ForeignKeys #3714

smitpatel · 2015-11-10T22:53:07Z

Resolves #700
Key & ForeignKey has Index object associated. IsUnique and IsClustered from key/foreignkey moved to index object.
Need review from @bricelam for Migrations & @AndriySvyryd for metadata.

AndriySvyryd · 2015-11-10T23:06:54Z

src/EntityFramework.Core/Metadata/IForeignKey.cs

+        /// </summary>
+        IIndex Index { get; }
+
+        /// <summary>
        ///     Gets a value indicating whether the values assigned to the foreign key properties are unique.
        /// </summary>
        bool IsUnique { get; }


IsUnique should be removed

I don't remember it being the intention that foreign keys, which define relationships in our metadata, would no longer control whether the relationship is 1:1 or 1:*. Does this mean that you can no longer have a 1:1 relationship in the model without also having an index?

I think we should discuss in the design meeting.

Foreign key does control the relationship is 1:1 or 1:* but it won't store the information with in itself. It will modify the associated index uniqueness appropriately. So you cannot have unique & non-unique FKs using same set of properties ( #3551 enables that scenario). It also mean that you can no longer have 1:1 relationship in the model without having unique index on same properties which is present situation in migration and database. All our relationships in database are non-unique regardless of FK's uniqueness. Correcting it exposed #3648 because we were inserting duplicate values in unique FK temporarily.

I will bring this to design meeting for further discussion.

bricelam · 2015-11-11T17:26:24Z

Migrations changes look good.

natemcmaster · 2015-11-11T18:13:17Z

test/EntityFramework.Sqlite.Design.FunctionalTests/SqliteDatabaseModelFactoryTest.cs

@@ -251,7 +251,8 @@ FOREIGN KEY (BuddyId) REFERENCES Friends(Id)
            Assert.Equal(table.FindPrimaryKey(), fk.PrincipalKey);
        }

-        [Fact]
+        // TODO: See issue #3710
+        //[Fact]


😢 Can you add this to #3710 to ensure it is re-enabled?

it is reported on #3642 which is superseded by #3710

MarcoLoetscher · 2015-11-11T20:11:51Z

Please note the following issues:
EF creates index subsumed by clustered key
EF creates unnecessary index
Too many indexes on One-to–Zero-or-One Relationship
Thank you

smitpatel · 2015-11-11T20:59:36Z

@MarcoLoetscher - regarding issues you mentioned,

As for unnecessary indexes, there won't be any. That is the reason for this code change. Essentially now every key & foreign key will have index object associated with it. And there can be only one index on a fixed set of properties. So if a key & a foreign key uses the same set of properties, there will be only one index for both of them. That should resolve out last 2 issues where PK & FK is on same set of properties. We will be creating only one index for that.
For the index subsumed by clustered key - By looking at the issue, I suppose the index on fk property is unnecessary because it is part of clustered key. Clustered key is SQL specific default for PK. Since EF7 targets other datastores also, the index on FK property may not be unnecessary.

lajones · 2015-11-11T23:00:06Z

src/EntityFramework.Sqlite.Design/Internal/SqliteDmlParser.cs

@@ -75,7 +75,8 @@ public static void ParseColumnDefinition(TableModel table, string statement)
                return;
            }

-            if (statement.IndexOf(" UNIQUE", StringComparison.OrdinalIgnoreCase) > 0)
+            if (statement.IndexOf(" UNIQUE", StringComparison.OrdinalIgnoreCase) > 0
+                || statement.IndexOf(" PRIMARY KEY", StringComparison.OrdinalIgnoreCase) > 0)


I believe these changes fix #3642. Could you update that issue as well? The Design changes look good to me but I would get others to check the other changes.

AndriySvyryd · 2015-11-12T01:22:10Z

pending design discussion

natemcmaster · 2015-11-12T17:18:04Z

src/EntityFramework.Sqlite.Design/Internal/SqliteDmlParser.cs

@@ -90,7 +91,8 @@ public static void ParseColumnDefinition(TableModel table, string statement)
        public static void ParseConstraints(TableModel table, string statement)
        {
            var constraint = statement.Split(' ', '(')[0];
-            if (constraint.Equals("UNIQUE", StringComparison.OrdinalIgnoreCase))
+            if (constraint.Equals("UNIQUE", StringComparison.OrdinalIgnoreCase)
+                || statement.Equals(" PRIMARY KEY", StringComparison.OrdinalIgnoreCase))


Tests? I don't think this will work because of the space at the beginning of the string literal.

smitpatel · 2015-11-17T22:44:40Z

closing this in favour of #3764

dnfclas added the cla-already-signed label Nov 10, 2015

smitpatel added the needs-review label Nov 10, 2015

smitpatel force-pushed the indexObject branch 2 times, most recently from 5cd1f5a to 8116f62 Compare November 10, 2015 23:06

AndriySvyryd reviewed Nov 10, 2015
View reviewed changes

smitpatel force-pushed the indexObject branch 2 times, most recently from a6f44b0 to cb53a5f Compare November 11, 2015 01:10

natemcmaster reviewed Nov 11, 2015
View reviewed changes

natemcmaster mentioned this pull request Nov 11, 2015

Review how we create/handle indexes in Reverse Engineer #3710

Closed

smitpatel force-pushed the indexObject branch 2 times, most recently from 039b158 to 0b89341 Compare November 11, 2015 22:46

lajones reviewed Nov 11, 2015
View reviewed changes

smitpatel mentioned this pull request Nov 11, 2015

RevEng: Sqlite: Indexes for PK are scaffolded wrongly #3642

Closed

smitpatel force-pushed the indexObject branch from 0b89341 to f5df44f Compare November 12, 2015 00:04

Create explicit index object for Keys & ForeignKeys

5651d3e

smitpatel force-pushed the indexObject branch from f5df44f to 5651d3e Compare November 12, 2015 01:09

AndriySvyryd removed the needs-review label Nov 12, 2015

natemcmaster reviewed Nov 12, 2015
View reviewed changes

smitpatel closed this Nov 17, 2015

bricelam deleted the indexObject branch November 30, 2015 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create explicit index object for Keys & ForeignKeys #3714

Create explicit index object for Keys & ForeignKeys #3714

smitpatel commented Nov 10, 2015

AndriySvyryd Nov 10, 2015

ajcvickers Nov 11, 2015

smitpatel Nov 12, 2015

bricelam commented Nov 11, 2015

natemcmaster Nov 11, 2015

smitpatel Nov 11, 2015

smitpatel Nov 11, 2015

natemcmaster Nov 12, 2015

MarcoLoetscher commented Nov 11, 2015

smitpatel commented Nov 11, 2015

lajones Nov 11, 2015

AndriySvyryd commented Nov 12, 2015

natemcmaster Nov 12, 2015

smitpatel commented Nov 17, 2015

Create explicit index object for Keys & ForeignKeys #3714

Create explicit index object for Keys & ForeignKeys #3714

Conversation

smitpatel commented Nov 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam commented Nov 11, 2015

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

MarcoLoetscher commented Nov 11, 2015

smitpatel commented Nov 11, 2015

Choose a reason for hiding this comment

AndriySvyryd commented Nov 12, 2015

Choose a reason for hiding this comment

smitpatel commented Nov 17, 2015