Modifying a value returned by SiteSettings.As<T>() causes incorrect values to be returned for subsequent calls

[rwawr]

Describe the bug

In Orchard Core 1.8.0, in PR #14372, an As<T>() method was added to SiteSettings to allow caching for more efficient retrieval of the settings properties stored on the SiteSettings document. The way this is implemented is technically a breaking change, as the value returned from .As<T>() is a reference to the object stored in the _cache field on SiteSettings. Therefore, making changes to the returned value in the calling code will also update the value in the cache, which will cause .As<T>() to return a value that doesn't match the settings stored in the SiteSettings document.

To Reproduce

Steps to reproduce the behavior:

1. Using the base Orchard Core 1.8.2 solution, add a reference to OrchardCore.Demo to OrchardCore.Cms.Web, cook the site using the Blog recipe, then add the following code in DemoController.cs under OrchardCore.Demo

private readonly ISiteService _siteService;

public DemoController(ISiteService siteService)
{
    _siteService = siteService;
}

[Route("Demo/GetSettings")]
public async Task<IActionResult> GetSettingsAsync()
{
    var localizationSettings = (await _siteService.GetSiteSettingsAsync()).As<LocalizationSettings>();
    return Content(localizationSettings.DefaultCulture);
}

[Route("Demo/GetAndChangeSettings")]
public async Task<IActionResult> GetAndChangeSettingsAsync()
{
    var localizationSettings = (await _siteService.GetSiteSettingsAsync()).As<LocalizationSettings>();
    localizationSettings.DefaultCulture = "fr-FR";
    return Content("Culture updated to fr-FR");
}

2. Launch the site and navigate to ~/Demo/GetSettings. This will read the LocalizationSettings in a manner that should cause them to be cached in the SiteSettings._cache field, and the page should display the default culture code, "en-US"

3. Navigate to ~/Demo/GetAndChangeSettings. This will read the LocalizationSettings, then update the return value and display a hardcoded message indicating that the default culture has been updated to fr-FR. Updating the return value will also have updated the value stored in SiteSettings._cache.

4. Navigate to ~/Demo/GetSettings again. This will read the LocalizationSettings, which should be retrieved from the cache this time. The DefaultCulture setting will be displayed again, but will incorrectly show "fr-FR".

5. Stop the site, start it again, and navigate to ~/Demo/GetSettings. This should once again show the correct default culture value, "en-US", since the changes to the settings only affected the cache and were not persisted to the SiteSettings document in the database.

Expected behavior

I would expect that the .As<T>() method in SiteSettings.cs would always return a value consistent with the settings stored in the database and that mutating the return value of that method should not affect the cached value.

[rwawr]

Looking at SiteService.cs, it does appear that the summary comments indicate that GetSiteSettings is intended to get site settings from the cache "and that should not be updated", though it's not clear whether that means the return value shouldn't be updated or that the method isn't intended to be used to update the stored settings.

So I guess I'm not sure if this behavior is intentional or not. It does seem like my particular use case is a bit unorthodox, but it also seems like mutations to the return value updating the cached value is rather subtly dangerous. I only have one place in my code that I'm updating the value returned by SiteSettings.As<T>(), and I'm just working around it by casting SiteSettings to IEntity so that .As<T> goes straight to the extension method on IEntity that doesn't cache the values.

[MikeAlhayek]

to update the settings, you'll need something like this

public async Task<IActionResult> GetAndChangeSettingsAsync()
{
   // First load the settings for updating instead of using GetSiteSettingsAsync.
    var site = await _siteService.LoadSiteSettingsAsync();

    // get the existing value
    var localizationSettings = site.As<LocalizationSettings>();

    // Make your setting change
    localizationSettings.DefaultCulture = "fr-FR";

    // save the settings to the loaded site.
    site.Put(localizationSettings);

    // update the site so next call will force reloading new values.
    await _siteService.UpdateSiteSettingsAsync(site);

    return Content("Culture updated to fr-FR");
}

[rwawr]

@MikeAlhayek I'm not actually trying to persist my changes to the settings though. The particular issue that I'm running into is that I'm reading some address display settings that are used to control how/whether certain fields in a postal address form are displayed in a variety of contexts on my site. In one context, I always want to hide two of the fields from the address form, so, before rendering the address form shape, I update the settings object returned from SiteSettings.As<T>() to indicate that those fields should not be displayed. Previously this worked fine, since I was just modifying an object that wasn't referenced anywhere else, as the version of .As<T>() defined in EntityExtensions.cs didn't cache the return value (thereby storing a reference to it). Now, the cache has a reference to that object I'm changing, so subsequent calls that I make to get those settings will reflect those changes to an object I expected to be garbage collected when it went out of scope, rather than what's actually in the database. This is not what I want, since that will cause those two fields that I hide in one specific case to be hidden everywhere else that I display my address form.

[MikeAlhayek]

@rwawr you should not be trying to update the settings on every call and should not use SiteSettings for the problem on hand. You could use a dedicated service that would return the correct culture DefaultCulture so it is handled outside the site settings for every request.

[rwawr]

@MikeAlhayek The DefaultCulture example was meant more as a barebones demonstration of the issue than a precise representation of my specific case. I can accept that it's probably not particularly orthodox to be updating a value returned by that SiteSettings.As<T>() method, so I won't complain if you want to close this. But it does seem a bit dangerous to return a reference to the value in the cache rather than a copy of it. It seems like the issue could be avoided while maintaining the cache functionality by doing something like this, but maybe the serialization and deserialization nullifies the gains from the caching behavior.

[MikeAlhayek]

@rwawr the ISiteService is a singleton object by nature. So all of it's content it's cached by default to improve performance. The cache is cleared once the settings are properly updated. So we do want the cache here to avoid having to deserialize the same thing over and over which can be expensive if it is done on every request.

The reason you noticed this breaking change is because you are incorrectly using the SiteService.

[rwawr]

@MikeAlhayek Fair enough. Of the dozens of places we use the SiteService, this particular case is the only one where the return value is being modified, so it's simple enough to change our implementation, given that it sounds like this is intended behavior in Orchard Core. I'll go ahead and close this issue.