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.

Sign up for GitHub

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

Propagate MaxLength, Precision, Scale and IsUnicode to sharing columns #22396

Merged
merged 1 commit into from
Sep 4, 2020
Merged

Propagate MaxLength, Precision, Scale and IsUnicode to sharing columns #22396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 10 additions & 12 deletions src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ private void GenerateKey(IKey key, IEntityType entityType, bool useDataAnnotatio
if (explicitName)
{
lines.Add(
$".{nameof(RelationalKeyBuilderExtensions.HasName)}" + $"({_code.Literal(key.GetName())})");
$".{nameof(RelationalKeyBuilderExtensions.HasName)}({_code.Literal(key.GetName())})");
}

lines.AddRange(
Expand Down Expand Up @@ -535,8 +535,7 @@ private void GenerateIndex(IIndex index)

var lines = new List<string>
{
$".{nameof(EntityTypeBuilder.HasIndex)}"
+ $"({_code.Lambda(index.Properties, "e")}, "
$".{nameof(EntityTypeBuilder.HasIndex)}({_code.Lambda(index.Properties, "e")}, "
+ $"{_code.Literal(index.GetDatabaseName())})"
};
annotations.Remove(RelationalAnnotationNames.Name);
Expand Down Expand Up @@ -587,15 +586,15 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations)
if (columnType != null)
{
lines.Add(
$".{nameof(RelationalPropertyBuilderExtensions.HasColumnType)}" + $"({_code.Literal(columnType)})");
$".{nameof(RelationalPropertyBuilderExtensions.HasColumnType)}({_code.Literal(columnType)})");
}

var maxLength = property.GetMaxLength();

if (maxLength.HasValue)
{
lines.Add(
$".{nameof(PropertyBuilder.HasMaxLength)}" + $"({_code.Literal(maxLength.Value)})");
$".{nameof(PropertyBuilder.HasMaxLength)}({_code.Literal(maxLength.Value)})");
}
}

Expand All @@ -604,18 +603,18 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations)
if (precision != null && scale != null && scale != 0)
{
lines.Add(
$".{nameof(PropertyBuilder.HasPrecision)}" + $"({_code.Literal(precision.Value)}, {_code.Literal(scale.Value)})");
$".{nameof(PropertyBuilder.HasPrecision)}({_code.Literal(precision.Value)}, {_code.Literal(scale.Value)})");
}
else if (precision != null)
{
lines.Add(
$".{nameof(PropertyBuilder.HasPrecision)}" + $"({_code.Literal(precision.Value)})");
$".{nameof(PropertyBuilder.HasPrecision)}({_code.Literal(precision.Value)})");
}

if (property.IsUnicode() != null)
{
lines.Add(
$".{nameof(PropertyBuilder.IsUnicode)}" + $"({(property.IsUnicode() == false ? "false" : "")})");
$".{nameof(PropertyBuilder.IsUnicode)}({(property.IsUnicode() == false ? "false" : "")})");
}

var defaultValue = property.GetDefaultValue();
Expand All @@ -627,7 +626,7 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations)
else if (defaultValue != null)
{
lines.Add(
$".{nameof(RelationalPropertyBuilderExtensions.HasDefaultValue)}" + $"({_code.UnknownLiteral(defaultValue)})");
$".{nameof(RelationalPropertyBuilderExtensions.HasDefaultValue)}({_code.UnknownLiteral(defaultValue)})");
annotations.Remove(RelationalAnnotationNames.DefaultValue);
}

Expand Down Expand Up @@ -715,7 +714,7 @@ private void GenerateRelationship(IForeignKey foreignKey, bool useDataAnnotation
{
canUseDataAnnotations = false;
lines.Add(
$".{nameof(ReferenceReferenceBuilder.OnDelete)}" + $"({_code.Literal(foreignKey.DeleteBehavior)})");
$".{nameof(ReferenceReferenceBuilder.OnDelete)}({_code.Literal(foreignKey.DeleteBehavior)})");
}

if (!string.IsNullOrEmpty((string)foreignKey[RelationalAnnotationNames.Name]))
Expand Down Expand Up @@ -800,7 +799,6 @@ private void GenerateSequence(ISequence sequence)

private IList<string> GenerateAnnotations(IEnumerable<IAnnotation> annotations)
=> annotations.Select(
a =>
$".HasAnnotation({_code.Literal(a.Name)}, " + $"{_code.UnknownLiteral(a.Value)})").ToList();
a => $".HasAnnotation({_code.Literal(a.Name)}, {_code.UnknownLiteral(a.Value)})").ToList();
}
}
83 changes: 83 additions & 0 deletions src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,89 @@ private static object ConvertDefaultValue([NotNull] IProperty property, [CanBeNu
public static ConfigurationSource? GetDefaultValueConfigurationSource([NotNull] this IConventionProperty property)
=> property.FindAnnotation(RelationalAnnotationNames.DefaultValue)?.GetConfigurationSource();

/// <summary>
/// Gets the maximum length of data that is allowed in this property. For example, if the property is a <see cref="string" />
/// then this is the maximum number of characters.
/// </summary>
/// <param name="property"> The property. </param>
/// <param name="storeObject"> The identifier of the table-like store object containing the column. </param>
/// <returns> The maximum length, or <see langword="null" /> if none if defined. </returns>
public static int? GetMaxLength([NotNull] this IProperty property, in StoreObjectIdentifier storeObject)
Copy link

@Neme12 Neme12 Dec 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@AndriySvyryd @smitpatel @ajcvickers Honestly I'm not surprised that so many bugs and regressions creep into EF Core with every release. This PR changes a whole bunch of code with only 1 small test added. These are new public APIs and are completely untested. Please, this needs to change 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

@Neme12 The test exercises and validates all the added code. But you have a point, we can certainly spend more time writing tests instead of adding more features. However our efforts would still fall short of the real-world scenarios that you and other early adopters run with the preview versions.
Thank you for your patience and diligence in reporting the issues!

{
Check.NotNull(property, nameof(property));

var maxLength = property.GetMaxLength();
if (maxLength != null)
{
return maxLength.Value;
}

var sharedTableRootProperty = property.FindSharedStoreObjectRootProperty(storeObject);
return sharedTableRootProperty != null ? GetMaxLength(sharedTableRootProperty, storeObject) : null;
}

/// <summary>
/// Gets the precision of data that is allowed in this property.
/// For example, if the property is a <see cref="decimal" /> then this is the maximum number of digits.
/// </summary>
/// <param name="property"> The property. </param>
/// <param name="storeObject"> The identifier of the table-like store object containing the column. </param>
/// <returns> The precision, or <see langword="null" /> if none is defined. </returns>
public static int? GetPrecision([NotNull] this IProperty property, in StoreObjectIdentifier storeObject)
{
Check.NotNull(property, nameof(property));

var precision = property.GetPrecision();
if (precision != null)
{
return precision;
}

var sharedTableRootProperty = property.FindSharedStoreObjectRootProperty(storeObject);
return sharedTableRootProperty != null ? GetPrecision(sharedTableRootProperty, storeObject) : null;
}

/// <summary>
/// Gets the scale of data that is allowed in this property.
/// For example, if the property is a <see cref="decimal" /> then this is the maximum number of decimal places.
/// </summary>
/// <param name="property"> The property. </param>
/// <param name="storeObject"> The identifier of the table-like store object containing the column. </param>
/// <returns> The scale, or <see langword="null" /> if none is defined. </returns>
public static int? GetScale([NotNull] this IProperty property, in StoreObjectIdentifier storeObject)
{
Check.NotNull(property, nameof(property));

var scale = property.GetScale();
if (scale != null)
{
return scale.Value;
}

var sharedTableRootProperty = property.FindSharedStoreObjectRootProperty(storeObject);
return sharedTableRootProperty != null ? GetScale(sharedTableRootProperty, storeObject) : null;
}

/// <summary>
/// Gets a value indicating whether or not the property can persist Unicode characters.
/// </summary>
/// <param name="property"> The property. </param>
/// <param name="storeObject"> The identifier of the table-like store object containing the column. </param>
/// <returns> The Unicode setting, or <see langword="null" /> if none is defined. </returns>
public static bool? IsUnicode([NotNull] this IProperty property, in StoreObjectIdentifier storeObject)
{
Check.NotNull(property, nameof(property));

var unicode = property.IsUnicode();
if (unicode != null)
{
return unicode.Value;
}

var sharedTableRootProperty = property.FindSharedStoreObjectRootProperty(storeObject);
return sharedTableRootProperty != null ? IsUnicode(sharedTableRootProperty, storeObject) : null;
}

/// <summary>
/// Returns a flag indicating if the property as capable of storing only fixed-length data, such as strings.
/// </summary>
Expand Down
40 changes: 36 additions & 4 deletions src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ protected virtual void ValidateCompatible(
in StoreObjectIdentifier storeObject,
[NotNull] IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger)
{
if (property.IsNullable != duplicateProperty.IsNullable)
if (property.IsColumnNullable(storeObject) != duplicateProperty.IsColumnNullable(storeObject))
{
throw new InvalidOperationException(
RelationalStrings.DuplicateColumnNameNullabilityMismatch(
Expand All @@ -702,8 +702,8 @@ protected virtual void ValidateCompatible(
storeObject.DisplayName()));
}

var currentMaxLength = property.GetMaxLength();
var previousMaxLength = duplicateProperty.GetMaxLength();
var currentMaxLength = property.GetMaxLength(storeObject);
var previousMaxLength = duplicateProperty.GetMaxLength(storeObject);
if (currentMaxLength != previousMaxLength)
{
throw new InvalidOperationException(
Expand All @@ -718,7 +718,7 @@ protected virtual void ValidateCompatible(
currentMaxLength));
}

if (property.IsUnicode() != duplicateProperty.IsUnicode())
if (property.IsUnicode(storeObject) != duplicateProperty.IsUnicode(storeObject))
{
throw new InvalidOperationException(
RelationalStrings.DuplicateColumnNameUnicodenessMismatch(
Expand All @@ -742,6 +742,38 @@ protected virtual void ValidateCompatible(
storeObject.DisplayName()));
}

var currentPrecision = property.GetPrecision(storeObject);
var previousPrecision = duplicateProperty.GetPrecision(storeObject);
if (currentPrecision != previousPrecision)
{
throw new InvalidOperationException(
RelationalStrings.DuplicateColumnNamePrecisionMismatch(
duplicateProperty.DeclaringEntityType.DisplayName(),
duplicateProperty.Name,
property.DeclaringEntityType.DisplayName(),
property.Name,
columnName,
storeObject.DisplayName(),
currentPrecision,
previousPrecision));
}

var currentScale = property.GetScale(storeObject);
var previousScale = duplicateProperty.GetScale(storeObject);
if (currentScale != previousScale)
{
throw new InvalidOperationException(
RelationalStrings.DuplicateColumnNameScaleMismatch(
duplicateProperty.DeclaringEntityType.DisplayName(),
duplicateProperty.Name,
property.DeclaringEntityType.DisplayName(),
property.Name,
columnName,
storeObject.DisplayName(),
currentScale,
previousScale));
}

if (property.IsConcurrencyToken != duplicateProperty.IsConcurrencyToken)
{
throw new InvalidOperationException(
Expand Down
8 changes: 4 additions & 4 deletions src/EFCore.Relational/Metadata/IColumn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,27 @@ public interface IColumn : IColumnBase
/// then this is the maximum number of characters.
/// </summary>
int? MaxLength
=> PropertyMappings.First().Property.GetMaxLength();
=> PropertyMappings.First().Property.GetMaxLength(StoreObjectIdentifier.Table(Table.Name, Table.Schema));

/// <summary>
/// Gets the precision of data that is allowed in this column. For example, if the property is a <see cref="decimal" /> '
/// then this is the maximum number of digits.
/// </summary>
int? Precision
=> PropertyMappings.First().Property.GetPrecision();
=> PropertyMappings.First().Property.GetPrecision(StoreObjectIdentifier.Table(Table.Name, Table.Schema));

/// <summary>
/// Gets the scale of data that is allowed in this column. For example, if the property is a <see cref="decimal" /> '
/// then this is the maximum number of decimal places.
/// </summary>
int? Scale
=> PropertyMappings.First().Property.GetScale();
=> PropertyMappings.First().Property.GetScale(StoreObjectIdentifier.Table(Table.Name, Table.Schema));

/// <summary>
/// Gets a value indicating whether or not the property can persist Unicode characters.
/// </summary>
bool? IsUnicode
=> PropertyMappings.First().Property.IsUnicode();
=> PropertyMappings.First().Property.IsUnicode(StoreObjectIdentifier.Table(Table.Name, Table.Schema));

/// <summary>
/// Returns a flag indicating if the property as capable of storing only fixed-length data, such as strings.
Expand Down
18 changes: 17 additions & 1 deletion src/EFCore.Relational/Properties/RelationalStrings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion src/EFCore.Relational/Properties/RelationalStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,17 @@
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured to use different stored computed column settings ('{value1}' and '{value2}').</value>
</data>
<data name="DuplicateColumnNameMaxLengthMismatch" xml:space="preserve">
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured to use different max lengths ('{maxLength1}' and '{maxLength2}').</value>
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different max lengths ('{maxLength1}' and '{maxLength2}').</value>
</data>
<data name="DuplicateColumnNameNullabilityMismatch" xml:space="preserve">
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different nullability.</value>
</data>
<data name="DuplicateColumnNamePrecisionMismatch" xml:space="preserve">
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different precision ('{precision1}' and '{precision2}').</value>
</data>
<data name="DuplicateColumnNameScaleMismatch" xml:space="preserve">
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different scale ('{scale1}' and '{scale2}').</value>
</data>
<data name="DuplicateColumnNameUnicodenessMismatch" xml:space="preserve">
<value>'{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different unicode configuration</value>
</data>
Expand Down
Loading