Feature: 2.1: Initial impl. of Query Types. #10647

anpete · 2018-01-04T21:47:00Z

Adds Query Types to the type system. Query Types are like Entity Types but do not require a key to be defined and can only be used for querying. Query Types allow for ad-hoc querying (like anonymous types), but are more flexible because they allow mapping configuration to be specified. E.g. ToTable, HasColumnName etc. Many advanced mapping capabilities are supported, too, such as inheritance mapping and navigations (dependent -> principal only).

#3932, #9290

ErikEJ · 2018-01-05T09:53:13Z

Formerly known a ViewType??

ajcvickers · 2018-01-05T17:53:39Z

src/EFCore/DbView`.cs

+    ///     </para>
+    /// </summary>
+    /// <typeparam name="TQuery"> The type of view being operated on by this view. </typeparam>
+    public abstract class DbQuery<TQuery>


DbQuery [](start = 26, length = 7)

Rename file to match type name

The rename was pretty heinous 😄

ajcvickers · 2018-01-05T17:55:55Z

src/EFCore/Extensions/EntityTypeExtensions.cs

+        /// </summary>
+        /// <param name="entityType"> The base type to find types that derive from. </param>
+        /// <returns> The derived types. </returns>
+        public static bool IsQueryType([NotNull] this IEntityType entityType)


Copy paste doc error

ajcvickers · 2018-01-05T17:58:35Z

src/EFCore/Internal/InternalDbView.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 class InternalDbQuery<TQuery> :


InternalDbQuery [](start = 17, length = 15)

Rename file

ajcvickers · 2018-01-05T17:59:10Z

src/EFCore/Metadata/Builders/ViewTypeBuilder.cs

+    ///         and it is not designed to be directly constructed in your application code.
+    ///     </para>
+    /// </summary>
+    public class QueryTypeBuilder : IInfrastructure<IMutableModel>, IInfrastructure<InternalEntityTypeBuilder>


Rename file here and generic

ajcvickers · 2018-01-05T18:03:45Z

src/EFCore/Metadata/Internal/Model.cs

+            var entityType = new EntityType(type, this, ConfigurationSource.Explicit);
+
+            var keyProperty = entityType.AddProperty("__QueryTypeKey__", typeof(int), ConfigurationSource.Convention);
+


What is the reason for adding the fake key here? Does it ever get set to any value, or used in some other way?

This may have been an early requirement to work around the validator. Will try and remove.

ajcvickers · 2018-01-05T18:07:02Z

src/EFCore.Specification.Tests/DatabindingTestBase.cs

@@ -450,7 +450,7 @@ public virtual void Adding_entity_to_local_view_that_is_already_in_the_state_man
        [Theory]
        [InlineData(false)]
        [InlineData(true)]
-        public virtual void Adding_entity_to_state_manager_of_different_type_than_local_view_type_has_no_effect_on_local_view(


Adding_entity_to_state_manager_of_different_type_than_local_view_type_has_no_effect_on_local_view [](start = 28, length = 97)

Inappropriate rename?

ajcvickers · 2018-01-05T18:11:22Z

A few questions:

Does there need to be more test coverage for different types of queries--joins, etc?
Can you add a test to WithConstructors... to test creating query types that have parameterized constructors
Can query types have owned types?

anpete · 2018-01-05T18:23:47Z

@ErikEJ Yes!

pmiddleton · 2018-01-08T05:40:39Z

src/EFCore/Metadata/Internal/Model.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 EntityType AddQueryType([NotNull] Type type)


Can this be added to IMutableModel? I have been working on table value function support based off your vt-new-* branches and need access to this method.

@ajcvickers This would be a break.

Is the version after 2.1 a breakable change version? I think some of the other changes for TVF will introduce some breaks as well so it might be better to hold off. I'm also not sure of the current timeline for 2.1, but it seems to be getting close, and I don't want to force something if I can't make that deadline.

Not sure at this stage. I would still try to target 2.1 if you think it's feasible. Include the breaks you would like to make and we can deal with them on a case-by-case basis. - We can often put it workarounds in the short term or suggest alternatives.

anpete · 2018-01-08T20:59:55Z

@ajcvickers

Does there need to be more test coverage for different types of queries--joins, etc?

Yes. Hoping to get a hand from @maumar on this.

Can you add a test to WithConstructors... to test creating query types that have parameterized constructors

Done.

Can query types have owned types?

No. That would imply identity.

maumar · 2018-01-08T22:25:23Z

I'm planning to test this extensively as soon as I'm done with correlated collections (hopefully one last issue remaining)

anpete · 2018-01-08T22:42:54Z

@maumar Thanks...I think.

anpete · 2018-01-08T22:46:22Z

@ajcvickers Fake key was removed. This made me think about your question on Owned Types. I'm guessing you were getting at nested objects on Query Types? If so, yeah, feels like we should be able to make it work from the query side. cc @AndriySvyryd for insight here 😄

anpete · 2018-01-09T18:27:27Z

@bricelam Can you take a look at the minor Migrations changes?
@AndriySvyryd Can you take a look at the less minor CF changes?

AndriySvyryd · 2018-01-09T19:59:07Z

src/EFCore/Infrastructure/Annotatable.cs

-            SetAnnotation(name, annotation);
-
-            if (previousLength == _annotations.Value.Count)
+            if (FindAnnotation(name) != null)


Why would you do a double lookup?

What happens if conventions change annotations but _annotations.Count is unchanged?

Conventions use SetAnnotation instead of AddAnnotation

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

Discussed offline. Problem happens when a convention removes an annotation as a result of a different annotation being added.

AndriySvyryd · 2018-01-09T20:01:39Z

src/EFCore/Infrastructure/ModelCustomizer.cs

@@ -61,7 +61,14 @@ protected virtual void FindSets([NotNull] ModelBuilder modelBuilder, [NotNull] D
        {
            foreach (var setInfo in Dependencies.SetFinder.FindSets(context))
            {
-                modelBuilder.Entity(setInfo.ClrType);
+                if (!setInfo.IsQueryType)


Invert condition

AndriySvyryd · 2018-01-09T20:02:35Z

src/EFCore/Infrastructure/ModelValidator.cs

+            var entityTypeWithNullPk
+                = model.GetEntityTypes()
+                    .Where(et => !et.IsQueryType())
+                    .FirstOrDefault(et => et.FindPrimaryKey() == null);


Combine conditions

AndriySvyryd · 2018-01-09T20:15:54Z

src/EFCore/Internal/DbSetInitializer.cs

-                setInfo.Setter.SetClrValue(context, ((IDbSetCache)context).GetOrAddSet(_setSource, setInfo.ClrType));
+                setInfo.Setter.SetClrValue(
+                    context,
+                    !setInfo.IsQueryType


Invert condition

AndriySvyryd · 2018-01-09T20:18:52Z

src/EFCore/Metadata/Builders/QueryTypeBuilder.cs

+        }
+
+        /// <summary>
+        ///     Sets the base type of this view in an inheritance hierarchy.


Some leftover references to "view" in this file

AndriySvyryd · 2018-01-09T20:19:33Z

src/EFCore/Metadata/Builders/QueryTypeBuilder`.cs

+            (QueryTypeBuilder<TQuery>)base.HasAnnotation(annotation, value);
+
+        /// <summary>
+        ///     Sets the base type of this view in an inheritance hierarchy.


And here too

AndriySvyryd · 2018-01-09T20:25:48Z

src/EFCore/Metadata/Builders/QueryTypeBuilder.cs

+            Check.NotEmpty(relatedTypeName, nameof(relatedTypeName));
+            Check.NullButNotEmpty(navigationName, nameof(navigationName));
+
+            var relatedEntityType = Builder.Metadata.FindInDefinitionPath(relatedTypeName) ??


Will we support weak query types? If not then you can omit the FindInDefinitionPath call.

Will remove.

AndriySvyryd · 2018-01-09T20:32:54Z

src/EFCore/Metadata/Conventions/Internal/ForeignKeyPropertyDiscoveryConvention.cs

@@ -55,6 +56,7 @@ public virtual InternalRelationshipBuilder Apply(InternalRelationshipBuilder rel
                // Try to invert if one to one or can be converted to one to one
                else if ((foreignKey.IsUnique
                          || foreignKey.PrincipalToDependent == null)
+                         && !foreignKey.DeclaringEntityType.IsQueryType()


foreignKey.DeclaringEntityType.IsQueryType() [](start = 29, length = 44)

Do we support relationships between query types?

Not currently.

AndriySvyryd · 2018-01-09T20:36:54Z

src/EFCore/Extensions/EntityTypeExtensions.cs

+        /// </summary>
+        /// <param name="entityType"> The entity type. </param>
+        /// <returns> true if the entity type is a query type; otherwise false. </returns>
+        public static bool IsQueryType([NotNull] this IEntityType entityType)


This should be a property on IEntityType

AndriySvyryd · 2018-01-09T20:40:00Z

src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs

+            {
+                throw new InvalidOperationException(
+                    CoreStrings.ErrorNavCannotTargetQueryType(targetEntityTypeBuilder.Metadata.DisplayName()));
+            }


Move this to ForeignKey.Navigation

AndriySvyryd · 2018-01-09T20:44:17Z

src/EFCore/Metadata/Internal/InternalModelBuilder.cs

-            [NotNull] Type type, ConfigurationSource configurationSource)
-            => Entity(new TypeIdentity(type), configurationSource);
+            [NotNull] Type type, ConfigurationSource configurationSource, bool throwOnQuery = false)
+            => Entity(new TypeIdentity(type), configurationSource, throwOnQuery);


Separate the throwOnQuery = false overload as something like EntityOrQuery. You can do this as a separate PR if you want to keep the size down.

@AndriySvyryd - Is this needed? If yes then file an issue.

AndriySvyryd · 2018-01-09T21:23:53Z

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

+                        entityType.RemoveAnnotation(CoreAnnotationNames.DefiningQuery);
+                        break;
+                    case CoreAnnotationNames.DefiningQuery:
+                        entityType.RemoveAnnotation(RelationalAnnotationNames.TableName);


This should only remove annotations added by conventions. Explicit annotations should be removed or trigger an exception in the extension methods themselves.

Removed convention.

AndriySvyryd · 2018-01-09T21:26:37Z

src/EFCore.Relational/Query/ExpressionVisitors/RelationalEntityQueryableExpressionVisitor.cs

-                            out var typeIndexMap).Compile();
+                            out var typeIndexMap);
+
+                var materializer = materializerExpression.Compile();


Why was this separated?

Easier debugging.

AndriySvyryd · 2018-01-09T21:30:00Z

test/EFCore.Tests/DbContextTest.cs

@@ -621,7 +622,7 @@ public override void DetectChanges(IStateManager stateManager)
            await Assert.ThrowsAsync<ObjectDisposedException>(() => context.FindAsync(typeof(Random), 77));

            var methodCount = typeof(DbContext).GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly).Count();
-            var expectedMethodCount = 41;
+            var expectedMethodCount = 42;


Finally we've reached the perfect API size

AndriySvyryd · 2018-01-09T21:32:41Z

test/EFCore.Tests/ModelBuilding/ModelBuilderTestBase.cs

@@ -134,6 +134,10 @@ public TestModelBuilder HasAnnotation(string annotation, object value)
            public abstract TestModelBuilder Entity<TEntity>(Action<TestEntityTypeBuilder<TEntity>> buildAction)
                where TEntity : class;

+            public virtual QueryTypeBuilder<TQuery> Query<TQuery>()
+                where TQuery : class
+                => ModelBuilder.Query<TQuery>();


Don't forget to test the non-generic version

bricelam

Migrations changes look good to me

Adds Query Types to the type system. Query Types are like Entity Types but do not require a key to be defined and can only be used for querying. Query Types allow for ad-hoc querying (like anonymous types), but are more flexible because they allow mapping configuration to be specified. E.g. ToTable, HasColumnName etc. Many advanced mapping capabilities are supported, too, such as inheritance mapping and navigations (dependent -> principal only).

smitpatel · 2018-02-07T23:17:28Z

src/EFCore/Query/EntityQueryModelVisitor.cs

@@ -579,7 +590,8 @@ var lastTrackingModifier
                    .OfType<TrackingResultOperator>()
                    .LastOrDefault();

-            return !(queryModel.GetOutputDataInfo() is StreamedScalarValueInfo)
+            return !_modelExpressionApplyingExpressionVisitor.IsViewTypeQuery


left over of ViewType

smitpatel · 2018-02-07T23:19:53Z

src/EFCore.Relational/RelationalViewTypeBuilderExtensions.cs

+    /// <summary>
+    ///     Relational database specific extension methods for <see cref="QueryTypeBuilder" />.
+    /// </summary>
+    public static class RelationalQueryTypeBuilderExtensions


File name mismatch

smitpatel · 2018-02-07T23:30:05Z

��U�s�i�n�g� �K�o�r�e�B�u�i�l�d� �2�.�1�.�0�-�p�r�e�v�i�e�w�1�-�1�5�6�5�1�

Why this file?

Refers to: temp.txt:1 in 1325b6e. [](commit_id = 1325b6e, deletion_comment = False)

anpete requested review from bricelam, divega, ajcvickers, smitpatel, maumar and AndriySvyryd January 4, 2018 21:47

ajcvickers reviewed Jan 5, 2018

View reviewed changes

pmiddleton reviewed Jan 8, 2018

View reviewed changes

anpete force-pushed the vt-new-5 branch 2 times, most recently from 1e5f8ae to 368a0e2 Compare January 8, 2018 20:59

anpete force-pushed the vt-new-5 branch from 368a0e2 to 08dc4c2 Compare January 8, 2018 21:07

ajcvickers approved these changes Jan 9, 2018

View reviewed changes

AndriySvyryd reviewed Jan 9, 2018

View reviewed changes

bricelam approved these changes Jan 9, 2018

View reviewed changes

anpete force-pushed the vt-new-5 branch from 92a3251 to 82a8553 Compare January 10, 2018 20:36

anpete added 2 commits January 10, 2018 13:19

Query Types: CR feedback.

1325b6e

anpete force-pushed the vt-new-5 branch from 82a8553 to 1325b6e Compare January 10, 2018 21:19

anpete merged commit 1325b6e into dev Jan 10, 2018

smitpatel deleted the vt-new-5 branch January 26, 2018 00:43

smitpatel reviewed Feb 7, 2018

View reviewed changes

smitpatel mentioned this pull request May 11, 2018

Query: compilation error (dangling qsre) for queries with SelectMany whose selector is a subquery referencing outer qsre #11941

Closed

		var entityType = new EntityType(type, this, ConfigurationSource.Explicit);

		var keyProperty = entityType.AddProperty("__QueryTypeKey__", typeof(int), ConfigurationSource.Convention);

Feature: 2.1: Initial impl. of Query Types. #10647

Feature: 2.1: Initial impl. of Query Types. #10647

Conversation

anpete commented Jan 4, 2018 • edited Loading

ErikEJ commented Jan 5, 2018

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 commented Jan 5, 2018

anpete commented Jan 5, 2018

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

anpete commented Jan 8, 2018

maumar commented Jan 8, 2018

anpete commented Jan 8, 2018

anpete commented Jan 8, 2018

anpete commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anpete Jan 9, 2018 • 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

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

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

smitpatel commented Feb 7, 2018

anpete commented Jan 4, 2018 •

edited

Loading

anpete Jan 9, 2018 •

edited

Loading