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.

Sign up for GitHub

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

Fix duplicate JOIN statements when expanding an IQueryable that already has a join #1040

Open
wants to merge 3 commits into
base: release-8.x
Choose a base branch
from
Open

Fix duplicate JOIN statements when expanding an IQueryable that already has a join #1040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a39a74
Fix $expand with null properties
ElizabethOkerio Sep 4, 2023
d504172
fix failing tests
ElizabethOkerio Sep 5, 2023
7512799
update changes
ElizabethOkerio Nov 6, 2023
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
5 changes: 5 additions & 0 deletions src/Microsoft.AspNetCore.OData/Query/Expressions/BinderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,11 @@ public static IQueryable ApplyBind(this ISelectExpandBinder binder, IQueryable s
throw Error.ArgumentNull(nameof(context));
}

if (string.IsNullOrEmpty(context.QueryProvider))
{
context.QueryProvider = source.Provider.GetType().Namespace;
}

Type elementType = context.ElementClrType;

LambdaExpression projectionLambda = binder.BindSelectExpand(selectExpandClause, context) as LambdaExpression;
Expand Down
7 changes: 7 additions & 0 deletions src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinderContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ public QueryBinderContext(QueryBinderContext context, ODataQuerySettings querySe

ElementType = Model.GetEdmTypeReference(ElementClrType)?.Definition;

QueryProvider = context.QueryProvider;

if (ElementType == null)
{
throw new ODataException(Error.Format(SRResources.ClrTypeNotInModel, ElementClrType.FullName));
Expand Down Expand Up @@ -175,6 +177,11 @@ public QueryBinderContext(QueryBinderContext context, ODataQuerySettings querySe
/// </summary>
public Expression Source { get;set; }

/// <summary>
/// Gets or sets the query provider.
/// </summary>
internal string QueryProvider { get; set; }

/// <summary>
/// Gets the parameter using parameter name.
/// </summary>
Expand Down
91 changes: 90 additions & 1 deletion src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,23 @@ internal Expression ProjectElement(QueryBinderContext context, Expression source
bool isSelectedAll = IsSelectAll(selectExpandClause);
if (isSelectedAll)
{
// If we have an EF query provider, then we remove the non-structural properties. The reason for this is to avoid
// Creating an expression that will generate duplicate LEFT joins when a LEFT join already exists in the IQueryable
// as observed here: https://github.com/OData/AspNetCoreOData/issues/497

Expression updatedExpression = null;
if (IsEfQueryProvider(context))
Copy link

@wachugamaina wachugamaina Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

           var updatedExpression = source;
            if (IsEfQueryProvider(context))
            {
                updatedExpression = RemoveNonStructuralProperties(context, source, structuredType);
            }

{
updatedExpression = RemoveNonStructuralProperties(context, source, structuredType);
}
else
{
updatedExpression = source;
}

// Initialize property 'Instance' on the wrapper class
wrapperProperty = wrapperType.GetProperty("Instance");
wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, source));
wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, updatedExpression));

wrapperProperty = wrapperType.GetProperty("UseInstanceForProperties");
wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, Expression.Constant(true)));
Expand Down Expand Up @@ -427,6 +441,81 @@ internal Expression ProjectElement(QueryBinderContext context, Expression source
return Expression.MemberInit(Expression.New(wrapperType), wrapperTypeMemberAssignments);
}

// Generates the expression
// { Instance = new Customer() {Id = $it.Id, Name= $it.Name}}
private static Expression RemoveNonStructuralProperties(QueryBinderContext context, Expression source, IEdmStructuredType structuredType)
{
IEdmModel model = context.Model;
Expression updatedSource = null;

Type elementType = source.Type;
IEnumerable<IEdmStructuralProperty> structuralProperties = structuredType.StructuralProperties();

PropertyInfo[] props = elementType.GetProperties();
List<MemberBinding> bindings = new List<MemberBinding>();
ODataQuerySettings settings = context.QuerySettings;
bool isSourceNullable = ExpressionBinderHelper.IsNullable(source.Type);

foreach (PropertyInfo prop in props)
{
bool isPropertyNullable = ExpressionBinderHelper.IsNullable(prop.PropertyType);

foreach (var sp in structuralProperties)
{
if (prop.CanWrite && model.GetClrPropertyName(sp) == prop.Name)
{
Expression propertyValue = Expression.Property(source, prop);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we create a test case to test a big schema, for example, more than 1k properties for an entity type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have an entity type with more than 1k properties?

Type nullablePropertyType = TypeHelper.ToNullable(propertyValue.Type);
Expression nullablePropertyValue = ExpressionHelpers.ToNullable(propertyValue);

if ((settings.HandleNullPropagation == HandleNullPropagationOption.True) || (isSourceNullable && isPropertyNullable))
{
propertyValue = Expression.Condition(
test: Expression.Equal(source, Expression.Constant(value: null)),
ifTrue: Expression.Constant(value: null, type: nullablePropertyType),
ifFalse: nullablePropertyValue);
}
else if (isSourceNullable)
{
object defaultValue = prop.PropertyType.IsValueType ? Activator.CreateInstance(prop.PropertyType) : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about if the property type doesn't have the default constructor?

Copy link
Contributor Author

@ElizabethOkerio ElizabethOkerio Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All value types have a default constructor. So I don't think there will ever be a case where the property type does not have a default constructor. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/types#833-default-constructors


// property is non-nullable
propertyValue = Expression.Condition(
test: Expression.Equal(source, Expression.Constant(value: null)),
ifTrue: Expression.Constant(value: defaultValue),
ifFalse: propertyValue);
}
else if (isPropertyNullable)
{
propertyValue = nullablePropertyValue;
}

bindings.Add(Expression.Bind(prop, propertyValue));
break;
}
}
}

NewExpression newExpression = Expression.New(source.Type);
updatedSource = Expression.MemberInit(newExpression, bindings);

return updatedSource;
}

private bool IsEfQueryProvider(QueryBinderContext context)
{
if (context.QueryProvider != null &&
context.QueryProvider == HandleNullPropagationOptionHelper.EntityFrameworkQueryProviderNamespace ||
context.QueryProvider == HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF5 ||
context.QueryProvider == HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF6 ||
context.QueryProvider == HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEFCore2)
{
return true;
}

return false;
}

private static bool ParseComputedDynamicProperties(QueryBinderContext context, IList<DynamicPathSegment> dynamicPathSegments, bool isSelectedAll,
out IList<string> computedProperties)
{
Expand Down
175 changes: 173 additions & 2 deletions test/Microsoft.AspNetCore.OData.Tests/Query/Expressions/SelectExpandBinderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Collections.Generic;
using System.ComponentModel;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
Expand All @@ -36,6 +37,7 @@ public class SelectExpandBinderTest
private readonly SelectExpandBinder _binder;
private readonly SelectExpandBinder _binder_lowerCamelCased;
private readonly IQueryable<QueryCustomer> _queryable;
private readonly IQueryable<QueryCustomer> _queryable1;
private readonly IQueryable<QueryCustomer> _queryable_lowerCamelCased;
private readonly ODataQueryContext _context;
private readonly ODataQueryContext _context_lowerCamelCased;
Expand Down Expand Up @@ -82,7 +84,19 @@ public SelectExpandBinderTest()
QueryOrder order = new QueryOrder { Id = 42, Title = "The order", Customer = customer };
customer.Orders.Add(order);

QueryCustomer customer1 = new QueryCustomer
ElizabethOkerio marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us stick to var for consistency.

{
Orders = new List<QueryOrder>()
};

QueryOrder order1 = new QueryOrder { Id = 42, Title = "The order", Customer = customer1 };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us stick to var for consistency.

QueryOrder order2 = null;

customer1.Orders.Add(order1);
customer1.Orders.Add(order2);

_queryable = new[] { customer }.AsQueryable();
_queryable1 = new[] { customer1 }.AsQueryable();

SelectExpandQueryOption selectExpandQueryOption = new SelectExpandQueryOption("Orders", expand: null, context: _context);

Expand Down Expand Up @@ -257,6 +271,139 @@ public void Bind_GeneratedExpression_ContainsExpandedObject()
Assert.Same(_queryable.First(), innerInnerCustomer.Instance);
}

[Theory]
[InlineData(HandleNullPropagationOptionHelper.EntityFrameworkQueryProviderNamespace)]
[InlineData(HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEFCore2)]
[InlineData(HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF5)]
[InlineData(HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF6)]
public void Bind_UsingEFQueryProvider_GeneratedExpression__DoesNot_ContainExpandedObject(string queryProvider)
{
// Arrange
SelectExpandQueryOption selectExpand = new SelectExpandQueryOption("Orders", "Orders,Orders($expand=Customer)", _context);

// Act
SelectExpandBinder binder = new SelectExpandBinder();
_queryBinderContext.QueryProvider = queryProvider;
IQueryable queryable = binder.ApplyBind(_queryable1, selectExpand.SelectExpandClause, _queryBinderContext);

// Assert
IEnumerator enumerator = queryable.GetEnumerator();
Assert.True(enumerator.MoveNext());
var partialCustomer = Assert.IsAssignableFrom<SelectExpandWrapper<QueryCustomer>>(enumerator.Current);
Assert.False(enumerator.MoveNext());
Assert.Null(partialCustomer.Instance);
Assert.Equal("Microsoft.AspNetCore.OData.Tests.Query.Expressions.QueryCustomer", partialCustomer.InstanceType);
IEnumerable<SelectExpandWrapper<QueryOrder>> innerOrders = partialCustomer.Container
.ToDictionary(PropertyMapper)["Orders"] as IEnumerable<SelectExpandWrapper<QueryOrder>>;
Assert.NotNull(innerOrders);
SelectExpandWrapper<QueryOrder> partialOrder = innerOrders.FirstOrDefault();

// We only write structural properties to the instance.
// This means that navigation properties on the instance property will be null
// when using any instance of EF query provider, and all structural properties
// will be assigned.
Assert.Null(partialOrder.Instance.Customer);
Assert.NotEqual(0, partialOrder.Instance.Id);
Assert.NotNull(partialOrder.Instance.Title);

object customer = partialOrder.Container.ToDictionary(PropertyMapper)["Customer"];
SelectExpandWrapper<QueryCustomer> innerInnerCustomer = Assert.IsAssignableFrom<SelectExpandWrapper<QueryCustomer>>(customer);

Assert.Null(innerInnerCustomer.Instance.Orders);
Assert.Equal(2, innerInnerCustomer.Instance.TestReadonlyProperty.Count);
Assert.Equal("Test1", innerInnerCustomer.Instance.TestReadonlyProperty[0]);
Assert.Equal("Test2", innerInnerCustomer.Instance.TestReadonlyProperty[1]);
Assert.Equal(2, innerInnerCustomer.Instance.TestReadOnlyWithAttribute);
}

[Theory]
[InlineData(HandleNullPropagationOptionHelper.EntityFrameworkQueryProviderNamespace)]
[InlineData(HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEFCore2)]
[InlineData(HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF5)]
[InlineData(HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF6)]
public void Bind_UsingEFQueryProvider_LowerCamelCasedModel_GeneratedExpression__DoesNot_ContainExpandedObject(string queryProvider)
{
// Arrange
SelectExpandQueryOption selectExpand = new SelectExpandQueryOption("Orders", "Orders,Orders($expand=Customer)", _context_lowerCamelCased);

// Act
SelectExpandBinder binder = new SelectExpandBinder();
_queryBinderContext_lowerCamelCased.QueryProvider = queryProvider;
IQueryable queryable = binder.ApplyBind(_queryable_lowerCamelCased, selectExpand.SelectExpandClause, _queryBinderContext_lowerCamelCased);

// Assert
IEnumerator enumerator = queryable.GetEnumerator();
Assert.True(enumerator.MoveNext());
var partialCustomer = Assert.IsAssignableFrom<SelectExpandWrapper<QueryCustomer>>(enumerator.Current);
Assert.False(enumerator.MoveNext());
Assert.Null(partialCustomer.Instance);
Assert.Equal("Microsoft.AspNetCore.OData.Tests.Query.Expressions.QueryCustomer", partialCustomer.InstanceType);
IEnumerable<SelectExpandWrapper<QueryOrder>> innerOrders = partialCustomer.Container
.ToDictionary(PropertyMapper)["orders"] as IEnumerable<SelectExpandWrapper<QueryOrder>>;
Assert.NotNull(innerOrders);
SelectExpandWrapper<QueryOrder> partialOrder = innerOrders.FirstOrDefault();

// We only write structural properties to the instance.
// This means that navigation properties on the instance property will be null
// when using any instance of EF query provider, and all structural properties
// will be assigned.
Assert.Null(partialOrder.Instance.Customer);
Assert.NotEqual(0, partialOrder.Instance.Id);
Assert.NotNull(partialOrder.Instance.Title);

object customer = partialOrder.Container.ToDictionary(PropertyMapper)["customer"];
SelectExpandWrapper<QueryCustomer> innerInnerCustomer = Assert.IsAssignableFrom<SelectExpandWrapper<QueryCustomer>>(customer);

Assert.Null(innerInnerCustomer.Instance.Orders);
}

[Fact]
public void Bind_SelectAndExpand_WithNullProperties_DoesNotThrowException()
{
// Arrange
IQueryable<User> users;
string expand = "FileRefNavigation";

User user = new User
{
UserId = 1,
Name = "Alex",
Age = 35,
DataFileRef = null,
FileRefNavigation = null
};

users = new[] { user }.AsQueryable();
ODataQueryContext context = new ODataQueryContext(_model, typeof(User)) { RequestContainer = new MockServiceProvider() };

SelectExpandQueryOption selectExpand = new SelectExpandQueryOption(select: null, expand: expand, context: context);

QueryBinderContext queryBinderContext = new QueryBinderContext(_model, _settings, selectExpand.Context.ElementClrType)
{
NavigationSource = context.NavigationSource
};

queryBinderContext.QueryProvider = HandleNullPropagationOptionHelper.EntityFrameworkQueryProviderNamespace;

// Act
SelectExpandBinder binder = new SelectExpandBinder();
IQueryable queryable = binder.ApplyBind(users, selectExpand.SelectExpandClause, queryBinderContext);

// Assert
Assert.NotNull(queryable);

IEnumerator enumerator = queryable.GetEnumerator();
Assert.True(enumerator.MoveNext());
var usr = Assert.IsAssignableFrom<SelectExpandWrapper<User>>(enumerator.Current);
Assert.False(enumerator.MoveNext());
Assert.NotNull(usr.Instance);
Assert.Equal("Microsoft.AspNetCore.OData.Tests.Query.Expressions.User", usr.Instance.GetType().ToString());
IEnumerable<SelectExpandWrapper<DataFile>> fileRefsNavigations = usr.Container
.ToDictionary(PropertyMapper)["FileRefNavigation"] as IEnumerable<SelectExpandWrapper<DataFile>>;

Assert.Null(fileRefsNavigations);
}

[Fact]
public void Bind_GeneratedExpression_CheckNullObjectWithinChainProjectionByKey()
{
Expand Down Expand Up @@ -1996,6 +2143,7 @@ public static IEdmModel GetEdmModel()
builder.EntitySet<QueryOrder>("Orders");
builder.EntitySet<QueryCity>("Cities");
builder.EntitySet<QueryProduct>("Products");
builder.EntitySet<User>("Users");

customer.Collection.Function("IsUpgraded").Returns<bool>().Namespace="NS";
customer.Collection.Action("UpgradeAll").Namespace = "NS";
Expand Down Expand Up @@ -2143,9 +2291,9 @@ public class QueryCustomer

public IList<QueryAddress> Addresses { get; set; }

public QueryOrder PrivateOrder { get; set; }
public QueryOrder? PrivateOrder { get; set; }

public IList<QueryOrder> Orders { get; set; }
public IList<QueryOrder>? Orders { get; set; }

public List<string> TestReadonlyProperty
{
Expand Down Expand Up @@ -2206,4 +2354,27 @@ public enum QueryColor

Blue
}

public class User
{
[Key]
public int UserId { get; set; }
[Required]
public string Name { get; set; }
[Required]
public int Age { get; set; }

//Navigations
[ForeignKey("FileRefNavigation")]
public int? DataFileRef { get; set; }
public DataFile? FileRefNavigation { get; set; }
}

public class DataFile
{
[Key]
public int FileId { get; set; }
[Required]
public string FileName { get; set; }
}
}