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.

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

Json Columns query work #28171

Merged
merged 1 commit into from
Aug 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1758,10 +1758,10 @@ public static void SetJsonPropertyName(this IMutableEntityType entityType, strin
Check.NullButNotEmpty(name, nameof(name)));

/// <summary>
/// Gets the <see cref="ConfigurationSource" /> for the JSON property name for a given entity Type.
/// Gets the <see cref="ConfigurationSource" /> for the JSON property name for a given entity type.
/// </summary>
/// <param name="entityType">The entity type.</param>
/// <returns>The <see cref="ConfigurationSource" /> for the JSON property name for a given navigation.</returns>
/// <returns>The <see cref="ConfigurationSource" /> for the JSON property name for a given entity type.</returns>
public static ConfigurationSource? GetJsonPropertyNameConfigurationSource(this IConventionEntityType entityType)
=> entityType.FindAnnotation(RelationalAnnotationNames.JsonPropertyName)?.GetConfigurationSource();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public static bool AreCompatible(
return null;
}

if (key.DeclaringEntityType.IsMappedToJson())
{
return null;
}

string? name;
if (key.IsPrimaryKey())
{
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Metadata/Internal/RelationalModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ private static void CreateTableMapping(
var jsonColumnTypeMapping = relationalTypeMappingSource.FindMapping(typeof(JsonElement))!;
var jsonColumn = new JsonColumn(containerColumnName, jsonColumnTypeMapping.StoreType, table, jsonColumnTypeMapping.ProviderValueComparer);
table.Columns.Add(containerColumnName, jsonColumn);
jsonColumn.IsNullable = !ownership.IsRequired || !ownership.IsUnique;
jsonColumn.IsNullable = !ownership.IsRequiredDependent || !ownership.IsUnique;

if (ownership.PrincipalEntityType.BaseType != null)
{
Expand Down
30 changes: 30 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,9 @@
<data name="InvalidPropertyInSetProperty" xml:space="preserve">
<value>The following lambda argument to 'SetProperty' does not represent a valid property to be set: '{propertyExpression}'.</value>
</data>
<data name="JsonCantNavigateToParentEntity" xml:space="preserve">
<value>Can't navigate from JSON-mapped entity '{jsonEntity}' to its parent entity '{parentEntity}' using navigation '{navigation}'. Entities mapped to JSON can only navigate to their children.</value>
</data>
<data name="JsonEntityMappedToDifferentViewThanOwner" xml:space="preserve">
<value>Entity '{jsonType}' is mapped to JSON and also to a view '{viewName}', but its owner '{ownerType}' is mapped to a different view '{ownerViewName}'. Every entity mapped to JSON must also map to the same view as its owner.</value>
</data>
Expand Down Expand Up @@ -517,9 +520,18 @@
<data name="JsonEntityWithTableSplittingIsNotSupported" xml:space="preserve">
<value>Table splitting is not supported for entities containing entities mapped to JSON.</value>
</data>
<data name="JsonErrorExtractingJsonProperty" xml:space="preserve">
<value>An error occurred while reading a JSON value for property '{entityType}.{propertyName}'. See the inner exception for more information.</value>
</data>
<data name="JsonNodeMustBeHandledByProviderSpecificVisitor" xml:space="preserve">
<value>This node should be handled by provider-specific sql generator.</value>
</data>
<data name="JsonPropertyNameShouldBeConfiguredOnNestedNavigation" xml:space="preserve">
<value>The JSON property name should only be configured on nested owned navigations.</value>
</data>
<data name="JsonRequiredEntityWithNullJson" xml:space="preserve">
<value>Entity {entity} is required but the JSON element containing it is null.</value>
</data>
<data name="KeylessMappingStrategy" xml:space="preserve">
<value>The mapping strategy '{mappingStrategy}' used for '{entityType}' is not supported for keyless entity types. See https://go.microsoft.com/fwlink/?linkid=2130430 for more information.</value>
</data>
Expand Down
98 changes: 94 additions & 4 deletions src/EFCore.Relational/Query/EntityProjectionExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Microsoft.EntityFrameworkCore.Query;
public class EntityProjectionExpression : Expression
{
private readonly IReadOnlyDictionary<IProperty, ColumnExpression> _propertyExpressionMap;
private readonly Dictionary<INavigation, EntityShaperExpression> _ownedNavigationMap = new();
private readonly Dictionary<INavigation, EntityShaperExpression> _ownedNavigationMap;

/// <summary>
/// Creates a new instance of the <see cref="EntityProjectionExpression" /> class.
Expand All @@ -29,9 +29,23 @@ public EntityProjectionExpression(
IEntityType entityType,
IReadOnlyDictionary<IProperty, ColumnExpression> propertyExpressionMap,
SqlExpression? discriminatorExpression = null)
: this(
entityType,
propertyExpressionMap,
new Dictionary<INavigation, EntityShaperExpression>(),
discriminatorExpression)
{
}

private EntityProjectionExpression(
IEntityType entityType,
IReadOnlyDictionary<IProperty, ColumnExpression> propertyExpressionMap,
Dictionary<INavigation, EntityShaperExpression> ownedNavigationMap,
SqlExpression? discriminatorExpression = null)
{
EntityType = entityType;
_propertyExpressionMap = propertyExpressionMap;
_ownedNavigationMap = ownedNavigationMap;
DiscriminatorExpression = discriminatorExpression;
}

Expand Down Expand Up @@ -69,8 +83,16 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
var discriminatorExpression = (SqlExpression?)visitor.Visit(DiscriminatorExpression);
changed |= discriminatorExpression != DiscriminatorExpression;

var ownedNavigationMap = new Dictionary<INavigation, EntityShaperExpression>();
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be verified. Earlier we didn't do it for owned navigations which are table sharing. Visiting this can cause unintended side-effects. While I agree that we may need this for JSON columns, we should verify the scenarios, why and where are we doing VisitChildren for Entityprojection

Copy link
Contributor Author

@maumar maumar Jul 10, 2022

Choose a reason for hiding this comment

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

we have 180 tests in in ownedtypes that hit this code path. We could only perform update for json entities and leave other ones as they are, just to be safe.

foreach (var (navigation, entityShaperExpression) in _ownedNavigationMap)
{
var newExpression = (EntityShaperExpression)visitor.Visit(entityShaperExpression);
changed |= newExpression != entityShaperExpression;
ownedNavigationMap[navigation] = newExpression;
}

return changed
? new EntityProjectionExpression(EntityType, propertyExpressionMap, discriminatorExpression)
? new EntityProjectionExpression(EntityType, propertyExpressionMap, ownedNavigationMap, discriminatorExpression)
: this;
}

Expand All @@ -92,7 +114,65 @@ public virtual EntityProjectionExpression MakeNullable()
// if discriminator is column then we need to make it nullable
discriminatorExpression = ce.MakeNullable();
}
return new EntityProjectionExpression(EntityType, propertyExpressionMap, discriminatorExpression);

var primaryKeyProperties = GetMappedKeyProperties(EntityType.FindPrimaryKey()!);
var ownedNavigationMap = new Dictionary<INavigation, EntityShaperExpression>();
foreach (var (navigation, shaper) in _ownedNavigationMap)
{
if (shaper.EntityType.IsMappedToJson())
maumar marked this conversation as resolved.
Show resolved Hide resolved
maumar marked this conversation as resolved.
Show resolved Hide resolved
{
// even if shaper is nullable, we need to make sure key property map contains nullable keys,
// if json entity itself is optional, the shaper would be null, but the PK of the owner entity would be non-nullable intially
Debug.Assert(primaryKeyProperties != null, "Json entity type can't be keyless");

var jsonQueryExpression = (JsonQueryExpression)shaper.ValueBufferExpression;
var ownedPrimaryKeyProperties = GetMappedKeyProperties(shaper.EntityType.FindPrimaryKey()!)!;
var nullableKeyPropertyMap = new Dictionary<IProperty, ColumnExpression>();
for (var i = 0; i < primaryKeyProperties.Count; i++)
{
nullableKeyPropertyMap[ownedPrimaryKeyProperties[i]] = propertyExpressionMap[primaryKeyProperties[i]];
}

// reuse key columns from owner (that we just made nullable), so that the references are the same
var newJsonQueryExpression = jsonQueryExpression.MakeNullable(nullableKeyPropertyMap);
var newShaper = shaper.Update(newJsonQueryExpression).MakeNullable();
ownedNavigationMap[navigation] = newShaper;
}
maumar marked this conversation as resolved.
Show resolved Hide resolved
}

return new EntityProjectionExpression(
EntityType,
propertyExpressionMap,
ownedNavigationMap,
discriminatorExpression);

static IReadOnlyList<IProperty>? GetMappedKeyProperties(IKey? key)
{
if (key == null)
{
return null;
}

if (!key.DeclaringEntityType.IsMappedToJson())
{
return key.Properties;
}

// TODO: fix this once we enable json entity being owned by another owned non-json entity (issue #28441)

// for json collections we need to filter out the ordinal key as it's not mapped to any column
// there could be multiple of these in deeply nested structures,
// so we traverse to the outermost owner to see how many mapped keys there are
var currentEntity = key.DeclaringEntityType;
while (currentEntity.IsMappedToJson())
{
currentEntity = currentEntity.FindOwnership()!.PrincipalEntityType;
}

var count = currentEntity.FindPrimaryKey()!.Properties.Count;

return key.Properties.Take(count).ToList();
}
}

/// <summary>
Expand All @@ -119,6 +199,16 @@ public virtual EntityProjectionExpression UpdateEntityType(IEntityType derivedTy
}
}

var ownedNavigationMap = new Dictionary<INavigation, EntityShaperExpression>();
foreach (var (navigation, entityShaperExpression) in _ownedNavigationMap)
maumar marked this conversation as resolved.
Show resolved Hide resolved
{
if (derivedType.IsAssignableFrom(navigation.DeclaringEntityType)
|| navigation.DeclaringEntityType.IsAssignableFrom(derivedType))
{
ownedNavigationMap[navigation] = entityShaperExpression;
}
}

var discriminatorExpression = DiscriminatorExpression;
if (DiscriminatorExpression is CaseExpression caseExpression)
{
Expand All @@ -130,7 +220,7 @@ public virtual EntityProjectionExpression UpdateEntityType(IEntityType derivedTy
discriminatorExpression = caseExpression.Update(operand: null, whenClauses, elseResult: null);
}

return new EntityProjectionExpression(derivedType, propertyExpressionMap, discriminatorExpression);
return new EntityProjectionExpression(derivedType, propertyExpressionMap, ownedNavigationMap, discriminatorExpression);
}

/// <summary>
Expand Down
Loading