Fix/363 dynamic values

[Simply007]

Motivation

Fixes #363

Focus on

• Think about record classes fot IContentElementValue elements
• is withCodename extension OK to use?
• Do we want to have base calss for univeral responses' linked items based on IEnumerable vs IList?
• Do we want to base IUniversalItemModelProvider on async method, or sync?
• Is retrieval key for caching client correctly set?
• Interesting to find out CacheHelpers.MAX_DEPENDENCY_ITEMS that prevents to cache more than 50 items => are we OK with that?

Checklist

• Code follows coding conventions held in this repo
• Automated tests have been added
• Tests are passing
• Docs have been updated (if applicable)
• Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

[codecov-commenter]

Codecov Report

Merging #367 (aacabd2) into master (49fde38) will increase coverage by 0.02%.
The diff coverage is 92.05%.

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   93.21%   93.23%   +0.02%     
==========================================
  Files         128      133       +5     
  Lines        2756     2898     +142     
  Branches      349      361      +12     
==========================================
+ Hits         2569     2702     +133     
- Misses        179      188       +9     
  Partials        8        8              

Impacted Files	Coverage Δ
...ivery/ContentItems/Elements/ContentElementValue.cs	100.00% <ø> (ø)
...very/ContentItems/Elements/RichTextElementValue.cs	100.00% <ø> (ø)
Kontent.Ai.Delivery/ContentItems/ModelProvider.cs	96.09% <44.44%> (-1.02%)	⬇️
Kontent.Ai.Delivery/DeliveryClient.cs	89.57% <88.67%> (-0.23%)	⬇️
...ntentItems/Universal/UniversalItemModelProvider.cs	96.77% <96.77%> (ø)
Kontent.Ai.Delivery.Caching/CacheHelpers.cs	96.32% <100.00%> (+0.16%)	⬆️
Kontent.Ai.Delivery.Caching/DeliveryClientCache.cs	90.10% <100.00%> (+2.10%)	⬆️
...iveryClient/DeliveryClientBuilderImplementation.cs	100.00% <100.00%> (ø)
.../Universal/DeliveryUniversalItemListingResponse.cs	100.00% <100.00%> (ø)
...ntItems/Universal/DeliveryUniversalItemResponse.cs	100.00% <100.00%> (ø)
... and 3 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Simply007]

I have tried 3 approached

1. Use IProperyValueConverter to detect a wrapper property (ContentItemElements (basically Dictionsry<string, IContentElementValue> which does not really fit to the architecture since the method ModelProvider.GetContentItemModelAsync -> GetPropertyValueAsync -> GetElementData does not fit to any shape (do Name and Value) property=> master...aeab7fa -> so stat the user just use existion method GetItemAsync, or GetItemsAsync => No Value convertor, or property mapper work for this property
2. Extract the logic for "UniversalContentItem" to 2 more method for Delivery Client - seems more clean, it devides the way of working with "Universal" Items and Strongly typed Items => master...892644f <= This one seems the most suitable
3. Tried to extract both methods from trial 2, but this would require major rewrite of the Delivery Client since it should provide URLBuilder, DeliveryOptions, Serializer (Currenlty from Newtonsoft.JSON) => master...6f11e78

[dzmitryk-kontent-ai]

Answering the questions in the task description.

1. Using of records for IContentElementValue interface implementations. As these implementations are just immutable data bags, it's possible to use record here. It's even possible to use different Json attributes for record's properties - https://stackoverflow.com/questions/63778276/how-do-i-target-attributes-for-a-record-class and https://stackoverflow.com/questions/64915034/jsonproperty-on-c-sharp-records-in-constructor. Nevertheless, record is just a syntax sugar. In fact, compilator creates regular classes from records. So, personally I don't see some advantages of using records instead of classes here. Probably, our back-end architect has more strong opinion about it :)
2. WithCodename extension method. In my opinion, it's closely related to previous question. Currently, classes which implement IContentElementValue are not immutable and can be changed internally. It allows you to change codename property after Element was deserialized. Moreover, you can change any Element's property whenever you want. And it doesn't seem to be clean enough.
   On the other hand, if you use Records instead, you can use with expression. E.g. with Record the code would look like
   "rich_text" => element.ToObject<RichTextElementValue>(Serializer) with { Codename = key }
   instead of current
   "rich_text" => element.ToObject<RichTextElementValue>(Serializer).WithCodename(key)
   It will create new instance of Record with required Codename property. And it still be immutable. As result, you will not need WithCodename extension method at all.
   Probably, it can be valid reason for using Records instead of Classes
3. IList vs IEnumerable for univeral responses' linked items. I'm not sure that I understood this concern correctly. As I can see, LinkedItems property in response is public Dictionary<string, IUniversalContentItem> LinkedItems { get; }. I can see IList for public IList<IUniversalContentItem> Items property.
   Nevertheless, in general IList of IEnumerable usage depends on what we expect from users to use it. If we expect them to modify this collection, e.g. add or remove items in the collection, we should use IList. If we expect them just to enumerate the collection, IEnumerable is preferable type
4. Sync vs. async UniversalItemModelProvider. Honestly, I don't have a strong opinion about it. On one hand, probably, it makes sense to have this method consistent with what we have in regular ModelProvider. On the other hand, GetContentItemModelAsync in UniversalItemModelProvider doens't call any async method and simply can by sync. So, I don't see any advantage of having it async
5. Retrieval key for caching client. It's consistent with GetItemAsync and GetItemsAsync method respectively. I guess, it's correct.
6. CacheHelpers.MAX_DEPENDENCY_ITEMS. It took me some time to understand this code. And even now I'm not sure that I understand it 100%. Nevertheless, we don't cache dependencies ourselves. Instead, when we Get some item, we AddExpirationToken for item itself and for each item's dependency. So, we don't cache dependency, but CancellationTokenSource object for each item using dependency_item{codename} cache key. It allows to expire item's cache when one of it's dependencies' cache is expired.
   If some item has too many dependencies, i.e. more than 50 dependencies, we don't AddExpirationToken for each it's dependency to valueCacheOptions, but add one generic ExpirationToken, which is cached under dependency_item_listing cache key.
   I guess, it means, that all items with more than 50 dependencies will be expired if one of some such items' cache be expired.
   At the same time, for some reason, we don't want to expire a cache of item with more than 50 dependencies when one of it's dependencies is expired. Probably, it would cause too complex expiration chains. And, probably, some performance issues. Not sure, why exactly this threshold was chosen, but I don't have any strong reason to change it.
   You can find more information here - https://jakeydocs.readthedocs.io/en/latest/performance/caching/memory.html#cache-dependencies-and-callbacks

[Simply007]

Beta version with the functionality: https://www.nuget.org/packages/Kontent.Ai.Delivery/17.7.0-beta.0

[Simply007]

I am about to close the PR for now - see the explanation in the linked issue #363