Add owned entity types support to more places in query #8249

AndriySvyryd · 2017-04-21T17:08:13Z

Record the EntityType when visiting existing JoinClause
Update QuerySourceMapping when cloning
Add provider-specific model building extensions
Add more query tests for owned entity types

maumar · 2017-04-21T17:35:41Z

src/EFCore.Specification.Tests/ComplexNavigationsQueryTestBase.cs

@@ -4249,9 +4439,11 @@ where l2.OneToOne_Required_FK.OneToMany_Optional.Select(i => i.OneToOne_Optional
        {
            using (var context = CreateContext())
            {
+                var expected = l2oQuery(ExpectedSet<TItem1>()).ToArray();
+                var actual = efQuery(Set<TItem1>(context)).ToArray();


switch the order so that actual executes first (in case expected has a null ref or something)

Wouldn't you want to see the exception in expected first, so that you don't waste time debugging actual?

if expected runs first and throws, then the product code never runs - in my experience, exceptions coming from expected (in this test suite at least) are mostly due to null refs during nav access, something that is perfectly fine for the product query.

maumar · 2017-04-21T17:38:52Z

test/EFCore.SqlServer.FunctionalTests/ComplexNavigationsOwnedQuerySqlServerTest.cs

+        private bool SupportsOffset => TestEnvironment.GetFlag(nameof(SqlServerCondition.SupportsOffset)) ?? true;
+
+        [ConditionalFact]
+        public override void Where_nav_prop_reference_optional1()


maumar · 2017-04-21T18:09:07Z

src/EFCore.Specification.Tests/ComplexNavigationsOwnedQueryTestBase.cs

+        }
+
+        // One-to-many not supported yet
+        public override void Multiple_SelectMany_with_string_based_Include()


add skip annotation here also? or would that be too much spam in the build.cmd?

I think Skip is only for tests that are expected to pass

anpete · 2017-04-21T18:14:44Z

src/EFCore/Metadata/Internal/EntityMaterializerSource.cs

            int index,
            IPropertyBase property = null)
        {
-            object untypedValue = null;
-
+            var untypedValue = valueBuffer[index];


This can throw null ref

Yes and in that case the exception message would be wrong:
An exception occurred while reading a database value for property 'Order.ReplacedOrderId'. The expected type was 'int?' but the actual value was null.

No existing tests fails, right? If not, then I think this is OK. We are effectively saying that seeing ValueBuffer.Empty is an error here.

Well we throw in either case, just the exception is different

Yes, and we are discussing whether that is correct 😄 This change asserts that a null coming from the database (incorrectly) never results in ValueBuffer.Empty appearing in the executing query.

maumar

test changes look good

anpete · 2017-04-21T19:24:54Z

src/EFCore/Query/Internal/QueryOptimizer.cs

@@ -81,18 +69,18 @@ public QueryOptimizer([NotNull] IModel model)
        ///     directly from your code. This API may change or be removed in future releases.
        /// </summary>
        public virtual void Optimize(
-            IReadOnlyCollection<IQueryAnnotation> queryAnnotations,
+            QueryCompilationContext queryCompilationContext,


ctor dependency?

QueryCompilationContext is not in DI

Ah, I had assumed that QueryOptimizer was not in DI. Makes me wonder whether having QueryOptimizer in DI is correct as it has state - We have to be careful here 💣

Actually, I see that your change improves this situation! 🙌

Filed #8256

anpete · 2017-04-21T19:50:32Z

src/EFCore.Relational/Metadata/RelationalEntityTypeAnnotations.cs

            }
            [param: CanBeNull] set { SetTableName(value); }
        }

+        private string GetDefaultTableName()
+            => EntityType.HasDelegatedIdentity()


Should we cache this?

Sure, more caching never hurts

Since this is a relational concept, proper caching would require non-trivial infrastructure changes, I think it's better to do it as part of #8258

smitpatel · 2017-04-21T21:49:19Z

src/EFCore/Query/ExpressionVisitors/Internal/EntityEqualityRewritingExpressionVisitor.cs


        /// <summary>
        ///     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 EntityEqualityRewritingExpressionVisitor([NotNull] IModel model)
+        public EntityEqualityRewritingExpressionVisitor([NotNull] QueryCompilationContext queryCompilationContext)
        {


~~null check?~~

We don't need checks on .Internal API

smitpatel · 2017-04-21T21:53:43Z

src/EFCore/Query/QueryCompilationContext.cs

+        ///     Updates the query source mappings to the new query sources
+        /// </summary>
+        /// <param name="querySourceMapping"> The new query source mapping </param>
+        public virtual void UpdateMapping(QuerySourceMapping querySourceMapping)


null annotation.

Record the EntityType when visiting existing JoinClause Update QuerySourceMapping when cloning Add provider-specific model building extensions Add more query tests for owned entity types

AndriySvyryd requested review from anpete, smitpatel and maumar April 21, 2017 17:08

dnfclas added the cla-already-signed label Apr 21, 2017

maumar reviewed Apr 21, 2017

View reviewed changes

anpete reviewed Apr 21, 2017

View reviewed changes

maumar approved these changes Apr 21, 2017

View reviewed changes

anpete reviewed Apr 21, 2017

View reviewed changes

anpete approved these changes Apr 21, 2017

View reviewed changes

smitpatel reviewed Apr 21, 2017

View reviewed changes

AndriySvyryd force-pushed the DelegatedIdentityQueryTests branch from 347686c to c52548b Compare April 21, 2017 22:39

Add owned entity types support to more places in query

e6f0e19

Record the EntityType when visiting existing JoinClause Update QuerySourceMapping when cloning Add provider-specific model building extensions Add more query tests for owned entity types

AndriySvyryd force-pushed the DelegatedIdentityQueryTests branch from c52548b to e6f0e19 Compare April 22, 2017 00:31

AndriySvyryd merged commit e6f0e19 into dev Apr 22, 2017

AndriySvyryd deleted the DelegatedIdentityQueryTests branch April 22, 2017 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add owned entity types support to more places in query #8249

Add owned entity types support to more places in query #8249

AndriySvyryd commented Apr 21, 2017

maumar Apr 21, 2017

AndriySvyryd Apr 21, 2017

maumar Apr 21, 2017

AndriySvyryd Apr 21, 2017

maumar Apr 21, 2017

maumar Apr 21, 2017

AndriySvyryd Apr 21, 2017

anpete Apr 21, 2017

AndriySvyryd Apr 21, 2017

anpete Apr 21, 2017

AndriySvyryd Apr 21, 2017

anpete Apr 21, 2017

maumar left a comment

anpete Apr 21, 2017

AndriySvyryd Apr 21, 2017

anpete Apr 21, 2017

anpete Apr 21, 2017

AndriySvyryd Apr 21, 2017

anpete Apr 21, 2017

AndriySvyryd Apr 21, 2017

AndriySvyryd Apr 21, 2017

smitpatel Apr 21, 2017 •

edited

Loading

AndriySvyryd Apr 21, 2017

smitpatel Apr 21, 2017

Add owned entity types support to more places in query #8249

Add owned entity types support to more places in query #8249

Conversation

AndriySvyryd commented Apr 21, 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

maumar 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

smitpatel Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel Apr 21, 2017 •

edited

Loading