dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.55k stars 3.13k forks source link

Raw SQL queries for unmapped types - how to define enum properties to map from strings? #33206

Open jasekiw opened 5 months ago

jasekiw commented 5 months ago

I was following the following guide: https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-8.0/whatsnew#raw-sql-queries-for-unmapped-types

I tried writing a query from a table that has some columns that are varchar or textual and are enum based values in the database.

public class CoreAlertStatus
    {
        public int Id { get; set; }
        public required string Name { get; set; }
        public int SortOrder { get; set; }

        public EAlertColor AlertColor { get; set; }

        public EAlertSymbol AlertSymbol { get; set; }
        public int NextLevelCount { get; set; }
        public int NextLevelId { get; set; }
        public string NextLevelName { get; set; } = "";

        public EOrgType NextLevelType { get; set; }
        public int BypassCount { get; set; }
    }
var result = await _context.Database.SqlQuery<CoreAlertStatus>("...").ToListAsync();

I received the following error:

System.InvalidCastException: Can't convert VarChar to Int32
   at MySqlConnector.Core.Row.GetInt32(Int32 ordinal) in /_/src/MySqlConnector/Core/Row.cs:line 247
   at MySqlConnector.MySqlDataReader.GetInt32(Int32 ordinal) in /_/src/MySqlConnector/MySqlDataReader.cs:line 240
   at lambda_method311(Closure, QueryContext, DbDataReader, Int32[])
   at Microsoft.EntityFrameworkCore.Query.Internal.FromSqlQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()

Turns out the enum properties are trying to be converted from integers even though my column is string based. I normally handle with mapping the type in the OnModelCreating method. But since I am purposely trying to avoid mapping, I didn't know how to do this.

I eventually figured out if I add [Column(TypeName = "VARCHAR(255)")] it figures it out.

    public class CoreAlertStatus
    {
        public int Id { get; set; }
        public required string Name { get; set; }
        public int SortOrder { get; set; }

        [Column(TypeName = "VARCHAR(255)")] // required for ef core to interpret it as a string
        public EAlertColor AlertColor { get; set; }

        [Column(TypeName = "VARCHAR(255)")] // required for ef core to interpret it as a string
        public EAlertSymbol AlertSymbol { get; set; }
        public int NextLevelCount { get; set; }
        public int NextLevelId { get; set; }
        public string NextLevelName { get; set; } = "";

        [Column(TypeName = "VARCHAR(255)")] // required for ef core to interpret it as a string
        public EOrgType NextLevelType { get; set; }
        public int BypassCount { get; set; }
    }

Is this the correct way to specify this? Will this work across different databases? Is there a better database agnostic way of defining this?

I am using MYSQL.

EF Core version: 8.0.2 Database provider: Pomelo.EntityFrameworkCore.MySql 8.0.0 Target framework: NET 8.0 Operating system: Windows 11 IDE: Rider 2023.3.3

ajcvickers commented 5 months ago

@jasekiw Fundamentally, if you need to configure the mapping, then you're going to have to explicitly configure it. Is there a reason you are explicitly trying to avoid this?

jasekiw commented 5 months ago

@ajcvickers I am particularly excited about querying unmapped types because I don't want to have the mappings appear in the model snapshot and migration designer files. Especially because they don't cause any actual change to the database.

Why?

It would be nice if there was a way to define mappings that don't go into the model snapshot. Maybe I'm missing something and there is already a way to do that?

As I mentioned the Column attribute worked. I don't want to rely on undocumented behavior however or it break when I switch database drivers. I think varchar is pretty universal but I'm mostly familiar with mysql and can't say for sure. It seems like SQL server and postgres have varchar though so maybe it's ok.

ajcvickers commented 5 months ago

I think varchar is pretty universal but I'm mostly familiar with mysql and can't say for sure.

It's not universal. SQLite, for example, doesn't have it.

I will discuss with the team, but generally speaking, the way to configure the mapping of a type is to map the type.

jasekiw commented 5 months ago

@ajcvickers When discussing with the team, I would like to inject the following question:

What was the reason for creating the queries for unmapped types feature if common cases aren't going to be covered? I would argue enum to string conversion is a common case.

Thank your for your time and your amazing work!

roji commented 4 months ago

@jasekiw when submitting issues, please always submit a minimal, runnable code sample (e.g. as a console program) rather than snippets and textual descriptions - this helps understand what exactly you're doing.

First, by default, EF maps enums to ints, not to strings; the [JsonConverter] attributes on your properties don't change that - that's a System.Text.Json attribute that doesn't affect EF's mapping in any way.

So now, when you execute SqlQuery<CoreAlertStatus>(...), you're asking EF to materiailze the results of your SQL as a CoreAlertStatus; EF simply does the default thing, and for enum properties it attempts to read an int - which fails, since the database returned a string. In other words, as @ajcvickers wrote, you need to somehow let EF know that a string is coming back from the database, rather than an int.

I am particularly excited about querying unmapped types because I don't want to have the mappings appear in the model snapshot and migration designer files.

I'm a bit confused - is CoreAlertStatus in your EF model (i.e. mapped to a table), or is it a type you only use with SqlQuery? If it's the former, it's already in your model snapshot, and it seems you're simply missing the correct column configuration (map to string instead of the default int). If CoreAlertStatus is only used with SqlQuery, then adding [Column(TypeName = "VARCHAR(255)")] shouldn't make anything go into the model snapshot, since CoreAlertStatus isn't in there in the first place.

roji commented 4 months ago

Note for triage: my expectation was for pre-convention model configuration to affect SqlQuery, but that doesn't seem to be the case (full test code below):

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<EAlertColor>().HaveConversion<EnumToStringConverter<EAlertColor>>();
}

Something like this could allow for what the user is trying to achieve.

Full test code ```c# await using var context = new BlogContext(); await context.Database.EnsureDeletedAsync(); await context.Database.EnsureCreatedAsync(); await context.Database.ExecuteSqlAsync($""" ALTER TABLE Statuses ALTER COLUMN AlertColor nvarchar(max); INSERT INTO Statuses (AlertColor) VALUES ('One'), ('Two'); """); _ = await context.Database.SqlQuery($"SELECT * FROM Statuses").ToListAsync(); public class BlogContext : DbContext { public DbSet Statuses { get; set; } protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) => optionsBuilder .UseSqlServer("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false") .LogTo(Console.WriteLine, LogLevel.Information) .EnableSensitiveDataLogging(); protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder) { configurationBuilder.Properties().HaveConversion>(); } } public class CoreAlertStatus { public int Id { get; set; } public EAlertColor AlertColor { get; set; } } public class CoreAlertStatusDto { public int Id { get; set; } // [Column(TypeName = "nvarchar(max)")] public EAlertColor AlertColor { get; set; } } public enum EAlertColor { One, Two} ```
jasekiw commented 4 months ago

@roji Ignore the Json converter statements, I forgot to removed them from my class for this example as it is off topic.

CoreAlertStatus is only used with SqlQuery and is not a table in my database. It's a complex query that I have to write in SQL.

In my original post I wasn't saying that [Column(TypeName = "VARCHAR(255)")] did not work. I was saying I worried about using it being undocumented behavior. That annotation is used to configure a backing column and not to configure a conversion.

Your last comment would do what I want for sure if it worked, you're saying it doesn't?

I'll try and create a reproduction repo for this, I'll probably change the query to make it simpler as you don't need to know my business logic.

roji commented 4 months ago

That annotation is used to configure a backing column and not to configure a conversion.

In my original post I wasn't saying that [Column(TypeName = "VARCHAR(255)")] did not work. I was saying I worried about using it being undocumented behavior. That annotation is used to configure a backing column and not to configure a conversion.

I do agree it's slightly odd that [Column(TypeName)] works here, given that the CLR type and property aren't in the model; we'll discuss this as well (/cc @ajcvickers @AndriySvyryd). But other than that, the data annotation does also affect e.g. EF querying behavior, and not just the column that gets created via migrations.

Your last comment would do what I want for sure if it worked, you're saying it doesn't?

That's right, see the code above.

I'll try and create a reproduction repo for this, I'll probably change the query to make it simpler as you don't need to know my business logic.

No need for that at this point - I already did that based on your OP (you can expand "full test code" in my above comment to see that).

stevendarby commented 4 months ago

Note for triage: my expectation was for pre-convention model configuration to affect SqlQuery, but that doesn't seem to be the case (full test code below):

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<EAlertColor>().HaveConversion<EnumToStringConverter<EAlertColor>>();
}

Something like this could allow for what the user is trying to achieve.

Full test code

I was actually looking at this recently for my own application. FWIW you can replace the IProviderConventionSetBuilder service with one that uses the default implementation and then adds your own custom convention. That convention then applies to both the main model and the models that are built ad-hoc for SqlQuery. I do think it would be nice if things added via ConfigureConventions applied to both though, without needing to do this.

roji commented 4 months ago

/cc @ajcvickers @AndriySvyryd

jasekiw commented 4 months ago

@roji Something else to consider is the Convention builder would use a string across any class that uses that EAlertColor enum which might be undesirable for some people (maybe your know another way around this?). It might make sense to allow finer grain control and specify the conversion for the property instead similar to the ColumnAttribute behavior

Someone can currently do the following but this causes it to be added to the snapshot which is undesirable. (it's also undesirable in my situation because I have different queries that I run for the same dto and there is no defautl sql query to run)

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<CoreAlertStatusDto>().ToSqlQuery("<THIS QUERY SHOULD NEVER BE USED>").Property(x => x.AlertColor).HasConversion<string>();
}

It would be helpful to do something like this:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    // would like to do something like this
    modelBuilder.Entity<CoreAlertStatusDto>().ToUnmappedSqlQuery().Property(x => x.AlertColor).HasConversion<string>();
}

I can see how this is a much large feature request though as the current ToSqlQuery method adds the definition to the model snapshot and it is probably hard to change that behavior.

Important Update: I just figured out that the following does not configure the string conversion for SqlQuery, but only when you use the dbset. modelBuilder.Entity<CoreAlertStatusDto>().ToSqlQuery("<THIS QUERY SHOULD NEVER BE USED>").Property(x => x.AlertColor).HasConversion<string>();

Console.WriteLine("Querying from DbSet Works");
_ = await context.StatusesDtos.FromSqlRaw("SELECT * FROM Statuses").ToListAsync();
Console.WriteLine("Querying from SqlQuery does not");
_ = await context.Database.SqlQuery<CoreAlertStatusDto>($"SELECT * FROM Statuses").ToListAsync();
roji commented 4 months ago

It might make sense to allow finer grain control and specify the conversion for the property instead similar to the ColumnAttribute behavior

In theory maybe, but I think that this is going a bit far; trying to map the same property type differently when it's used in mapped and unmapped types seems quite contrived.. One may as well as well ask to map AlertColor differently on a per-query basis, at which point even your proposed ToUnmappedSqlQuery() becomes insufficient.

Remember, at the end of the day your unmapped DTO can simply have a string property instead, and possibly another property that exposes that as an enum if you really need to.

[...] the current ToSqlQuery method adds the definition to the model snapshot and it is probably hard to change that behavior.

Stepping back... unmapped querying (with context.Database.SqlQuery()) is appropriate for a one-off SQL which you will not be repeating, and for cases where LINQ doesn't work or isn't appropriate for some reason. If the same SQL is used in multiple places - wherever you want to query for the type - then unmapped querying really doesn't much sense (ToSqlQuery is the better mechanism, again assuming you must use SQL rather than LINQ).

In other words, keeping things out of your model snapshot shouldn't generally be a goal, to be achieved by using unmapped SqlQuery. If your model snapshot is getting too big, that's something to be examined as a different issue (e.g. this typically happens when users erroneously include big seed data, or other similar mistakes).

jasekiw commented 4 months ago

@roji

In theory maybe, but I think that this is going a bit far; trying to map the same property type differently when it's used in mapped and unmapped types seems quite contrived.. One may as well as well ask to map AlertColor differently on a per-query basis, at which point even your proposed ToUnmappedSqlQuery() becomes insufficient.

Fair enough. I think if we can get the original goal below working, it should be sufficient, or at least document the ColumnAttribute behavior.

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<EAlertColor>().HaveConversion<EnumToStringConverter<EAlertColor>>();
}

Stepping back... unmapped querying (with context.Database.SqlQuery()) is appropriate for a one-off SQL which you will not be repeating, and for cases where LINQ doesn't work or isn't appropriate for some reason. If the same SQL is used in multiple places - wherever you want to query for the type - then unmapped querying really doesn't much sense (ToSqlQuery is the better mechanism, again assuming you must use SQL rather than LINQ).

I agree with this statement. My specific situation is I have a complex query that runs for each level of a a business hierarchy, At at site level, company level, or global level. The query has to be modified a little bit for each level so I am technically using it in 3 places but I put the code for these queries right next to each other for high cohesion. I am not repeating that query over and over across my codebase so I don't think it fits into that category but let me know your thoughts.

In other words, keeping things out of your model snapshot shouldn't generally be a goal, to be achieved by using unmapped SqlQuery. If your model snapshot is getting too big, that's something to be examined as a different issue (e.g. this typically happens when users erroneously include big seed data, or other similar mistakes).

This might start getting off topic but the snapshot size is increasing as my application grows. Every db related development process starts to slow down - maybe I'm blaming the wrong thing here but I believe it slows down compilation. My ModelSnapshot is 9403 lines of code and that is duplicated for every migration (490K lines of code in my migration project in total - that is over 10x the lines of codes in my codebase). I have 150 tables in my database and no seed data. Every time a migration is created and a PR is made, I see 10k added lines of code. Maybe there is a recommended way to manage these to prevent this situation (or maybe it's just something to deal with), some documentation around this would be great. I can help write documentation if needed.

I consider this a separate issue however. I am not using SqlQuery to avoid adding to my model snapshot, I came upon SqlQuery organically. Rather, I am using SqlQuery and then I am surpised when a string to enum conversion is not supported without adding to to the model snapshot, and add a fake query .ToSqlQuery("<IGNORE ME>").

roji commented 4 months ago

The model snapshot size is indeed off-topic - but I'd recommend (a) looking if the 9403 lines in your snapshot make sense (especially that there isn't a lot taken up by seeding, as I wrote above), and (b) possibly squashing your migrations - it's frequently not needed to keep the entire history once all servers have been updated to the latest migration, etc.

roji commented 4 months ago

@AndriySvyryd @ajcvickers the actionable item on this issue is making pre-convention model configuration value converters apply to raw SQL queries - see this comment with a repro. I'm not quite sure who's best to look at it - @ajcvickers did you do work on unmapped SQL queries?

ajcvickers commented 4 months ago

@roji We decided not to do this when we did the initial work. We use data annotations because they are local to the types being ad-hoc mapped. By design, the mapping is independent of the model configuration for the context. We could, of course, change this, but there is nothing new here.

(With regard the new triage process, I knew all this when I stopped responding and put it in the query milestone. The most appropriate next step would have been to discuss this as a team. I'm not sure how we indicate that with the new process.)

ajcvickers commented 4 months ago

Note from team triage: check that configuration of default type mappings are used. Configurations for properties in the model will not be used.

defunky commented 4 months ago

I was actually looking at this recently for my own application. FWIW you can replace the IProviderConventionSetBuilder service with one that uses the default implementation and then adds your own custom convention. That convention then applies to both the main model and the models that are built ad-hoc for SqlQuery. I do think it would be nice if things added via ConfigureConventions applied to both though, without needing to do this.

@stevendarby How do you do this? I couldn't find any docs around this

stevendarby commented 3 months ago

@defunky you can read about creating custom conventions here: https://learn.microsoft.com/en-us/ef/core/modeling/bulk-configuration#adding-a-new-convention

Here's an example of replacing the SqlServer implementation of IProviderConventionSetBuilder to add a custom convention which applies to ad hoc queries too. There may be better ways to do this, this is just an example. As the documentation above mentions, it can be tricky to get conventions right (and why the ConfigureConventions is preferred where possible).

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Builders;
using Microsoft.EntityFrameworkCore.Metadata.Conventions;
using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
using Microsoft.Extensions.Logging;
using System;
using System.Linq;

await using var context = new MyContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

_ = await context.Database.SqlQueryRaw<CoreAlertStatus>("SELECT 'Red' AS AlertColor").ToListAsync();

public class MyContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=AdHocConvention;Trusted_Connection=True;Encrypt=False")
            .ReplaceService<IProviderConventionSetBuilder, CustomProviderConventionSetBuilder>()
            .LogTo(Console.WriteLine, LogLevel.Information);
}

public class CoreAlertStatus
{
    public EAlertColor AlertColor { get; set; }
}

public enum EAlertColor
{
    Red = 0,
    Green = 1
}

internal class CustomProviderConventionSetBuilder(
    ProviderConventionSetBuilderDependencies dependencies,
    RelationalConventionSetBuilderDependencies relationalDependencies,
    ISqlGenerationHelper sqlGenerationHelper)
    : SqlServerConventionSetBuilder(dependencies, relationalDependencies, sqlGenerationHelper)
{
    public override ConventionSet CreateConventionSet()
    {
        var conventionSet = base.CreateConventionSet();
        conventionSet.Add(new CustomConvention());
        return conventionSet;
    }
}

internal class CustomConvention : IModelFinalizingConvention
{
    public void ProcessModelFinalizing(IConventionModelBuilder modelBuilder, IConventionContext<IConventionModelBuilder> context)
    {
        foreach (var property in modelBuilder.Metadata.GetEntityTypes()
                     .SelectMany(e => e.GetProperties().Where(p => p.ClrType == typeof(EAlertColor))))
        {
            property.SetValueConverter(typeof(EnumToStringConverter<EAlertColor>));
        }
    }
}