Change ViewDataDictionary copy constructor to ensure ModelMetadata is never that for object
#1441
Conversation
| // If we're constructing a derived ViewDataDictionary (or any other value type), | ||
| // SetModel will throw if we try to set it to null. We should not throw in that case. | ||
| // Slightly optimize SetModel and avoid creating unnecessary new ModelMetadata instances. Tests also depend | ||
| // on ModelMetadata being copied along when the model type does not change. |
There was a problem hiding this comment.
@pranavkm tests were one reason I found changing ViewDataDictionary<TModel>.ModelMetadata untenable.
There was a problem hiding this comment.
Is this really an optimization? Why do tests depend on this behavior if it's not guaranteed on the main codepath during execution?
There was a problem hiding this comment.
the tests may be driving this choice 😸 but on the other hand, they're demonstrating an undocumented capability our users may be relying on. that is our users may also be doing things like the following and expecting their DisplayName value to be used in the generated HTML.
helper.ViewData.ModelMetadata.DisplayName = displayName;
...
@Html.EditorFor(m => m)we had an offline conversation about this flow and I'm going to take a crack at updating the various ViewDataDictionary constructors to be more explicit about intent.
|
|
| // SetModel will throw if we try to set it to null. We should not throw in that case. | ||
| // Avoid copying information about the object type. To do so when model==null would confuse the | ||
| // ViewDataDictionary<TModel>.ModelMetadata getter. | ||
| if (typeof(object) != source.ModelMetadata?.ModelType) |
|
|
…` is never that for `object` - `ViewDataDictionary<TModel>.ModelMetadata` was for `object` after base copy constructor got value from `ViewDataDictionary<object>` - problem led to #1426 symptoms - with copy constructor leaving `base.ModelMetadata==null` more often, `ViewDataDictionary<TModel>.ModelMetadata` usually tracks `TModel` if `Model==null` nit: - fix existing comment in main `ViewDataDictionary` copy constructor
- exercises display and editor helpers when `Model==null`
dougbu
force-pushed
the
release.correct.metadata.1426
branch
from
2df96b5 to
0f34c61
Compare
ViewDataDictionary<TModel>.ModelMetadatawas forobjectafter basecopy constructor got value from
ViewDataDictionary<object>ViewDataDictionary<TModel>.ModelMetadatawhenModelisnull#1426 symptomsbase.ModelMetadata==nullmore often,ViewDataDictionary<TModel>.ModelMetadatausually tracksTModelifModel==nullnit:
ViewDataDictionarycopy constructor<remarks/>to the fourViewDataDictionarycopy constructorsplus tests