Skip to content

Commit

Permalink
Merge ADD COLUMN/MODIFY COLUMN with AUTO_INCREMENT statement with ADD…
Browse files Browse the repository at this point in the history 
… CONSTRAINT PRIMARY KEY statement where necessary, because MySQL requires an AUTO_INCREMENT column to be a key (and making it a key after column creation is already too late). (#1844)

* Merge ADD COLUMN/MODIFY COLUMN with AUTO_INCREMENT statement with ADD CONSTRAINT PRIMARY KEY statement where necessary, because MySQL requires an AUTO_INCREMENT column to be a key (and making it a key after column creation is already too late).

* Streamline code.
  • Loading branch information
@lauxjpn
lauxjpn authored Feb 23, 2024
1 parent ea52863 commit 90072f0
Show file tree
Hide file tree
Showing 2 changed files with 159 additions and 34 deletions.
182 changes: 155 additions & 27 deletions src/EFCore.MySql/Migrations/MySqlMigrationsSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ namespace Pomelo.EntityFrameworkCore.MySql.Migrations
/// </summary>
public class MySqlMigrationsSqlGenerator : MigrationsSqlGenerator
{
private const string InternalAnnotationPrefix = MySqlAnnotationNames.Prefix + "MySqlMigrationsSqlGenerator:";
private const string OutputPrimaryKeyConstraintOnAutoIncrementAnnotationName = InternalAnnotationPrefix + "OutputPrimaryKeyConstraint";

private static readonly Regex _typeRegex = new Regex(@"(?<Name>[a-z0-9]+)\s*?(?:\(\s*(?<Length>\d+)?\s*\))?",
RegexOptions.IgnoreCase);

Expand Down Expand Up @@ -69,6 +72,86 @@ public MySqlMigrationsSqlGenerator(
_stringTypeMapping = dependencies.TypeMappingSource.GetMapping(typeof(string));
}

public override IReadOnlyList<MigrationCommand> Generate(
IReadOnlyList<MigrationOperation> operations,
IModel model = null,
MigrationsSqlGenerationOptions options = MigrationsSqlGenerationOptions.Default)
{
try
{
var filteredOperations = FilterOperations(operations, model);
var migrationCommands = base.Generate(filteredOperations, model, options);

return migrationCommands;
}
finally
{
CleanUpInternalAnnotations(operations);
}
}

private static void CleanUpInternalAnnotations(IReadOnlyList<MigrationOperation> filteredOperations)
{
foreach (var filteredOperation in filteredOperations)
{
foreach (var annotation in filteredOperation.GetAnnotations().ToList())
{
if (annotation.Name.StartsWith(InternalAnnotationPrefix))
{
filteredOperation.RemoveAnnotation(annotation.Name);
}
}
}
}

protected virtual IReadOnlyList<MigrationOperation> FilterOperations(IReadOnlyList<MigrationOperation> operations, IModel model)
{
if (operations.Count <= 0)
{
return operations;
}

var filteredOperations = new List<MigrationOperation>();

var previousOperation = operations.First();
filteredOperations.Add(previousOperation);

foreach (var currentOperation in operations.Skip(1))
{
// Merge a ColumnOperation immediately followed by an AddPrimaryKeyOperation into a single operation (and SQL statement), if
// the ColumnOperation is for an AUTO_INCREMENT column. The *immediately followed* restriction could be lifted, if it later
// turns out to be necessary.
// MySQL dictates that there can be only one AUTO_INCREMENT column and it has to be a key.
// If we first add a new column with the AUTO_INCREMENT flag and *then* make it a primary key in the *next* statement, the
// first statement will fail, because the column is not a key yet, and AUTO_INCREMENT columns have to be keys.
if (previousOperation is ColumnOperation columnOperation &&
currentOperation is AddPrimaryKeyOperation addPrimaryKeyOperation &&
addPrimaryKeyOperation.Schema == columnOperation.Schema &&
addPrimaryKeyOperation.Table == columnOperation.Table &&
addPrimaryKeyOperation.Columns.Length == 1 &&
addPrimaryKeyOperation.Columns[0] == columnOperation.Name &&
// The following 3 conditions match the ones from `ColumnDefinition()`.
MySqlValueGenerationStrategyCompatibility.GetValueGenerationStrategy(columnOperation.GetAnnotations().OfType<IAnnotation>().ToArray()) is var valueGenerationStrategy &&
GetColumBaseTypeAndLength(columnOperation, model) is var (columnBaseType, _) &&
IsAutoIncrement(columnOperation, columnBaseType, valueGenerationStrategy))
{
// This internal annotation lets our `ColumnDefinition()` implementation generate a second clause for the primary key
// constraint in the same statement.
columnOperation[OutputPrimaryKeyConstraintOnAutoIncrementAnnotationName] = true;

// We now skip adding the AddPrimaryKeyOperation to the list of operations.
}
else
{
filteredOperations.Add(currentOperation);
}

previousOperation = currentOperation;
}

return filteredOperations.AsReadOnly();
}

/// <summary>
/// <para>
/// Builds commands for the given <see cref="MigrationOperation" /> by making calls on the given
Expand Down Expand Up @@ -1031,33 +1114,16 @@ protected override void ColumnDefinition(
Check.NotNull(operation, nameof(operation));
Check.NotNull(builder, nameof(builder));

var matchType = GetColumnType(schema, table, name, operation, model);
var matchLen = "";
var match = _typeRegex.Match(matchType ?? "-");
if (match.Success)
{
matchType = match.Groups["Name"].Value.ToLower();
if (match.Groups["Length"].Success)
{
matchLen = match.Groups["Length"].Value;
}
}

var (matchType, matchLen) = GetColumBaseTypeAndLength(schema, table, name, operation, model);
var valueGenerationStrategy = MySqlValueGenerationStrategyCompatibility.GetValueGenerationStrategy(operation.GetAnnotations().OfType<IAnnotation>().ToArray());
var autoIncrement = IsAutoIncrement(operation, matchType, valueGenerationStrategy);

var autoIncrement = false;
if (valueGenerationStrategy == MySqlValueGenerationStrategy.IdentityColumn &&
string.IsNullOrWhiteSpace(operation.DefaultValueSql) && operation.DefaultValue == null)
if (!autoIncrement &&
valueGenerationStrategy == MySqlValueGenerationStrategy.IdentityColumn &&
string.IsNullOrWhiteSpace(operation.DefaultValueSql))
{
switch (matchType)
{
case "tinyint":
case "smallint":
case "mediumint":
case "int":
case "bigint":
autoIncrement = true;
break;
case "datetime":
if (!_options.ServerVersion.Supports.DateTimeCurrentTimestamp)
{
Expand Down Expand Up @@ -1102,15 +1168,29 @@ protected override void ColumnDefinition(
{
ColumnDefinitionWithCharSet(schema, table, name, operation, model, builder);

GenerateComment(operation.Comment, builder);

// AUTO_INCREMENT has priority over reference definitions.
if (autoIncrement)
{
builder.Append(" AUTO_INCREMENT");
}

GenerateComment(operation.Comment, builder);

// AUTO_INCREMENT has priority over reference definitions.
if (onUpdateSql != null && !autoIncrement)
// TODO: Add support for a non-primary key that is used as with auto_increment.
if (model?.GetRelationalModel().FindTable(table, schema) is { PrimaryKey: { Columns.Count: 1 } primaryKey } &&
primaryKey.Columns[0].Name == operation.Name &&
(bool?)operation[OutputPrimaryKeyConstraintOnAutoIncrementAnnotationName] == true)
{
builder
.AppendLine(",")
.Append("ADD ");

PrimaryKeyConstraint(
AddPrimaryKeyOperation.CreateFrom(primaryKey),
model,
builder);
}
}
else if (onUpdateSql != null)
{
builder
.Append(" ON UPDATE ")
Expand Down Expand Up @@ -1141,6 +1221,54 @@ protected override void ColumnDefinition(
}
}

protected virtual (string matchType, string matchLen) GetColumBaseTypeAndLength(
ColumnOperation operation,
IModel model)
=> GetColumBaseTypeAndLength(operation.Schema, operation.Table, operation.Name, operation, model);

protected virtual (string matchType, string matchLen) GetColumBaseTypeAndLength(
string schema,
string table,
string name,
ColumnOperation operation,
IModel model)
{
var matchType = GetColumnType(schema, table, name, operation, model);
var matchLen = "";
var match = _typeRegex.Match(matchType ?? "-");
if (match.Success)
{
matchType = match.Groups["Name"].Value.ToLower();
if (match.Groups["Length"].Success)
{
matchLen = match.Groups["Length"].Value;
}
}

return (matchType, matchLen);
}

protected virtual bool IsAutoIncrement(ColumnOperation operation,
string columnType,
MySqlValueGenerationStrategy? valueGenerationStrategy)
{
if (valueGenerationStrategy == MySqlValueGenerationStrategy.IdentityColumn &&
string.IsNullOrWhiteSpace(operation.DefaultValueSql))
{
switch (columnType)
{
case "tinyint":
case "smallint":
case "mediumint":
case "int":
case "bigint":
return true;
}
}

return false;
}

private void GenerateComment(string comment, MigrationCommandListBuilder builder)
{
if (comment == null)
Expand Down
11 changes: 4 additions & 7 deletions test/EFCore.MySql.FunctionalTests/MigrationsMySqlTest.cs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using System;
using System.Linq;
using System.Threading.Tasks;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;
Expand Down Expand Up @@ -269,12 +268,6 @@ await Test(
});
}

[ConditionalTheory(Skip = "TODO")]
public override Task Add_primary_key_int()
{
return base.Add_primary_key_int();
}

[ConditionalTheory(Skip = "TODO")]
public override async Task Add_primary_key_string()
{
Expand Down Expand Up @@ -1309,6 +1302,10 @@ public override Task Rename_table()
},
withConventions: false);

// The constraint name for a primary key is always PRIMARY in MySQL.
protected override bool AssertConstraintNames
=> false;

protected virtual string DefaultCollation => ((MySqlTestStore)Fixture.TestStore).DatabaseCollation;

protected override string NonDefaultCollation
Expand Down

0 comments on commit 90072f0

Please sign in to comment.