Skip to content

Commit da08e25

Browse files
rojiroji
authored
Properly detect nullability in nested types (#24713)
Fixes #24686
1 parent 44243f0 commit da08e25

File tree

2 files changed

+72
-69
lines changed

2 files changed

+72
-69
lines changed

‎src/EFCore/Metadata/Conventions/NonNullableConventionBase.cs‎

Lines changed: 40 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -96,46 +96,55 @@ PropertyInfo p
9696

9797
// No attribute on the member, try to find a NullableContextAttribute on the declaring type
9898
var type = memberInfo.DeclaringType;
99-
if (type != null)
99+
if (type is not null)
100100
{
101-
if (state.TypeCache.TryGetValue(type, out var cachedTypeNonNullable))
101+
// We currently don't calculate support nullability for generic properties, since calculating that is complex
102+
// (depends on the nullability of generic type argument).
103+
// However, we special case Dictionary as it's used for property bags, and specifically don't identify its indexer
104+
// as non-nullable.
105+
if (memberInfo is PropertyInfo property
106+
&& property.IsIndexerProperty()
107+
&& type.IsGenericType
108+
&& type.GetGenericTypeDefinition() == typeof(Dictionary<,>))
102109
{
103-
return cachedTypeNonNullable;
110+
return false;
104111
}
105112

106-
if (Attribute.GetCustomAttributes(type)
107-
.FirstOrDefault(a => a.GetType().FullName == NullableContextAttributeFullName) is Attribute contextAttr)
113+
return DoesTypeHaveNonNullableContext(type, state);
114+
}
115+
116+
return false;
117+
}
118+
119+
private bool DoesTypeHaveNonNullableContext(Type type, NonNullabilityConventionState state)
120+
{
121+
if (state.TypeCache.TryGetValue(type, out var cachedTypeNonNullable))
122+
{
123+
return cachedTypeNonNullable;
124+
}
125+
126+
if (Attribute.GetCustomAttributes(type)
127+
.FirstOrDefault(a => a.GetType().FullName == NullableContextAttributeFullName) is Attribute contextAttr)
128+
{
129+
var attributeType = contextAttr.GetType();
130+
131+
if (attributeType != state.NullableContextAttrType)
108132
{
109-
var attributeType = contextAttr.GetType();
110-
111-
if (attributeType != state.NullableContextAttrType)
112-
{
113-
state.NullableContextFlagFieldInfo = attributeType.GetField("Flag");
114-
state.NullableContextAttrType = attributeType;
115-
}
116-
117-
if (state.NullableContextFlagFieldInfo?.GetValue(contextAttr) is byte flag)
118-
{
119-
// We currently don't calculate support nullability for generic properties, since calculating that is complex
120-
// (depends on the nullability of generic type argument).
121-
// However, we special case Dictionary as it's used for property bags, and specifically don't identify its indexer
122-
// as non-nullable.
123-
if (memberInfo is PropertyInfo property
124-
&& property.IsIndexerProperty()
125-
&& type.IsGenericType
126-
&& type.GetGenericTypeDefinition() == typeof(Dictionary<,>))
127-
{
128-
return false;
129-
}
130-
131-
return state.TypeCache[type] = flag == 1;
132-
}
133+
state.NullableContextFlagFieldInfo = attributeType.GetField("Flag");
134+
state.NullableContextAttrType = attributeType;
133135
}
134136

135-
return state.TypeCache[type] = false;
137+
if (state.NullableContextFlagFieldInfo?.GetValue(contextAttr) is byte flag)
138+
{
139+
return state.TypeCache[type] = flag == 1;
140+
}
141+
}
142+
else if (type.IsNested)
143+
{
144+
return state.TypeCache[type] = DoesTypeHaveNonNullableContext(type.DeclaringType!, state);
136145
}
137146

138-
return false;
147+
return state.TypeCache[type] = false;
139148
}
140149

141150
private NonNullabilityConventionState GetOrInitializeState(IConventionModelBuilder modelBuilder)

‎test/EFCore.Specification.Tests/ValueConvertersEndToEndTestBase.cs‎

Lines changed: 32 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -631,9 +631,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
631631
b.Property(e => e.NullableIntAsNullableLong).HasConversion(new CastingConverter<int?, long?>());
632632

633633
b.Property(e => e.BytesAsString).HasConversion(
634-
(ValueConverter?)new BytesToStringConverter(), new ArrayStructuralComparer<byte>()).IsRequired(); // Issue #24686
634+
(ValueConverter?)new BytesToStringConverter(), new ArrayStructuralComparer<byte>());
635635
b.Property(e => e.BytesAsNullableString).HasConversion(
636-
(ValueConverter?)new BytesToStringConverter(), new ArrayStructuralComparer<byte>()).IsRequired(); // Issue #24686
636+
(ValueConverter?)new BytesToStringConverter(), new ArrayStructuralComparer<byte>());
637637
b.Property(e => e.NullableBytesAsString).HasConversion(
638638
new BytesToStringConverter(), new ArrayStructuralComparer<byte>());
639639
b.Property(e => e.NullableBytesAsNullableString).HasConversion(
@@ -684,29 +684,25 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
684684
b.Property(e => e.NullableGuidToBytes).HasConversion(new GuidToBytesConverter());
685685
b.Property(e => e.NullableGuidToNullableBytes).HasConversion(new GuidToBytesConverter());
686686

687-
b.Property(e => e.IPAddressToString).HasConversion((ValueConverter?)new IPAddressToStringConverter()).IsRequired(); // Issue #24686
688-
b.Property(e => e.IPAddressToNullableString).HasConversion((ValueConverter?)new IPAddressToStringConverter())
689-
.IsRequired(); // Issue #24686
687+
b.Property(e => e.IPAddressToString).HasConversion((ValueConverter?)new IPAddressToStringConverter());
688+
b.Property(e => e.IPAddressToNullableString).HasConversion((ValueConverter?)new IPAddressToStringConverter());
690689
b.Property(e => e.NullableIPAddressToString).HasConversion(new IPAddressToStringConverter());
691690
b.Property(e => e.NullableIPAddressToNullableString).HasConversion(new IPAddressToStringConverter());
692691

693-
b.Property(e => e.IPAddressToBytes).HasConversion((ValueConverter?)new IPAddressToBytesConverter()).IsRequired(); // Issue #24686
694-
b.Property(e => e.IPAddressToNullableBytes).HasConversion((ValueConverter?)new IPAddressToBytesConverter())
695-
.IsRequired(); // Issue #24686
692+
b.Property(e => e.IPAddressToBytes).HasConversion((ValueConverter?)new IPAddressToBytesConverter());
693+
b.Property(e => e.IPAddressToNullableBytes).HasConversion((ValueConverter?)new IPAddressToBytesConverter());
696694
b.Property(e => e.NullableIPAddressToBytes).HasConversion(new IPAddressToBytesConverter());
697695
b.Property(e => e.NullableIPAddressToNullableBytes).HasConversion(new IPAddressToBytesConverter());
698696

699-
b.Property(e => e.PhysicalAddressToString).HasConversion((ValueConverter?)new PhysicalAddressToStringConverter())
700-
.IsRequired(); // Issue #24686
697+
b.Property(e => e.PhysicalAddressToString).HasConversion((ValueConverter?)new PhysicalAddressToStringConverter());
701698
b.Property(e => e.PhysicalAddressToNullableString)
702-
.HasConversion((ValueConverter?)new PhysicalAddressToStringConverter()).IsRequired(); // Issue #24686
699+
.HasConversion((ValueConverter?)new PhysicalAddressToStringConverter());
703700
b.Property(e => e.NullablePhysicalAddressToString).HasConversion(new PhysicalAddressToStringConverter());
704701
b.Property(e => e.NullablePhysicalAddressToNullableString).HasConversion(new PhysicalAddressToStringConverter());
705702

706-
b.Property(e => e.PhysicalAddressToBytes).HasConversion((ValueConverter?)new PhysicalAddressToBytesConverter())
707-
.IsRequired(); // Issue #24686
703+
b.Property(e => e.PhysicalAddressToBytes).HasConversion((ValueConverter?)new PhysicalAddressToBytesConverter());
708704
b.Property(e => e.PhysicalAddressToNullableBytes)
709-
.HasConversion((ValueConverter?)new PhysicalAddressToBytesConverter()).IsRequired(); // Issue #24686
705+
.HasConversion((ValueConverter?)new PhysicalAddressToBytesConverter());
710706
b.Property(e => e.NullablePhysicalAddressToBytes).HasConversion(new PhysicalAddressToBytesConverter());
711707
b.Property(e => e.NullablePhysicalAddressToNullableBytes).HasConversion(new PhysicalAddressToBytesConverter());
712708

@@ -720,54 +716,52 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
720716
b.Property(e => e.NullableNumberToBytes).HasConversion(new NumberToBytesConverter<sbyte>());
721717
b.Property(e => e.NullableNumberToNullableBytes).HasConversion(new NumberToBytesConverter<sbyte>());
722718

723-
b.Property(e => e.StringToBool).HasConversion(new StringToBoolConverter()).IsRequired(); // Issue #24686
724-
b.Property(e => e.StringToNullableBool).HasConversion(new StringToBoolConverter()).IsRequired(); // Issue #24686
719+
b.Property(e => e.StringToBool).HasConversion(new StringToBoolConverter());
720+
b.Property(e => e.StringToNullableBool).HasConversion(new StringToBoolConverter());
725721
b.Property(e => e.NullableStringToBool).HasConversion((ValueConverter?)new StringToBoolConverter());
726722
b.Property(e => e.NullableStringToNullableBool).HasConversion((ValueConverter?)new StringToBoolConverter());
727723

728-
b.Property(e => e.StringToBytes).HasConversion((ValueConverter?)new StringToBytesConverter(Encoding.UTF32))
729-
.IsRequired(); // Issue #24686
730-
b.Property(e => e.StringToNullableBytes).HasConversion((ValueConverter?)new StringToBytesConverter(Encoding.UTF32))
731-
.IsRequired(); // Issue #24686
724+
b.Property(e => e.StringToBytes).HasConversion((ValueConverter?)new StringToBytesConverter(Encoding.UTF32));
725+
b.Property(e => e.StringToNullableBytes).HasConversion((ValueConverter?)new StringToBytesConverter(Encoding.UTF32));
732726
b.Property(e => e.NullableStringToBytes).HasConversion(new StringToBytesConverter(Encoding.UTF32));
733727
b.Property(e => e.NullableStringToNullableBytes).HasConversion(new StringToBytesConverter(Encoding.UTF32));
734728

735-
b.Property(e => e.StringToChar).HasConversion(new StringToCharConverter()).IsRequired(); // Issue #24686
736-
b.Property(e => e.StringToNullableChar).HasConversion(new StringToCharConverter()).IsRequired(); // Issue #24686
729+
b.Property(e => e.StringToChar).HasConversion(new StringToCharConverter());
730+
b.Property(e => e.StringToNullableChar).HasConversion(new StringToCharConverter());
737731
b.Property(e => e.NullableStringToChar).HasConversion((ValueConverter?)new StringToCharConverter());
738732
b.Property(e => e.NullableStringToNullableChar).HasConversion((ValueConverter?)new StringToCharConverter());
739733

740-
b.Property(e => e.StringToDateTime).HasConversion(new StringToDateTimeConverter()).IsRequired(); // Issue #24686
741-
b.Property(e => e.StringToNullableDateTime).HasConversion(new StringToDateTimeConverter()).IsRequired(); // Issue #24686
734+
b.Property(e => e.StringToDateTime).HasConversion(new StringToDateTimeConverter());
735+
b.Property(e => e.StringToNullableDateTime).HasConversion(new StringToDateTimeConverter());
742736
b.Property(e => e.NullableStringToDateTime).HasConversion((ValueConverter?)new StringToDateTimeConverter());
743737
b.Property(e => e.NullableStringToNullableDateTime).HasConversion((ValueConverter?)new StringToDateTimeConverter());
744738

745-
b.Property(e => e.StringToDateTimeOffset).HasConversion(new StringToDateTimeOffsetConverter()).IsRequired(); // Issue #24686
746-
b.Property(e => e.StringToNullableDateTimeOffset).HasConversion(new StringToDateTimeOffsetConverter()).IsRequired(); // Issue #24686
739+
b.Property(e => e.StringToDateTimeOffset).HasConversion(new StringToDateTimeOffsetConverter());
740+
b.Property(e => e.StringToNullableDateTimeOffset).HasConversion(new StringToDateTimeOffsetConverter());
747741
b.Property(e => e.NullableStringToDateTimeOffset)
748742
.HasConversion((ValueConverter?)new StringToDateTimeOffsetConverter());
749743
b.Property(e => e.NullableStringToNullableDateTimeOffset)
750744
.HasConversion((ValueConverter?)new StringToDateTimeOffsetConverter());
751745

752-
b.Property(e => e.StringToEnum).HasConversion(new StringToEnumConverter<TheExperience>()).IsRequired(); // Issue #24686
753-
b.Property(e => e.StringToNullableEnum).HasConversion(new StringToEnumConverter<TheExperience>()).IsRequired(); // Issue #24686
746+
b.Property(e => e.StringToEnum).HasConversion(new StringToEnumConverter<TheExperience>());
747+
b.Property(e => e.StringToNullableEnum).HasConversion(new StringToEnumConverter<TheExperience>());
754748
b.Property(e => e.NullableStringToEnum).HasConversion((ValueConverter?)new StringToEnumConverter<TheExperience>());
755749
b.Property(e => e.NullableStringToNullableEnum)
756750
.HasConversion((ValueConverter?)new StringToEnumConverter<TheExperience>());
757751

758-
b.Property(e => e.StringToGuid).HasConversion(new StringToGuidConverter()).IsRequired(); // Issue #24686
759-
b.Property(e => e.StringToNullableGuid).HasConversion(new StringToGuidConverter()).IsRequired(); // Issue #24686
752+
b.Property(e => e.StringToGuid).HasConversion(new StringToGuidConverter());
753+
b.Property(e => e.StringToNullableGuid).HasConversion(new StringToGuidConverter());
760754
b.Property(e => e.NullableStringToGuid).HasConversion((ValueConverter?)new StringToGuidConverter());
761755
b.Property(e => e.NullableStringToNullableGuid).HasConversion((ValueConverter?)new StringToGuidConverter());
762756

763-
b.Property(e => e.StringToNumber).HasConversion(new StringToNumberConverter<byte>()).IsRequired(); // Issue #24686
764-
b.Property(e => e.StringToNullableNumber).HasConversion(new StringToNumberConverter<byte>()).IsRequired(); // Issue #24686
757+
b.Property(e => e.StringToNumber).HasConversion(new StringToNumberConverter<byte>());
758+
b.Property(e => e.StringToNullableNumber).HasConversion(new StringToNumberConverter<byte>());
765759
b.Property(e => e.NullableStringToNumber).HasConversion((ValueConverter?)new StringToNumberConverter<byte>());
766760
b.Property(e => e.NullableStringToNullableNumber)
767761
.HasConversion((ValueConverter?)new StringToNumberConverter<byte>());
768762

769-
b.Property(e => e.StringToTimeSpan).HasConversion(new StringToTimeSpanConverter()).IsRequired(); // Issue #24686
770-
b.Property(e => e.StringToNullableTimeSpan).HasConversion(new StringToTimeSpanConverter()).IsRequired(); // Issue #24686
763+
b.Property(e => e.StringToTimeSpan).HasConversion(new StringToTimeSpanConverter());
764+
b.Property(e => e.StringToNullableTimeSpan).HasConversion(new StringToTimeSpanConverter());
771765
b.Property(e => e.NullableStringToTimeSpan).HasConversion((ValueConverter?)new StringToTimeSpanConverter());
772766
b.Property(e => e.NullableStringToNullableTimeSpan).HasConversion((ValueConverter?)new StringToTimeSpanConverter());
773767

@@ -781,13 +775,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
781775
b.Property(e => e.NullableTimeSpanToString).HasConversion(new TimeSpanToStringConverter());
782776
b.Property(e => e.NullableTimeSpanToNullableString).HasConversion(new TimeSpanToStringConverter());
783777

784-
b.Property(e => e.UriToString).HasConversion((ValueConverter?)new UriToStringConverter()).IsRequired(); // Issue #24686
785-
b.Property(e => e.UriToNullableString).HasConversion((ValueConverter?)new UriToStringConverter()).IsRequired(); // Issue #24686
778+
b.Property(e => e.UriToString).HasConversion((ValueConverter?)new UriToStringConverter());
779+
b.Property(e => e.UriToNullableString).HasConversion((ValueConverter?)new UriToStringConverter());
786780
b.Property(e => e.NullableUriToString).HasConversion(new UriToStringConverter());
787781
b.Property(e => e.NullableUriToNullableString).HasConversion(new UriToStringConverter());
788782

789-
b.Property(e => e.NonNullIntToNullString).HasConversion(new NonNullIntToNullStringConverter()).IsRequired(); // Issue #24686
790-
b.Property(e => e.NonNullIntToNonNullString).HasConversion(new NonNullIntToNonNullStringConverter()).IsRequired(); // Issue #24686
783+
b.Property(e => e.NonNullIntToNullString).HasConversion(new NonNullIntToNullStringConverter());
784+
b.Property(e => e.NonNullIntToNonNullString).HasConversion(new NonNullIntToNonNullStringConverter());
791785
b.Property(e => e.NullIntToNullString).HasConversion(new NullIntToNullStringConverter()).IsRequired(false);
792786
b.Property(e => e.NullIntToNonNullString).HasConversion(new NullIntToNonNullStringConverter()).IsRequired(false);
793787

0 commit comments

Comments
 (0)