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

Fixes #1286: String Collections support in $select. #1282

Merged
merged 50 commits into from
Nov 3, 2024

Conversation

anasik
Copy link

@anasik anasik commented Jul 16, 2024

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 $select that 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.

  • With SQL Server this works perfectly fine for all collections except for strings.
  • With SQLite, pretty much all primitive collections fail.

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

@anasik
Copy link
Author

anasik commented Jul 17, 2024

@microsoft-github-policy-service agree

@anasik anasik marked this pull request as ready for review July 17, 2024 18:12
@anasik anasik changed the title Primitive Collections support in $select. String Collections support in $select. Jul 17, 2024
@anasik anasik changed the title String Collections support in $select. Fixes issue#1286: String Collections support in $select. Jul 18, 2024
@anasik anasik changed the title Fixes issue#1286: String Collections support in $select. Fixes #1286: String Collections support in $select. Jul 18, 2024
@xuzhg xuzhg self-requested a review July 18, 2024 18:13
@xuzhg
Copy link
Member

xuzhg commented Jul 18, 2024

Do you mind to add tests

@anasik
Copy link
Author

anasik commented Jul 18, 2024

Do you mind to add tests

I can. I'm just not sure where. Should I create a folder named "Strings" under E2E?

@anasik
Copy link
Author

anasik commented Jul 20, 2024

@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?

@xuzhg
Copy link
Member

xuzhg commented Jul 23, 2024

I'm just not sure where. Should I create a folder named "Strings" unde

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?

@xuzhg
Copy link
Member

xuzhg commented Jul 23, 2024

@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?

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.
However, We'd make sure the CI/CD can handle it. You can try that and let's review it.

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?

@habbes
Copy link
Contributor

habbes commented Jul 24, 2024

@xuzhg AspNetCore OData 9 preview uses .NET 8 as the target framework

@anasik anasik closed this Jul 24, 2024
@anasik anasik reopened this Jul 24, 2024
@anasik
Copy link
Author

anasik commented Jul 24, 2024

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. However, We'd make sure the CI/CD can handle it. You can try that and let's review it.

@xuzhg, Yeah I have already added the NET8 target framework locally, I just wanted to ask first.

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?

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.

Can you find a folder to add the tests within the E2E test?

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.

@anasik
Copy link
Author

anasik commented Jul 25, 2024

@xuzhg,

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 E2E folder:

dotnet test --filter DisplayName~ListsTest -f net8.0

A bunch of observations you could make here:

  • I added SQLite to the project packages and a *.db file. That's a SQLite DB. I had to include it because the bug in question is limited to SQL. I originally encountered it on an SQL Server instance but SQLite seemed much more friendly for automated testing.
  • I also added some more cases to the SelectExpandBinder file. That's because when working with SQLite, I noticed that, unlike SQL Server, it wasn't just List<string> that was failing but also every other primitive collection.
  • For some of the test theories I created, I left the List<Uri> case commented out because it's causing the tests to fail. Even in the original description I added to these PRs, I hade made some observations about some undefined behaviour around the Uri type and like the other problems, it's just a lot more pronounced in SQLite. Sadly, it felt a little too outside the scope of this branch for me to continue investigating just yet.
  • Similarly, for the $filter theory, I also commented out the List<DateTimeOffset> tests. It doesn't seem like they can be passed since the exception thrown insists on using client evaluation.
  • For both of these cases, I left the commented cases stay in case it's possible to pass them later.

I'm gonna maybe see if I can clean up a bit more

@anasik anasik force-pushed the fix/primitive-collections branch from 816ca7d to 411cf73 Compare July 25, 2024 17:00
if (isCollection)
{
if (elementType == typeof(string)
|| elementType == typeof(Uri)
|| elementType == typeof(DateTime)
#if NET6_0_OR_GREATER
Copy link
Contributor

Choose a reason for hiding this comment

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

In my view, this is a good indicator that we should consider moving this feature to AspNetCoreOData 9.

Copy link
Author

@anasik anasik Jul 26, 2024

Choose a reason for hiding this comment

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

@habbes. I am currently architecting a large-scale project that heavily uses primitive collections. It was in the middle of work that I both discovered and fixed this. As of now, we are using a modified package reference in our repository to get around this. If we fix it in AspNetCoreOData 8, we will no longer have to do that.

We can obviously continue to do so and we could even start using the 9.x preview for our purposes but I'm just highlighting that this is an issue that affects production and it's already been addressed sufficiently here. For a week or so, I have virtually only been heavily testing this to ensure there's no negative effects and I plan to do a little more testing on my own to be even more sure.

There could be other projects like mine out there. Some of them closer to completion than others where upgrading from net8 to net9 may not be feasible right now.

That being said, you'd have seen that initially I made the whole block conditional for only net8 but then I had to remove that condition when I realized that the project doesn't target net8 and therefore that block would be ignored during compilation although we can also conclude that the block wouldn't affect anything below net8 since it only affects primitive collections which aren't supported below net8 anyway.

Furthermore, net8 is LTS and supported till November 2026 whereas net9 is not only STS but also supported a few months shy of net8 (May 2026 I think?).

Anyone currently experiencing this bug will have to not only upgrade to the 9.x preview right away, then upgrade to 9.0 in November, but also to upgrade to net10 sooner than they would otherwise have to 😁

@@ -253,8 +253,8 @@ public async Task QueryEntitySelect(string format, string select)
[InlineData("application/json;odata.metadata=full", "ListTestDouble", "/all(d: d gt 5.5)", 2)]
[InlineData("application/json;odata.metadata=full", "ListTestInt", "/any(i: i le 5)", 2)]
[InlineData("application/json;odata.metadata=full", "ListTestInt", "/all(i: i le 5)", 1)]
[InlineData("application/json;odata.metadata=full", "ListTestDateTime", "/any(d: d gt 2024-07-26T00:00:00Z)", 4)]
[InlineData("application/json;odata.metadata=full", "ListTestDateTime", "/all(d: d gt 2024-07-26T00:00:00Z)", 4)]
//[InlineData("application/json;odata.metadata=full", "ListTestDateTime", "/any(d: d gt 2024-07-26T00:00:00Z)", 4)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these commented out?

Copy link
Author

Choose a reason for hiding this comment

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

For some of the test theories I created, I left the List<Uri> case commented out because it's causing the tests to fail. Even in the original description I added to these PRs, I hade made some observations about some undefined behavior around the Uri type and like the other problems, it's just a lot more pronounced in SQLite. Sadly, it felt a little too outside the scope of this branch for me to continue investigating just yet.
Similarly, for the $filter theory, I also commented out the List<DateTimeOffset> tests. It doesn't seem like they can be passed since the exception thrown insists on using client evaluation.

I explained this here in the last comment. But basically:

  • Both DateTimeOffset and Uri are a little more complicated than the rest. I've been experiencing a lot of weird behavior with List<Uri> even in the absence of OData. I've been meaning to investigate this a bit more but I feel like it lies outside the scope of this PR.
  • If you run the commented out List<DateTimeOffset> tests, you'd see a very verbose exception which instructs you to convert the DbSet to IEnumerable or IList before querying, which basically means that it's both not exactly a bug as much as an unimplemented feature and also outside the scope of this PR.
  • To add a little more context, deferred execution is a prerequisite for this PR because the bug in question cannot be recreated with in-memory lists. It happens only when we attempt to run certain queries directly on the DbSet. Which is why an exception telling me to convert the DbSet to an Enumerable is effectively telling me to move on.

Since I plan to at least attempt to address both of these issues in a subsequent PR, I chose to comment these tests out instead of straight up deleting them so that I can just pass them later.

I think a couple of them might actually pass if the test was using SQL Server instead of SQLite, but then again, half of these cases won't even be needed on SQL Server. Furthermore, even on SQL Server, List<Uri> behaves weirdly. Except it just doesn't crash. Just gives wrong/bad output.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the context could be added to the comments so that it's clear from reading the code why the code is commented and what the pending work is to address them.

@WanjohiSammy WanjohiSammy requested review from marabooy, habbes, xuzhg and gathogojr and removed request for marabooy, habbes, xuzhg and gathogojr September 30, 2024 11:41
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
anasik and others added 5 commits September 30, 2024 20:45
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
….OData.E2E.Tests.csproj

Co-authored-by: Samuel Wanjohi <WanjohiSammy@users.noreply.github.com>
@anasik
Copy link
Author

anasik commented Oct 31, 2024

@WanjohiSammy, is there an ETA on this?

{
return type.IsPrimitive
|| type == typeof(string)
|| type == typeof(Uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Team had a concern about Uri
We currently do not support Uri as a primitive type @gathogojr

Copy link
Author

@anasik anasik Nov 2, 2024

Choose a reason for hiding this comment

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

@WanjohiSammy Then why didn't you just say so? thought we'd already discussed this. But lets discuss it again.

Personally, I don't consider even string a primitive type, and neither does anyone else in our profession. But for the purpose of the task at hand, it is and the documentation for ASP NET Core says so. Just like it says the same thing for Uri. If it's too concerning, we can remove this for now and wait for someone else to attempt to add it back a few PRs down the line.

With all due respect sir, this is actually a pretty major bug, on our last stable version, that has been fixed since June but we are not rolling it out for unknown reasons. Me and my team are personally suffering from this enough to be using a fork in our project. And there could be people out there who have been experiencing this ever since it was first released.

All I am saying is I would appreciate us doing anything we can to move things along. Which means that if a line of code that adds an extra case for the URL type bothers the team so much, I don't mind removing it because that way the 99 other types don't retain the bug for another 6 months.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WanjohiSammy Then why didn't you just say so? thought we'd already discussed this. But lets discuss it again.

Personally, I don't consider even string a primitive type, and neither does anyone else in our profession. But for the purpose of the task at hand, it is and the documentation for ASP NET Core says so. Just like it says the same thing for Uri. If it's too concerning, we can remove this for now and wait for someone else to attempt to add it back a few PRs down the line.

With all due respect sir, this is actually a pretty major bug, on our last stable version, that has been fixed since June but we are not rolling it out for unknown reasons. Me and my team are personally suffering from this enough to be using a fork in our project. And there could be people out there who have been experiencing this ever since it was first released.

All I am saying is I would appreciate us doing anything we can to move things along. Which means that if a line of code that adds an extra case for the URL type bothers the team so much, I don't mind removing it because that way the 99 other types don't retain the bug for another 6 months.

This looks good to me.
@gathogojr, @habbes what do you think about this change.
@anasik apologies

@WanjohiSammy WanjohiSammy merged commit d9fe744 into OData:release-8.x Nov 3, 2024
1 check passed
@anasik anasik deleted the fix/primitive-collections branch November 4, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$select doesn't work on fields of type IList<string>
4 participants