From 534311670dd9e65d910266c02aef6e02143b7387 Mon Sep 17 00:00:00 2001 From: Georg Dangl Date: Wed, 16 Oct 2019 11:19:04 +0200 Subject: [PATCH] Fix invalid reflection being used when sorting in EF Core 3.0, see #5 --- CHANGELOG.md | 3 + src/LightQuery.Shared/QueryableProcessor.cs | 23 ++-- .../AsyncLightQueryControllerTests.cs | 10 +- ...AsyncPaginatedLightQueryControllerTests.cs | 22 +++- .../Startup.cs | 1 - .../LightQuery.Shared.Tests.csproj | 8 ++ .../Regression/InheritedPropertiesSort.cs | 118 ++++++++++++++++++ .../LightQueryControllerTests.cs | 10 +- .../PaginatedLightQueryControllerTests.cs | 22 +++- 9 files changed, 185 insertions(+), 32 deletions(-) create mode 100644 test/LightQuery.Shared.Tests/Regression/InheritedPropertiesSort.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bb4dcb..3bd806f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/LightQuery.Shared/QueryableProcessor.cs b/src/LightQuery.Shared/QueryableProcessor.cs index 123747b..7ffe295 100644 --- a/src/LightQuery.Shared/QueryableProcessor.cs +++ b/src/LightQuery.Shared/QueryableProcessor.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections; using System.Collections.Generic; using System.Linq; @@ -9,19 +9,21 @@ 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; if (propertyInfo == null) { - return null; + return (null, null); } for (int i = 1; i < nameParts.Length; i++) @@ -29,10 +31,10 @@ private static PropertyInfo GetPropertyInfoRecursively(this IQueryable queryable 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) @@ -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; @@ -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); @@ -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]; @@ -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); diff --git a/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncLightQueryControllerTests.cs b/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncLightQueryControllerTests.cs index 034eb31..68eef5c 100644 --- a/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncLightQueryControllerTests.cs +++ b/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncLightQueryControllerTests.cs @@ -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>(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); } } } diff --git a/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncPaginatedLightQueryControllerTests.cs b/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncPaginatedLightQueryControllerTests.cs index fa3c2d5..d39e00f 100644 --- a/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncPaginatedLightQueryControllerTests.cs +++ b/test/LightQuery.EntityFrameworkCore.Tests.Integration/ControllerTests/AsyncPaginatedLightQueryControllerTests.cs @@ -250,7 +250,7 @@ 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>(url); @@ -258,9 +258,23 @@ public async Task CanFilterByNestedProperty() 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>(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] diff --git a/test/LightQuery.IntegrationTestsServer/Startup.cs b/test/LightQuery.IntegrationTestsServer/Startup.cs index cef11cb..872f055 100644 --- a/test/LightQuery.IntegrationTestsServer/Startup.cs +++ b/test/LightQuery.IntegrationTestsServer/Startup.cs @@ -13,7 +13,6 @@ public void ConfigureServices(IServiceCollection services) services.AddDbContext(options => options.UseInMemoryDatabase(_inMemoryDatabaseName)); services.AddMvc(); - #if NETCORE3 services.AddMvc(mvcOptions => mvcOptions.EnableEndpointRouting = false); ; #else diff --git a/test/LightQuery.Shared.Tests/LightQuery.Shared.Tests.csproj b/test/LightQuery.Shared.Tests/LightQuery.Shared.Tests.csproj index f211c6e..17e5d3a 100644 --- a/test/LightQuery.Shared.Tests/LightQuery.Shared.Tests.csproj +++ b/test/LightQuery.Shared.Tests/LightQuery.Shared.Tests.csproj @@ -16,6 +16,14 @@ + + + + + + + + diff --git a/test/LightQuery.Shared.Tests/Regression/InheritedPropertiesSort.cs b/test/LightQuery.Shared.Tests/Regression/InheritedPropertiesSort.cs new file mode 100644 index 0000000..e9b07bd --- /dev/null +++ b/test/LightQuery.Shared.Tests/Regression/InheritedPropertiesSort.cs @@ -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 Users { get; set; } + } + + private static Dictionary _connections = new Dictionary(); + + private static async Task 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(options => options + .UseSqlite(connectionString)) + .BuildServiceProvider(); + using (var setupContext = serviceProvider.CreateScope().ServiceProvider.GetRequiredService()) + { + 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(); + } + + [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); + } + } +} diff --git a/test/LightQuery.Tests.Integration/ControllerTests/LightQueryControllerTests.cs b/test/LightQuery.Tests.Integration/ControllerTests/LightQueryControllerTests.cs index 5de310f..fbb95a8 100644 --- a/test/LightQuery.Tests.Integration/ControllerTests/LightQueryControllerTests.cs +++ b/test/LightQuery.Tests.Integration/ControllerTests/LightQueryControllerTests.cs @@ -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>(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); } } } diff --git a/test/LightQuery.Tests.Integration/ControllerTests/PaginatedLightQueryControllerTests.cs b/test/LightQuery.Tests.Integration/ControllerTests/PaginatedLightQueryControllerTests.cs index 8e1d43b..652f10d 100644 --- a/test/LightQuery.Tests.Integration/ControllerTests/PaginatedLightQueryControllerTests.cs +++ b/test/LightQuery.Tests.Integration/ControllerTests/PaginatedLightQueryControllerTests.cs @@ -250,7 +250,7 @@ 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>(url); @@ -258,9 +258,23 @@ public async Task CanFilterByNestedProperty() 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>(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]