-
Notifications
You must be signed in to change notification settings - Fork 746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Dependency Injection Support for ClientResourceManager #3985
Comments
@ahoefling this honestly sounds amazing! I am looking forward to seeing what this leads to. |
I created a table on the initial comment of this issue to document the required features needed before work on this can start. |
Only 2 more items to convert to Dependency Injection and then we can submit a PR for this. Making progress! |
@bdukes @mitchelsellers The last bit of required work is for the Curious if you have any ideas of the path of least resistance here |
I think that makes sense |
Does it make sense to look at what's used by the |
When I was working on the
It appears that it is only using the |
Do we want to add an API structure similar to |
What if we created a new 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);
} |
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 |
We could encapsulate the 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. |
I started tinkering with some solutions and I think I have a recommendation that will give us some building blocks to use a shared Consider the following code snippets, where I am using the public interface IPortalSettingsManager
{
ISettingsService GetPortalSettings(int portalId);
ISettingsService GetPortalSettings(int portalId, string cultureCode);
} There will be a new class in 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 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 Why Not PortalSettingsControllerI took a look at the If this sounds like a good idea, I can start working on a pull request |
This sounds like a good option |
👍🏻 thanks @ahoefling, sounds great! |
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 I am not a really big fan of the If you didn't notice I explicitly left out the
|
The type handling could make use of 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. |
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. |
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. 😕
Great question as I didn't clearly explain what is happening with retrieving data. To the best of my understanding it follows these steps
Here is a reference to the code I am referring to from the Dnn.Platform/DNN Platform/Library/Entities/Portals/PortalController.cs Lines 2121 to 2155 in 093fbf4
My change plans to rip out the dependency on the 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); |
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/) |
We have some disagreements on extension methods, but you have valid points
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 (#3985 (comment)).
|
Sorry, I guess I'm more looking at The main point being that we already have code to take a string and convert it to the desired type. |
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.... |
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 |
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. |
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 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 |
Perfect, it's pinned |
Description of problem
There is no interface for
ClientResourceManager
and it does not currently support Dependency InjectionDescription 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 withDotNetNuke.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 DNNIApplicationInfo
&IApplicationStatusInfo
IHostController
IPortalController
IPortalAliasController
The text was updated successfully, but these errors were encountered: