Add Dependency Injection Support for ClientResourceManager

[SkyeHoefling]

Description of problem

There is no interface for ClientResourceManager and it does not currently support Dependency Injection

Description of solution

Extract interface and register with Dependency Injection container.

Description of alternatives considered

N/A

Screenshots

N/A

Additional context

This extension point will allow developers to add significant performance improvements to DNN. After modifying my ClientResourceManager with async scripts and adjusting orders I had DNN loading on average 100ms-300ms faster on each request. This performance change is a breaking change to DNN which is why I want to extract an interface instead. If we open up this extension point I can implement my performance changes as a module that can be used in current releases of DNN.

A proper implementation of this change will completely de-couple DNN from the 3rd party Client Resource Management tool used in DNN. It'll give us an opportunity to investigate new ways to handle client scripts that will work in a potential .NET Core solution of DNN.

Affected browser

N/A

I will be submitting a pull request for this, I have implemented this in a fork already and it is working

Required Work

DotNetNuke.Web.Client has a circular dependency with DotNetNuke.Library and the chosen solution at time of implementation was to side-load the dlls into memory using reflection. This is an anti-pattern but was needed to solve the problem at the time. Prior to completing any feature for this we should add Dependency Injection for the following areas of DNN

Feature/Control	Interface	Pull Request ID	Status
Globals.Status	IApplicationInfo & IApplicationStatusInfo	#3988	✅ Completed
HostController	IHostController	#3990	✅ Completed
PortalController	IPortalController
PortalAliasController	IPortalAliasController	#4021	✅ Completed

[david-poindexter]

@ahoefling this honestly sounds amazing! I am looking forward to seeing what this leads to.

[SkyeHoefling]

I created a table on the initial comment of this issue to document the required features needed before work on this can start.

[SkyeHoefling]

Only 2 more items to convert to Dependency Injection and then we can submit a PR for this. Making progress!

[SkyeHoefling]

@bdukes @mitchelsellers The last bit of required work is for the PortalController which is a very large chunk of work. Any idea on how we can break this into more manageable chunks for submit as PRs? I am thinking evaluate all the properties that are used in the PortalController and it's dependencies and extract those to interfaces first. That alone will be touching many files.

Curious if you have any ideas of the path of least resistance here

[mitchelsellers]

I think that makes sense

[bdukes]

Does it make sense to look at what's used by the ClientResourceManager, and see if those make sense to be extracted into their own smaller interface? We don't want to have 100 portal-related interfaces, but it could be nice and clean to have five or ten…

[SkyeHoefling]

When I was working on the ClientResourceManager I had everything ready to go, until I found this code that uses reflection to deal with a circular dependency. https://github.com/dnnsoftware/Dnn.Platform/blob/bcf5fb7e912461b2f613e3c0e88226d7c1b8c938/DNN%20Platform/DotNetNuke.Web.Client/ClientResourceSettings.cs#L34-L48

It appears that it is only using the GetPortalSettingsDictionary. Maybe we implement an IPortalSettingsService?

[SkyeHoefling]

Do we want to add an API structure similar to IHostSettingsService? This has very simple APIs for ready from the HostSettings table. The PortalSettings APIs on the PortalController seem to be get and update with a very verbose api structure. I think we should make it consistent

[SkyeHoefling]

What if we created a new ISettingsService interface or ISettingsServiceBase and then IHostSettingsService and IPortalSettingsService can both inherit from it.

public interface ISettingsService
{
    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    bool GetBoolean(string key);

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <param name="defaultValue">this value will be return if setting's value is empty.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    bool GetBoolean(string key, bool defaultValue);

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    double GetDouble(string key);

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <param name="defaultValue">this value will be return if setting's value is empty.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    double GetDouble(string key, double defaultValue);

    /// <summary>
    /// takes in a text value, decrypts it with a FIPS compliant algorithm and returns the value.
    /// </summary>
    /// <param name="key">the host setting to read.</param>
    /// <param name="passPhrase">the pass phrase used for encryption/decryption.</param>
    /// <returns>The setting value as a <see cref="string"/>.</returns>
    string GetEncryptedString(string key, string passPhrase);

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    int GetInteger(string key);

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <param name="defaultValue">this value will be return if setting's value is empty.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    int GetInteger(string key, int defaultValue);

    /// <summary>
    /// Gets all host settings.
    /// </summary>
    /// <returns>host setting.</returns>
    Dictionary<string, IConfigurationSetting> GetSettings();

    /// <summary>
    /// Gets all host settings as dictionary.
    /// </summary>
    /// <returns>host setting's value.</returns>
    Dictionary<string, string> GetSettingsDictionary();

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    string GetString(string key);

    /// <summary>
    /// Gets the setting value by the specific key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <param name="defaultValue">this value will be return if setting's value is empty.</param>
    /// <returns>host setting's value.</returns>
    /// <exception cref="System.ArgumentException">key is empty.</exception>
    string GetString(string key, string defaultValue);

    /// <summary>
    /// Updates the specified config.
    /// </summary>
    /// <param name="config">The config.</param>
    void Update(IConfigurationSetting config);

    /// <summary>
    /// Updates the specified config.
    /// </summary>
    /// <param name="config">The config.</param>
    /// <param name="clearCache">if set to <c>true</c> will clear cache after updating the setting.</param>
    void Update(IConfigurationSetting config, bool clearCache);

    /// <summary>
    /// Updates the specified settings.
    /// </summary>
    /// <param name="settings">The settings.</param>
    void Update(Dictionary<string, string> settings);

    /// <summary>
    /// Updates the setting for a specified key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <param name="value">The value.</param>
    void Update(string key, string value);

    /// <summary>
    /// Updates the specified key.
    /// </summary>
    /// <param name="key">The key.</param>
    /// <param name="value">The value.</param>
    /// <param name="clearCache">if set to <c>true</c> will clear cache after update settings.</param>
    void Update(string key, string value, bool clearCache);

    /// <summary>
    /// Takes in a <see cref="string"/> value, encrypts it with a FIPS compliant algorithm and stores it.
    /// </summary>
    /// <param name="key">host settings key.</param>
    /// <param name="value">host settings value.</param>
    /// <param name="passPhrase">pass phrase to allow encryption/decryption.</param>
    void UpdateEncryptedString(string key, string value, string passPhrase);
}

[bdukes]

I like that. If we can fit both into the same API shape, that'd be great.

The other settings don't support encrypted settings values (at least not right now), so my only concern would be that we create this base interface and then can't use it for future settings services.

I guess where does portal ID come into play with all of this? Would this always be for the current portal? What about tab/module/tabmodule/role/etc.? Do we need a settings service factory that takes the ID and returns an ISettingsService?

[SkyeHoefling]

We could encapsulate the ISettingsService inside the IPortalSettingsService. This could look like the code snippet below

ISettingsService GetPortalSettings(int portalId);

Then the consuming code would have a settings object that can easily manipulate any portal settings. This is going to require some thinking and rapid prototypes to see what works best.

[SkyeHoefling]

I started tinkering with some solutions and I think I have a recommendation that will give us some building blocks to use a shared ISettingsService as we were discussing earlier. Instead of using a IPortalSettingsFactory I propose we create an IPortalSettingsManager. I believe this is a better name because it isn't really instantiating a new portal setting object it is retrieving a dictionary of properties in the database.

Consider the following code snippets, where I am using the ISettingsService referenced above.

public interface IPortalSettingsManager
{
    ISettingsService GetPortalSettings(int portalId);
    ISettingsService GetPortalSettings(int portalId, string cultureCode);
}

There will be a new class in DotNetNuke.Entities.Portals called PortalSettingsService. This class will be an implementation of ISettingsService and instantiated by the IPortalSettingsManager.

public class PortalSettingsService : ISettingsService
{
    IDictionary<string, string> settings;
    public PortalSettingsService(int portalId)
    {
        this.settings = PortalController.Instance.GetPortalSettings(portalId);
    }

    public PortalSettingsService(int portalId, string cultureCode)
    {
        this.settings = PortalController.Instance.GetPortalSettings(portalId, cultureCode);
    }

    // omitted 'ISettingsService` implementation
}

Now we can implement the IPortalSettingsManager which will be added to the PortalController.

public class PortalController : ServiceLocator<IPortalController, PortalController>, IPortalController, IPortalSettingsManager
{
    // omitted code

    ISettingsService IPortalSettingsManager.GetPortalSettings(int portalId)
    {
        return new PortalSettingsService(portalId);
    }

    ISettingsService IPortalSettingsManager.GetPortalSettings(int portalId, string cultureCode)
    {
        return new PortalSettingsService(portalId, cultureCode);
    }

    // omitted code
}

Once this is completed we will just need to register the IPortalSettingsManager with the dependency injection container. I believe a change like this will be very lightweight and doesn't require moving any APIs. The challenge is it creates an additional class in the DNN Library project. This could add confusion on what is the correct thing to use. I like this solution because it really simplifies PortalSettings to it's own class.

Why Not PortalSettingsController

I took a look at the PortalSettingsController and this class appears to be a wrapper around the Dictionary<string, string> of the PortalSettings table in the database. After implementing this solution we could update the inner workings of the PortalSettingsController to use our IPortalSettingsManager

If this sounds like a good idea, I can start working on a pull request

[mitchelsellers]

This sounds like a good option

[bdukes]

👍🏻 thanks @ahoefling, sounds great!

[SkyeHoefling]

Thanks for the feedback, I started working on this over the weekend and it is really coming together. I was thinking about the API structure and I would like to propose a few changes as it appears to be designed from a time before .NET generics were available. Many of the APIs have methods such as

double GetDouble(string settingName);
string GetString(string settingName);

I am proposing we simplify the API to take full advantage of modern techniques in C#/.NET

public interface ISettingsService
{
    // Add indexer support to simplify retrieving string values
    string this[string key] { get; }

    // We can't add another special indexer, so creating a simple get
    // encrypted method will solve that req. This will use the default
    // decryption key
    GetEncrypted(string key);

    // Retrieves an encrypted setting with a customized
    // encryptiong pass phrase.
    GetEncrypted(string key, string passPhrase);

    // Using generics we can retrieve any object that can be casted.
    // This can be done with either a cast or using the as operator
    T GetValue<T>(string key);
    T GetEncryptedValue<T>(string key);

    // This wasn't in the original interface but is needed
    void Delete(string key);

    // Standard update methods
    void Update(string key, string value);
    void UpdateEncrypted(string key, string value);
    void UpdateEncrypted(string key, string value, string passPhrase);

    // Update methods that force the cache to be cleared
    void Update(string key, string value, bool clearCache);
    void UpdateEncrypted(string key, string value, bool clearCache);
    void UpdateEncrypted(string key, string value, string passPhrase, bool clearCache);
}

The idea behind the generics is to allow consumers to plug in their primitives. We could add additional error checking or type checking to ensure the T is of type short, int, float, long, double, bool, string, or other primitive that I am probably missing. I don't think it is really necessary as they will get an exception if it fails.

I am not a really big fan of the clearCache technique used across our settings in DNN. The actual dictionary is stored on the HttpContext.Current.Items which only exists for the scope of the request. With Dependency Injection this is really not needed anymore since we have lifetime management. Request scope is a very short life-cycle and it will need to re-query the database every time a new request attempts to access the portal settings. I wonder if it makes sense to invalidate the cache every time there is an update? Is there any DNN history I am not aware of on how this cache works?

If you didn't notice I explicitly left out the IConfigurationSetting and the IDictionary<string, string> APIs as I want to focus the discussion on the core APIs. I am still tinkering with the code to find the best proposal.

• IConfigurationSetting - This one needs more thought as it was a direct port from DotNetNuke.Library and I think it may be unnecessary
• IDictionary<string, string> - I am thinking this will be best used as an extension method to support range manipulation APIs. I'm not 100% sure and would like to flush out the primary APIs first.

[bdukes]

The type handling could make use of ISettingsSerializer<T>.

I think the idea with cache clearing is that if you know you're going to be updating a lot of settings at once, you only clear the cache at the end, rather then continually clearing it for each setting you update.

[mitchelsellers]

I'm all for anything that can be done with Generics and modern processes, no need to bring forward old methods of implementation.

As for cache clear, I think that Brian is correct with this, but I'm going to question your thought about re-quering the DB on each request, as I do NOT believe that is the case. The clearing of that cache item for example would also trigger a synchronize call in the load-balanced environments. It is possible that we adjusted the cache lifecycle with recent changes, and if we did, we might need to revisit for performance reasons.

[SkyeHoefling]

@bdukes

The type handling could make use of ISettingsSerializer<T>

This doesn't appear to be used that much in the platform. Why not use newtonsoft.json or even better use system.text.json.serialization? Could you provide some pseudo code or more details on how this would work? I am not fully understanding your idea. 😕

@mitchelsellers

I'm going to question your thought about re-quering the DB on each request

Great question as I didn't clearly explain what is happening with retrieving data. To the best of my understanding it follows these steps

1. Attempt to retrieve data from HttpContext.Current.Items (request scope)
2. If null, retrieve it from the cache

Here is a reference to the code I am referring to from the PortalController https://github.com/dnnsoftware/Dnn.Platform/blob/093fbf4671c9232ef1180056379bd3e1b1b235d6/DNN%20Platform/Library/Entities/Portals/PortalController.cs#L2121-L2155

My change plans to rip out the dependency on the HttpContext because I don't believe it is really providing much optimization if any. Also, any code that we can add that doesn't depend on the HttpContext will make migrations to .NET 6+ easier. I plan to still leverage the caching mechanism built into DNN.

If developers are making several updates to a Settings at once and then clear the cache at the end should we add a specific ClearCache API instead of tightly coupling it to the Update APIs? We could add an extension method to wrap a combination of the APIs like this

public interface ISettingsService
{
    // omitted methods

    void ClearCache();
}

public static SettingsServiceExetnsions
{
    public static void Update(this ISettingsService settingsService, string key, string value, bool clearCache)
    {
        settingsService.Update(key, value);
        if (clearCache)
        {
            settingsService.ClearCache();
        }
    }
}

Then the usage could look something like this

ISettingsService portalSettings = this.portalSettingsManager.GetPortalSettings(portalId, cultureCode);
portalSettings.Update("my portal setting", "My new value", true);

[mitchelsellers]

Some of this will be developers some of this will be internal code, with the plethora of settings that we enact.

This is a NEW API so I can be ok with a slight change in behavior, but I want to be wary of "undiscoverable" elements. I don't find it intuitive that if I update a setting I would need to manually call clear cache once done because I could have updated multiple properties. I don't necessarily like coupling the update with the clear cache decision, but the performance implications not allowing the granularity here are epic in proportion.

Furthermore, I'm NOT a fan of extension methods, at least not on things that would be used on a regular basis, as you harm the ability to unit test anything downstream that uses that method without creating a bunch of extra junk (This post for example is one of the best workarounds I've seen for making testable extensions and even that is a bit crummy - https://codethug.com/2017/09/09/Mocking-Extension-Methods/)

[SkyeHoefling]

We have some disagreements on extension methods, but you have valid points

• Testability can be difficult
• Methods aren't easily discovered

I have no problem with your thoughts on my API structure above, glad we are having a dialogue about what works and what doesn't work. I can keep moving forward with the documented API structure in the comment above (https://github.com/dnnsoftware/Dnn.Platform/issues/3985#issuecomment-688861383).

• No specific ClearCache() API
• No extension methods

[bdukes]

Sorry, I guess I'm more looking at SerializationController.DeserializeProperty<T>, but we'd need to refactor it a little bit to get access to the deserialization logic without the extra reflection pieces used by the settings attributes.

The main point being that we already have code to take a string and convert it to the desired type.

[mitchelsellers]

Otherwise, yes I'm ok with the direction, Totally understand the opinionated point of Extension Methods....we see this in the .NET Core apis everywhere....

[SkyeHoefling]

Thanks everyone for the feedback on this, I submitted an initial PR to get early feedback to give everyone an idea of what I am thinking of. Please take a look and let me know what you all think. The PR is not ready to be merged, but I am trying to get feedback early prior to making it production ready and complete with unit testing

[stale[bot]]

We have detected this issue has not had any activity during the last 90 days. That could mean this issue is no longer relevant and/or nobody has found the necessary time to address the issue. We are trying to keep the list of open issues limited to those issues that are relevant to the majority and to close the ones that have become 'stale' (inactive). If no further activity is detected within the next 14 days, the issue will be closed automatically. If new comments are are posted and/or a solution (pull request) is submitted for review that references this issue, the issue will not be closed. Closed issues can be reopened at any time in the future. Please remember those participating in this open source project are volunteers trying to help others and creating a better DNN Platform for all. Thank you for your continued involvement and contributions!

[SkyeHoefling]

I think this is still a valid issue and should be pinned. I haven't had an opportunity to finish the pre-reqs for this as we need some work to be done on on the IPortalController. Once that is complete we can start the work to close this issue out.

This is going to be a super powerful feature for DNN as you'll be able to easily use the CRM anywhere that supports DI. It should open up a lot of doors especially for the future of DNN

[valadas]

Perfect, it's pinned