Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't duplicate query filter parameters #29422

Merged
merged 2 commits into from
Mar 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
// Apply defining query only when it is not custom query root
&& entityQueryRootExpression.GetType() == typeof(EntityQueryRootExpression))
{
var processedDefiningQueryBody = _parameterExtractingExpressionVisitor.ExtractParameters(definingQuery.Body);
var processedDefiningQueryBody =
_parameterExtractingExpressionVisitor.ExtractParameters(definingQuery.Body, clearEvaluatedValues: false);
processedDefiningQueryBody = _queryTranslationPreprocessor.NormalizeQueryableMethod(processedDefiningQueryBody);
processedDefiningQueryBody = _nullCheckRemovingExpressionVisitor.Visit(processedDefiningQueryBody);
processedDefiningQueryBody =
Expand Down Expand Up @@ -1711,7 +1712,8 @@ private Expression ApplyQueryFilter(IEntityType entityType, NavigationExpansionE
if (!_parameterizedQueryFilterPredicateCache.TryGetValue(rootEntityType, out var filterPredicate))
{
filterPredicate = queryFilter;
filterPredicate = (LambdaExpression)_parameterExtractingExpressionVisitor.ExtractParameters(filterPredicate);
filterPredicate = (LambdaExpression)_parameterExtractingExpressionVisitor.ExtractParameters(
filterPredicate, clearEvaluatedValues: false);
filterPredicate = (LambdaExpression)_queryTranslationPreprocessor.NormalizeQueryableMethod(filterPredicate);

// We need to do entity equality, but that requires a full method call on a query root to properly flow the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,13 @@ public ParameterExtractingExpressionVisitor(
public virtual Expression ExtractParameters(Expression expression)
=> ExtractParameters(expression, clearEvaluatedValues: true);

private Expression ExtractParameters(Expression expression, bool clearEvaluatedValues)
/// <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 ExtractParameters(Expression expression, bool clearEvaluatedValues)
{
var oldEvaluatableExpressions = _evaluatableExpressions;
_evaluatableExpressions = _evaluatableExpressionFindingExpressionVisitor.Find(expression);
Expand Down Expand Up @@ -274,7 +280,13 @@ private Expression Evaluate(Expression expression, bool generateParameter)
string? parameterName;
if (_evaluatedValues.TryGetValue(expression, out var cachedValue))
{
var existingExpression = generateParameter ? cachedValue.Parameter : cachedValue.Constant;
// The _generateContextAccessors condition allows us to reuse parameter expressions evaluated in query filters.
// In principle, _generateContextAccessors is orthogonal to query filters, but in practice it is only used in the
// nav expansion query filters (and defining query). If this changes in future, they would need to be decoupled.
var existingExpression = generateParameter || _generateContextAccessors
stevendarby marked this conversation as resolved.
Show resolved Hide resolved
? cachedValue.Parameter
: cachedValue.Constant;

if (existingExpression != null)
{
return existingExpression;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,4 +340,21 @@ public virtual void Using_multiple_context_in_filter_parametrize_only_current_co
query = context.Set<MultiContextFilter>().ToList();
Assert.Equal(2, query.Count);
}

[ConditionalFact]
public virtual void Using_multiple_entities_with_filters_reuses_parameters()
{
using var context = CreateContext();
context.Tenant = 1;
context.Property = false;

var query = context.Set<DeDupeFilter1>()
.Include(x => x.DeDupeFilter2s)
.Include(x => x.DeDupeFilter3s)
.ToList();

Assert.Single(query);
Assert.Single(query[0].DeDupeFilter2s);
Assert.Single(query[0].DeDupeFilter3s);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
// Multiple context used in filter
modelBuilder.Entity<MultiContextFilter>()
.HasQueryFilter(e => e.IsEnabled == Property && e.BossId == new IncorrectDbContext().BossId);

modelBuilder.Entity<DeDupeFilter1>().HasQueryFilter(x => x.Tenant == Tenant);
modelBuilder.Entity<DeDupeFilter2>().HasQueryFilter(x => x.TenantX == Tenant);
modelBuilder.Entity<DeDupeFilter3>().HasQueryFilter(x => x.Tenant == Tenant);
}

private void ConfigureFilter(EntityTypeBuilder<LocalMethodFilter> builder)
Expand Down Expand Up @@ -205,7 +209,14 @@ public static void SeedData(QueryFilterFuncletizationContext context)
new MultiContextFilter { BossId = 1, IsEnabled = false },
new MultiContextFilter { BossId = 1, IsEnabled = true },
new MultiContextFilter { BossId = 2, IsEnabled = true },
new MultiContextFilter { BossId = 2, IsEnabled = false }
new MultiContextFilter { BossId = 2, IsEnabled = false },
new DeDupeFilter1
{
Tenant = 1,
DeDupeFilter2s = new List<DeDupeFilter2> { new() { TenantX = 1 }, new() { TenantX = 2 } },
DeDupeFilter3s = new List<DeDupeFilter3> { new() { Tenant = 1 }, new() { Tenant = 2 } }
},
new DeDupeFilter1 { Tenant = 2 }
);

context.SaveChanges();
Expand Down Expand Up @@ -418,4 +429,28 @@ public class MultiContextFilter
public bool IsEnabled { get; set; }
}

public class DeDupeFilter1
{
public int Id { get; set; }
public int Tenant { get; set; }
public ICollection<DeDupeFilter2> DeDupeFilter2s { get; set; }
public ICollection<DeDupeFilter3> DeDupeFilter3s { get; set; }
}

public class DeDupeFilter2
{
public int Id { get; set; }
public int TenantX { get; set; }
public int? DeDupeFilter1Id { get; set; }
public DeDupeFilter1 DeDupeFilter1 { get; set; }
}

public class DeDupeFilter3
{
public int Id { get; set; }
public short Tenant { get; set; }
public int? DeDupeFilter1Id { get; set; }
public DeDupeFilter1 DeDupeFilter1 { get; set; }
}

#endregion
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,32 @@ FROM [MultiContextFilter] AS [m]
""");
}

public override void Using_multiple_entities_with_filters_reuses_parameters()
{
base.Using_multiple_entities_with_filters_reuses_parameters();

AssertSql(
"""
@__ef_filter__Tenant_0='1'
@__ef_filter__Tenant_0_1='1' (DbType = Int16)

SELECT [d].[Id], [d].[Tenant], [t].[Id], [t].[DeDupeFilter1Id], [t].[TenantX], [t0].[Id], [t0].[DeDupeFilter1Id], [t0].[Tenant]
FROM [DeDupeFilter1] AS [d]
LEFT JOIN (
SELECT [d0].[Id], [d0].[DeDupeFilter1Id], [d0].[TenantX]
FROM [DeDupeFilter2] AS [d0]
WHERE [d0].[TenantX] = @__ef_filter__Tenant_0
) AS [t] ON [d].[Id] = [t].[DeDupeFilter1Id]
LEFT JOIN (
SELECT [d1].[Id], [d1].[DeDupeFilter1Id], [d1].[Tenant]
FROM [DeDupeFilter3] AS [d1]
WHERE [d1].[Tenant] = @__ef_filter__Tenant_0_1
) AS [t0] ON [d].[Id] = [t0].[DeDupeFilter1Id]
WHERE [d].[Tenant] = @__ef_filter__Tenant_0
ORDER BY [d].[Id], [t].[Id]
""");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,34 @@ public override void DbContext_list_is_parameterized()
Assert.Equal(2, query.Count);
}

public override void Using_multiple_entities_with_filters_reuses_parameters()
{
base.Using_multiple_entities_with_filters_reuses_parameters();

AssertSql(
"""
@__ef_filter__Tenant_0='1'

SELECT "d"."Id", "d"."Tenant", "t"."Id", "t"."DeDupeFilter1Id", "t"."TenantX", "t0"."Id", "t0"."DeDupeFilter1Id", "t0"."Tenant"
FROM "DeDupeFilter1" AS "d"
LEFT JOIN (
SELECT "d0"."Id", "d0"."DeDupeFilter1Id", "d0"."TenantX"
FROM "DeDupeFilter2" AS "d0"
WHERE "d0"."TenantX" = @__ef_filter__Tenant_0
) AS "t" ON "d"."Id" = "t"."DeDupeFilter1Id"
LEFT JOIN (
SELECT "d1"."Id", "d1"."DeDupeFilter1Id", "d1"."Tenant"
FROM "DeDupeFilter3" AS "d1"
WHERE "d1"."Tenant" = @__ef_filter__Tenant_0
) AS "t0" ON "d"."Id" = "t0"."DeDupeFilter1Id"
WHERE "d"."Tenant" = @__ef_filter__Tenant_0
ORDER BY "d"."Id", "t"."Id"
""");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

public class QueryFilterFuncletizationSqliteFixture : QueryFilterFuncletizationRelationalFixture
{
protected override ITestStoreFactory TestStoreFactory
Expand Down