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

Use case-insensitive string key comparisons on SQL Server #27526

Closed
ajcvickers opened this issue Mar 1, 2022 · 8 comments · Fixed by #29955
Closed

Use case-insensitive string key comparisons on SQL Server #27526

ajcvickers opened this issue Mar 1, 2022 · 8 comments · Fixed by #29955
Labels
area-change-tracking area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Contributor

For example, on SQL Server.

See #27467

@leppie
Copy link

leppie commented May 9, 2022

Also remember about how some databases deal with (and basically ignore) trailing whitespace.

@ajcvickers ajcvickers changed the title Use set case-insensitive key comparisons by convention when underlying database is case-insenstive Use case-insensitive key comparisons by convention when underlying database is case-insenstive Dec 31, 2022
@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed consider-for-current-release labels Dec 31, 2022
@ajcvickers ajcvickers modified the milestones: Backlog, 8.0.0 Dec 31, 2022
@ajcvickers ajcvickers changed the title Use case-insensitive key comparisons by convention when underlying database is case-insenstive Use case-insensitive string key comparisons on SQL Server Jan 5, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0, 8.0.0-preview1 Jan 29, 2023
@Timovzl
Copy link

Timovzl commented Jun 27, 2023

Reading the change, I'm trying to understand if this will cause any string key to be compared in a case-insensitive fashion. That seems like a risky assumption. I have used alphanumeric keys in the past, and they are very much case-sensitive. It's a dangerous assumption that casing can be ignored, even more so in the presence of a case-sensitive collation.

@ajcvickers
Copy link
Contributor Author

@Timovzl This changes the comparisons done by EF by default to be consistent with the comparisons done by SQL Server by default. If you have configured SQL Server to be case-sensitive in general or in this particular case, then switching back to case-sensitive in EF would make sense, but that is rare in my experience.

@Timovzl
Copy link

Timovzl commented Jun 27, 2023

@ajcvickers Thanks for clearing that up!

To switch EF back to case-sensitive, would the sensible approach be to configure a convention for string and pass a case-sensitive comparer to it using HaveConversion()?

@ajcvickers
Copy link
Contributor Author

@Timovzl I would do this, with whatever comparer makes sense for the key: https://learn.microsoft.com/en-us/ef/core/modeling/value-conversions?tabs=data-annotations#use-case-insensitive-string-keys

@ajcvickers ajcvickers modified the milestones: 8.0.0-preview1, 8.0.0 Nov 14, 2023
@shaofing
Copy link

A global or table level switch should be set to address compatibility issues during upgrades

@houbi56
Copy link

houbi56 commented Aug 29, 2024

Would it be possible for EF to control this behaviour depending on collation?

@Timovzl
Copy link

Timovzl commented Aug 29, 2024

Would it be possible for EF to control this behaviour depending on collation?

@houbi56 I wholeheartedly agree with this suggestion.

In fact, I currently use a custom convention that checks the collation for the substring "_CI" and uses it to decide between an Ordinal vs. an OrdinalIgnoreCase ValueComparer. It passes that to SetProviderValueComparer() and, if the ClrType is string, also to SetValueComparer(). I'll include the code below in case it's of use to anyone.

Ignoring casing by default is risky. With it, case-sensitive string IDs can collide in-memory, even if the developer has correctly chosen a case-sensitive collation for their column.

Side note: When using DDD-style custom ID types, with conversions and comparisons appropriately mapped, the default convention for strings is more of a just-in-case configuration than a necessity.

Convention for Collation-Based String Casing

/// <summary>
/// A convention that uses string comparisons with case-sensitivity matching each relevant column's collation, for every property mapped to a string column.
/// This helps align comparisons with those made by the database.
/// </summary>
internal sealed class StringCasingConvention : IModelFinalizingConvention
{
	private static readonly ValueComparer OrdinalComparer = new ValueComparer<string>(
		equalsExpression: (left, right) => String.Equals(left, right, StringComparison.Ordinal),
		hashCodeExpression: value => String.GetHashCode(value, StringComparison.Ordinal),
		snapshotExpression: value => value);

	private static readonly ValueComparer OrdinalIgnoreCaseComparer = new ValueComparer<string>(
		equalsExpression: (left, right) => String.Equals(left, right, StringComparison.OrdinalIgnoreCase),
		hashCodeExpression: value => String.GetHashCode(value, StringComparison.OrdinalIgnoreCase),
		snapshotExpression: value => value);

	public void ProcessModelFinalizing(IConventionModelBuilder modelBuilder, IConventionContext<IConventionModelBuilder> context)
	{
		foreach (var property in modelBuilder.Metadata.GetEntityTypes().SelectMany(entityBuilder => entityBuilder.GetProperties()))
		{
			// Either a plain string property or a property mapped to string
			if (property.ClrType != typeof(string) && property.GetValueConverter()?.ProviderClrType != typeof(string))
				continue;

			var collation = property.FindAnnotation(RelationalAnnotationNames.Collation) ??
				modelBuilder.Metadata.FindAnnotation(RelationalAnnotationNames.Collation);

			// Use case-sensitive comparisons unless ignore-case is explicitly used by the collation
			var comparer = collation?.Value is string collationName && collationName.Contains("_CI", StringComparison.OrdinalIgnoreCase)
				? OrdinalIgnoreCaseComparer
				: OrdinalComparer;

			// We already confirmed that the provider type is string
			if (property.GetProviderValueComparer() is null)
				property.SetProviderValueComparer(comparer, fromDataAnnotation: true); // Using fromDataAnnotation=true allows explicit configuration to override ours

			// The model type COULD be string
			if (property.ClrType == typeof(string) && property.GetValueComparer() is null)
				property.SetValueComparer(comparer, fromDataAnnotation: true); // Using fromDataAnnotation=true allows explicit configuration to override ours
		}
	}
}

@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-change-tracking area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants