PropertyEntry.CurrentValue should throw an exception

[sjb-sjb]

When the value assigned to propertyEntry.CurrentValue cannot be assigned into the database field, then the CurrentValue is not updated, but also no exception is thrown. This makes it harder to debug failed assignments. An exception should be thrown by CurrentValue (and, I suppose, OriginalValue) if the value cannot be assigned.

An example of this is a database table created with an entity property of type bool, which in the CLR was then changed to bool?. However no migration or change was made to the table. When the database was reopened, the bool values were assigned into the bool? CLR property. The propertyEntry CLR type was bool?, but propertyEntry.IsNullable was false. Then I tried to assign null to propertyEntry.CurrentValue. It did not do the assignment (value of CurrentValue was unchanged), but it also did not throw an exception.

[ajcvickers]

@sjb-sjb The correct behavior (per current design) for this case would be that setting the CurrentValue should succeed and the value should be set to null. I have verified that this does not happen--the value does not get set to null. This is a bug we will fix.

That being said, CurrentValue should not throw in this case. In general, setting the CurrentValue should succeed any time that the property value is compatible with the CLR type of the property. One example of where this is important is store generated values where the CLR type may be nullable to allow null sentinels that indicate that the value to be used must be obtained from the store. In this case, the CLR type is bool?, null values are allowed in the entity, but the property can be marked as Required (IsNullable false) and no null value will be stored in the database.

[sjb-sjb]

I should mention then that my entity property had the [Required] attribute. I had assumed, though, that this attribute would only be used for validation and would not affect the database table scheme. Does it?

…

On Thu, Mar 16, 2017 at 8:46 PM, Arthur Vickers ***@***.***> wrote: @sjb-sjb <https://github.com/sjb-sjb> The correct behavior (per current design) for this case would be that setting the CurrentValue should succeed and the value should be set to null. I have verified that this does not happen--the value does not get set to null. This is a bug we will fix. That being said, CurrentValue should not throw in this case. In general, setting the CurrentValue should succeed any time that the property value is compatible with the CLR type of the property. One example of where this is important is store generated values where the CLR type may be nullable to allow null sentinels that indicate that the value to be used must be obtained from the store. In this case, the CLR type is bool?, null values are allowed in the entity, but the property can be marked as Required (IsNullable false) and no null value will be stored in the database. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7920 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ASQzRkQF5B9gKGNP0uJN8293LfYgJPDsks5rmdfNgaJpZM4MgCPV> .

[ajcvickers]

@sjb-sjb [Required] will make the column non-nullable in the store.

[sjb-sjb]

So: non-nullable in the database, but nullable CLR type; currently cannot assign null to CurrentValue but this is a bug and one should be able to assign null to it. So if I assign null in the entity and then try SaveChanges … an exception occurs, I guess. Of course that would only happen if I didn’t validate the entity properly (using the [Required] attribute). First assign null to the CLR entity and then validate it to find out the value is not valid. Makes sense. Steve From: Arthur Vickers [mailto:notifications@github.com] Sent: Monday, March 20, 2017 12:23 PM To: aspnet/EntityFramework Cc: sjb; Mention Subject: Re: [aspnet/EntityFramework] PropertyEntry.CurrentValue should throw an exception (#7920) @sjb-sjb <https://github.com/sjb-sjb> [Required] will make the column non-nullable in the store. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7920 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/ASQzRjhHazMoMaT31e3Tus8qS2A_h6UCks5rnqfggaJpZM4MgCPV> . <https://github.com/notifications/beacon/ASQzRkfVCpKAWfc-PrpfCl3qO5vGGPPNks5rnqfggaJpZM4MgCPV.gif>

[sjb-sjb]

Thanks for the fix!

Just a comment on a potential 'gotcha' ... if you have a non-nullable CLR property, say of type bool, and you try to assign null to it using

 propertyInfo.SetValue( this, null)

then it will assign the default value in the CLR property instead of throwing an exception. All perfectly legitimate and it is documented that way in PropertyInfo.SetValue.

Because of this, if you have a Required property of type bool? and you change it to bool, you may not get the same results even though the table property was bool all along.

Not really an EntityFrameworkCore problem at all, it's just one of those things that can creep out and bite you.