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

Allow query operations on the underlying type when a value converter is defined #30197

Closed
glen-84 opened this issue Feb 2, 2023 · 10 comments
Closed
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@glen-84
Copy link

glen-84 commented Feb 2, 2023

Ask a question

I have a value object named ArticleTitle, that wraps a string. I also have the necessary type converter:

    .HasConversion(
        x => x.Value, // string
        x => ArticleTitle.From(x));

A query like this works:

dbContext.Articles.Where(a => a.Title == ArticleTitle.From(title))

But if you try to compare with the underlying type (string):

dbContext.Articles.Where(a => a.Title == title)

It fails with:

System.InvalidCastException: Invalid cast from 'System.String' to 'Articles.ArticleTitle'.

I don't see a reason why the underlying type could not be accepted, and passed directly to the database.

EF Core knows the underlying type of the property, and could simply accept that (in addition to the exact type).

This would allow us to perform queries without requiring value objects to be constructed, which could throw exceptions. It would also mean that a web/UI layer would not be forced to work with the value objects in the domain.

Include provider and version information

EF Core version: 7.0.2
Database provider: Pomelo.EntityFrameworkCore.MySql
Target framework: .NET 7.0
Operating system: Windows, WSL
IDE: n/a

@ajcvickers
Copy link
Member

@glen-84 Please post the full stack trace.

@glen-84
Copy link
Author

glen-84 commented Feb 2, 2023

Apologies.

An exception occurred while iterating over the results of a query for context type 'Api.Application.Database.ApplicationDbContext'.
System.InvalidCastException: Invalid cast from 'System.String' to 'Api.Modules.Editorial.Domain.Model.Articles.ArticleTitle'.
   at System.Convert.DefaultToType(IConvertible value, Type targetType, IFormatProvider provider)
   at Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter`2.Sanitize[T](Object value)
   at Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter`2.<>c__DisplayClass4_0`2.<SanitizeConverter>b__1(Object v)
   at Microsoft.EntityFrameworkCore.Storage.RelationalTypeMapping.GenerateSqlLiteral(Object value)
   at Microsoft.EntityFrameworkCore.Query.QuerySqlGenerator.VisitSqlConstant(SqlConstantExpression sqlConstantExpression)
   at Microsoft.EntityFrameworkCore.Query.SqlExpressionVisitor.VisitExtension(Expression extensionExpression)
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlQuerySqlGenerator.VisitExtension(Expression extensionExpression)
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlQuerySqlGenerator.VisitSqlBinary(SqlBinaryExpression sqlBinaryExpression)
   at Microsoft.EntityFrameworkCore.Query.SqlExpressionVisitor.VisitExtension(Expression extensionExpression)
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlQuerySqlGenerator.VisitExtension(Expression extensionExpression)
   at Microsoft.EntityFrameworkCore.Query.QuerySqlGenerator.VisitSelect(SelectExpression selectExpression)
   at Microsoft.EntityFrameworkCore.Query.QuerySqlGenerator.GenerateRootCommand(Expression queryExpression)
   at Microsoft.EntityFrameworkCore.Query.QuerySqlGenerator.GetCommand(Expression queryExpression)
   at Microsoft.EntityFrameworkCore.Query.Internal.RelationalCommandCache.GetRelationalCommandTemplate(IReadOnlyDictionary`2 parameters)
   at Microsoft.EntityFrameworkCore.Internal.RelationCommandCacheExtensions.RentAndPopulateRelationalCommand(RelationalCommandCache relationalCommandCache, RelationalQueryContext queryContext)
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.InitializeReaderAsync(AsyncEnumerator enumerator, CancellationToken cancellationToken)
   at Pomelo.EntityFrameworkCore.MySql.Storage.Internal.MySqlExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()

@glen-84
Copy link
Author

glen-84 commented Feb 3, 2023

@ajcvickers Do you know if this is a bug or a missing feature? I opened it as a question because I wasn't sure.

It's quite a big issue for us because we are making quite extensive use of value objects.

@ajcvickers
Copy link
Member

@glen-84 Do you have operator overloads defined? Because otherwise this won't compile. It would be a good idea to attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.

In the general case, this would require #10434. In simple cases the following can work:

context.Articles.Where(a => a.Title == (object)title)

But without understanding more about your specific case I don't know if this will help.

@glen-84
Copy link
Author

glen-84 commented Feb 5, 2023

@ajcvickers

Yes, we have operator overloads defined, and the project does compile. It is a runtime issue.

Simple reproduction here.

@ajcvickers
Copy link
Member

Note for triage: this is a case where the == operator is defined between string and the custom value object. We might be able to handle this when translating the expression tree.

@glen-84 An explicit cast can be used until we implement this kind of translation, assuming we decide to do so. For example:

var title = "title";
var queryable = context.Articles.Where(a => a.Title == (ArticleTitle)title);

@glen-84
Copy link
Author

glen-84 commented Feb 6, 2023

The cast will invoke the From method, which will run validation on the value object (potentially throwing an exception). We'd like to avoid this.

Would you be open to a PR, if I could somehow figure out how to implement this? (I may not be capable though)

@ajcvickers
Copy link
Member

@glen-84 I will need to discuss with the team.

@ajcvickers
Copy link
Member

@glen-84 We discussed this and came to the conclusion that handling this case requires conversion on one side or the other. From my testing, this works:

var title = "title";
var queryable = context.Articles.Where(a => (string)a.Title == title);

As does this:

var title = "title";
var queryable = context.Articles.Where(a => (object)a.Title == title);

This tells EF to treat both sides like a string, without doing any evaluation on the parameter. We don't intend to support anything more than this.

@glen-84
Copy link
Author

glen-84 commented Feb 10, 2023

Thanks a lot for getting back to me on this.

Both of those options result in an unnecessary SQL CAST:

SELECT `e`.`id` AS `Id`, `e`.`title` AS `Title`
FROM `editorial_articles` AS `e`
WHERE CAST(`e`.`title` AS char) = @__title_0

I'm also not sure if tools designed for building EF Core queries would know to add the casts anyway.

Having said this, we've decided to move away from using value objects in our web and infrastructure layers, as there are a lot of issues between EF Core and Hot Chocolate (the framework that we're using for our API) when trying to make this all work together.

I appreciate your help.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Feb 13, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

2 participants