-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Raw SQL queries for unmapped types - how to define enum properties to map from strings? #33206
Comments
@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? |
@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. |
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. |
@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! |
@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
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 |
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 codeawait 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<CoreAlertStatusDto>($"SELECT * FROM Statuses").ToListAsync();
public class BlogContext : DbContext
{
public DbSet<CoreAlertStatus> 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<EAlertColor>().HaveConversion<EnumToStringConverter<EAlertColor>>();
}
}
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} |
@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 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. |
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.
That's right, see the code above.
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). |
I was actually looking at this recently for my own application. FWIW you can replace the |
@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 Important Update: I just figured out that the following does not configure the string conversion for SqlQuery, but only when you use the dbset. 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(); |
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.
Stepping back... unmapped querying (with 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). |
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>>();
}
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.
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 |
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. |
@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? |
@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.) |
Note from team triage: check that configuration of default type mappings are used. Configurations for properties in the model will not be used. |
@stevendarby How do you do this? I couldn't find any docs around this |
@defunky you can read about creating custom conventions here: Here's an example of replacing the SqlServer implementation of 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>));
}
}
} |
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.
I received the following error:
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.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
The text was updated successfully, but these errors were encountered: