Skip to content

Commit 0c57357

Browse files
rojiroji
committed
Fully implement element nullability for primitive collections
Closes #31416
1 parent c68ea72 commit 0c57357

File tree

15 files changed

+641
-337
lines changed

15 files changed

+641
-337
lines changed

‎src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ when entityQueryRootExpression.GetType() == typeof(EntityQueryRootExpression)
219219
Check.DebugAssert(sqlParameterExpression is not null, "sqlParameterExpression is not null");
220220
return TranslateCollection(
221221
sqlParameterExpression,
222-
elementTypeMapping: null,
222+
property: null,
223223
char.ToLowerInvariant(sqlParameterExpression.Name.First(c => c != '_')).ToString())
224224
?? base.VisitExtension(extensionExpression);
225225

@@ -265,20 +265,24 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
265265
if (translated == QueryCompilationContext.NotTranslatedExpression)
266266
{
267267
// Attempt to translate access into a primitive collection property (i.e. array column)
268-
if (_sqlTranslator.TryTranslatePropertyAccess(methodCallExpression, out var propertyAccessExpression)
269-
&& propertyAccessExpression is SqlExpression
268+
269+
// TODO: We should be detecting primitive collections by looking at GetElementType() of the property and not at its type
270+
// mapping; but #31469 is blocking that for shadow properties.
271+
if (_sqlTranslator.TryTranslatePropertyAccess(methodCallExpression, out var translatedExpression, out var property)
272+
&& property is IProperty regularProperty
273+
&& translatedExpression is SqlExpression
270274
{
271-
TypeMapping.ElementTypeMapping: RelationalTypeMapping elementTypeMapping
272-
} collectionPropertyAccessExpression)
275+
TypeMapping.ElementTypeMapping: RelationalTypeMapping
276+
} sqlExpression)
273277
{
274-
var tableAlias = collectionPropertyAccessExpression switch
278+
var tableAlias = sqlExpression switch
275279
{
276280
ColumnExpression c => c.Name[..1].ToLowerInvariant(),
277281
JsonScalarExpression { Path: [.., { PropertyName: string propertyName }] } => propertyName[..1].ToLowerInvariant(),
278282
_ => "j"
279283
};
280284

281-
if (TranslateCollection(collectionPropertyAccessExpression, elementTypeMapping, tableAlias) is
285+
if (TranslateCollection(sqlExpression, regularProperty, tableAlias) is
282286
{ } primitiveCollectionTranslation)
283287
{
284288
return primitiveCollectionTranslation;
@@ -316,17 +320,15 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
316320
/// collections.
317321
/// </remarks>
318322
/// <param name="sqlExpression">The expression to try to translate as a primitive collection expression.</param>
319-
/// <param name="elementTypeMapping">
320-
/// The type mapping of the collection's element, or <see langword="null" /> when it's not known (i.e. for parameters).
323+
/// <param name="property">
324+
/// If the primitive collection is a property, contains the <see cref="IProperty" /> for that property. Otherwise, the collection
325+
/// represents a parameter, and this contains <see langword="null" />.
321326
/// </param>
322327
/// <param name="tableAlias">
323328
/// Provides an alias to be used for the table returned from translation, which will represent the collection.
324329
/// </param>
325330
/// <returns>A <see cref="ShapedQueryExpression" /> if the translation was successful, otherwise <see langword="null" />.</returns>
326-
protected virtual ShapedQueryExpression? TranslateCollection(
327-
SqlExpression sqlExpression,
328-
RelationalTypeMapping? elementTypeMapping,
329-
string tableAlias)
331+
protected virtual ShapedQueryExpression? TranslateCollection(SqlExpression sqlExpression, IProperty? property, string tableAlias)
330332
=> null;
331333

332334
/// <summary>

‎src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs‎

Lines changed: 135 additions & 118 deletions
Large diffs are not rendered by default.

‎src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
129129
/// </summary>
130130
protected override ShapedQueryExpression? TranslateCollection(
131131
SqlExpression sqlExpression,
132-
RelationalTypeMapping? elementTypeMapping,
132+
IProperty? property,
133133
string tableAlias)
134134
{
135135
if (_sqlServerCompatibilityLevel < 130)
@@ -145,6 +145,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
145145
// (i.e. with a columnInfo), which determines the type conversion to apply to the JSON elements coming out.
146146
// For parameter collections, the element type mapping will only be inferred and applied later (see
147147
// SqlServerInferredTypeMappingApplier below), at which point the we'll apply it to add the WITH clause.
148+
var elementTypeMapping = (RelationalTypeMapping?)sqlExpression.TypeMapping?.ElementTypeMapping;
149+
148150
var openJsonExpression = elementTypeMapping is null
149151
? new SqlServerOpenJsonExpression(tableAlias, sqlExpression)
150152
: new SqlServerOpenJsonExpression(
@@ -159,17 +161,24 @@ protected override Expression VisitExtension(Expression extensionExpression)
159161
}
160162
});
161163

162-
// TODO: This is a temporary CLR type-based check; when we have proper metadata to determine if the element is nullable, use it here
163164
var elementClrType = sqlExpression.Type.GetSequenceType();
164-
var isColumnNullable = elementClrType.IsNullableType();
165+
166+
// If this is a collection property, get the element's nullability out of metadata. Otherwise, this is a parameter property, in
167+
// which case we only have the CLR type (note that we cannot produce different SQLs based on the nullability of an *element* in
168+
// a parameter collection - our caching mechanism only supports varying by the nullability of the parameter itself (i.e. the
169+
// collection).
170+
// TODO: if property is non-null, GetElementType() should never be null, but we have #31469 for shadow properties
171+
var isElementNullable = property?.GetElementType() is null
172+
? elementClrType.IsNullableType()
173+
: property.GetElementType()!.IsNullable;
165174

166175
#pragma warning disable EF1001 // Internal EF Core API usage.
167176
var selectExpression = new SelectExpression(
168177
openJsonExpression,
169178
columnName: "value",
170179
columnType: elementClrType,
171180
columnTypeMapping: elementTypeMapping,
172-
isColumnNullable,
181+
isElementNullable,
173182
identifierColumnName: "key",
174183
identifierColumnType: typeof(string),
175184
identifierColumnTypeMapping: _typeMappingSource.FindMapping("nvarchar(4000)"));

‎src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
203203
/// </summary>
204204
protected override ShapedQueryExpression? TranslateCollection(
205205
SqlExpression sqlExpression,
206-
RelationalTypeMapping? elementTypeMapping,
206+
IProperty? property,
207207
string tableAlias)
208208
{
209209
// Support for JSON functions (e.g. json_each) was added in Sqlite 3.38.0 (2022-02-22, see https://www.sqlite.org/json1.html).
@@ -215,19 +215,26 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
215215
return null;
216216
}
217217

218+
var elementTypeMapping = (RelationalTypeMapping?)sqlExpression.TypeMapping?.ElementTypeMapping;
218219
var elementClrType = sqlExpression.Type.GetSequenceType();
219220
var jsonEachExpression = new JsonEachExpression(tableAlias, sqlExpression);
220221

221-
// TODO: This is a temporary CLR type-based check; when we have proper metadata to determine if the element is nullable, use it here
222-
var isColumnNullable = elementClrType.IsNullableType();
222+
// If this is a collection property, get the element's nullability out of metadata. Otherwise, this is a parameter property, in
223+
// which case we only have the CLR type (note that we cannot produce different SQLs based on the nullability of an *element* in
224+
// a parameter collection - our caching mechanism only supports varying by the nullability of the parameter itself (i.e. the
225+
// collection).
226+
// TODO: if property is non-null, GetElementType() should never be null, but we have #31469 for shadow properties
227+
var isElementNullable = property?.GetElementType() is null
228+
? elementClrType.IsNullableType()
229+
: property.GetElementType()!.IsNullable;
223230

224231
#pragma warning disable EF1001 // Internal EF Core API usage.
225232
var selectExpression = new SelectExpression(
226233
jsonEachExpression,
227234
columnName: "value",
228235
columnType: elementClrType,
229236
columnTypeMapping: elementTypeMapping,
230-
isColumnNullable,
237+
isElementNullable,
231238
identifierColumnName: "key",
232239
identifierColumnType: typeof(int),
233240
identifierColumnTypeMapping: _typeMappingSource.FindMapping(typeof(int)));

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Diagnostics.CodeAnalysis;
5+
46
namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
57

68
/// <summary>
@@ -33,13 +35,19 @@ protected NonNullableConventionBase(ProviderConventionSetBuilderDependencies dep
3335
/// </summary>
3436
/// <param name="modelBuilder">The model builder used to build the model.</param>
3537
/// <param name="memberInfo">The member info.</param>
38+
/// <param name="nullabilityInfo">
39+
/// The nullability info for the <paramref name="memberInfo" />, or <see langword="null" /> if it does not represent a valid reference
40+
/// type.
41+
/// </param>
3642
/// <returns><see langword="true" /> if the member type is a non-nullable reference type.</returns>
37-
protected virtual bool IsNonNullableReferenceType(
43+
protected virtual bool TryGetNullabilityInfo(
3844
IConventionModelBuilder modelBuilder,
39-
MemberInfo memberInfo)
45+
MemberInfo memberInfo,
46+
[NotNullWhen(true)] out NullabilityInfo? nullabilityInfo)
4047
{
4148
if (memberInfo.GetMemberType().IsValueType)
4249
{
50+
nullabilityInfo = null;
4351
return false;
4452
}
4553

@@ -49,14 +57,14 @@ protected virtual bool IsNonNullableReferenceType(
4957

5058
var nullabilityInfoContext = (NullabilityInfoContext)annotation.Value!;
5159

52-
var nullabilityInfo = memberInfo switch
60+
nullabilityInfo = memberInfo switch
5361
{
5462
PropertyInfo propertyInfo => nullabilityInfoContext.Create(propertyInfo),
5563
FieldInfo fieldInfo => nullabilityInfoContext.Create(fieldInfo),
5664
_ => null
5765
};
5866

59-
return nullabilityInfo?.ReadState == NullabilityState.NotNull;
67+
return nullabilityInfo is not null;
6068
}
6169

6270
/// <inheritdoc />

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,5 +77,6 @@ private void ProcessNavigation(IConventionNavigationBuilder navigationBuilder)
7777

7878
private bool IsNonNullable(IConventionModelBuilder modelBuilder, IConventionNavigation navigation)
7979
=> navigation.DeclaringEntityType.GetRuntimeProperties().Find(navigation.Name) is PropertyInfo propertyInfo
80-
&& IsNonNullableReferenceType(modelBuilder, propertyInfo);
80+
&& TryGetNullabilityInfo(modelBuilder, propertyInfo, out var nullabilityInfo)
81+
&& nullabilityInfo.ReadState == NullabilityState.NotNull;
8182
}

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,29 @@ public NonNullableReferencePropertyConvention(ProviderConventionSetBuilderDepend
2929
private void Process(IConventionPropertyBuilder propertyBuilder)
3030
{
3131
if (propertyBuilder.Metadata.GetIdentifyingMemberInfo() is MemberInfo memberInfo
32-
&& IsNonNullableReferenceType(propertyBuilder.ModelBuilder, memberInfo))
32+
&& TryGetNullabilityInfo(propertyBuilder.ModelBuilder, memberInfo, out var nullabilityInfo))
3333
{
34-
propertyBuilder.IsRequired(true);
34+
if (nullabilityInfo.ReadState == NullabilityState.NotNull)
35+
{
36+
propertyBuilder.IsRequired(true);
37+
}
38+
39+
// If there's an element type, this is a primitive collection; check and apply the element's nullability as well.
40+
if (propertyBuilder.Metadata.GetElementType() is IConventionElementType elementType
41+
&& nullabilityInfo is
42+
{ ElementType.ReadState: NullabilityState.NotNull } or
43+
{ GenericTypeArguments: [{ ReadState: NullabilityState.NotNull }] })
44+
{
45+
elementType.SetIsNullable(false);
46+
}
3547
}
3648
}
3749

3850
private void Process(IConventionComplexPropertyBuilder propertyBuilder)
3951
{
4052
if (propertyBuilder.Metadata.GetIdentifyingMemberInfo() is MemberInfo memberInfo
41-
&& IsNonNullableReferenceType(propertyBuilder.ModelBuilder, memberInfo))
53+
&& TryGetNullabilityInfo(propertyBuilder.ModelBuilder, memberInfo, out var nullabilityInfo)
54+
&& nullabilityInfo.ReadState == NullabilityState.NotNull)
4255
{
4356
propertyBuilder.IsRequired(true);
4457
}

‎test/EFCore.Cosmos.FunctionalTests/JsonTypesCosmosTest.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,27 @@
33

44
#nullable enable
55

6+
using Xunit.Sdk;
7+
68
namespace Microsoft.EntityFrameworkCore.Cosmos;
79

810
public class JsonTypesCosmosTest : JsonTypesTestBase
911
{
12+
// #25765 - the Cosmos type mapping source doesn't support primitive collections, so we end up with a Property
13+
// that has no ElementType; that causes the assertion on the element nullability to fail.
14+
public override void Can_read_write_collection_of_string_JSON_values()
15+
=> Assert.Throws<EqualException>(() => base.Can_read_write_collection_of_string_JSON_values());
16+
17+
// #25765 - the Cosmos type mapping source doesn't support primitive collections, so we end up with a Property
18+
// that has no ElementType; that causes the assertion on the element nullability to fail.
19+
public override void Can_read_write_collection_of_binary_JSON_values()
20+
=> Assert.Throws<EqualException>(() => base.Can_read_write_collection_of_binary_JSON_values());
21+
22+
// #25765 - the Cosmos type mapping source doesn't support primitive collections, so we end up with a Property
23+
// that has no ElementType; that causes the assertion on the element nullability to fail.
24+
public override void Can_read_write_collection_of_nullable_string_JSON_values()
25+
=> Assert.Throws<EqualException>(() => base.Can_read_write_collection_of_nullable_string_JSON_values());
26+
1027
public override void Can_read_write_point()
1128
// No built-in JSON support for spatial types in the Cosmos provider
1229
=> Assert.Throws<InvalidOperationException>(() => base.Can_read_write_point());

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3443,9 +3443,29 @@ protected virtual void Can_read_and_write_JSON_value<TEntity, TModel>(
34433443

34443444
Assert.Equal(typeof(TModel).GetSequenceType(), element.ClrType);
34453445
Assert.Same(property, element.CollectionProperty);
3446-
Assert.Equal(json.Contains("null", StringComparison.Ordinal) || !element.ClrType.IsValueType, element.IsNullable);
34473446
Assert.Null(element.FindTypeMapping()!.ElementTypeMapping);
34483447

3448+
bool elementNullable;
3449+
if (element.ClrType.IsValueType)
3450+
{
3451+
elementNullable = element.ClrType.IsNullableType();
3452+
}
3453+
else
3454+
{
3455+
var nullabilityInfo = property switch
3456+
{
3457+
{ PropertyInfo: PropertyInfo p } => _nullabilityInfoContext.Create(p),
3458+
{ FieldInfo: FieldInfo f } => _nullabilityInfoContext.Create(f),
3459+
_ => throw new UnreachableException()
3460+
};
3461+
3462+
elementNullable = nullabilityInfo is not
3463+
{ ElementType.ReadState: NullabilityState.NotNull } and not
3464+
{ GenericTypeArguments: [{ ReadState: NullabilityState.NotNull }] };
3465+
}
3466+
3467+
Assert.Equal(elementNullable, element.IsNullable);
3468+
34493469
var comparer = element.GetValueComparer()!;
34503470
var elementReaderWriter = element.GetJsonValueReaderWriter()!;
34513471
foreach (var item in (IEnumerable)value!)
@@ -3741,4 +3761,6 @@ protected virtual void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
37413761
CoreEventId.MappedEntityTypeIgnoredWarning,
37423762
CoreEventId.MappedPropertyIgnoredWarning,
37433763
CoreEventId.MappedNavigationIgnoredWarning));
3764+
3765+
private readonly NullabilityInfoContext _nullabilityInfoContext = new();
37443766
}

0 commit comments

Comments
 (0)