Make discriminator read-only by default #21796
Conversation
ajcvickers
force-pushed
the
CantTouchThis0726
branch
from
ab02ef3
to
0071c9e
Compare
| Assert.Null(operation.ColumnTypes); | ||
| AssertMultidimensionalArray( | ||
| operation.Values, | ||
| v => Assert.Equal("Dog", v), |
There was a problem hiding this comment.
This is incorrect. If the discriminator value is not updated the seed entities end up with a null there.
|
@AndriySvyryd I've been thinking about the force/recreate options. First, for the force option, we would have to assume that the property is really not read-only, even though the metadata says it is. We can't tell if a discriminator was explicitly configured as read-only (without putting more information in the model.) This also requires us to build the mechanism to allow forcing a write in the update pipeline. Second, if model data updates don't work correctly when a discriminator is marked read-only, then doesn't that mean that they may not work if any property is marked as read-only? Or are the errors here specific to it being a discriminator? Do you think it's still correct to go the forced update route? Or if we have to fix read-only properties anyway, then should discriminators really be special-cased? |
So far we've assumed that readonly properties would be store generated. Instead we can check and if they aren't then we'll recreate the row.
I don't think it's correct, but it will be a better experience in most cases. However I don't feel strongly. |
|
I've convinced myself that recreating the row is fine as there's still the workaround of configuring the discriminator as writable just while creating the migrations |
|
@AndriySvyryd Would appreciate if you can take this over. Doesn't need to go into preview 8 at this point. |
Fixes #21154