Fixes #1286: String Collections support in $select. #1282
Conversation
…per. This gets rid of the excetion resulting from selecting a List<String> field.
|
@microsoft-github-policy-service agree |
anasik
changed the title
anasik
changed the title
anasik
changed the title
|
Do you mind to add tests |
I can. I'm just not sure where. Should I create a folder named "Strings" under E2E? |
|
@xuzhg Since primitive collections only started getting supported as recent as EF Core 8, I'm gonna have to add net8 as a target framework and add a test that only runs on net8. Is that okay? |
We have 'Microsoft.AspNetCore.OData.Tests' for Unit test, and Microsoft.AspNetCore.OData.E2E.Tests for End to End test. Can you find a folder to add the tests within the E2E test? |
I am thinking it. Add '.NET8' target framework to test project will run all the test cases on this target framework. Basically, it's ok. By the way, can we mock the 'EF Core 8' primitive collection? Or can we add a sample project to play the EF Core 8? |
|
@xuzhg AspNetCore OData 9 preview uses .NET 8 as the target framework |
@xuzhg, Yeah I have already added the NET8 target framework locally, I just wanted to ask first.
I'm not quite sure what you mean by this but I did add a model for testing purposes that has all primitive collection fields.
I decided to just create a new one specifically for Lists. I explored a lot of existing folders and none seemed like a good enough candidate for such a specific case. |
…crash on SQLite so added other primitive checks back to SelectExpandBinder.
…nditional for net8
|
I have added the test. Assuming you have net8.0 installed on your machine, you can directly run said test through the following command inside the
A bunch of observations you could make here:
|
… weird lines from the csproj
anasik
force-pushed
the
fix/primitive-collections
branch
from
816ca7d
to
411cf73
Compare
… insists on deferring execution. Removed #if net8 condition since base project doesnt target net8 and instead wrapped DateOnly check in an #if net6
|
LGTM |
|
@WanjohiSammy, @xuzhg, I just wanted to ask, are you guys waiting on me for something on this? |
| /// </summary> | ||
| /// <param name="type">The type to validate.</param> | ||
| /// <returns>True if type is primitive or known type, otherwise False.</returns> | ||
| public static bool IsPrimitiveOrKnownType(Type type) |
There was a problem hiding this comment.
The primitive types covered here are not sufficient. You can refer to IsPrimitiveType(Type clrType) in ODL which contains all the types that we consider primitive both Nullable or Non-Nullable primitive types. Also looking at this method, you will notice that we do not consider Uri primitive type.
Please use it to rewrite this method.
There was a problem hiding this comment.
@WanjohiSammy, I am well-aware of the correct primitive types but I had to cover all the types that are not only affected by the bug but were also documented to support Primitive Collections in the EF Core 8 change log.
That being said, I took a look at said method it's not immediately evident to me exactly which types of mine you're concerned about. It appears that the only difference between that method and mine is that I explicitly added cases for some types that the other function might still support.
I am almost certain that there's not a single type mentioned there that wasn't first confirmed to be affected.
Since you've already mentioned Uri,
- Here it is being given express treatment therefore I had to support it.
- It's also affected by the bug at hand.
- In fact, this type is actually the most buggy of all, as I have mentioned several times in earlier interactions. In fact, I was hoping this would get merged quickly so I could debug Uri more.
There was a problem hiding this comment.
I think the confusion here stems from the fact that these two methods have similar names but serve very different purposes. The core IsPrimitiveType() test for types that can be mapped to EDM primitive types. But this method checks for types that should be treated as primitive collections in the context of EF core, without regards to EDM types. Maybe using a different name, more specific to this use case would alleviate the confusion. But I think it's fine to restrict this method to only the types that matter with respect to the EF core bug.
test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsEdmModel.cs
Outdated
Show resolved
Hide resolved
anasik
dismissed stale reviews from WanjohiSammy, habbes, and xuzhg
via
167b3e8
EF Core 8 comes with support for primtive collections in models.
Naturally, OData should pick up on that support. As of now, if you create a
List<string>field in your model and then fetch all the records for that model, it works flawlessly. You see an array of strings in the response.However, if you try to
$selectthat field, it fails with the following error:Click to expand error
``` System.NullReferenceException: Object reference not set to an instance of an object. at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.CreateGetValueExpression(ParameterExpression dbDataReader, Int32 index, Boolean nullable, RelationalTypeMapping typeMapping, Type type, IPropertyBase property) at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.VisitExtension(Expression extensionExpression) at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ProcessShaper(Expression shaperExpression, RelationalCommandCache& relationalCommandCache, IReadOnlyList`1& readerColumns, LambdaExpression& relatedDataLoaders, Int32& collectionId) at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.VisitExtension(Expression extensionExpression) at System.Linq.Expressions.ExpressionVisitor.VisitMemberAssignment(MemberAssignment node) at System.Linq.Expressions.ExpressionVisitor.VisitMemberBinding(MemberBinding node) at System.Linq.Expressions.ExpressionVisitor.Visit[T](ReadOnlyCollection`1 nodes, Func`2 elementVisitor) at System.Linq.Expressions.ExpressionVisitor.VisitMemberInit(MemberInitExpression node) at System.Linq.Expressions.ExpressionVisitor.VisitMemberAssignment(MemberAssignment node) at System.Linq.Expressions.ExpressionVisitor.VisitMemberBinding(MemberBinding node) at System.Linq.Expressions.ExpressionVisitor.Visit[T](ReadOnlyCollection`1 nodes, Func`2 elementVisitor) at System.Linq.Expressions.ExpressionVisitor.VisitMemberInit(MemberInitExpression node) at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ProcessShaper(Expression shaperExpression, RelationalCommandCache& relationalCommandCache, IReadOnlyList`1& readerColumns, LambdaExpression& relatedDataLoaders, Int32& collectionId) at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.VisitShapedQuery(ShapedQueryExpression shapedQueryExpression) at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.VisitExtension(Expression extensionExpression) at Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor.VisitExtension(Expression extensionExpression) at Microsoft.EntityFrameworkCore.Query.QueryCompilationContext.CreateQueryExecutor[TResult](Expression query) at Microsoft.EntityFrameworkCore.Storage.Database.CompileQuery[TResult](Expression query, Boolean async) at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async) at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass12_0`1.b__0() at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler) at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.ExecuteAsync[TResult](Expression query, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.ExecuteAsync[TResult](Expression expression, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1.GetAsyncEnumerator(CancellationToken cancellationToken) at System.Text.Json.Serialization.Converters.IAsyncEnumerableOfTConverter`2.OnWriteResume(Utf8JsonWriter writer, TAsyncEnumerable value, JsonSerializerOptions options, WriteStack& state) at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryWrite(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options, WriteStack& state) at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state) at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state) at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.SerializeAsync(Stream utf8Json, T rootValue, CancellationToken cancellationToken, Object rootValueBoxed) at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.SerializeAsync(Stream utf8Json, T rootValue, CancellationToken cancellationToken, Object rootValueBoxed) at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.SerializeAsync(Stream utf8Json, T rootValue, CancellationToken cancellationToken, Object rootValueBoxed) at Microsoft.AspNetCore.Mvc.Formatters.SystemTextJsonOutputFormatter.WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|30_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters() --- End of stack trace from previous location --- at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope) at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context) at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext) at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context) at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context) ```This happens because in
ProjectAsWrapper, we check if it's a collection, and if yes, we wrap it as a collection rather than an element.That's because queries of the form:
Entity().Select( d=> d.StringListProperty.Select(d=> d)))just inherently don't work. It appears to be a bug in EF Core that one can easily recreate without even needing to initialize OData.However, for our purposes, we can easily get around this by simply wrapping the List as an element as opposed to a collection.
I tested this as much as I could and so far I haven't discovered any negative side-effects of this. It's working perfectly fine and I am even able to filter on the string list using any/all .
Fixes #1286