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

Enum inequality comparison triggers query warning #12507

Closed
breyed opened this issue Jun 29, 2018 · 20 comments
Closed

Enum inequality comparison triggers query warning #12507

breyed opened this issue Jun 29, 2018 · 20 comments

Comments

@breyed
Copy link

breyed commented Jun 29, 2018

Comparing two enums via inequality within a SQL query causes EF Core to report a warning like this:

Microsoft.EntityFrameworkCore.Query:Warning: A SQL parameter or literal was generated for the type 'Color' using the ValueConverter 'EnumToNumberConverter<Color, int>'. Review the generated SQL for correctness and consider evaluating the target expression in-memory instead.

The generated SQL is fine:

SELECT [b].[Id], [b].[Color]
FROM [Boxes] AS [b]
[b].[Color] >= 4

If the comparison is equality, the warning does not appear.

Repro:

enum Color { Red, Orange, Yellow, Green, Blue, Indigo, Violet}

class Box
{
	public int Id { get; set; }
	public Color Color { get; set; }
}

class BoxContext : DbContext
{
	public DbSet<Box> Boxes { get; set; }

	protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
	{
		optionsBuilder.UseSqlServer("Doesn't matter. The warning appears before attempting to connect.");
		optionsBuilder.UseLoggerFactory(debugLoggerFactory);
	}
	static readonly LoggerFactory debugLoggerFactory = new LoggerFactory(new[] { new DebugLoggerProvider((_, __) => true) });
}

public class Program
{
	public static void Main(string[] args)
	{
		var db = new BoxContext();
		var bluishBoxes = db.Boxes.Where(b => b.Color >= Color.Blue).ToArray();
	}
}
@ajcvickers
Copy link
Member

@breyed Can you give some more details as to what the issue is here? Is it that the warning exists at all? That there is a warning even though the SQL is correct? That there is no warning in other similar cases?

@breyed
Copy link
Author

breyed commented Jul 2, 2018

The problem is that the warning exists at all, i.e. it's a spurious warning that requires the developer to track down what's going on, only to discover that his code and the generated SQL are both just fine and no action was actually required.

There is no warning in the similar case of equality. You could say that the inconsistency between the behavior of equality and inequality is a problem, but I'd just say that the case of equality is working and inequality has a subtle bug. There shouldn't be a warning in either case.

@ajcvickers
Copy link
Member

@breyed The reason for this warning is that in some cases when value conversions are used the generated SQL may not be correct--for example, see #10265. So for now, the warning indicates that you should check to make sure the generated SQL is correct. This is a temporary measure until translations with value conversions become more robust.

@breyed
Copy link
Author

breyed commented Jul 2, 2018

This doesn't make sense to me. An enum equality comparison also needs a value conversion (enum to int), but it doesn't trigger a warning. So it would seem that somehow the scope of the warning is being limited. Couldn't it likewise be limited to not trigger on inequality?

There are two problems with the spurious warning approach:

  1. Evaluating the warning on a complex real-world query takes time, especially for someone (like I was) new to this area of EF. The whole concept of a value converter may be new territory.

  2. Once you've verified the warning is benign, you have the warning spam problem. Now when you run the code, you have to wade through the the spurious warnings to find any actionable warnings that haven't been addressed.

@Kukkimonsuta
Copy link
Contributor

@breyed I agree that this is quite annoying, hopefully it will be looked at soon - I've experienced the same warning with negated Contains call. Meanwhile you can disable the warning:

optionsBuilder.ConfigureWarnings(o => o.Ignore(RelationalEventId.ValueConversionSqlLiteralWarning));

@breyed
Copy link
Author

breyed commented Jul 9, 2018

Thanks. Disabling the warning brings up a meta issue, which is that there's no concrete mapping between the warning text and the enum field to disable it. It would be helpful to include the enum field in the warning output.

@MattHoneycutt
Copy link

Agreed, the resolution doesn't make sense here. The issue isn't the warning really, it's the inconsistency. There is no warning for a normal equals (or contains), only for not equals and not contains. There should be warnings for both, or warnings for neither.

@NickCraver
Copy link
Member

We're logging a truckload of these at Stack Overflow - can we please revisit the need to log this?

It seems silly given there's no "out". I've reviewed the SQL, it's correct...now what? How do I stop the logging? An error or warning for which the user doesn't need to and cannot take any action should not be a thing, or there should be a way to suppress it as addressed.

@ajcvickers
Copy link
Member

@NickCraver The warning can be disabled with:

optionsBuilder.ConfigureWarnings(o => o.Ignore(RelationalEventId.ValueConversionSqlLiteralWarning));

@NickCraver
Copy link
Member

@ajcvickers Gotcha, but that's global...is there a way to turn it off on the piece of code throwing it, as that's what I've verified as working? Disabling it globally when it's a per-instance thing isn't how most warnings work, take C# itself for example and #pragma.

@ajcvickers
Copy link
Member

@NickCraver Yes, it's far from ideal. We'll do something better in 3.0.

@EamonNerbonne
Copy link

So I just ran into this and have one (possibly obvious) observation to add: Although in general with collations and whatnot the ordering on the server can differ in complex ways from an ordering .net-side, this does not appear to be the case for all orderings. Specifically: is this warning ever correct for enums? enums sort like integers in C#; and they are integers DB-side, so this warning appears overly cautious.

Perhaps there is some DB engine somewhere where that's not true, but it doesn't seem to be all that likely.

So while the general case of semantic differences between code-side and DB-side evaluation is difficult, perhaps this special case is not?

@sandersaares
Copy link

Is this just about sort order? I am perfectly happy to have sort order be "whatever the database says" and disable the warning if so but the warning is rather vague about it.

@EamonNerbonne
Copy link

Is this just about sort order? I am perfectly happy to have sort order be "whatever the database says" and disable the warning if so but the warning is rather vague about it.

Well, the ordering doesn't just affect sorts, it also affects less-than / greater than operators, and may affect other operations if they're ordering dependant (e.g. window functions if those were supported in EF), and in principle collation affects equality too (but there 's no warning for that anyhow, IIRC).

For enums: seems safe to always disable.

@sandersaares
Copy link

Allow me to rephrase: is this just about ordering?

@EamonNerbonne
Copy link

Allow me to rephrase: is this just about ordering?

As far as I can tell: yes.

@akamud
Copy link

akamud commented Jan 8, 2020

I couldn't find anything in the documentation regarding changes to the ConfigureWarnings on 3.0 or 3.1 releases. Did something change or is it still a global configuration?

@ajcvickers
Copy link
Member

@akamud Nothing changed here; whether or not the configuration is "global" depends on the specific definition of "global".

@johnduhart
Copy link

@ajcvickers It looks like this specific warning was removed in 3.0, did it get replaced with something else? #12085

@ajcvickers
Copy link
Member

@johnduhart Yes, from the comments by @smitpatel further down the issue indicate that it was removed because the query changes made the warning unnecessary.

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants