Enable nullable backing field pattern for store-generated defaults #15820
Conversation
Fixes #15182 There is quite a fundamental change included here: the CLR type reported for a mapped property is now the backing field type, if it is known, not the property type. This is what allows a three states to be saved for a non-nullable property with a nullable backing field. Perceptive readers will realize that this means that the CLR type of an `IProperty` can now change if the backing field is changed. The most difficult case to handle seems to be properties backed by `object` fields. This likely isn't very common, but we had tests for it because of previous issues reported. One scenario is not working here--creating a shadow property FK by convention for a PK that is backed by an object field. I will file a bug for this. Query changes are just what I needed to do to re-enable some tests to get coverage for this change. In particular, the type inference is not a universal solution, but unblocks important tests.
ajcvickers
requested review from
smitpatel,
AndriySvyryd and
roji
and removed request for
smitpatel and
AndriySvyryd
| // literals in the expression tree. It adds a new converter onto the type mapper | ||
| // that takes the literal and converts it back to the type that the type mapping | ||
| // is expecting. | ||
| // This code is temporary until more complete type inference is completed. |
There was a problem hiding this comment.
Can you point me to the test which is failing so I can understand the type of tree encountered?
If this is dependent on a service then we can move the method to SqlExpressionFactory rather than extension method. Everyone using this method now having to take service looks ugly when they have already one service available.
There was a problem hiding this comment.
It's the type mapping tests--see my comment. Keep in mind that this is just to unblock tests. I fully expect it to change in the future when we have working type inference.
There was a problem hiding this comment.
Let's go ahead with this then.
| @@ -203,7 +204,7 @@ public virtual void Can_perform_query_with_ansi_strings_test() | |||
| } | |||
| } | |||
|
|
|||
| [Fact(Skip = "QueryIssue")] | |||
| [Fact] | |||
There was a problem hiding this comment.
@smitpatel: All these Can_query... tests
| @@ -1148,13 +1222,12 @@ protected class WithBackingFields | |||
| { | |||
| private int _id; | |||
|
|
|||
| #pragma warning disable RCS1085 // Use auto-implemented property. | |||
| // ReSharper disable once ConvertToAutoProperty | |||
There was a problem hiding this comment.
Moving VS pragma to Resharper disable?
There was a problem hiding this comment.
If we want to enable flexible O-C mapping in the future we should encapsulate O-space concerns better.
When a backing field is set for a property its conceptual type doesn't change. Store type mapping should only use the conceptual type.
Consumers that need the O-space type should get the appropriate one depending on the PropertyAccessMode that's currently set.
PR #15820 will be closed since Andriy suggested a potentially better way to add the feature. However, I wanted to get the additional test coverage checked in now, with the three tests that explicitly test the new functionality skipped for now.
|
Closing this since @AndriySvyryd suggested a different approach that is potentially less impactful and cleaner. |
PR #15820 will be closed since Andriy suggested a potentially better way to add the feature. However, I wanted to get the additional test coverage checked in now, with the three tests that explicitly test the new functionality skipped for now.
Fixes #15182
There is quite a fundamental change included here: the CLR type reported for a mapped property is now the backing field type, if it is known, not the property type. This is what allows a three states to be saved for a non-nullable property with a nullable backing field.
Perceptive readers will realize that this means that the CLR type of an
IPropertycan now change if the backing field is changed.The most difficult case to handle seems to be properties backed by
objectfields. This likely isn't very common, but we had tests for it because of previous issues reported. One scenario is not working here--creating a shadow property FK by convention for a PK that is backed by an object field. I will file a bug for this.Query changes are just what I needed to do to re-enable some tests to get coverage for this change. In particular, the type inference is not a universal solution, but unblocks important tests.