Reuse the same value converter instance when configured as type

[jscarle]

When a query uses the same parameter in several locations, Entity Framework may not detect the parameter as being the same parameter and will parameterize several instances of the same parameter.

Take for example the following LINQ:

dbContext.Projects
	.Join(
		dbContext.ProjectTranslations,
		outer => new {
		    outer.ProjectId,
		    TranslationId = translationId
		},
		inner => new {
		    inner.ProjectId,
		    inner.TranslationId
		},
		(outer, inner) => new {
			Project = outer,
			ProjectTranslation = inner
		}
	)
	.Join(
		dbContext.ProjectCategoryTranslations,
		outer => new {
		    outer.Project.ProjectCategoryId,
		    TranslationId = translationId
		},
		inner => new {
		    inner.ProjectCategoryId,
		    inner.TranslationId
		},
		(outer, inner) => new {
			outer.Project,
			outer.ProjectTranslation,
			ProjectCategoryTranslation = inner
		}
	)
	.Join(
		dbContext.Builders,
		outer => outer.Project.BuilderId,
		inner => inner.BuilderId,
		(outer, inner) => new {
			outer.Project,
			outer.ProjectTranslation,
			outer.ProjectCategoryTranslation,
			Builder = inner
		}
	)
	.Join(
		dbContext.DirectoryCategoryTranslations,
		outer => new {
		    outer.Builder.DirectoryCategoryId,
		    TranslationId = translationId
		},
		inner => new {
		    inner.DirectoryCategoryId,
		    inner.TranslationId
		},
		(outer, inner) => new
		{
			outer.Project,
			outer.ProjectTranslation,
			outer.ProjectCategoryTranslation,
			outer.Builder,
			DirectoryCategoryTranslation = inner,
		}
	)
	.FirstOrDefaultAsync();

This will generate the following command:

Executed DbCommand (39ms) [Parameters=[
    @__translationId_0='?' (DbType = Int32),
    @__translationId_0_1='?' (DbType = Int32),
    @__translationId_0_2='?' (DbType = Int32),
    @__projectId_1='?' (DbType = Int32)
], CommandType='Text', CommandTimeout='30']

Now that EF.Parameter is available with Entity Framework 9, perhaps this could be improved with an optional name passed to the method. Something like the following?

dbContext.Projects
	.Join(
		dbContext.ProjectTranslations,
		outer => new {
		    outer.ProjectId,
		    TranslationId = EF.Parameter(translationId, "translationId")
		},
		inner => new {
		    inner.ProjectId,
		    inner.TranslationId
		},
		(outer, inner) => new {
			Project = outer,
			ProjectTranslation = inner
		}
	)
	.Join(
		dbContext.ProjectCategoryTranslations,
		outer => new {
		    outer.Project.ProjectCategoryId,
		    TranslationId = EF.Parameter(translationId, "translationId")
		},
		inner => new {
		    inner.ProjectCategoryId,
		    inner.TranslationId
		},
		(outer, inner) => new {
			outer.Project,
			outer.ProjectTranslation,
			ProjectCategoryTranslation = inner
		}
	)
	.Join(
		dbContext.Builders,
		outer => outer.Project.BuilderId,
		inner => inner.BuilderId,
		(outer, inner) => new {
			outer.Project,
			outer.ProjectTranslation,
			outer.ProjectCategoryTranslation,
			Builder = inner
		}
	)
	.Join(
		dbContext.DirectoryCategoryTranslations,
		outer => new {
		    outer.Builder.DirectoryCategoryId,
		    TranslationId = EF.Parameter(translationId, "translationId")
		},
		inner => new {
		    inner.DirectoryCategoryId,
		    inner.TranslationId
		},
		(outer, inner) => new
		{
			outer.Project,
			outer.ProjectTranslation,
			outer.ProjectCategoryTranslation,
			outer.Builder,
			DirectoryCategoryTranslation = inner,
		}
	)
	.FirstOrDefaultAsync();

[roji]

@jscarle if EF is sending multiple parameters where it should be sending one, that's something we need to improve - users shouldn't need to manually use something like EF.Parameter for that. In any case, EF.Parameter() is for specifying that something should be a parameter (as opposed to a constant), rather than controlling the parameter name etc.

[jscarle]

Thanks @roji, keep up the great work.

[roji]

@jscarle can you please post a minimal, runnable console program that demonstrates multiple parameters being sent instead of one? The above is an isolated snippet that's lacking e.g. the model, a minimal console program would be the best way forward here.

[jscarle]

@roji I took me a bit of time to get the repro completely built, but you can clone it here: https://github.com/jscarle/Repro33612

It uses Entity Framework 9.0.0-preview.3.24172.4.

At first, I wasn't able to repro and after further investigation, I was. Turns out that the issue does not manifest itself if the parameter is a primitive type such as int. It only started manifesting itself when I switched to using strongly typed identifiers.

The code in the repro above will generate the following output:

      SELECT TOP(1) [c].[CityId], [c].[StateId], [c0].[CityId], [c0].[TranslationId], [c0].[Name], [s].[StateId], [s].[CountryId], [s0].[StateId], [s0].[TranslationId], [s0].[Name], [c1].[CountryId], [c2].[CountryId], [c2].[TranslationId], [c2].[Name]
      FROM [Cities] AS [c]
      INNER JOIN [CityTranslations] AS [c0] ON [c].[CityId] = [c0].[CityId] AND @__translationId_0 = [c0].[TranslationId]
      INNER JOIN [States] AS [s] ON [c].[StateId] = [s].[StateId]
      INNER JOIN [StateTranslations] AS [s0] ON [s].[StateId] = [s0].[StateId] AND @__translationId_0_1 = [s0].[TranslationId]
      INNER JOIN [Countries] AS [c1] ON [s].[CountryId] = [c1].[CountryId]
      INNER JOIN [CountryTranslations] AS [c2] ON [c1].[CountryId] = [c2].[CountryId] AND @__translationId_0_2 = [c2].[TranslationId]

To run the query, start the application and once the application is running, in the console window, hit the R key. Every press of the R key will run the query. Ctrl-C to quit as per usual host running behavior.

[roji]

Confirmed, see minimal repro below (@jscarle please take a look for what a really minimal repro looks like - it's the absolute minimum that's needed to show the bug, without all the extra stuff around. Putting in the effort to create these saves us a lot of time and is greatly appreciated).

The parameter duplication is happening because the same variable is being compared to entity properties with different instances of the value converter; we perform this duplication, since we don't know whether the different value converter instances are configured with different settings, and therefore may produce different results.

The code in question actually uses bulk configuration:

configurationBuilder.Properties<CultureId>().HaveConversion<CultureIdConverter>();

This seems to create a new instance of the value converter for each property, rather than creating a single instance and reusing across the entire model (which would fix this).

Note: the code which decides whether to reuse the same parameter or duplicate is here.

Note: in theory we could have compared the actual provider value coming out of the value converter, but different model values may yield different/same provider values, and the decision whether to duplicate the parameter or not is cached.

Repro

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

var cultureId = new CultureId(1);
_ = await context.Blogs.Where(b => b.CultureId1 == cultureId && b.CultureId2 == cultureId).ToListAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { 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 OnModelCreating(ModelBuilder modelBuilder)
    {
        // Same instance of value converter being used -> single parameter
        // modelBuilder.Entity<Blog>(b =>
        // {
        //     var converter = new CultureIdConverter();
        //     b.Property(b => b.CultureId1).HasConversion(converter);
        //     b.Property(b => b.CultureId2).HasConversion(converter);
        // });

        // Multiple instances of value converter being used -> multiple parameters.
        // This looks like the right behavior (we don't know if the instances are configured differently)
        // modelBuilder.Entity<Blog>(b =>
        // {
        //     b.Property(b => b.CultureId1).HasConversion(new CultureIdConverter());
        //     b.Property(b => b.CultureId2).HasConversion(new CultureIdConverter());
        // });

        // Multiple instances of value converter being used -> multiple parameters
        // This looks like wrong behavior (we know the instances are configured the same)
        // modelBuilder.Entity<Blog>(b =>
        // {
        //     b.Property(b => b.CultureId1).HasConversion<CultureIdConverter>();
        //     b.Property(b => b.CultureId2).HasConversion<CultureIdConverter>();
        // });
    }

    protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
    {
        // This apparently creates multiple instances of the converter under the hood -> multiple parameters
        // configurationBuilder.Properties<CultureId>().HaveConversion<CultureIdConverter>();
    }
}

public class Blog
{
    public int Id { get; set; }

    public CultureId CultureId1 { get; set; }
    public CultureId CultureId2 { get; set; }
}

public record struct CultureId(int Value);

public class CultureIdConverter() : ValueConverter<CultureId, int>(v => v.Value, v => new CultureId(v));

/cc @ajcvickers

[roji]

Note: this goes beyond bulk configuration (ConfigureConventions); when configuring two properties with the same value converter type via the generic HasConversion API, we also get multiple instances and therefore multiple parameters:

b.Property(b => b.CultureId1).HasConversion<CultureIdConverter>();
b.Property(b => b.CultureId2).HasConversion<CultureIdConverter>();

The same may also occur for built-in value converters returned by ITypeMappingSource, depending on how ValueConverterSelector works and where we cache (/cc @ajcvickers).

In general, ideally we should try to return the same converter instances wherever we know there can't be an actual converter behavior difference.

[jscarle]

I make quite heavy uses of value objects in my code bases. In some instances I've had 50+ value converters configured through conventions because of their use in several hundred properties. So this would definately be a nice improvement.