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

Proposed fix to #7172 - Consider non-default column collation in table variable #29518

Closed
wants to merge 3 commits into from

Conversation

david-wimmer
Copy link

@david-wimmer david-wimmer commented Nov 9, 2022

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

This PR fixes Issue #7172

- Add property annotation RelationalAnnotationNames.Collation to runtime model
- Consider column collation annotation when creating DELCARE TABLE statement
- Fixed unit test

Fixes dotnet#7172
@dnfadmin
Copy link

dnfadmin commented Nov 9, 2022

CLA assistant check
All CLA requirements met.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@david-wimmer the overall approaches looks good.

However, are you aware that the table variable is no longer used by default in EF Core 7.0 - only when the user explicitly specifies that triggers are defined on the table (see this comment, and this blog post). As a result, we're considering closing this issue as wont-fix, since it requires the intersection of both triggers and an output column with a collation defined (and workarounds exist, see in #7172).

So... Is this an actual problem you're running into with your application?

@@ -570,7 +570,6 @@ public override void Generate(IProperty property, CSharpRuntimeAnnotationCodeGen
{
annotations.Remove(RelationalAnnotationNames.ColumnOrder);
annotations.Remove(RelationalAnnotationNames.Comment);
annotations.Remove(RelationalAnnotationNames.Collation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This preserves the collation annotation in the runtime for all relational providers, to solve a problem that's specific to the SQL Server one. If we want to fix the collation issue, we should try to find a way to preserve the annotation only for SQL Server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to fix this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I committed a fix that the annotation is only preserved for SQL Server.
I am not 100% happy that the GetCollation-ExtensionMethod does not throw InvalidOperationException(CoreStrings.RuntimeModelMissingData) for non SQL Server but could find a better solution.

@david-wimmer
Copy link
Author

I am aware of this. I had to fix it because this is causing a problem in our application.
If you mean the workaround with setting MaxBatchSize(1) this is not suitable for us.

Some sample code to reproduce the issue. This is a similar configuration we are using in our application.

Sample Code

using Microsoft.EntityFrameworkCore;

using (var context = new TestDbContext())
{
    context.Database.EnsureDeleted();
    context.Database.EnsureCreated();
}

using (var context = new TestDbContext())
{
    var people = Enumerable.Range(0, 5)
                 .Select(i => new Person() { ShortName = $"JD_{i}", FirstName = $"John_{i}", LastName = $"Doe_{i}" });

    context.AddRange(people);
    context.SaveChanges();
}

public class Person
{
    public string ShortName { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public byte[] RowVersion { get; set; }
}

public class TestDbContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        base.OnConfiguring(optionsBuilder);

        optionsBuilder.UseSqlServer("Server=LOCALHOST;Database=NonDefaultCollationAndBatching;Integrated Security=True;Encrypt=False");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.Entity<Person>(ba =>
        {
            ba.ToTable(tb => tb.HasTrigger("TR_People_Insert"));

            ba.HasKey(p => p.ShortName);
            ba.Property(p => p.ShortName).HasMaxLength(4).UseCollation("Latin1_General_CS_AS").IsRequired();
            ba.Property(p => p.FirstName).IsRequired();
            ba.Property(p => p.LastName).IsRequired();
            ba.Property(p => p.RowVersion).IsRowVersion().IsRequired();
        });
    }

    public DbSet<Person> People { get; set; }
}

@ajcvickers
Copy link
Member

Note from triage: okay to do it this way, although ultimately the collation should be part of the type mapping: #29620

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@david-wimmer apologies for not reviewing this for so long. I've taken a look and overall it looks OK - but can you please add a test for this? It would go into NonSharedModelUpdatesSqlServerTest; take a look at Bulk_insert_result_set_mapping as an example. This allows you to create an ad-hoc model with a collation and configure your context to use the old SQL (please add a SQL assertion as well so we can be sure the collation is there).

As it's been so long, I can also take over this and complete it - let me know what you prefer.

/cc @AndriySvyryd in case you want to take a quick look for the metaside, this adds collation to the runtime model (for all providers) because of this legacy SQL Server need...

@@ -67,6 +67,11 @@ public class SqlServerRuntimeModelConvention : RelationalRuntimeModelConvention
{
annotations[SqlServerAnnotationNames.ValueGenerationStrategy] = property.GetValueGenerationStrategy();
}

if (!annotations.ContainsKey(RelationalAnnotationNames.Collation))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use TryAdd (here and elsewhere)

@@ -767,12 +767,15 @@ protected virtual int MergeIntoMinimumThreshold
private static string GetTypeNameForCopy(IProperty property)
{
var typeName = property.GetColumnType();
var collation = property[RelationalAnnotationNames.Collation]?.ToString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var collation = property[RelationalAnnotationNames.Collation]?.ToString();
var collation = property.GetCollation();

@ajcvickers
Copy link
Member

We discussed this again and decided it would be better to do anything related to collations as part of #32546 and/or #29620.

@ajcvickers ajcvickers closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants