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

Nullable boolean in MySQL condition always yielding false/true #1309

Closed
shoe-diamente opened this issue Jan 25, 2021 · 11 comments · Fixed by #1311
Closed

Nullable boolean in MySQL condition always yielding false/true #1309

shoe-diamente opened this issue Jan 25, 2021 · 11 comments · Fixed by #1311

Comments

@shoe-diamente
Copy link

shoe-diamente commented Jan 25, 2021

Steps to reproduce

  • Have an optional boolean column for a table
  • Write false == x.OptionalBoolean.HasValue on the C# side

The issue

I would expect that to be translated to:

false = (optional_boolean IS NOT NULL)

instead it's translated to:

false = optional_boolean IS NOT NULL

which is interpreted as:

(false = optional_boolean) IS NOT NULL

which always yields true.

Further technical details

MySQL version: 8.0
Operating system: macOS 10.13.6
Pomelo.EntityFrameworkCore.MySql version: 3.1.1
Framework version: netcoreapp3.1

@lauxjpn lauxjpn self-assigned this Jan 25, 2021
@lauxjpn
Copy link
Collaborator

lauxjpn commented Jan 25, 2021

I am unable to reproduce this issue, with the following code sample:

Program.cs
using System.Diagnostics;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pomelo.EntityFrameworkCore.MySql.Infrastructure;

namespace IssueConsoleTemplate
{
    public class DoomsdayMachine
    {
        public int DoomsdayMachineId { get; set; }
        public bool? IsUnpluggedAndSafe { get; set; }
    }
    
    public class Context : DbContext
    {
        public virtual DbSet<DoomsdayMachine> DoomsdayMachines { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseMySql(
                    "server=127.0.0.1;port=3306;user=root;password=;database=Issue1309",
                    options => options.CharSetBehavior(CharSetBehavior.NeverAppend)
                        .ServerVersion("8.0.21-mysql"))
                .UseLoggerFactory(
                    LoggerFactory.Create(
                        configure => configure
                            .AddConsole()
                            .AddFilter(level => level >= LogLevel.Information)))
                .EnableSensitiveDataLogging()
                .EnableDetailedErrors();
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<DoomsdayMachine>()
                .HasData(
                    new DoomsdayMachine {DoomsdayMachineId = 1, IsUnpluggedAndSafe = true},
                    new DoomsdayMachine {DoomsdayMachineId = 2, IsUnpluggedAndSafe = false},
                    new DoomsdayMachine {DoomsdayMachineId = 3, IsUnpluggedAndSafe = null});
        }
    }

    internal static class Program
    {
        private static void Main()
        {
            using var context = new Context();

            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            var unreliableDoomsdayMachines = context.DoomsdayMachines
                .Where(m => m.IsUnpluggedAndSafe.HasValue == false)
                .ToList();

            Trace.Assert(unreliableDoomsdayMachines.Count == 1);
            Trace.Assert(unreliableDoomsdayMachines[0].DoomsdayMachineId == 3);
        }
    }
}
Console output (SQL)
warn: Microsoft.EntityFrameworkCore.Model.Validation[10400]
      Sensitive data logging is enabled. Log entries and exception messages may include sensitive application data, this mode should only be enabled during development.

info: Microsoft.EntityFrameworkCore.Infrastructure[10403]
      Entity Framework Core 3.1.1 initialized 'Context' using provider 'Pomelo.EntityFrameworkCore.MySql' with options: ServerVersion 8.0.21 MySql SensitiveDataLoggingEnabled DetailedErrorsEnabled

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (21ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      DROP DATABASE `Issue1309`;

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (6ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE DATABASE `Issue1309`;

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (25ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE TABLE `DoomsdayMachines` (
          `DoomsdayMachineId` int NOT NULL AUTO_INCREMENT,
          `IsUnpluggedAndSafe` tinyint(1) NULL,
          CONSTRAINT `PK_DoomsdayMachines` PRIMARY KEY (`DoomsdayMachineId`)
      );

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (4ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      INSERT INTO `DoomsdayMachines` (`DoomsdayMachineId`, `IsUnpluggedAndSafe`)
      VALUES (1, TRUE);

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (3ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      INSERT INTO `DoomsdayMachines` (`DoomsdayMachineId`, `IsUnpluggedAndSafe`)
      VALUES (2, FALSE);

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (3ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      INSERT INTO `DoomsdayMachines` (`DoomsdayMachineId`, `IsUnpluggedAndSafe`)
      VALUES (3, NULL);

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (4ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      SELECT `d`.`DoomsdayMachineId`, `d`.`IsUnpluggedAndSafe`
      FROM `DoomsdayMachines` AS `d`
      WHERE `d`.`IsUnpluggedAndSafe` IS NULL

As you can see, the m.IsUnpluggedAndSafe.HasValue == false condition gets translated to `d`.`IsUnpluggedAndSafe` IS NULL, which is correct.


@shoe-diamente Please share your concrete LINQ query, the generated SQL, the model class with the issue and its Fluent API model definition (especially any HasDefaultValue() and HasDefaultValueSql() calls) with us.

Or better, just change my sample code in a way, that is able to reproduce the issue.

@shoe-diamente
Copy link
Author

shoe-diamente commented Jan 26, 2021

You should be able to reproduce it by changing:

             var unreliableDoomsdayMachines = context.DoomsdayMachines
                .Where(m => m.IsUnpluggedAndSafe.HasValue == false)
                .ToList();

to:

            var someBooleanVariable = false;
            var unreliableDoomsdayMachines = context.DoomsdayMachines
                .Where(m => someBooleanVariable == m.IsUnpluggedAndSafe.HasValue)
                .ToList();

The .HasValue has to occur on the right hand side for it to be ambiguous. And the use of an external variable should prevent EF Core from inlining the equality comparison.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Jan 26, 2021

@shoe-diamente Thanks, I am able to reproduce the issue now.

This seems to be a bug in EF Core itself.
The following code fails not just for Pomelo, but also for Sqlite (might not be the case for SQL Server, due to its customized bool handling) in both, EF Core 3.1.11 and 5.0.1 (@smitpatel @bricelam):

Program.cs
using System.Diagnostics;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pomelo.EntityFrameworkCore.MySql.Infrastructure;

namespace IssueConsoleTemplate
{
    public class DoomsdayMachine
    {
        public int DoomsdayMachineId { get; set; }
        public bool? IsUnpluggedAndSafe { get; set; }
    }
    
    public class Context : DbContext
    {
        public virtual DbSet<DoomsdayMachine> DoomsdayMachines { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder
                .UseMySql(
                    "server=127.0.0.1;port=3306;user=root;password=;database=Issue1309",
                    options => options.CharSetBehavior(CharSetBehavior.NeverAppend)
                        .ServerVersion("8.0.21-mysql"))
                // .UseSqlite("Data Source=Issue1309.db")
                .UseLoggerFactory(
                    LoggerFactory.Create(
                        configure => configure
                            .AddConsole()
                            .AddFilter(level => level >= LogLevel.Information)))
                .EnableSensitiveDataLogging()
                .EnableDetailedErrors();
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<DoomsdayMachine>()
                .HasData(
                    new DoomsdayMachine {DoomsdayMachineId = 1, IsUnpluggedAndSafe = true},
                    new DoomsdayMachine {DoomsdayMachineId = 2, IsUnpluggedAndSafe = false},
                    new DoomsdayMachine {DoomsdayMachineId = 3, IsUnpluggedAndSafe = null});
        }
    }

    internal static class Program
    {
        private static void Main()
        {
            using var context = new Context();

            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            var unreliable = false;
            var unreliableDoomsdayMachines = context.DoomsdayMachines
                .Where(m => unreliable == m.IsUnpluggedAndSafe.HasValue)
                .ToList();

            Trace.Assert(unreliableDoomsdayMachines.Count == 1);
            Trace.Assert(unreliableDoomsdayMachines[0].DoomsdayMachineId == 3);
        }
    }
}

The generated query is the following one, which is incorrect:

SELECT `d`.`DoomsdayMachineId`, `d`.`IsUnpluggedAndSafe`
FROM `DoomsdayMachines` AS `d`
WHERE @__unreliable_0 = `d`.`IsUnpluggedAndSafe` IS NOT NULL

As correctly pointed out by @shoe-diamente, the query should have looked like the following instead:

SELECT `d`.`DoomsdayMachineId`, `d`.`IsUnpluggedAndSafe`
FROM `DoomsdayMachines` AS `d`
WHERE @__unreliable_0 = (`d`.`IsUnpluggedAndSafe` IS NOT NULL)

@shoe-diamente
Copy link
Author

shoe-diamente commented Jan 26, 2021

I'm pretty sure Npgsql gets this right, by the way.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Jan 26, 2021

I'm pretty sure Npgsql gets this right, by the way.

@shoe-diamente That is correct. They use the following fix:

(From NpgsqlQuerySqlGenerator.VisitSqlUnary())

// EF uses unary Equal and NotEqual to represent is-null checking.
// These need to be surrounded in parentheses in various cases (e.g. where TRUE = x IS NOT NULL
case ExpressionType.Equal:
    Sql.Append("(");
    Visit(sqlUnaryExpression.Operand);
    Sql.Append(" IS NULL)");
    return sqlUnaryExpression;


case ExpressionType.NotEqual:
    Sql.Append("(");
    Visit(sqlUnaryExpression.Operand);
    Sql.Append(" IS NOT NULL)");
    return sqlUnaryExpression;

We can implement the same workaround as well.

As a quick workaround, use one of the following conditions:

  • unreliable == (m.IsUnpluggedAndSafe != null)
  • m.IsUnpluggedAndSafe.HasValue == unreliable

@lauxjpn
Copy link
Collaborator

lauxjpn commented Jan 26, 2021

I'll keep this open for the EF Core team response.

@lauxjpn lauxjpn reopened this Jan 26, 2021
@smitpatel
Copy link

Looks like this would be issue in EF Core too.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Jan 26, 2021

@shoe-diamente Fixes for both active EF Core versions (3.1 and 5.0) are already available as prereleases from our nightly builds.

@smitpatel
Copy link

Filed dotnet/efcore#23990

You can close this issue as it is fixed in MySql already.

@shoe-diamente
Copy link
Author

@lauxjpn Thank you very much for the quick response. 😄 👍

@lauxjpn lauxjpn closed this as completed Jan 27, 2021
@lauxjpn
Copy link
Collaborator

lauxjpn commented Jan 27, 2021

BTW, we implemented it a bit differently (more targeted) than @roji / Npgsql, but I have not done extensive testing on it (our additional test and the suite in general runs fine though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment