Skip to content

Commit

Permalink
Fix invalid reflection being used when sorting in EF Core 3.0, see #5
Browse files Browse the repository at this point in the history
  • Loading branch information
GeorgDangl committed Oct 16, 2019
1 parent 6b62d7a commit 5343116
Show file tree
Hide file tree
Showing 9 changed files with 185 additions and 32 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

All notable changes to **LightQuery** are documented here.

## v1.7.1:
- Fixed a bug that caused Entity Framework Core to throw an `InvalidOperationException` when sorting was applied to projections to a class that inherited from the base query type. The error was an incorrectly used reflection invocation to determine the type the query applies to

## v1.7.0:
- Add support for ASP.NET Core 3.0

Expand Down
23 changes: 12 additions & 11 deletions src/LightQuery.Shared/QueryableProcessor.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
Expand All @@ -9,30 +9,32 @@ namespace LightQuery.Shared
{
public static class QueryableProcessor
{
private static PropertyInfo GetPropertyInfoRecursively(this IQueryable queryable, string propName)
private static (Type declaringType, PropertyInfo property) GetPropertyInfoRecursively(this IQueryable queryable, string propName)
{
string[] nameParts = propName.Split('.');
if (nameParts.Length == 1)
{
return queryable.ElementType.GetTypeInfo().GetProperty(CamelizeString(propName)) ?? queryable.ElementType.GetTypeInfo().GetProperty(propName);
var property = queryable.ElementType.GetTypeInfo().GetProperty(CamelizeString(propName)) ?? queryable.ElementType.GetTypeInfo().GetProperty(propName);
return (property?.DeclaringType, property);
}

//Getting Root Property - Ex : propName : "User.Name" -> User
var propertyInfo = queryable.ElementType.GetTypeInfo().GetProperty(CamelizeString(nameParts[0])) ?? queryable.ElementType.GetTypeInfo().GetProperty(nameParts[0]);
var originalDeclaringType = propertyInfo.DeclaringType;

This comment has been minimized.

Copy link
@smitpatel

smitpatel Oct 16, 2019

propertyInfo can be null so this could throw NRE.

This comment has been minimized.

Copy link
@GeorgDangl

GeorgDangl Oct 16, 2019

Author Owner

Thank you for discovering that! It's fixed with 42d120d, release v1.7.2 should be available shorty.

if (propertyInfo == null)
{
return null;
return (null, null);
}

for (int i = 1; i < nameParts.Length; i++)
{
propertyInfo = propertyInfo.PropertyType.GetProperty(CamelizeString(nameParts[i])) ?? propertyInfo.PropertyType.GetProperty(nameParts[i]);
if (propertyInfo == null)
{
return null;
return (null, null);
}
}
return propertyInfo;
return (originalDeclaringType, propertyInfo);
}

private static LambdaExpression CreateExpression(Type type, string propertyName)
Expand Down Expand Up @@ -63,12 +65,13 @@ public static IQueryable ApplySorting(this IQueryable queryable, QueryOptions qu
}

var orderingProperty = GetPropertyInfoRecursively(queryable, queryOptions.SortPropertyName);
if (orderingProperty == null)
if (orderingProperty.declaringType == null
|| orderingProperty.property == null)
{
return queryable;
}

var orderByExp = CreateExpression(queryable.ElementType, queryOptions.SortPropertyName);
var orderByExp = CreateExpression(orderingProperty.declaringType, queryOptions.SortPropertyName);
if (orderByExp == null)
{
return queryable;
Expand All @@ -79,7 +82,7 @@ public static IQueryable ApplySorting(this IQueryable queryable, QueryOptions qu
queryable = queryable.WrapInNullChecksIfAccessingNestedProperties(queryable.ElementType, queryOptions.SortPropertyName);
var wrappedExpression = Expression.Call(typeof(Queryable),
orderMethodName,
new[] { queryable.ElementType, orderingProperty.PropertyType },
new [] { orderingProperty.declaringType, orderingProperty.property.PropertyType },
queryable.Expression,
Expression.Quote(orderByExp));
var result = queryable.Provider.CreateQuery(wrappedExpression);
Expand All @@ -100,7 +103,6 @@ private static IQueryable WrapInNullChecksIfAccessingNestedProperties(this IQuer
// queryable
// .Where(x => x.Product != null)
// .Where(x => x.Product.Data != null)

for (var i = 0; i < members.Length - 1; i++)
{
var member = members[i];
Expand All @@ -114,7 +116,6 @@ private static IQueryable WrapInNullChecksIfAccessingNestedProperties(this IQuer
var memberPath = members
.TakeWhile((mem, index) => index <= i)
.Aggregate((c, n) => c + "." + n);
var propertyType = GetPropertyInfoRecursively(queryable, memberPath).PropertyType;
var notNullExpression = Expression.NotEqual(body, Expression.Constant(null));
var notNullLambda = Expression.Lambda(notNullExpression, param);
var whereMethodName = nameof(Queryable.Where);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,15 +324,13 @@ public async Task CanOverrideDefaultSort()
}

[Fact]
public async Task CanFilterByNestedProperty()
public async Task CanSortByNestedProperty()
{
var url = "AsyncLightQueryWithDefaultSort?sort=favoriteAnimal.name";
var actualResponse = await GetResponse<List<User>>(url);
for (var i = 1; i < actualResponse.Count; i++)
{
var previousValueIsSmaller = actualResponse[i].UserName.CompareTo(actualResponse[i - 1].UserName) > 0;
Assert.False(previousValueIsSmaller);
}
Assert.Equal("Joe", actualResponse[0].UserName);
Assert.Equal("Iris", actualResponse[1].UserName);
Assert.Equal("Hank", actualResponse[2].UserName);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -250,17 +250,31 @@ public async Task CanOverrideDefaultSort()
}

[Fact]
public async Task CanFilterByNestedProperty()
public async Task CanSortByNestedProperty()
{
var url = "AsyncPaginatedLightQueryWithDefaultSort?sort=favoriteAnimal.name&page=1";
var pagedResult = await GetResponse<PaginationResult<User>>(url);
Assert.Equal(10, pagedResult.TotalCount);
Assert.Equal(1, pagedResult.Page);
Assert.Equal(3, pagedResult.PageSize);
Assert.Equal(3, pagedResult.Data.Count);
Assert.Contains(pagedResult.Data, u => u.UserName == "Joe");
Assert.Contains(pagedResult.Data, u => u.UserName == "Iris");
Assert.Contains(pagedResult.Data, u => u.UserName == "Hank");
Assert.Equal("Joe", pagedResult.Data[0].UserName);
Assert.Equal("Iris", pagedResult.Data[1].UserName);
Assert.Equal("Hank", pagedResult.Data[2].UserName);
}

[Fact]
public async Task CanSortByNestedPropertyDescending()
{
var url = "AsyncPaginatedLightQueryWithDefaultSort?sort=favoriteAnimal.name desc&page=1";
var pagedResult = await GetResponse<PaginationResult<User>>(url);
Assert.Equal(10, pagedResult.TotalCount);
Assert.Equal(1, pagedResult.Page);
Assert.Equal(3, pagedResult.PageSize);
Assert.Equal(3, pagedResult.Data.Count);
Assert.Equal("Alice", pagedResult.Data[0].UserName);
Assert.Equal("Bob", pagedResult.Data[1].UserName);
Assert.Equal("Caroline", pagedResult.Data[2].UserName);
}

[Fact]
Expand Down
1 change: 0 additions & 1 deletion test/LightQuery.IntegrationTestsServer/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ public void ConfigureServices(IServiceCollection services)
services.AddDbContext<LightQueryContext>(options => options.UseInMemoryDatabase(_inMemoryDatabaseName));
services.AddMvc();


#if NETCORE3
services.AddMvc(mvcOptions => mvcOptions.EnableEndpointRouting = false); ;
#else
Expand Down
8 changes: 8 additions & 0 deletions test/LightQuery.Shared.Tests/LightQuery.Shared.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@
<PackageReference Include="XunitXml.TestLogger" Version="2.1.26" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)'!='netcoreapp3.0'">
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="2.1.3" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)'=='netcoreapp3.0'">
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="3.0.0" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\src\LightQuery.Shared\LightQuery.Shared.csproj" />
</ItemGroup>
Expand Down
118 changes: 118 additions & 0 deletions test/LightQuery.Shared.Tests/Regression/InheritedPropertiesSort.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
using Microsoft.Data.Sqlite;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Xunit;

namespace LightQuery.Shared.Tests.Regression
{
public class InheritedPropertiesSortWithSqlite
{
public class UserBase
{
public int Id { get; set; }
public string Name { get; set; }
public int? FavoriteRestaurantId { get; set; }
public Restaurant FavoriteRestaurant { get; set; }
}

public class User : UserBase { }

public class Restaurant
{
public int Id { get; set; }
public string Street { get; set; }
}

public class AppDbContext : DbContext
{
public AppDbContext(DbContextOptions options) : base(options)
{
}

public DbSet<UserBase> Users { get; set; }
}

private static Dictionary<string, SqliteConnection> _connections = new Dictionary<string, SqliteConnection>();

private static async Task<AppDbContext> GetContextAsync()
{
var connectionId = Guid.NewGuid().ToString();
var connectionString = new SqliteConnectionStringBuilder
{
DataSource = $"in-memory-{connectionId}.db",
Cache = SqliteCacheMode.Shared,
Mode = SqliteOpenMode.Memory
}.ToString();
var connection = new SqliteConnection(connectionString);
await connection.OpenAsync();
_connections.Add(connectionId, connection);

var serviceProvider = new ServiceCollection()
.AddDbContext<AppDbContext>(options => options
.UseSqlite(connectionString))
.BuildServiceProvider();
using (var setupContext = serviceProvider.CreateScope().ServiceProvider.GetRequiredService<AppDbContext>())
{
await setupContext.Database.EnsureCreatedAsync();
setupContext.Users.Add(new UserBase { Name = "George", FavoriteRestaurant = new Restaurant { Street = "Main Street" } });
setupContext.Users.Add(new UserBase { Name = "Steve", FavoriteRestaurant = new Restaurant { Street = "Forest Street" } });
setupContext.Users.Add(new UserBase { Name = "Bob" });
await setupContext.SaveChangesAsync();
}

return serviceProvider.GetRequiredService<AppDbContext>();
}

[Fact]
public async Task CanSortOnPropertyFromBaseClassWhenProjectedToDerivedClass()
{
var context = await GetContextAsync();

var usersQueryable = context.Users
.Select(u => new User
{
Name = u.Name
});

var queryOptions = new QueryOptions
{
SortPropertyName = nameof(User.Name)
};

var orderedQuery = QueryableProcessor.ApplySorting(usersQueryable, queryOptions);

dynamic dynamicOrderedQuery = orderedQuery;

var count = Queryable.Count(dynamicOrderedQuery);

Assert.Equal(3, count);
}

[Fact]
public async Task CanSortOnNestedProperty()
{
// This test ensures that relational sorting works with SQLiteS
var context = await GetContextAsync();

var usersQueryable = context.Users;
var queryOptions = new QueryOptions
{
SortPropertyName = "FavoriteRestaurant.Street"
};

var orderedQuery = QueryableProcessor.ApplySorting(usersQueryable, queryOptions);

dynamic dynamicOrderedQuery = orderedQuery;

var count = Queryable.Count(dynamicOrderedQuery);

// It should only have two results since the added null checks remove
// the user with the missing FavoriteRestaurant
Assert.Equal(2, count);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -324,15 +324,13 @@ public async Task CanOverrideDefaultSort()
}

[Fact]
public async Task CanFilterByNestedProperty()
public async Task CanSortByNestedProperty()
{
var url = "LightQueryWithDefaultSort?sort=favoriteAnimal.name";
var actualResponse = await GetResponse<List<User>>(url);
for (var i = 1; i < actualResponse.Count; i++)
{
var previousValueIsSmaller = actualResponse[i].UserName.CompareTo(actualResponse[i - 1].UserName) > 0;
Assert.False(previousValueIsSmaller);
}
Assert.Equal("Joe", actualResponse[0].UserName);
Assert.Equal("Iris", actualResponse[1].UserName);
Assert.Equal("Hank", actualResponse[2].UserName);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -250,17 +250,31 @@ public async Task CanOverrideDefaultSort()
}

[Fact]
public async Task CanFilterByNestedProperty()
public async Task CanSortByNestedProperty()
{
var url = "PaginatedLightQueryWithDefaultSort?sort=favoriteAnimal.name&page=1";
var pagedResult = await GetResponse<PaginationResult<User>>(url);
Assert.Equal(10, pagedResult.TotalCount);
Assert.Equal(1, pagedResult.Page);
Assert.Equal(3, pagedResult.PageSize);
Assert.Equal(3, pagedResult.Data.Count);
Assert.Contains(pagedResult.Data, u => u.UserName == "Joe");
Assert.Contains(pagedResult.Data, u => u.UserName == "Iris");
Assert.Contains(pagedResult.Data, u => u.UserName == "Hank");
Assert.Equal("Joe", pagedResult.Data[0].UserName);
Assert.Equal("Iris", pagedResult.Data[1].UserName);
Assert.Equal("Hank", pagedResult.Data[2].UserName);
}

[Fact]
public async Task CanSortByNestedPropertyDescending()
{
var url = "PaginatedLightQueryWithDefaultSort?sort=favoriteAnimal.name desc&page=1";
var pagedResult = await GetResponse<PaginationResult<User>>(url);
Assert.Equal(10, pagedResult.TotalCount);
Assert.Equal(1, pagedResult.Page);
Assert.Equal(3, pagedResult.PageSize);
Assert.Equal(3, pagedResult.Data.Count);
Assert.Equal("Alice", pagedResult.Data[0].UserName);
Assert.Equal("Bob", pagedResult.Data[1].UserName);
Assert.Equal("Caroline", pagedResult.Data[2].UserName);
}

[Fact]
Expand Down

0 comments on commit 5343116

Please sign in to comment.