Skip to content

Commit b7ffe45

Browse files
ajcvickersajcvickers
committed
Handle value generation when value conversion is being used
Fixes #11010 Approach is: * Throw by default if a converter is associated with a property that will use value generation * However, a value generator to use can be specified: * On the property * On the type mapping * On a converter * A client-side generator is added to the Guid converters so that Guid key scenarios still work
1 parent 730b63b commit b7ffe45

File tree

12 files changed

+242
-52
lines changed

12 files changed

+242
-52
lines changed

‎samples/OracleProvider/src/OracleProvider/Metadata/OraclePropertyAnnotations.cs

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,13 @@ public virtual OracleValueGenerationStrategy? ValueGenerationStrategy
8080
var modelStrategy = Property.DeclaringEntityType.Model.Oracle().ValueGenerationStrategy;
8181

8282
if (modelStrategy == OracleValueGenerationStrategy.SequenceHiLo
83-
&& IsCompatibleSequenceHiLo(Property.ClrType))
83+
&& IsCompatibleSequenceHiLo(Property))
8484
{
8585
return OracleValueGenerationStrategy.SequenceHiLo;
8686
}
8787

8888
if (modelStrategy == OracleValueGenerationStrategy.IdentityColumn
89-
&& IsCompatibleIdentityColumn(Property.ClrType))
89+
&& IsCompatibleIdentityColumn(Property))
9090
{
9191
return OracleValueGenerationStrategy.IdentityColumn;
9292
}
@@ -101,7 +101,7 @@ protected virtual bool SetValueGenerationStrategy(OracleValueGenerationStrategy?
101101
var propertyType = Property.ClrType;
102102

103103
if (value == OracleValueGenerationStrategy.IdentityColumn
104-
&& !IsCompatibleIdentityColumn(propertyType))
104+
&& !IsCompatibleIdentityColumn(Property))
105105
{
106106
if (ShouldThrowOnInvalidConfiguration)
107107
{
@@ -114,7 +114,7 @@ protected virtual bool SetValueGenerationStrategy(OracleValueGenerationStrategy?
114114
}
115115

116116
if (value == OracleValueGenerationStrategy.SequenceHiLo
117-
&& !IsCompatibleSequenceHiLo(propertyType))
117+
&& !IsCompatibleSequenceHiLo(Property))
118118
{
119119
if (ShouldThrowOnInvalidConfiguration)
120120
{
@@ -280,9 +280,18 @@ protected override void ClearAllServerGeneratedValues()
280280
base.ClearAllServerGeneratedValues();
281281
}
282282

283-
private static bool IsCompatibleIdentityColumn(Type type)
284-
=> type.IsInteger() || type == typeof(decimal);
283+
private static bool IsCompatibleIdentityColumn(IProperty property)
284+
{
285+
var type = property.ClrType;
286+
287+
return (type.IsInteger() || type == typeof(decimal)) && !HasConverter(property);
288+
}
289+
290+
private static bool IsCompatibleSequenceHiLo(IProperty property)
291+
=> property.ClrType.IsInteger() && !HasConverter(property);
285292

286-
private static bool IsCompatibleSequenceHiLo(Type type) => type.IsInteger();
293+
private static bool HasConverter(IProperty property)
294+
=> (property.FindMapping()?.Converter
295+
?? property.GetValueConverter()) != null;
287296
}
288297
}

‎src/EFCore.Relational/Storage/ValueConversion/RelationalConverterMappingHints.cs

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
5+
using JetBrains.Annotations;
6+
using Microsoft.EntityFrameworkCore.Metadata;
7+
using Microsoft.EntityFrameworkCore.ValueGeneration;
8+
49
namespace Microsoft.EntityFrameworkCore.Storage.ValueConversion
510
{
611
/// <summary>
@@ -17,13 +22,15 @@ public class RelationalConverterMappingHints : ConverterMappingHints
1722
/// <param name="scale"> The suggested scale of the mapped data type. </param>
1823
/// <param name="unicode"> Whether or not the mapped data type should support Unicode. </param>
1924
/// <param name="fixedLength"> Whether or not the mapped data type is fixed length. </param>
25+
/// <param name="valueGeneratorFactory"> An optional factory for creating a specific <see cref="ValueGenerator"/>. </param>
2026
public RelationalConverterMappingHints(
2127
int? size = null,
2228
int? precision = null,
2329
int? scale = null,
2430
bool? unicode = null,
25-
bool? fixedLength = null)
26-
: base(size, precision, scale, unicode)
31+
bool? fixedLength = null,
32+
[CanBeNull] Func<IProperty, IEntityType, ValueGenerator> valueGeneratorFactory = null)
33+
: base(size, precision, scale, unicode, valueGeneratorFactory)
2734
{
2835
IsFixedLength = fixedLength;
2936
}
@@ -42,7 +49,8 @@ public override ConverterMappingHints With(ConverterMappingHints hints)
4249
hints.Precision ?? Precision,
4350
hints.Scale ?? Scale,
4451
hints.IsUnicode ?? IsUnicode,
45-
(hints as RelationalConverterMappingHints)?.IsFixedLength ?? IsFixedLength);
52+
(hints as RelationalConverterMappingHints)?.IsFixedLength ?? IsFixedLength,
53+
hints.ValueGeneratorFactory ?? ValueGeneratorFactory);
4654

4755
/// <summary>
4856
/// Whether or not the mapped data type is fixed length.

‎src/EFCore.Specification.Tests/StoreGeneratedTestBase.cs

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using Microsoft.EntityFrameworkCore.Internal;
99
using Microsoft.EntityFrameworkCore.Metadata;
1010
using Microsoft.EntityFrameworkCore.Storage;
11+
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
1112
using Microsoft.EntityFrameworkCore.TestUtilities;
1213
using Xunit;
1314

@@ -21,6 +22,54 @@ public abstract class StoreGeneratedTestBase<TFixture> : IClassFixture<TFixture>
2122

2223
protected TFixture Fixture { get; }
2324

25+
[Fact]
26+
public virtual void Value_generation_throws_for_common_cases()
27+
{
28+
ValueGenerationNegative<int, IntToString, NumberToStringConverter<int>>();
29+
ValueGenerationNegative<short, ShortToBytes, NumberToBytesConverter<short>>();
30+
}
31+
32+
private void ValueGenerationNegative<TKey, TEntity, TConverter>()
33+
where TEntity : WithConverter<TKey>, new()
34+
{
35+
using (var context = CreateContext())
36+
{
37+
Assert.Equal(
38+
CoreStrings.ValueGenWithConversion(
39+
typeof(TEntity).ShortDisplayName(),
40+
nameof(WithConverter<int>.Id),
41+
typeof(TConverter).ShortDisplayName()),
42+
Assert.Throws<NotSupportedException>(() => context.Add(new TEntity())).Message);
43+
}
44+
}
45+
46+
[Fact]
47+
public virtual void Value_generation_works_for_common_GUID_conversions()
48+
{
49+
ValueGenerationPositive<Guid, GuidToString>();
50+
ValueGenerationPositive<Guid, GuidToBytes>();
51+
}
52+
53+
private void ValueGenerationPositive<TKey, TEntity>()
54+
where TEntity : WithConverter<TKey>, new()
55+
{
56+
TKey id;
57+
58+
using (var context = CreateContext())
59+
{
60+
var entity = context.Add(new TEntity()).Entity;
61+
62+
context.SaveChanges();
63+
64+
id = entity.Id;
65+
}
66+
67+
using (var context = CreateContext())
68+
{
69+
Assert.Equal(id, context.Set<TEntity>().Single(e => e.Id.Equals(id)).Id);
70+
}
71+
}
72+
2473
[Theory]
2574
[InlineData(nameof(Anais.NeverThrowBeforeUseAfter))]
2675
[InlineData(nameof(Anais.NeverThrowBeforeIgnoreAfter))]
@@ -1169,6 +1218,27 @@ protected class Anais
11691218
public string OnUpdateThrowBeforeThrowAfter { get; set; }
11701219
}
11711220

1221+
protected class WithConverter<TKey>
1222+
{
1223+
public TKey Id { get; set; }
1224+
}
1225+
1226+
protected class IntToString : WithConverter<int>
1227+
{
1228+
}
1229+
1230+
protected class GuidToString: WithConverter<Guid>
1231+
{
1232+
}
1233+
1234+
protected class GuidToBytes: WithConverter<Guid>
1235+
{
1236+
}
1237+
1238+
protected class ShortToBytes: WithConverter<short>
1239+
{
1240+
}
1241+
11721242
protected virtual void ExecuteWithStrategyInTransaction(
11731243
Action<DbContext> testOperation,
11741244
Action<DbContext> nestedTestOperation1 = null,
@@ -1189,6 +1259,11 @@ public abstract class StoreGeneratedFixtureBase : SharedStoreFixtureBase<DbConte
11891259

11901260
protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
11911261
{
1262+
modelBuilder.Entity<IntToString>().Property(e => e.Id).HasConversion<string>();
1263+
modelBuilder.Entity<GuidToString>().Property(e => e.Id).HasConversion<string>();
1264+
modelBuilder.Entity<GuidToBytes>().Property(e => e.Id).HasConversion<byte[]>();
1265+
modelBuilder.Entity<ShortToBytes>().Property(e => e.Id).HasConversion<byte[]>();
1266+
11921267
modelBuilder.Entity<Gumball>(
11931268
b =>
11941269
{

‎src/EFCore.SqlServer/Metadata/SqlServerPropertyAnnotations.cs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using Microsoft.EntityFrameworkCore.Metadata.Internal;
88
using Microsoft.EntityFrameworkCore.SqlServer.Internal;
99
using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal;
10+
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
1011
using Microsoft.EntityFrameworkCore.Utilities;
1112

1213
namespace Microsoft.EntityFrameworkCore.Metadata
@@ -157,13 +158,13 @@ public virtual SqlServerValueGenerationStrategy? ValueGenerationStrategy
157158
var modelStrategy = Property.DeclaringEntityType.Model.SqlServer().ValueGenerationStrategy;
158159

159160
if (modelStrategy == SqlServerValueGenerationStrategy.SequenceHiLo
160-
&& IsCompatibleSequenceHiLo(Property.ClrType))
161+
&& IsCompatibleSequenceHiLo(Property))
161162
{
162163
return SqlServerValueGenerationStrategy.SequenceHiLo;
163164
}
164165

165166
if (modelStrategy == SqlServerValueGenerationStrategy.IdentityColumn
166-
&& IsCompatibleIdentityColumn(Property.ClrType))
167+
&& IsCompatibleIdentityColumn(Property))
167168
{
168169
return SqlServerValueGenerationStrategy.IdentityColumn;
169170
}
@@ -183,7 +184,7 @@ protected virtual bool SetValueGenerationStrategy(SqlServerValueGenerationStrate
183184
var propertyType = Property.ClrType;
184185

185186
if (value == SqlServerValueGenerationStrategy.IdentityColumn
186-
&& !IsCompatibleIdentityColumn(propertyType))
187+
&& !IsCompatibleIdentityColumn(Property))
187188
{
188189
if (ShouldThrowOnInvalidConfiguration)
189190
{
@@ -196,7 +197,7 @@ protected virtual bool SetValueGenerationStrategy(SqlServerValueGenerationStrate
196197
}
197198

198199
if (value == SqlServerValueGenerationStrategy.SequenceHiLo
199-
&& !IsCompatibleSequenceHiLo(propertyType))
200+
&& !IsCompatibleSequenceHiLo(Property))
200201
{
201202
if (ShouldThrowOnInvalidConfiguration)
202203
{
@@ -249,11 +250,13 @@ protected virtual bool CanSetValueGenerationStrategy(SqlServerValueGenerationStr
249250
throw new InvalidOperationException(
250251
RelationalStrings.ConflictingColumnServerGeneration(nameof(ValueGenerationStrategy), Property.Name, nameof(DefaultValue)));
251252
}
253+
252254
if (GetDefaultValueSql(false) != null)
253255
{
254256
throw new InvalidOperationException(
255257
RelationalStrings.ConflictingColumnServerGeneration(nameof(ValueGenerationStrategy), Property.Name, nameof(DefaultValueSql)));
256258
}
259+
257260
if (GetComputedColumnSql(false) != null)
258261
{
259262
throw new InvalidOperationException(
@@ -413,9 +416,18 @@ protected override void ClearAllServerGeneratedValues()
413416
base.ClearAllServerGeneratedValues();
414417
}
415418

416-
private static bool IsCompatibleIdentityColumn(Type type)
417-
=> type.IsInteger() || type == typeof(decimal);
419+
private static bool IsCompatibleIdentityColumn(IProperty property)
420+
{
421+
var type = property.ClrType;
422+
423+
return (type.IsInteger() || type == typeof(decimal)) && !HasConverter(property);
424+
}
425+
426+
private static bool IsCompatibleSequenceHiLo(IProperty property)
427+
=> property.ClrType.IsInteger() && !HasConverter(property);
418428

419-
private static bool IsCompatibleSequenceHiLo(Type type) => type.IsInteger();
429+
private static bool HasConverter(IProperty property)
430+
=> (property.FindMapping()?.Converter
431+
?? property.GetValueConverter()) != null;
420432
}
421433
}

‎src/EFCore.Sqlite.Core/Migrations/Internal/SqliteMigrationsAnnotationProvider.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using JetBrains.Annotations;
77
using Microsoft.EntityFrameworkCore.Infrastructure;
88
using Microsoft.EntityFrameworkCore.Metadata;
9+
using Microsoft.EntityFrameworkCore.Metadata.Internal;
910
using Microsoft.EntityFrameworkCore.Migrations;
1011
using Microsoft.EntityFrameworkCore.Sqlite.Metadata.Internal;
1112

@@ -33,10 +34,15 @@ public SqliteMigrationsAnnotationProvider([NotNull] MigrationsAnnotationProvider
3334
public override IEnumerable<IAnnotation> For(IProperty property)
3435
{
3536
if (property.ValueGenerated == ValueGenerated.OnAdd
36-
&& property.ClrType.UnwrapNullableType().IsInteger())
37+
&& property.ClrType.UnwrapNullableType().IsInteger()
38+
&& !HasConverter(property))
3739
{
3840
yield return new Annotation(SqliteAnnotationNames.Autoincrement, true);
3941
}
4042
}
43+
44+
private static bool HasConverter(IProperty property)
45+
=> (property.FindMapping()?.Converter
46+
?? property.GetValueConverter()) != null;
4147
}
4248
}

‎src/EFCore/Properties/CoreStrings.Designer.cs

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)