[implementation] Simplify how `ModelMetadata` gets defaults and is extended; eliminate `CachedModelMetadata<TPrototypeCache>`

[dougbu]

CachedModelMetadata<TPrototypeCache> is unnecessary and leads to problems setting default values for overridden ModelMetadata properties. We have strange things like subclasses calling base.Property from a CalculateProperty() method. This first came up when @yishaigalatzer got a good look at the "pattern" 😺

Current pattern for ModelMetadata properties is:

• Property is virtual in ModelMetadata and its constructor sets all default values
• Property is sealed in CachedModelMetadata<TPrototypeCache>
• CachedModelMetadata<TPrototypeCache> makes a protected virtual {type} Compute{name}() method available to subclasses
• Where useful, CachedDataAnnotationsModelMetadata overrides Compute{name}() to provide a value based on applied attributes

Should instead remove CachedModelMetadata<TPrototypeCache> and instead follow the following pattern:

• Property is not virtual in ModelMetadata
• ModelMetadata makes a protected virtual {type} Compute{name}() method available to subclasses
• Base implementations of these methods return appropriate default values
• Where useful, CachedDataAnnotationsModelMetadata overrides Compute{name}() to provide a value based on applied attributes

Details...

Overall this would push property lazy-initialization down into the base ModelMetadata class since the Compute{name}() are called lazily and just once. The current CachedModelMetadata<TPrototypeCache>.PrototypeCache property would move up into CachedDataAnnotationsModelMetadata, the only place that property is read.

Comments carried over from MVC 5.2 indicate CachedModelMetadata<TPrototypeCache> exists to allow "flexible caching strategies". But the comment is likely more about supporting alternate metadata sources, a separation maintained in the new pattern proposed above.

If we need an abstract layer in here somewhere, could create IModelMetadata. But that would impact lots more code than this issue. This issue shouldn't impact much beyond the 3 mentioned classes.

[dougbu]

Issue #1021 is about one inconsistency in the current pattern. There are a couple of other (odd) inconsistencies (see #933, #963, and #964) but they'll be cleaned up as we finish work to use attributes in setting ModelMetadata properties. In any case those bugs are about the public surface while this bug (#1034) is about the internal implementation of that surface.

[haneytron]

I feel that I could take care of it, under the guidance of the MVC team, if you're willing to consider external contributions. Let me know!

[dougbu]

The Contributing page in the Home repository contains a high-level description of the process. Start by submitting a Contributor License Agreement.
Feel free to follow up here with any questions on your approach to fixing this particular issue. No need for an offline conversation.

[haneytron]

CLA signed and working on it now.

[dougbu]

BTW may be fine for lazy-init to land in either ModelMetadata or CachedDataAnnotationsModelMetadata. Then we wouldn't need Compute*() methods. One important thing is to keep the default values together and avoid oddities mentioned in the summary.

[danroth27]

@Ironyx Are you still interested in working on this issue. Community contributions are more than welcome!