Make the DatabaseGeneratedOption annotation configure ValueGenerationStrategy for SqlServer

src/Microsoft.EntityFrameworkCore.Relational/Metadata/Conventions/Internal/RelationalConventionSetBuilder.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using JetBrains.Annotations;
@@ -90,7 +91,7 @@ protected virtual void ReplaceConvention<T1, T2>([NotNull] IList<T1> conventions
             var oldConvention = conventionsList.OfType<T2>().FirstOrDefault();
             if (oldConvention == null)
             {
-                return;
+                throw new InvalidOperationException();
             }
             var index = conventionsList.IndexOf(oldConvention);
             conventionsList.RemoveAt(index);

src/Microsoft.EntityFrameworkCore.Specification.Tests/DataAnnotationTestBase.cs

@@ -707,7 +707,7 @@ protected class KeyOnNavProp
         }
 
         [Fact]
-        public virtual void Key_property_is_not_used_for_FK_when_set_by_annotation()
+        public virtual ModelBuilder Key_property_is_not_used_for_FK_when_set_by_annotation()
         {
             var modelBuilder = CreateModelBuilder();
 
@@ -718,6 +718,8 @@ public virtual void Key_property_is_not_used_for_FK_when_set_by_annotation()
             Assert.False(toy.Metadata.GetForeignKeys().Any(fk => fk.IsUnique == false && fk.Properties.Any(p => p.Name == nameof(Toy.IdRow))));
 
             Validate(modelBuilder);
+
+            return modelBuilder;
         }
 
         public class Parent
@@ -750,6 +752,43 @@ public class Toy
             public string Name { get; set; }
         }
 
+        [Fact]
+        public virtual ModelBuilder DatabaseGeneratedOption_configures_the_property_correctly()
+        {
+            var modelBuilder = CreateModelBuilder();
+
+            modelBuilder.Entity<GeneratedEntity>();
+
+            var entity = modelBuilder.Model.FindEntityType(typeof(GeneratedEntity));
+
+            var id = entity.FindProperty(nameof(GeneratedEntity.Id));
+            Assert.Equal(ValueGenerated.Never, id.ValueGenerated);
+            Assert.False(id.RequiresValueGenerator);
+
+            var identity = entity.FindProperty(nameof(GeneratedEntity.Identity));
+            Assert.Equal(ValueGenerated.OnAdd, identity.ValueGenerated);
+
+            var version = entity.FindProperty(nameof(GeneratedEntity.Version));
+            Assert.Equal(ValueGenerated.OnAddOrUpdate, version.ValueGenerated);
+            Assert.False(version.RequiresValueGenerator);
+
+            Validate(modelBuilder);
+
+            return modelBuilder;
+        }
+
+        public class GeneratedEntity
+        {
+            [DatabaseGenerated(DatabaseGeneratedOption.None)]
+            public int Id { get; set; }
+
+            [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
+            public int Identity { get; set; }
+
+            [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
+            public Guid Version { get; set; }
+        }
+
         [Fact]
         public virtual ModelBuilder Timestamp_takes_precedence_over_MaxLength()
         {

src/Microsoft.EntityFrameworkCore.SqlServer/Extensions/SqlServerPropertyBuilderExtensions.cs

@@ -29,7 +29,7 @@ public static PropertyBuilder ForSqlServerHasColumnName(
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
             Check.NullButNotEmpty(name, nameof(name));
 
-            propertyBuilder.Metadata.SqlServer().ColumnName = name;
+            GetSqlServerInternalBuilder(propertyBuilder).ColumnName(name);
 
             return propertyBuilder;
         }
@@ -60,7 +60,7 @@ public static PropertyBuilder ForSqlServerHasColumnType(
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
             Check.NullButNotEmpty(typeName, nameof(typeName));
 
-            propertyBuilder.Metadata.SqlServer().ColumnType = typeName;
+            GetSqlServerInternalBuilder(propertyBuilder).ColumnType(typeName);
 
             return propertyBuilder;
         }
@@ -91,8 +91,7 @@ public static PropertyBuilder ForSqlServerHasDefaultValueSql(
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
             Check.NullButNotEmpty(sql, nameof(sql));
 
-            var internalPropertyBuilder = propertyBuilder.GetInfrastructure<InternalPropertyBuilder>();
-            internalPropertyBuilder.SqlServer(ConfigurationSource.Explicit).DefaultValueSql(sql);
+            GetSqlServerInternalBuilder(propertyBuilder).DefaultValueSql(sql);
 
             return propertyBuilder;
         }
@@ -121,8 +120,7 @@ public static PropertyBuilder ForSqlServerHasDefaultValue(
         {
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
 
-            var internalPropertyBuilder = propertyBuilder.GetInfrastructure<InternalPropertyBuilder>();
-            internalPropertyBuilder.SqlServer(ConfigurationSource.Explicit).DefaultValue(value);
+            GetSqlServerInternalBuilder(propertyBuilder).DefaultValue(value);
 
             return propertyBuilder;
         }
@@ -152,8 +150,7 @@ public static PropertyBuilder ForSqlServerHasComputedColumnSql(
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
             Check.NullButNotEmpty(sql, nameof(sql));
 
-            var internalPropertyBuilder = propertyBuilder.GetInfrastructure<InternalPropertyBuilder>();
-            internalPropertyBuilder.SqlServer(ConfigurationSource.Explicit).ComputedColumnSql(sql);
+            GetSqlServerInternalBuilder(propertyBuilder).ComputedColumnSql(sql);
 
             return propertyBuilder;
         }
@@ -198,9 +195,8 @@ public static PropertyBuilder ForSqlServerUseSequenceHiLo(
                 model.SqlServer().GetOrAddSequence(name, schema).IncrementBy = 10;
             }
 
-            property.SqlServer().ValueGenerationStrategy = SqlServerValueGenerationStrategy.SequenceHiLo;
-            property.ValueGenerated = ValueGenerated.OnAdd;
-            property.RequiresValueGenerator = true;
+            GetSqlServerInternalBuilder(propertyBuilder).ValueGenerationStrategy(SqlServerValueGenerationStrategy.SequenceHiLo);
+
             property.SqlServer().HiLoSequenceName = name;
             property.SqlServer().HiLoSequenceSchema = schema;
 
@@ -233,13 +229,7 @@ public static PropertyBuilder UseSqlServerIdentityColumn(
         {
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
 
-            var property = propertyBuilder.Metadata;
-
-            property.SqlServer().ValueGenerationStrategy = SqlServerValueGenerationStrategy.IdentityColumn;
-            property.ValueGenerated = ValueGenerated.OnAdd;
-            property.RequiresValueGenerator = true;
-            property.SqlServer().HiLoSequenceName = null;
-            property.SqlServer().HiLoSequenceSchema = null;
+            GetSqlServerInternalBuilder(propertyBuilder).ValueGenerationStrategy(SqlServerValueGenerationStrategy.IdentityColumn);
 
             return propertyBuilder;
         }
@@ -254,5 +244,8 @@ public static PropertyBuilder UseSqlServerIdentityColumn(
         public static PropertyBuilder<TProperty> UseSqlServerIdentityColumn<TProperty>(
             [NotNull] this PropertyBuilder<TProperty> propertyBuilder)
             => (PropertyBuilder<TProperty>)UseSqlServerIdentityColumn((PropertyBuilder)propertyBuilder);
+
+        private static SqlServerPropertyBuilderAnnotations GetSqlServerInternalBuilder(PropertyBuilder propertyBuilder)
+            => propertyBuilder.GetInfrastructure<InternalPropertyBuilder>().SqlServer(ConfigurationSource.Explicit);
     }
 }

src/Microsoft.EntityFrameworkCore.SqlServer/Metadata/Conventions/Internal/SqlServerValueGenerationStrategyConvention.cs

@@ -1,16 +1,29 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System.Reflection;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
+using System.ComponentModel.DataAnnotations.Schema;
 
 namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal
 {
     /// <summary>
     ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
     ///     directly from your code. This API may change or be removed in future releases.
     /// </summary>
-    public class SqlServerValueGenerationStrategyConvention : IModelConvention
+    public class SqlServerValueGenerationStrategyConvention : DatabaseGeneratedAttributeConvention, IModelConvention
     {
+        public override InternalPropertyBuilder Apply(
+            InternalPropertyBuilder propertyBuilder, DatabaseGeneratedAttribute attribute, MemberInfo clrMember)
+        {
+            propertyBuilder.SqlServer(ConfigurationSource.DataAnnotation).ValueGenerationStrategy(
+                attribute.DatabaseGeneratedOption == DatabaseGeneratedOption.Identity
+                    ? SqlServerValueGenerationStrategy.IdentityColumn
+                    : (SqlServerValueGenerationStrategy?)null);
+
+            return base.Apply(propertyBuilder, attribute, clrMember);
+        }
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.

src/Microsoft.EntityFrameworkCore.SqlServer/Metadata/Conventions/SqlServerConventionSetBuilder.cs

@@ -26,7 +26,10 @@ public override ConventionSet AddConventions(ConventionSet conventionSet)
 
             base.AddConventions(conventionSet);
 
-            conventionSet.ModelInitializedConventions.Add(new SqlServerValueGenerationStrategyConvention());
+            var valueGenerationStrategyConvention = new SqlServerValueGenerationStrategyConvention();
+            conventionSet.ModelInitializedConventions.Add(valueGenerationStrategyConvention);
+
+            ReplaceConvention(conventionSet.PropertyAddedConventions, (DatabaseGeneratedAttributeConvention)valueGenerationStrategyConvention);
 
             var sqlServerInMemoryTablesConvention = new SqlServerInMemoryTablesConvention();
             conventionSet.EntityTypeAnnotationSetConventions.Add(sqlServerInMemoryTablesConvention);
@@ -35,6 +38,8 @@ public override ConventionSet AddConventions(ConventionSet conventionSet)
 
             conventionSet.IndexAddedConventions.Add(sqlServerInMemoryTablesConvention);
 
+            ReplaceConvention(conventionSet.PropertyFieldChangedConventions, (DatabaseGeneratedAttributeConvention)valueGenerationStrategyConvention);
+
             return conventionSet;
         }
 

src/Microsoft.EntityFrameworkCore.SqlServer/Metadata/Internal/SqlServerPropertyBuilderAnnotations.cs

@@ -89,7 +89,30 @@ public SqlServerPropertyBuilderAnnotations(
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
-        public new virtual bool ValueGenerationStrategy(SqlServerValueGenerationStrategy? value) => SetValueGenerationStrategy(value);
+        public new virtual bool ValueGenerationStrategy(SqlServerValueGenerationStrategy? value)
+        {
+            if (value == null)
+            {
+                PropertyBuilder.ValueGenerated(ValueGenerated.Never, ConfigurationSource.Convention);
+                PropertyBuilder.RequiresValueGenerator(false, ConfigurationSource.Convention);
+                HiLoSequenceName(null);
+                HiLoSequenceSchema(null);
+            }
+            else if (value.Value == SqlServerValueGenerationStrategy.IdentityColumn)
+            {
+                PropertyBuilder.ValueGenerated(ValueGenerated.OnAdd, ConfigurationSource.Convention);
+                PropertyBuilder.RequiresValueGenerator(true, ConfigurationSource.Convention);
+                HiLoSequenceName(null);
+                HiLoSequenceSchema(null);
+            }
+            else if (value.Value == SqlServerValueGenerationStrategy.SequenceHiLo)
+            {
+                PropertyBuilder.ValueGenerated(ValueGenerated.OnAdd, ConfigurationSource.Convention);
+                PropertyBuilder.RequiresValueGenerator(true, ConfigurationSource.Convention);
+            }
+
+            return SetValueGenerationStrategy(value);
+        }
 #pragma warning restore 109
     }
 }

src/Microsoft.EntityFrameworkCore.SqlServer/Microsoft.EntityFrameworkCore.SqlServer.csproj

@@ -33,6 +33,7 @@
     <WarningLevel>4</WarningLevel>
   </PropertyGroup>
   <ItemGroup>
+    <Reference Include="System.ComponentModel.DataAnnotations" />
     <Reference Include="System.Data" />
   </ItemGroup>
   <ItemGroup>

test/Microsoft.EntityFrameworkCore.InMemory.FunctionalTests/DataAnnotationInMemoryTest.cs

@@ -13,6 +13,16 @@ public DataAnnotationInMemoryTest(DataAnnotationInMemoryFixture fixture)
         {
         }
 
+        public override ModelBuilder DatabaseGeneratedOption_configures_the_property_correctly()
+        {
+            var modelBuilder = base.DatabaseGeneratedOption_configures_the_property_correctly();
+
+            var identity = modelBuilder.Model.FindEntityType(typeof(GeneratedEntity)).FindProperty(nameof(GeneratedEntity.Identity));
+            Assert.False(identity.RequiresValueGenerator);
+
+            return modelBuilder;
+        }
+
         public override void ConcurrencyCheckAttribute_throws_if_value_in_database_changed()
         {
             using (var context = CreateContext())

test/Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests/DataAnnotationSqlServerTest.cs

@@ -3,6 +3,7 @@
 
 using System;
 using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Specification.Tests;
 using Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests.Utilities;
 using Microsoft.EntityFrameworkCore.Storage;
@@ -21,7 +22,6 @@ public DataAnnotationSqlServerTest(DataAnnotationSqlServerFixture fixture)
         protected override void UseTransaction(DatabaseFacade facade, IDbContextTransaction transaction)
             => facade.UseTransaction(transaction.GetDbTransaction());
 
-        [Fact]
         public override ModelBuilder Non_public_annotations_are_enabled()
         {
             var modelBuilder = base.Non_public_annotations_are_enabled();
@@ -33,7 +33,6 @@ public override ModelBuilder Non_public_annotations_are_enabled()
             return modelBuilder;
         }
 
-        [Fact]
         public override ModelBuilder Field_annotations_are_enabled()
         {
             var modelBuilder = base.Field_annotations_are_enabled();
@@ -45,7 +44,6 @@ public override ModelBuilder Field_annotations_are_enabled()
             return modelBuilder;
         }
 
-        [Fact]
         public override ModelBuilder Key_and_column_work_together()
         {
             var modelBuilder = base.Key_and_column_work_together();
@@ -57,7 +55,6 @@ public override ModelBuilder Key_and_column_work_together()
             return modelBuilder;
         }
 
-        [Fact]
         public override ModelBuilder Key_and_MaxLength_64_produce_nvarchar_64()
         {
             var modelBuilder = base.Key_and_MaxLength_64_produce_nvarchar_64();
@@ -68,7 +65,6 @@ public override ModelBuilder Key_and_MaxLength_64_produce_nvarchar_64()
             return modelBuilder;
         }
 
-        [Fact]
         public override ModelBuilder Timestamp_takes_precedence_over_MaxLength()
         {
             var modelBuilder = base.Timestamp_takes_precedence_over_MaxLength();
@@ -79,7 +75,6 @@ public override ModelBuilder Timestamp_takes_precedence_over_MaxLength()
             return modelBuilder;
         }
 
-        [Fact]
         public override ModelBuilder Timestamp_takes_precedence_over_MaxLength_with_value()
         {
             var modelBuilder = base.Timestamp_takes_precedence_over_MaxLength_with_value();
@@ -90,7 +85,6 @@ public override ModelBuilder Timestamp_takes_precedence_over_MaxLength_with_valu
             return modelBuilder;
         }
 
-        [Fact]
         public override ModelBuilder TableNameAttribute_affects_table_name_in_TPH()
         {
             var modelBuilder = base.TableNameAttribute_affects_table_name_in_TPH();
@@ -101,6 +95,17 @@ public override ModelBuilder TableNameAttribute_affects_table_name_in_TPH()
             return modelBuilder;
         }
 
+        public override ModelBuilder DatabaseGeneratedOption_configures_the_property_correctly()
+        {
+            var modelBuilder = base.DatabaseGeneratedOption_configures_the_property_correctly();
+
+            var identity = modelBuilder.Model.FindEntityType(typeof(GeneratedEntity)).FindProperty(nameof(GeneratedEntity.Identity));
+            Assert.True(identity.RequiresValueGenerator);
+            Assert.Equal(SqlServerValueGenerationStrategy.IdentityColumn, identity.SqlServer().ValueGenerationStrategy);
+
+            return modelBuilder;
+        }
+
         public override void ConcurrencyCheckAttribute_throws_if_value_in_database_changed()
         {
             base.ConcurrencyCheckAttribute_throws_if_value_in_database_changed();

test/Microsoft.EntityFrameworkCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs

@@ -372,7 +372,7 @@ public void Setting_SqlServer_identity_column_is_higher_priority_than_relational
             var property = model.FindEntityType(typeof(Customer)).FindProperty(nameof(Customer.Id));
 
             Assert.Equal(SqlServerValueGenerationStrategy.IdentityColumn, property.SqlServer().ValueGenerationStrategy);
-            Assert.Equal(ValueGenerated.OnAdd, property.ValueGenerated);
+            Assert.Equal(ValueGenerated.OnAddOrUpdate, property.ValueGenerated);
             Assert.Null(property.Relational().DefaultValue);
             Assert.Null(property.SqlServer().DefaultValue);
             Assert.Null(property.Relational().DefaultValueSql);