Adding support for owned types mapped to same table as well as different tables #26706
Conversation
| var foreignKey = navigation.ForeignKey; | ||
| if (navigation.IsCollection) | ||
| { | ||
| var innerShapedQuery = CreateShapedQueryExpression( | ||
| targetEntityType, _sqlExpressionFactory.Select(targetEntityType)); | ||
| var innerSelectExpression = BuildInnerSelectExpressionForOwnedTypeMappedToDifferentTable( |
There was a problem hiding this comment.
@smitpatel some refactoring here compared to 6.0 version, which didn't contain the logic for collection (which is also mapped to different table, the logic is pretty much the same) so I DRY'd it a bit
| } | ||
|
|
||
| // TODO: should we be match case here or not? | ||
| if (!relationalSharedTypeEntityExpansionHelper.TableMatchesMetadata(unwrappedTable, table)) |
There was a problem hiding this comment.
the code that this is supposed to improve matches case in some cases and not the others - what's the right thing here? Is metadata case sensitive in general? @AndriySvyryd @smitpatel
There was a problem hiding this comment.
Yes, we should always match case for strings that we control
| { | ||
| if (!mappedType.IsTemporal()) | ||
| { | ||
| throw new InvalidOperationException("either none or all types mapped to same table must be temporal"); |
| var requiredPeriodStartColumnName = default(string); | ||
| var requiredPeriodEndColumnName = default(string); | ||
|
|
||
| foreach (var mappedType in mappedTypes.Where(t => t.BaseType == null)) |
There was a problem hiding this comment.
we only look at based types - we already check elsewhere that derived types don't specify temporal information on them
|
|
||
| return new TemporalPeriodPropertyBuilder(_entityType, propertyName); | ||
| } | ||
|
|
||
| private void EnsurePeriodPropertyExists(string propertyName) |
There was a problem hiding this comment.
TODO: this is probably wrong @AndriySvyryd
There was a problem hiding this comment.
We should capture EntityTypeBuilder in TableBuilder and flow it here to use .Property() instead
| { | ||
| ab.ToTable(tb => tb.IsTemporal(ttb => | ||
| { | ||
| ttb.HasPeriodStart("PeriodStart").HasColumnName("PeriodStart"); |
There was a problem hiding this comment.
this sucks - every owned entity must have the same (redundant) temporal table mapping - we should be smart enough to propagate this information from the owner, but currently its way too much work (as we would need to do it when the new owned type is added as well as when the owner changes) - #15898 is supposed to make it easier
test/EFCore.SqlServer.FunctionalTests/Query/TemporalOwnedQuerySqlServerTest.cs
Show resolved
Hide resolved
| /// <summary> | ||
| /// Returns true if the given table expression matches table metadata, false otherwise. | ||
| /// </summary> | ||
| public bool TableMatchesMetadata( |
There was a problem hiding this comment.
The moment we needed to add this, we faulted our own design.
There was a problem hiding this comment.
yes, ideally there would be an abstract class for table-like sources (like "tableExpressionBase" and what currently is teb should be named something else?) from which TableExpression and temporalTable expression would inherit. Alternatively TemporalTableExpression should inherit from TableExpression rather than TableExpressionBase, but thats a smell also, since TemporalTableExpression is abstract and TableExpression is not :(
maumar
force-pushed
the
fix26469_take3
branch
from
f919bec
to
1e6c4c2
Compare
…ent tables Fix to #26451 - Temporal Table: Owned Entities support Fix to #26469 - Query: enable temporal tables for table splitting scenarios Fix to #26705 - Query: model building for temporal table with explicitly defined period columns could fail if conventions are executed in a delayed fashion Just like any other nav expansion, this only works for AsOf operation. Added extensibility point to SharedTypeEntityExpandingExpressionVisitor so that we can create temporal table expressions representing owned entities, if their owner is also a temporal table expression. We also need to match TableExpression information with ITable metadata, for provider specific table-like expressions. Relaxed validation for table splitting when the table is temporal - it's now allowed as long as all (base type) entities mapped to this table have congruent period and history table mappings. Column period property is now created explicitly in the temporal table builder rather than relying on conventions to avoid scenario when conventions are delayed and the period property they are supposed to create is not ready by the time we need it.
maumar
force-pushed
the
fix26469_take3
branch
from
1e6c4c2
to
36dcd84
Compare
| @@ -1171,7 +1186,12 @@ protected override Expression VisitExtension(Expression extensionExpression) | |||
| && navigation.DeclaringEntityType.IsStrictlyDerivedFrom(entityShaperExpression.EntityType)); | |||
|
|
|||
| var entityProjection = _selectExpression.GenerateWeakEntityProjectionExpression( | |||
| targetEntityType, table, identifyingColumn.Name, identifyingColumn.Table, principalNullable); | |||
| targetEntityType, | |||
There was a problem hiding this comment.
There are no weak entity types anymore
| @@ -325,6 +325,9 @@ | |||
| <data name="TemporalSetOperationOnMismatchedSources" xml:space="preserve"> | |||
| <value>Set operation can't be applied on entity '{entityType}' because temporal operations on both arguments don't match.</value> | |||
| </data> | |||
| <data name="TemporalOwnedTypeMappedToDifferentTableOnlySupportedForAsOf" xml:space="preserve"> | |||
| <value>Only '{operationName}' temporal operation is supported for entitiy that owns another entity which is mapped different table.</value> | |||
There was a problem hiding this comment.
| <value>Only '{operationName}' temporal operation is supported for entitiy that owns another entity which is mapped different table.</value> | |
| <value>Only '{operationName}' temporal operation is supported for entity types that own an entity mapped to a different table.</value> |
| } | ||
| //[EntityFrameworkInternal] |
|
this has been done in 1ca6380 instead |
Fix to #26451 - Temporal Table: Owned Entities support
Fix to #26469 - Query: enable temporal tables for table splitting scenarios
Fix to #26705 - Query: model building for temporal table with explicitly defined period columns could fail if conventions are executed in a delayed fashion
Just like any other nav expansion, this only works for AsOf operation.
Added extensibility point to SharedTypeEntityExpandingExpressionVisitor so that we can create temporal table expressions representing owned entities, if their owner is also a temporal table expression.
We also need to match TableExpression information with ITable metadata, for provider specific table-like expressions.
Relaxed validation for table splitting when the table is temporal - it's now allowed as long as all (base type) entities mapped to this table have congruent period and history table mappings.
Column period property is now created explicitly in the temporal table builder rather than relying on conventions to avoid scenario when conventions are delayed and the period property they are supposed to create is not ready by the time we need it.
Fixes #26451
Fixes #26469
Fixes #26705