Query: Remove public setters from customer expression properties

Resolves #19192 Changes - Only server side query expression are in place updated using private setters, public methods - Moved to Update method pattern in certain cases - TableExpressionBase still has internal set which will be fixed in #17337 - ShapedQueryExpression.VisitChildren throws now. Since ShapedQuery contains query expression (server side) and shaper expression (client side), it is unlikely that a visitor needs to visit both components. Hence we force visitor to decide which component to visit. This improves perf as we wouldn't go into client side shaper when updating something in sql tree
dotnet · Jan 6, 2020 · c41a2f1 · c41a2f1
1 parent c2406e7
commit c41a2f1
Show file tree

Hide file tree

Showing 17 changed files with 282 additions and 346 deletions.
diff --git a/src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs
@@ -184,14 +184,9 @@ protected override ShapedQueryExpression TranslateCast(ShapedQueryExpression sou
             Check.NotNull(source, nameof(source));
             Check.NotNull(resultType, nameof(resultType));
 
-            if (source.ShaperExpression.Type == resultType)
-            {
-                return source;
-            }
-
-            source.ShaperExpression = Expression.Convert(source.ShaperExpression, resultType);
-
-            return source;
+            return source.ShaperExpression.Type != resultType
+                ? source.UpdateShaperExpression(Expression.Convert(source.ShaperExpression, resultType))
+                : source;
         }
 
         /// <summary>
@@ -256,9 +251,7 @@ protected override ShapedQueryExpression TranslateCount(ShapedQueryExpression so
 
             selectExpression.ClearOrdering();
             selectExpression.ReplaceProjectionMapping(projectionMapping);
-            source.ShaperExpression = new ProjectionBindingExpression(source.QueryExpression, new ProjectionMember(), typeof(int));
-
-            return source;
+            return source.UpdateShaperExpression(new ProjectionBindingExpression(source.QueryExpression, new ProjectionMember(), typeof(int)));
         }
 
         /// <summary>
@@ -342,12 +335,9 @@ protected override ShapedQueryExpression TranslateFirstOrDefault(
             var selectExpression = (SelectExpression)source.QueryExpression;
             selectExpression.ApplyLimit(TranslateExpression(Expression.Constant(1)));
 
-            if (source.ShaperExpression.Type != returnType)
-            {
-                source.ShaperExpression = Expression.Convert(source.ShaperExpression, returnType);
-            }
-
-            return source;
+            return source.ShaperExpression.Type != returnType
+                ? source.UpdateShaperExpression(Expression.Convert(source.ShaperExpression, returnType))
+                : source;
         }
 
         /// <summary>
@@ -449,12 +439,9 @@ protected override ShapedQueryExpression TranslateLastOrDefault(
             selectExpression.ReverseOrderings();
             selectExpression.ApplyLimit(TranslateExpression(Expression.Constant(1)));
 
-            if (source.ShaperExpression.Type != returnType)
-            {
-                source.ShaperExpression = Expression.Convert(source.ShaperExpression, returnType);
-            }
-
-            return source;
+            return source.ShaperExpression.Type != returnType
+                ? source.UpdateShaperExpression(Expression.Convert(source.ShaperExpression, returnType))
+                : source;
         }
 
         /// <summary>
@@ -510,9 +497,7 @@ protected override ShapedQueryExpression TranslateLongCount(ShapedQueryExpressio
 
             selectExpression.ClearOrdering();
             selectExpression.ReplaceProjectionMapping(projectionMapping);
-            source.ShaperExpression = new ProjectionBindingExpression(source.QueryExpression, new ProjectionMember(), typeof(long));
-
-            return source;
+            return source.UpdateShaperExpression(new ProjectionBindingExpression(source.QueryExpression, new ProjectionMember(), typeof(long)));
         }
 
         /// <summary>
@@ -657,10 +642,7 @@ protected override ShapedQueryExpression TranslateSelect(ShapedQueryExpression s
 
             var newSelectorBody = ReplacingExpressionVisitor.Replace(selector.Parameters.Single(), source.ShaperExpression, selector.Body);
 
-            source.ShaperExpression = _projectionBindingExpressionVisitor
-                .Translate(selectExpression, newSelectorBody);
-
-            return source;
+            return source.UpdateShaperExpression(_projectionBindingExpressionVisitor.Translate(selectExpression, newSelectorBody));
         }
 
         /// <summary>
@@ -717,12 +699,9 @@ protected override ShapedQueryExpression TranslateSingleOrDefault(
             var selectExpression = (SelectExpression)source.QueryExpression;
             selectExpression.ApplyLimit(TranslateExpression(Expression.Constant(2)));
 
-            if (source.ShaperExpression.Type != returnType)
-            {
-                source.ShaperExpression = Expression.Convert(source.ShaperExpression, returnType);
-            }
-
-            return source;
+            return source.ShaperExpression.Type != returnType
+                ? source.UpdateShaperExpression(Expression.Convert(source.ShaperExpression, returnType))
+                : source;
         }
 
         /// <summary>
@@ -948,9 +927,7 @@ private ShapedQueryExpression AggregateResultShaper(
                 shaper = Expression.Convert(shaper, resultType);
             }
 
-            source.ShaperExpression = shaper;
-
-            return source;
+            return source.UpdateShaperExpression(shaper);
         }
     }
 }
diff --git a/src/EFCore.Cosmos/Query/Internal/EntityProjectionExpression.cs b/src/EFCore.Cosmos/Query/Internal/EntityProjectionExpression.cs
@@ -90,12 +90,19 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
         {
             Check.NotNull(visitor, nameof(visitor));
 
-            var accessExpression = visitor.Visit(AccessExpression);
+            return Update(visitor.Visit(AccessExpression));
+        }
 
-            return accessExpression != AccessExpression
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        public virtual Expression Update([NotNull] Expression accessExpression)
+            => accessExpression != AccessExpression
                 ? new EntityProjectionExpression(EntityType, accessExpression)
                 : this;
-        }
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

diff --git a/src/EFCore.Cosmos/Query/Internal/KeyAccessExpression.cs b/src/EFCore.Cosmos/Query/Internal/KeyAccessExpression.cs
@@ -68,9 +68,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
         {
             Check.NotNull(visitor, nameof(visitor));
 
-            var outerExpression = visitor.Visit(AccessExpression);
-
-            return Update(outerExpression);
+            return Update(visitor.Visit(AccessExpression));
         }
 
         /// <summary>

diff --git a/src/EFCore.Cosmos/Query/Internal/ObjectAccessExpression.cs b/src/EFCore.Cosmos/Query/Internal/ObjectAccessExpression.cs
@@ -87,9 +87,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
         {
             Check.NotNull(visitor, nameof(visitor));
 
-            var outerExpression = visitor.Visit(AccessExpression);
-
-            return Update(outerExpression);
+            return Update(visitor.Visit(AccessExpression));
         }
 
         /// <summary>

diff --git a/src/EFCore.Cosmos/Query/Internal/OrderingExpression.cs b/src/EFCore.Cosmos/Query/Internal/OrderingExpression.cs
@@ -81,11 +81,9 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
         public virtual OrderingExpression Update([NotNull] SqlExpression expression)
-        {
-            return expression != Expression
+            => expression != Expression
                 ? new OrderingExpression(expression, IsAscending)
                 : this;
-        }
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

diff --git a/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs b/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs
@@ -34,7 +34,7 @@ private readonly IDictionary<EntityProjectionExpression, IDictionary<IProperty,
         private ParameterExpression _groupingParameter;
 
         public virtual IReadOnlyList<Expression> Projection => _valueBufferSlots;
-        public virtual Expression ServerQueryExpression { get; [param: NotNull] set; }
+        public virtual Expression ServerQueryExpression { get; private set; }
         public virtual ParameterExpression CurrentParameter => _groupingParameter ?? _valueBufferParameter;
         public override Type Type => typeof(IEnumerable<ValueBuffer>);
         public sealed override ExpressionType NodeType => ExpressionType.Extension;
@@ -198,6 +198,9 @@ private IEnumerable<IProperty> GetAllPropertiesInHierarchy(IEntityType entityTyp
         public virtual Expression GetMappedProjection([NotNull] ProjectionMember member)
             => _projectionMapping[member];
 
+        public virtual void UpdateServerQueryExpression([NotNull] Expression serverQueryExpression)
+            => ServerQueryExpression = serverQueryExpression;
+
         public virtual void PushdownIntoSubquery()
         {
             var clientProjection = _valueBufferSlots.Count != 0;
@@ -317,16 +320,11 @@ public virtual void ApplyDefaultIfEmpty()
         }
 
         private static IPropertyBase InferPropertyFromInner(Expression expression)
-        {
-            if (expression is MethodCallExpression methodCallExpression
+            => expression is MethodCallExpression methodCallExpression
                 && methodCallExpression.Method.IsGenericMethod
-                && methodCallExpression.Method.GetGenericMethodDefinition() == EntityMaterializerSource.TryReadValueMethod)
-            {
-                return (IPropertyBase)((ConstantExpression)methodCallExpression.Arguments[2]).Value;
-            }
-
-            return null;
-        }
+                && methodCallExpression.Method.GetGenericMethodDefinition() == EntityMaterializerSource.TryReadValueMethod
+                ? (IPropertyBase)((ConstantExpression)methodCallExpression.Arguments[2]).Value
+                : null;
 
         public virtual void ApplyProjection()
         {