Add complex type support to the update pipeline #31409
Conversation
AndriySvyryd
force-pushed
the
Issue13947_part6
branch
3 times, most recently
from
a72dea7
to
94285b4
Compare
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public bool IsModified(IProperty property) |
There was a problem hiding this comment.
I haven't looked through, but there seems to be at least some duplicated code between InternalEntityEntry and InternalComplexEntry. We could have InternalEntry as a base class instead of an interface and move the common code there.
If most of the logic is the same, we could even just have a single InternalEntry that contains an ITypeBase, and just checks entity vs. complex in the specific paths where the logic diverges. For example, we may not support navigations on complex types today, but I think we want to do that in the future; at that point I'm not sure what the differences between the entry types are.
(am going through all these considerations in the query pipeline at the moment...)
There was a problem hiding this comment.
Currently they are very similar indeed, but as I mentioned in the other comment I don't think this is going to be the case long term, so it's not really worth the effort right now.
| Expression.Property(parameter!, "Entity"), | ||
| Expression.Property(parameter!, parameter!.Type == typeof(InternalEntityEntry) | ||
| ? nameof(InternalEntityEntry.Entity) | ||
| : nameof(IInternalEntry.Object)), |
There was a problem hiding this comment.
Any specific reason to not always use IInternalEntry.Object? If there is one, does it not also apply for using InternalComplexEntry.ComplexObject?
There was a problem hiding this comment.
Mostly because I don't think IInternalEntry.Object is a good solution in the long term as it won't work for value types.
src/EFCore.Relational/Metadata/Conventions/SharedTableConvention.cs
Outdated
Show resolved
Hide resolved
| column.IsNullable = property.IsColumnNullable(mappedQuery); | ||
| column = new SqlQueryColumn(columnName, property.GetColumnType(mappedQuery), sqlQuery) | ||
| { | ||
| IsNullable = property.IsColumnNullable(mappedQuery) |
There was a problem hiding this comment.
Maybe accept nullable in the ctor for SqlQueryColumn (it isn't instantiated anywhere else)
There was a problem hiding this comment.
It's instantiated in the compiled relational model.
| @@ -111,7 +111,8 @@ public virtual IReadOnlyList<IColumnModification> ColumnModifications | |||
| [EntityFrameworkInternal] | |||
| public virtual void AssertColumnsNotInitialized() | |||
| { | |||
| if (_columnModifications != null) | |||
| if (_columnModifications != null | |||
| && !Debugger.IsAttached) | |||
There was a problem hiding this comment.
Is this because looking at _columnModifications in the debugger populates it and causes an exception later? If so, yeah - I also found it annoying, but maybe we should simply have a more explicit step for calculating the modifications rather than doing it automatically upon read?
There was a problem hiding this comment.
Yes, but an explicit step decreases the usability in code. I prefer to sacrifice the debugging experience.
| //Assert.Equal(scooter.SeatingCapacity, scooterEntry.ComplexProperty(v => v.Engine).TargetEntry.Property<int>("SeatingCapacity").CurrentValue); | ||
| } | ||
|
|
||
| //using (var context = CreateContext()) |
There was a problem hiding this comment.
Depends on query implementation
AndriySvyryd
force-pushed
the
Issue13947_part5
branch
from
2f95488
to
f765332
Compare
AndriySvyryd
force-pushed
the
Issue13947_part6
branch
from
94285b4
to
921d8b6
Compare
The state management part is prototype quality, expected to be replaced by @ajcvickers.
Part of #13947