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

Jump to bottom

Change from sequence-per-model to sequence-per-hierarchy when using sequences for key generation #28501

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Change from sequence-per-model to sequence-per-hierarchy when using sequences for key generation #28501

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
13 changes: 8 additions & 5 deletions src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,15 +401,18 @@ protected override bool IsHandledByConvention(IModel model, IAnnotation annotati

case SqlServerValueGenerationStrategy.Sequence:
{
var name = GetAndRemove<string>(annotations, SqlServerAnnotationNames.KeySequenceName);
var schema = GetAndRemove<string>(annotations, SqlServerAnnotationNames.KeySequenceSchema);
var nameOrSuffix = GetAndRemove<string>(
annotations,
onModel ? SqlServerAnnotationNames.SequenceNameSuffix : SqlServerAnnotationNames.SequenceName);

var schema = GetAndRemove<string>(annotations, SqlServerAnnotationNames.SequenceSchema);
return new MethodCallCodeFragment(
onModel ? ModelUseKeySequencesMethodInfo : PropertyUseSequenceMethodInfo,
(name, schema) switch
(name: nameOrSuffix, schema) switch
{
(null, null) => Array.Empty<object>(),
(_, null) => new object[] { name },
_ => new object[] { name!, schema }
(_, null) => new object[] { nameOrSuffix },
_ => new object[] { nameOrSuffix!, schema }
});
}

Expand Down
43 changes: 19 additions & 24 deletions src/EFCore.SqlServer/Extensions/SqlServerModelBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ public static ModelBuilder UseHiLo(
model.SetValueGenerationStrategy(SqlServerValueGenerationStrategy.SequenceHiLo);
model.SetHiLoSequenceName(name);
model.SetHiLoSequenceSchema(schema);
model.SetKeySequenceName(null);
model.SetKeySequenceSchema(null);
model.SetSequenceNameSuffix(null);
model.SetSequenceSchema(null);
model.SetIdentitySeed(null);
model.SetIdentityIncrement(null);

Expand Down Expand Up @@ -115,7 +115,7 @@ public static bool CanSetHiLoSequence(
}

/// <summary>
/// Configures the model to use a sequence to generate values for key properties
/// Configures the model to use a sequence per hierarchy to generate values for key properties
/// marked as <see cref="ValueGenerated.OnAdd" />, when targeting SQL Server.
/// </summary>
/// <remarks>
Expand All @@ -124,29 +124,24 @@ public static bool CanSetHiLoSequence(
/// for more information and examples.
/// </remarks>
/// <param name="modelBuilder">The model builder.</param>
/// <param name="name">The name of the sequence.</param>
/// <param name="nameSuffix">The name that will suffix the table name for each sequence created automatically.</param>
/// <param name="schema">The schema of the sequence.</param>
/// <returns>The same builder instance so that multiple calls can be chained.</returns>
public static ModelBuilder UseKeySequences(
this ModelBuilder modelBuilder,
string? name = null,
string? nameSuffix = null,
string? schema = null)
{
Check.NullButNotEmpty(name, nameof(name));
Check.NullButNotEmpty(nameSuffix, nameof(nameSuffix));
Check.NullButNotEmpty(schema, nameof(schema));

var model = modelBuilder.Model;

name ??= SqlServerModelExtensions.DefaultKeySequenceName;

if (model.FindSequence(name, schema) == null)
{
modelBuilder.HasSequence(name, schema);
}
nameSuffix ??= SqlServerModelExtensions.DefaultSequenceNameSuffix;

model.SetValueGenerationStrategy(SqlServerValueGenerationStrategy.Sequence);
model.SetKeySequenceName(name);
model.SetKeySequenceSchema(schema);
model.SetSequenceNameSuffix(nameSuffix);
model.SetSequenceSchema(schema);
model.SetHiLoSequenceName(null);
model.SetHiLoSequenceSchema(null);
model.SetIdentitySeed(null);
Expand Down Expand Up @@ -180,10 +175,10 @@ public static ModelBuilder UseKeySequences(
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove this method and CanSetKeySequences, it doesn't seem very useful and the name doesn't really match the functionality

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndriySvyryd In this code, should the KeySequences annotations be unconditionally set to null, regardless of configuration source?

public static IConventionModelBuilder? HasValueGenerationStrategy(
        this IConventionModelBuilder modelBuilder,
        SqlServerValueGenerationStrategy? valueGenerationStrategy,
        bool fromDataAnnotation = false)
    {
        if (modelBuilder.CanSetValueGenerationStrategy(valueGenerationStrategy, fromDataAnnotation))
        {
            modelBuilder.Metadata.SetValueGenerationStrategy(valueGenerationStrategy, fromDataAnnotation);
            if (valueGenerationStrategy != SqlServerValueGenerationStrategy.IdentityColumn)
            {
                modelBuilder.HasIdentityColumnSeed(null, fromDataAnnotation);
                modelBuilder.HasIdentityColumnIncrement(null, fromDataAnnotation);
                modelBuilder.HasKeySequences(null, null, fromDataAnnotation);
            }
            if (valueGenerationStrategy != SqlServerValueGenerationStrategy.SequenceHiLo)
            {
                modelBuilder.HasHiLoSequence(null, null, fromDataAnnotation);
                modelBuilder.HasKeySequences(null, null, fromDataAnnotation);
            }
            if (valueGenerationStrategy != SqlServerValueGenerationStrategy.Sequence)
            {
                modelBuilder.HasIdentityColumnSeed(null, fromDataAnnotation);
                modelBuilder.HasIdentityColumnIncrement(null, fromDataAnnotation);
                modelBuilder.HasHiLoSequence(null, null, fromDataAnnotation);
            }
            return modelBuilder;
        }
        return null;
    }

Copy link
Member

Choose a reason for hiding this comment

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

No, it should be changed to check the source


modelBuilder.Metadata.SetKeySequenceName(name, fromDataAnnotation);
modelBuilder.Metadata.SetKeySequenceSchema(schema, fromDataAnnotation);
modelBuilder.Metadata.SetSequenceNameSuffix(name, fromDataAnnotation);
modelBuilder.Metadata.SetSequenceSchema(schema, fromDataAnnotation);

return name == null ? null : modelBuilder.HasSequence(name, schema, fromDataAnnotation);
return null;
}

/// <summary>
Expand All @@ -195,21 +190,21 @@ public static ModelBuilder UseKeySequences(
/// for more information and examples.
/// </remarks>
/// <param name="modelBuilder">The model builder.</param>
/// <param name="name">The name of the sequence.</param>
/// <param name="nameSuffix">The name of the sequence.</param>
/// <param name="schema">The schema of the sequence.</param>
/// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param>
/// <returns><see langword="true" /> if the given name and schema can be set for the hi-lo sequence.</returns>
public static bool CanSetKeySequences(
this IConventionModelBuilder modelBuilder,
string? name,
string? nameSuffix,
string? schema,
bool fromDataAnnotation = false)
{
Check.NullButNotEmpty(name, nameof(name));
Check.NullButNotEmpty(nameSuffix, nameof(nameSuffix));
Check.NullButNotEmpty(schema, nameof(schema));

return modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.KeySequenceName, name, fromDataAnnotation)
&& modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.KeySequenceSchema, schema, fromDataAnnotation);
return modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceNameSuffix, nameSuffix, fromDataAnnotation)
&& modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceSchema, schema, fromDataAnnotation);
}

/// <summary>
Expand All @@ -236,8 +231,8 @@ public static ModelBuilder UseIdentityColumns(
model.SetValueGenerationStrategy(SqlServerValueGenerationStrategy.IdentityColumn);
model.SetIdentitySeed(seed);
model.SetIdentityIncrement(increment);
model.SetKeySequenceName(null);
model.SetKeySequenceSchema(null);
model.SetSequenceNameSuffix(null);
model.SetSequenceSchema(null);
model.SetHiLoSequenceName(null);
model.SetHiLoSequenceSchema(null);

Expand Down
48 changes: 24 additions & 24 deletions src/EFCore.SqlServer/Extensions/SqlServerModelExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ public static class SqlServerModelExtensions
public const string DefaultHiLoSequenceName = "EntityFrameworkHiLoSequence";

/// <summary>
/// The default name for the hi-lo sequence.
/// The default prefix for sequences applied to properties.
/// </summary>
public const string DefaultKeySequenceName = "EntityFrameworkKeySequence";
public const string DefaultSequenceNameSuffix = "Sequence";

/// <summary>
/// Returns the name to use for the default hi-lo sequence.
Expand Down Expand Up @@ -124,72 +124,72 @@ public static void SetHiLoSequenceSchema(this IMutableModel model, string? value


/// <summary>
/// Returns the name to use for the default key value generation sequence.
/// Returns the suffix to append to the name of automatically created sequences.
/// </summary>
/// <param name="model">The model.</param>
/// <returns>The name to use for the default key value generation sequence.</returns>
public static string GetKeySequenceName(this IReadOnlyModel model)
=> (string?)model[SqlServerAnnotationNames.KeySequenceName]
?? DefaultKeySequenceName;
public static string GetSequenceNameSuffix(this IReadOnlyModel model)
=> (string?)model[SqlServerAnnotationNames.SequenceNameSuffix]
?? DefaultSequenceNameSuffix;

/// <summary>
/// Sets the name to use for the default key value generation sequence.
/// Sets the suffix to append to the name of automatically created sequences.
/// </summary>
/// <param name="model">The model.</param>
/// <param name="name">The value to set.</param>
public static void SetKeySequenceName(this IMutableModel model, string? name)
public static void SetSequenceNameSuffix(this IMutableModel model, string? name)
{
Check.NullButNotEmpty(name, nameof(name));

model.SetOrRemoveAnnotation(SqlServerAnnotationNames.KeySequenceName, name);
model.SetOrRemoveAnnotation(SqlServerAnnotationNames.SequenceNameSuffix, name);
}

/// <summary>
/// Sets the name to use for the default key value generation sequence.
/// Sets the suffix to append to the name of automatically created sequences.
/// </summary>
/// <param name="model">The model.</param>
/// <param name="name">The value to set.</param>
/// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param>
/// <returns>The configured value.</returns>
public static string? SetKeySequenceName(
public static string? SetSequenceNameSuffix(
this IConventionModel model,
string? name,
bool fromDataAnnotation = false)
{
Check.NullButNotEmpty(name, nameof(name));

model.SetOrRemoveAnnotation(SqlServerAnnotationNames.KeySequenceName, name, fromDataAnnotation);
model.SetOrRemoveAnnotation(SqlServerAnnotationNames.SequenceNameSuffix, name, fromDataAnnotation);

return name;
}

/// <summary>
/// Returns the <see cref="ConfigurationSource" /> for the default key value generation sequence name.
/// Returns the <see cref="ConfigurationSource" /> for the default value generation sequence name suffix.
/// </summary>
/// <param name="model">The model.</param>
/// <returns>The <see cref="ConfigurationSource" /> for the default key value generation sequence name.</returns>
public static ConfigurationSource? GetKeySequenceNameConfigurationSource(this IConventionModel model)
=> model.FindAnnotation(SqlServerAnnotationNames.KeySequenceName)?.GetConfigurationSource();
public static ConfigurationSource? GetSequenceNameSuffixConfigurationSource(this IConventionModel model)
=> model.FindAnnotation(SqlServerAnnotationNames.SequenceNameSuffix)?.GetConfigurationSource();

/// <summary>
/// Returns the schema to use for the default hi-lo sequence.
/// Returns the schema to use for the default value generation sequence.
/// <see cref="SqlServerPropertyBuilderExtensions.UseSequence" />
/// </summary>
/// <param name="model">The model.</param>
/// <returns>The schema to use for the default key value generation sequence.</returns>
public static string? GetKeySequenceSchema(this IReadOnlyModel model)
=> (string?)model[SqlServerAnnotationNames.KeySequenceSchema];
public static string? GetSequenceSchema(this IReadOnlyModel model)
=> (string?)model[SqlServerAnnotationNames.SequenceSchema];

/// <summary>
/// Sets the schema to use for the default key value generation sequence.
/// </summary>
/// <param name="model">The model.</param>
/// <param name="value">The value to set.</param>
public static void SetKeySequenceSchema(this IMutableModel model, string? value)
public static void SetSequenceSchema(this IMutableModel model, string? value)
{
Check.NullButNotEmpty(value, nameof(value));

model.SetOrRemoveAnnotation(SqlServerAnnotationNames.KeySequenceSchema, value);
model.SetOrRemoveAnnotation(SqlServerAnnotationNames.SequenceSchema, value);
}

/// <summary>
Expand All @@ -199,14 +199,14 @@ public static void SetKeySequenceSchema(this IMutableModel model, string? value)
/// <param name="value">The value to set.</param>
/// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param>
/// <returns>The configured value.</returns>
public static string? SetKeySequenceSchema(
public static string? SetSequenceSchema(
this IConventionModel model,
string? value,
bool fromDataAnnotation = false)
{
Check.NullButNotEmpty(value, nameof(value));

model.SetOrRemoveAnnotation(SqlServerAnnotationNames.KeySequenceSchema, value, fromDataAnnotation);
model.SetOrRemoveAnnotation(SqlServerAnnotationNames.SequenceSchema, value, fromDataAnnotation);

return value;
}
Expand All @@ -216,8 +216,8 @@ public static void SetKeySequenceSchema(this IMutableModel model, string? value)
/// </summary>
/// <param name="model">The model.</param>
/// <returns>The <see cref="ConfigurationSource" /> for the default key value generation sequence schema.</returns>
public static ConfigurationSource? GetKeySequenceSchemaConfigurationSource(this IConventionModel model)
=> model.FindAnnotation(SqlServerAnnotationNames.HiLoSequenceSchema)?.GetConfigurationSource();
public static ConfigurationSource? GetSequenceSchemaConfigurationSource(this IConventionModel model)
=> model.FindAnnotation(SqlServerAnnotationNames.SequenceSchema)?.GetConfigurationSource();

/// <summary>
/// Returns the default identity seed.
Expand Down
14 changes: 3 additions & 11 deletions src/EFCore.SqlServer/Extensions/SqlServerPropertyBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal;

// ReSharper disable once CheckNamespace
Expand Down Expand Up @@ -159,15 +160,6 @@ public static PropertyBuilder UseSequence(

var property = propertyBuilder.Metadata;

name ??= SqlServerModelExtensions.DefaultKeySequenceName;

var model = property.DeclaringEntityType.Model;

if (model.FindSequence(name, schema) == null)
{
model.AddSequence(name, schema).IncrementBy = 1;
}

property.SetValueGenerationStrategy(SqlServerValueGenerationStrategy.Sequence);
property.SetSequenceName(name);
property.SetSequenceSchema(schema);
Expand Down Expand Up @@ -254,8 +246,8 @@ public static bool CanSetSequence(
Check.NullButNotEmpty(name, nameof(name));
Check.NullButNotEmpty(schema, nameof(schema));

return propertyBuilder.CanSetAnnotation(SqlServerAnnotationNames.KeySequenceName, name, fromDataAnnotation)
&& propertyBuilder.CanSetAnnotation(SqlServerAnnotationNames.KeySequenceSchema, schema, fromDataAnnotation);
return propertyBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceName, name, fromDataAnnotation)
&& propertyBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceSchema, schema, fromDataAnnotation);
}

/// <summary>
Expand Down
Loading