Skip to content

Commit

Permalink
Query: Mark SelectExpression as immutable after projection has been a…
Browse files Browse the repository at this point in the history
…pplied

- Add an internal state to remember the mutability which is set to false after ApplyProjection is called.
- Add overload of ApplyProjection which ignores shaper to create an immutable select expression to be used as subquery
- Add debug check to verify that all SelectExpression are marked as immutable

Resolves #26532 - all those expressions have debug check in ctor
Resolves #26167 - by having more accurate flag for immutability
  • Loading branch information
smitpatel committed Jan 20, 2022
1 parent e42407c commit f8a4bca
Show file tree
Hide file tree
Showing 13 changed files with 200 additions and 117 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 28 additions & 31 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
<!--
Microsoft ResX Schema
Version 2.0
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.
Example:
... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>
There are any number of "resheader" rows that contain simple
There are any number of "resheader" rows that contain simple
name/value pairs.
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.
mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.
mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.
mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -726,9 +726,6 @@
<data name="PropertyNotMappedToTable" xml:space="preserve">
<value>The property '{property}' on entity type '{entityType}' is not mapped to '{table}'.</value>
</data>
<data name="QueryFromSqlInsideExists" xml:space="preserve">
<value>The query contains usage of 'Any' or 'AnyAsync' operation after 'FromSqlRaw' or 'FromSqlInterpolated' method. Using this raw SQL query more than once isn't currently supported. Replace the use of 'Any' or 'AnyAsync' with 'Count' or 'CountAsync'. See https://go.microsoft.com/fwlink/?linkid=2174053 for more information.</value>
</data>
<data name="ReadonlyEntitySaved" xml:space="preserve">
<value>The entity type '{entityType}' is not mapped to a table, therefore the entities cannot be persisted to the database. Call 'ToTable' in 'OnModelCreating' to map it to a table.</value>
</data>
Expand Down Expand Up @@ -840,4 +837,4 @@
<data name="VisitChildrenMustBeOverridden" xml:space="preserve">
<value>'VisitChildren' must be overridden in the class deriving from 'SqlExpression'.</value>
</data>
</root>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ public EnumHasFlagTranslator(ISqlExpressionFactory sqlExpressionFactory)
var argument = arguments[0];
return instance.Type != argument.Type
? null
// TODO: If argument is SelectExpression, we need to clone it.
// See issue#26532
: (SqlExpression)_sqlExpressionFactory.Equal(_sqlExpressionFactory.And(instance, argument), argument);
: _sqlExpressionFactory.Equal(_sqlExpressionFactory.And(instance, argument), argument);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,6 @@ public virtual SelectExpression Expand(
Expression.Constant(new CompositeRelationalParameter(parameterExpression.Name!, subParameters)));

case ConstantExpression constantExpression:
if (constantExpression.Value is not object?[]
&& new FromSqlInExistVerifyingExpressionVisitor(fromSql).Verify(_selectExpression))
{
throw new InvalidOperationException(RelationalStrings.QueryFromSqlInsideExists);
}

var existingValues = constantExpression.GetConstantValue<object?[]>();
var constantValues = new object?[existingValues.Length];
for (var i = 0; i < existingValues.Length; i++)
Expand Down Expand Up @@ -173,43 +167,4 @@ public virtual SelectExpression Expand(
return null;
}
}

private sealed class FromSqlInExistVerifyingExpressionVisitor : ExpressionVisitor
{
private readonly FromSqlExpression _mutatedExpression;
private bool _faulty;

public FromSqlInExistVerifyingExpressionVisitor(FromSqlExpression mutatedExpression)
{
_mutatedExpression = mutatedExpression;
}

public bool Verify(SelectExpression selectExpression)
{
_faulty = false;

Visit(selectExpression);

return _faulty;
}

[return: NotNullIfNotNull("expression")]
public override Expression? Visit(Expression? expression)
{
if (_faulty)
{
return expression;
}

if (expression is ExistsExpression existsExpression
&& existsExpression.Subquery.Tables.Contains(_mutatedExpression, ReferenceEqualityComparer.Instance))
{
_faulty = true;

return expression;
}

return base.Visit(expression);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,13 @@ public override Expression Process(Expression query)
query = base.Process(query);
query = new SelectExpressionProjectionApplyingExpressionVisitor(
((RelationalQueryCompilationContext)QueryCompilationContext).QuerySplittingBehavior).Visit(query);

#if DEBUG
query = new TableAliasVerifyingExpressionVisitor().Visit(query);
// Verifies that all SelectExpression are marked as immutable after this point.
new SelectExpressionMutableVerifyingExpressionVisitor().Visit(query);
// Verifies that all table aliases are uniquely assigned without skipping over
// Which points to possible mutation of a SelectExpression being used in multiple places.
new TableAliasVerifyingExpressionVisitor().Visit(query);
#endif
query = new SelectExpressionPruningExpressionVisitor().Visit(query);
query = new SqlExpressionSimplifyingExpressionVisitor(RelationalDependencies.SqlExpressionFactory, _useRelationalNulls)
Expand All @@ -50,6 +55,31 @@ public override Expression Process(Expression query)
return query;
}

#if DEBUG
private sealed class SelectExpressionMutableVerifyingExpressionVisitor : ExpressionVisitor
{
[return: NotNullIfNotNull("expression")]
public override Expression? Visit(Expression? expression)
{
if (expression is SelectExpression selectExpression)
{
if (selectExpression.IsMutable())
{
throw new InvalidDataException(selectExpression.Print());
}
}

if (expression is ShapedQueryExpression shapedQueryExpression)
{
Visit(shapedQueryExpression.QueryExpression);

return shapedQueryExpression;
}

return base.Visit(expression);
}
}

private sealed class TableAliasVerifyingExpressionVisitor : ExpressionVisitor
{
private readonly ScopedVisitor _scopedVisitor = new();
Expand Down Expand Up @@ -130,4 +160,5 @@ public Expression EntryPoint(Expression expression)
}
}
}
#endif
}
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,8 @@ private static ShapedQueryExpression CreateShapedQueryExpression(IEntityType ent

var selectExpression = (SelectExpression)source.QueryExpression;
selectExpression.ApplyPredicate(_sqlExpressionFactory.Not(translation));
selectExpression.ReplaceProjection(new Dictionary<ProjectionMember, Expression>());
selectExpression.ReplaceProjection(new List<Expression>());
selectExpression.ApplyProjection();
if (selectExpression.Limit == null
&& selectExpression.Offset == null)
{
Expand Down Expand Up @@ -215,7 +216,8 @@ private static ShapedQueryExpression CreateShapedQueryExpression(IEntityType ent
}

var selectExpression = (SelectExpression)source.QueryExpression;
selectExpression.ReplaceProjection(new Dictionary<ProjectionMember, Expression>());
selectExpression.ReplaceProjection(new List<Expression>());
selectExpression.ApplyProjection();
if (selectExpression.Limit == null
&& selectExpression.Offset == null)
{
Expand Down Expand Up @@ -284,8 +286,8 @@ private static ShapedQueryExpression CreateShapedQueryExpression(IEntityType ent
var projection = selectExpression.GetProjection(projectionBindingExpression);
if (projection is SqlExpression sqlExpression)
{
selectExpression.ReplaceProjection(new List<Expression>());
selectExpression.AddToProjection(sqlExpression);
selectExpression.ReplaceProjection(new List<Expression> { sqlExpression });
selectExpression.ApplyProjection();

translation = _sqlExpressionFactory.In(translation, selectExpression, false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
return sqlExpression;
}

subquery.ReplaceProjection(new List<Expression>());
subquery.AddToProjection(sqlExpression);
subquery.ReplaceProjection(new List<Expression> { sqlExpression });
subquery.ApplyProjection();

SqlExpression scalarSubqueryExpression = new ScalarSubqueryExpression(subquery);

Expand Down Expand Up @@ -706,7 +706,8 @@ protected override Expression VisitTypeBinary(TypeBinaryExpression typeBinaryExp
var predicate = GeneratePredicateTpt(entityProjection);

subSelectExpression.ApplyPredicate(predicate);
subSelectExpression.ReplaceProjection(new Dictionary<ProjectionMember, Expression>());
subSelectExpression.ReplaceProjection(new List<Expression>());
subSelectExpression.ApplyProjection();
if (subSelectExpression.Limit == null
&& subSelectExpression.Offset == null)
{
Expand Down Expand Up @@ -842,7 +843,7 @@ protected override Expression VisitUnary(UnaryExpression unaryExpression)

private Expression? TryBindMember(Expression? source, MemberIdentity member)
{
if (!(source is EntityReferenceExpression entityReferenceExpression))
if (source is not EntityReferenceExpression entityReferenceExpression)
{
return null;
}
Expand Down Expand Up @@ -940,8 +941,8 @@ protected override Expression VisitUnary(UnaryExpression unaryExpression)
var projectionBindingExpression = (ProjectionBindingExpression)entityShaper.ValueBufferExpression;
var entityProjectionExpression = (EntityProjectionExpression)subSelectExpression.GetProjection(projectionBindingExpression);
var innerProjection = entityProjectionExpression.BindProperty(property);
subSelectExpression.ReplaceProjection(new List<Expression>());
subSelectExpression.AddToProjection(innerProjection);
subSelectExpression.ReplaceProjection(new List<Expression> { innerProjection });
subSelectExpression.ApplyProjection();

return new ScalarSubqueryExpression(subSelectExpression);
}
Expand Down Expand Up @@ -1009,7 +1010,7 @@ private bool TryRewriteContainsEntity(Expression source, Expression item, [NotNu
{
result = null;

if (!(item is EntityReferenceExpression itemEntityReference))
if (item is not EntityReferenceExpression itemEntityReference)
{
return false;
}
Expand Down Expand Up @@ -1299,7 +1300,7 @@ when memberInitExpression.Bindings.SingleOrDefault(
string baseParameterName,
IProperty property)
{
if (!(context.ParameterValues[baseParameterName] is IEnumerable<TEntity> baseListParameter))
if (context.ParameterValues[baseParameterName] is not IEnumerable<TEntity> baseListParameter)
{
return null;
}
Expand Down Expand Up @@ -1337,7 +1338,7 @@ private static bool IsNullSqlConstantExpression(Expression expression)
private static bool TranslationFailed(Expression? original, Expression? translation, out SqlExpression? castTranslation)
{
if (original != null
&& !(translation is SqlExpression))
&& translation is not SqlExpression)
{
castTranslation = null;
return true;
Expand Down Expand Up @@ -1399,7 +1400,7 @@ private sealed class SqlTypeMappingVerifyingExpressionVisitor : ExpressionVisito
protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is SqlExpression sqlExpression
&& !(extensionExpression is SqlFragmentExpression))
&& extensionExpression is not SqlFragmentExpression)
{
if (sqlExpression.TypeMapping == null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ public ExistsExpression(
RelationalTypeMapping? typeMapping)
: base(typeof(bool), typeMapping)
{

#if DEBUG
if (subquery.IsMutable() == true)
{
throw new InvalidOperationException();
}
#endif
Subquery = subquery;
IsNegated = negated;
}
Expand Down
6 changes: 6 additions & 0 deletions src/EFCore.Relational/Query/SqlExpressions/InExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ private InExpression(
RelationalTypeMapping? typeMapping)
: base(typeof(bool), typeMapping)
{
#if DEBUG
if (subquery?.IsMutable() == true)
{
throw new InvalidOperationException();
}
#endif
Item = item;
Subquery = subquery;
Values = values;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ private static SelectExpression Verify(SelectExpression selectExpression)
throw new InvalidOperationException(CoreStrings.TranslationFailed(selectExpression.Print()));
}

#if DEBUG
if (selectExpression.IsMutable() == true)
{
throw new InvalidOperationException();
}
#endif

return selectExpression;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,7 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
_groupingCorrelationPredicate = groupingCorrelationPredicate,
_groupingParentSelectExpressionId = selectExpression._groupingParentSelectExpressionId
};
newSelectExpression._mutable = selectExpression._mutable;

newSelectExpression._tptLeftJoinTables.AddRange(selectExpression._tptLeftJoinTables);
// Since identifiers are ColumnExpression, they are not visited since they don't contain SelectExpression inside it.
Expand Down
Loading

0 comments on commit f8a4bca

Please sign in to comment.