Skip to content

Commit

Permalink
Stop using case-insensitive comparisons for indexed properties (#32899)
Browse files Browse the repository at this point in the history 
Fixes #32898

Note that we didn't change what we do in the update pipeline, which appears to already be using provider values.
  • Loading branch information
@ajcvickers
ajcvickers committed Jan 25, 2024
1 parent 1b51aa3 commit d2df9cf
Show file tree
Hide file tree
Showing 5 changed files with 216 additions and 8 deletions.
58 changes: 54 additions & 4 deletions src/EFCore.Relational/Storage/RelationalTypeMappingInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public RelationalTypeMappingInfo(
int? scale)
{
// Note: Empty string is allowed for store type name because SQLite
_coreTypeMappingInfo = new TypeMappingInfo(null, null, false, unicode, size, null, precision, scale);
_coreTypeMappingInfo = new TypeMappingInfo(null, null, false, unicode, size, null, precision, scale, false);
StoreTypeName = storeTypeName;
StoreTypeNameBase = storeTypeNameBase;
IsFixedLength = null;
Expand Down Expand Up @@ -225,6 +225,7 @@ public RelationalTypeMappingInfo(
/// <param name="precision">Specifies a precision for the mapping, or <see langword="null" /> for default.</param>
/// <param name="scale">Specifies a scale for the mapping, or <see langword="null" /> for default.</param>
/// <param name="dbType">The suggested <see cref="DbType" />, or <see langword="null" /> for default.</param>
/// <param name="key">If <see langword="true" />, then a special mapping for a key may be returned.</param>
public RelationalTypeMappingInfo(
Type? type = null,
RelationalTypeMapping? elementTypeMapping = null,
Expand All @@ -237,16 +238,56 @@ public RelationalTypeMappingInfo(
bool? fixedLength = null,
int? precision = null,
int? scale = null,
DbType? dbType = null)
DbType? dbType = null,
bool key = false)
{
_coreTypeMappingInfo = new TypeMappingInfo(type, elementTypeMapping, keyOrIndex, unicode, size, rowVersion, precision, scale);
_coreTypeMappingInfo = new TypeMappingInfo(type, elementTypeMapping, keyOrIndex, unicode, size, rowVersion, precision, scale, key);

IsFixedLength = fixedLength;
StoreTypeName = storeTypeName;
StoreTypeNameBase = storeTypeNameBase;
DbType = dbType;
}

/// <summary>
/// Creates a new instance of <see cref="TypeMappingInfo" />.
/// </summary>
/// <param name="type">The CLR type in the model for which mapping is needed.</param>
/// <param name="typeMappingConfiguration">The type mapping configuration.</param>
/// <param name="elementTypeMapping">The type mapping for elements, if known.</param>
/// <param name="storeTypeName">The database type name.</param>
/// <param name="storeTypeNameBase">The provider-specific relational type name, with any facets removed.</param>
/// <param name="unicode">Specifies Unicode or ANSI mapping, or <see langword="null" /> for default.</param>
/// <param name="size">Specifies a size for the mapping, or <see langword="null" /> for default.</param>
/// <param name="precision">Specifies a precision for the mapping, or <see langword="null" /> for default.</param>
/// <param name="scale">Specifies a scale for the mapping, or <see langword="null" /> for default.</param>
public RelationalTypeMappingInfo(
Type type,
ITypeMappingConfiguration typeMappingConfiguration,
RelationalTypeMapping? elementTypeMapping = null,
string? storeTypeName = null,
string? storeTypeNameBase = null,
bool? unicode = null,
int? size = null,
int? precision = null,
int? scale = null)
{
_coreTypeMappingInfo = new TypeMappingInfo(
typeMappingConfiguration.GetValueConverter()?.ProviderClrType ?? type,
elementTypeMapping,
keyOrIndex: false,
unicode ?? typeMappingConfiguration.IsUnicode(),
size ?? typeMappingConfiguration.GetMaxLength(),
rowVersion: false,
precision ?? typeMappingConfiguration.GetPrecision(),
scale ?? typeMappingConfiguration.GetScale(),
key: false);

IsFixedLength = (bool?)typeMappingConfiguration[RelationalAnnotationNames.IsFixedLength];
StoreTypeName = storeTypeName;
StoreTypeNameBase = storeTypeNameBase;
}

/// <summary>
/// The core type mapping info.
/// </summary>
Expand Down Expand Up @@ -301,7 +342,16 @@ public int? Scale
public DbType? DbType { get; init; }

/// <summary>
/// Indicates whether or not the mapping is part of a key or index.
/// Indicates whether or not the mapping is part of a key or foreign key.
/// </summary>
public bool IsKey
{
get => _coreTypeMappingInfo.IsKey;
init => _coreTypeMappingInfo = _coreTypeMappingInfo with { IsKey = value };
}

/// <summary>
/// Indicates whether or not the mapping is part of a key, foreign key, or index.
/// </summary>
public bool IsKeyOrIndex
{
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ public SqlServerTypeMappingSource(
size: size,
fixedLength: isFixedLength,
storeTypePostfix: storeTypeName == null ? StoreTypePostfix.Size : StoreTypePostfix.None,
useKeyComparison: mappingInfo.IsKeyOrIndex);
useKeyComparison: mappingInfo.IsKey);
}

if (clrType == typeof(byte[]))
Expand Down
16 changes: 13 additions & 3 deletions src/EFCore/Storage/TypeMappingInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ public TypeMappingInfo(
var property = principals[0];

ElementTypeMapping = property.GetElementType()?.FindTypeMapping();
IsKeyOrIndex = property.IsKey() || property.IsForeignKey() || property.IsIndex();
IsKey = property.IsKey() || property.IsForeignKey();
IsKeyOrIndex = IsKey || property.IsIndex();
Size = fallbackSize ?? mappingHints?.Size;
IsUnicode = fallbackUnicode ?? mappingHints?.IsUnicode;
IsRowVersion = property is { IsConcurrencyToken: true, ValueGenerated: ValueGenerated.OnAddOrUpdate };
Expand Down Expand Up @@ -227,6 +228,7 @@ public TypeMappingInfo(
/// <param name="rowVersion">Specifies a row-version, or <see langword="null" /> for default.</param>
/// <param name="precision">Specifies a precision for the mapping, or <see langword="null" /> for default.</param>
/// <param name="scale">Specifies a scale for the mapping, or <see langword="null" /> for default.</param>
/// <param name="key">If <see langword="true" />, then a special mapping for a key may be returned.</param>
public TypeMappingInfo(
Type? type = null,
CoreTypeMapping? elementTypeMapping = null,
Expand All @@ -235,11 +237,13 @@ public TypeMappingInfo(
int? size = null,
bool? rowVersion = null,
int? precision = null,
int? scale = null)
int? scale = null,
bool key = false)
{
ClrType = type?.UnwrapNullableType();
ElementTypeMapping = elementTypeMapping;

IsKey = key;
IsKeyOrIndex = keyOrIndex;
Size = size;
IsUnicode = unicode;
Expand All @@ -266,6 +270,7 @@ public TypeMappingInfo(
int? scale = null)
{
IsRowVersion = source.IsRowVersion;
IsKey = source.IsKey;
IsKeyOrIndex = source.IsKeyOrIndex;

var mappingHints = converter.MappingHints;
Expand Down Expand Up @@ -295,7 +300,12 @@ public TypeMappingInfo WithConverter(in ValueConverterInfo converterInfo)
=> new(this, converterInfo);

/// <summary>
/// Indicates whether or not the mapping is part of a key or index.
/// Indicates whether or not the mapping is part of a key or foreign key.
/// </summary>
public bool IsKey { get; init; }

/// <summary>
/// Indicates whether or not the mapping is part of a key, foreign key, or index.
/// </summary>
public bool IsKeyOrIndex { get; init; }

Expand Down
11 changes: 11 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,17 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
b.Property(e => e.IdUserState).HasDefaultValue(1).HasSentinel(667);
b.HasOne(e => e.UserState).WithMany(e => e.Users).HasForeignKey(e => e.IdUserState);
});

modelBuilder.Entity<StringKeyAndIndexParent>(
b =>
{
b.HasAlternateKey(e => e.AlternateId);
b.OwnsOne(
x => x.Child, b =>
{
b.WithOwner(e => e.Parent).HasForeignKey(e => e.ParentId);
});
});
}
}
}
137 changes: 137 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,134 @@ protected GraphUpdatesSqlServerTestBase(TFixture fixture)
{
}

[ConditionalFact] // Issue #32638
public virtual void Key_and_index_properties_use_appropriate_comparer()
{
var parent = new StringKeyAndIndexParent
{
Id = "Parent",
AlternateId = "Parent",
Index = "Index",
UniqueIndex = "UniqueIndex"
};

var child = new StringKeyAndIndexChild
{
Id = "Child",
ParentId = "parent"
};

using var context = CreateContext();
context.AttachRange(parent, child);

Assert.Same(child, parent.Child);
Assert.Same(parent, child.Parent);

parent.Id = "parent";
parent.AlternateId = "parent";
parent.Index = "index";
parent.UniqueIndex = "uniqueIndex";
child.Id = "child";
child.ParentId = "Parent";

context.ChangeTracker.DetectChanges();

var parentEntry = context.Entry(parent);
Assert.Equal(EntityState.Modified, parentEntry.State);
Assert.False(parentEntry.Property(e => e.Id).IsModified);
Assert.False(parentEntry.Property(e => e.AlternateId).IsModified);
Assert.True(parentEntry.Property(e => e.Index).IsModified);
Assert.True(parentEntry.Property(e => e.UniqueIndex).IsModified);

var childEntry = context.Entry(child);

if (childEntry.Metadata.IsOwned())
{
Assert.Equal(EntityState.Modified, childEntry.State);
Assert.True(childEntry.Property(e => e.Id).IsModified); // Not a key for the owned type
Assert.False(childEntry.Property(e => e.ParentId).IsModified);
}
else
{
Assert.Equal(EntityState.Unchanged, childEntry.State);
Assert.False(childEntry.Property(e => e.Id).IsModified);
Assert.False(childEntry.Property(e => e.ParentId).IsModified);
}

}

protected class StringKeyAndIndexParent : NotifyingEntity
{
private string _id;
private string _alternateId;
private string _uniqueIndex;
private string _index;
private StringKeyAndIndexChild _child;

public string Id
{
get => _id;
set => SetWithNotify(value, ref _id);
}

public string AlternateId
{
get => _alternateId;
set => SetWithNotify(value, ref _alternateId);
}

public string Index
{
get => _index;
set => SetWithNotify(value, ref _index);
}

public string UniqueIndex
{
get => _uniqueIndex;
set => SetWithNotify(value, ref _uniqueIndex);
}

public StringKeyAndIndexChild Child
{
get => _child;
set => SetWithNotify(value, ref _child);
}
}

protected class StringKeyAndIndexChild : NotifyingEntity
{
private string _id;
private string _parentId;
private int _foo;
private StringKeyAndIndexParent _parent;

public string Id
{
get => _id;
set => SetWithNotify(value, ref _id);
}

public string ParentId
{
get => _parentId;
set => SetWithNotify(value, ref _parentId);
}


public int Foo
{
get => _foo;
set => SetWithNotify(value, ref _foo);
}

public StringKeyAndIndexParent Parent
{
get => _parent;
set => SetWithNotify(value, ref _parent);
}
}

protected override IQueryable<Root> ModifyQueryRoot(IQueryable<Root> query)
=> query.AsSplitQuery();

Expand Down Expand Up @@ -59,6 +187,15 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con

modelBuilder.Entity<SomethingOfCategoryA>().Property<int>("CategoryId").HasDefaultValue(1);
modelBuilder.Entity<SomethingOfCategoryB>().Property(e => e.CategoryId).HasDefaultValue(2);

modelBuilder.Entity<StringKeyAndIndexParent>(
b =>
{
b.HasOne(e => e.Child)
.WithOne(e => e.Parent)
.HasForeignKey<StringKeyAndIndexChild>(e => e.ParentId)
.HasPrincipalKey<StringKeyAndIndexParent>(e => e.AlternateId);
});
}
}
}

0 comments on commit d2df9cf

Please sign in to comment.