Fixed configuration-managed settings remaining active after their configuration plugin was removed (#892)

Co-authored-by: Thorsten Sommer <SommerEngineering@users.noreply.github.com>

app/MindWork AI Studio/Settings/ManagedConfiguration.cs

@@ -9,7 +9,10 @@ namespace AIStudio.Settings;
 public static partial class ManagedConfiguration
 {
     private static readonly ConcurrentDictionary<string, IConfig> METADATA = new();
+    
     private static SettingsManager SettingsManagerAccess => Program.SERVICE_PROVIDER.GetRequiredService<SettingsManager>();
+    
+    private static ILogger Log => Program.LOGGER_FACTORY.CreateLogger(nameof(ManagedConfiguration));
 
     /// <summary>
     /// Attempts to retrieve the configuration metadata for a given configuration selection and
@@ -28,15 +31,13 @@ public static partial class ManagedConfiguration
     /// <typeparam name="TClass">The type of the configuration class.</typeparam>
     /// <typeparam name="TValue">The type of the property within the configuration class.</typeparam>
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
-    public static bool TryGet<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, TValue>> propertyExpression,
-        out ConfigMeta<TClass, TValue> configMeta)
+    public static bool TryGet<TClass, TValue>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, TValue>> propertyExpression, out ConfigMeta<TClass, TValue> configMeta)
         where TValue : Enum
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, TValue> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -65,14 +66,12 @@ public static partial class ManagedConfiguration
     /// if found.</param>
     /// <typeparam name="TClass">The type of the configuration class.</typeparam>
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
-    public static bool TryGet<TClass>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, string>> propertyExpression,
-        out ConfigMeta<TClass, string> configMeta)
+    public static bool TryGet<TClass>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, string>> propertyExpression, out ConfigMeta<TClass, string> configMeta)
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, string> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -104,16 +103,13 @@ public static partial class ManagedConfiguration
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
 
     // ReSharper disable MethodOverloadWithOptionalParameter
-    public static bool TryGet<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, TValue>> propertyExpression,
-        out ConfigMeta<TClass, TValue> configMeta,
-        ISpanParsable<TValue>? _ = null)
+    public static bool TryGet<TClass, TValue>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, TValue>> propertyExpression, out ConfigMeta<TClass, TValue> configMeta, ISpanParsable<TValue>? _ = null)
         where TValue : struct, ISpanParsable<TValue>
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, TValue> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -143,14 +139,12 @@ public static partial class ManagedConfiguration
     /// <typeparam name="TClass">The type of the configuration class.</typeparam>
     /// <typeparam name="TValue">The type of the property within the configuration class.</typeparam>
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
-    public static bool TryGet<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, IList<TValue>>> propertyExpression,
-        out ConfigMeta<TClass, IList<TValue>> configMeta)
+    public static bool TryGet<TClass, TValue>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, IList<TValue>>> propertyExpression, out ConfigMeta<TClass, IList<TValue>> configMeta)
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, IList<TValue>> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -178,14 +172,12 @@ public static partial class ManagedConfiguration
     /// <typeparam name="TClass">The type of the configuration class.</typeparam>
     /// <typeparam name="TValue">The type of the property within the configuration class.</typeparam>
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
-    public static bool TryGet<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, ISet<TValue>>> propertyExpression,
-        out ConfigMeta<TClass, ISet<TValue>> configMeta)
+    public static bool TryGet<TClass, TValue>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, ISet<TValue>>> propertyExpression, out ConfigMeta<TClass, ISet<TValue>> configMeta)
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, ISet<TValue>> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -212,14 +204,12 @@ public static partial class ManagedConfiguration
     /// if found.</param>
     /// <typeparam name="TClass">The type of the configuration class.</typeparam>
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
-    public static bool TryGet<TClass>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, IDictionary<string, string>>> propertyExpression,
-        out ConfigMeta<TClass, IDictionary<string, string>> configMeta)
+    public static bool TryGet<TClass>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, IDictionary<string, string>>> propertyExpression, out ConfigMeta<TClass, IDictionary<string, string>> configMeta)
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, IDictionary<string, string>> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -248,16 +238,14 @@ public static partial class ManagedConfiguration
     /// <typeparam name="TKey">The enum type of the dictionary keys.</typeparam>
     /// <typeparam name="TValue">The enum type of the dictionary values.</typeparam>
     /// <returns>True if the configuration metadata was found, otherwise false.</returns>
-    public static bool TryGet<TClass, TKey, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, Dictionary<TKey, TValue>>> propertyExpression,
-        out ConfigMeta<TClass, Dictionary<TKey, TValue>> configMeta)
+    public static bool TryGet<TClass, TKey, TValue>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, Dictionary<TKey, TValue>>> propertyExpression, out ConfigMeta<TClass, Dictionary<TKey, TValue>> configMeta)
         where TKey : struct, Enum
         where TValue : struct, Enum
     {
         var configPath = Path(configSelection, propertyExpression);
         if (METADATA.TryGetValue(configPath, out var value) && value is ConfigMeta<TClass, Dictionary<TKey, TValue>> meta)
         {
+            meta.RestoreLockedConfiguration();
             configMeta = meta;
             return true;
         }
@@ -270,211 +258,106 @@ public static partial class ManagedConfiguration
     }
 
     /// <summary>
-    /// Checks if a configuration setting is left over from a configuration plugin that is no longer available.
-    /// If the configuration setting is locked and managed by a configuration plugin that is not available,
-    /// it resets the managed state of the configuration setting and returns true.
-    /// Otherwise, it returns false.
+    /// Removes all managed states whose configuration plugin is not available anymore.
     /// </summary>
-    /// <param name="configSelection">The expression to select the configuration class.</param>
-    /// <param name="propertyExpression">The expression to select the property within the configuration class.</param>
+    /// <remarks>
+    /// This covers every registered setting, regardless of its type: locked settings, editable
+    /// defaults, and additive plugin contributions. Settings do not need to be listed anywhere for
+    /// this cleanup to work, so adding a new managed setting cannot be forgotten here.<br/><br/>
+    /// A locked setting whose plugin is gone is reset to its default value. That is intended: the
+    /// value belonged to the organization, not to the user, and the user might not be able to
+    /// change it at all.
+    /// </remarks>
     /// <param name="availablePlugins">The collection of available plugins to check against.</param>
-    /// <typeparam name="TClass">The type of the configuration class.</typeparam>
-    /// <typeparam name="TValue">The type of the property within the configuration class.</typeparam>
-    /// <returns>True if the configuration setting is left over and was reset, otherwise false.</returns>
-    public static bool IsConfigurationLeftOver<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, TValue>> propertyExpression,
-        IReadOnlyList<IAvailablePlugin> availablePlugins)
-        where TValue : Enum
+    /// <param name="deployedConfigPluginIds">
+    /// The IDs of all configuration plugins which are deployed on this machine, including those which
+    /// could not be loaded. A deployed plugin was not removed, so its settings must stay untouched.
+    /// </param>
+    /// <returns>True when at least one setting was changed, otherwise false.</returns>
+    public static bool CleanupLeftOverManagedConfigurations(IReadOnlyCollection<IAvailablePlugin> availablePlugins, IReadOnlySet<Guid> deployedConfigPluginIds)
     {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
+        var wasChanged = false;
+        var registeredSettingNames = new HashSet<string>(StringComparer.Ordinal);
 
-        if (configMeta.LockedByConfigPluginId != Guid.Empty && configMeta.IsLocked)
+        foreach (var config in METADATA.Values)
         {
-            var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-            if (plugin is null)
+            if (config is not ConfigMetaBase configMeta)
+                continue;
+
+            registeredSettingNames.Add(configMeta.SettingName);
+
+            //
+            // Restore the persisted ownership first. Otherwise, we would not recognize a left-over
+            // lock when nobody has read this setting since the settings were loaded:
+            //
+            configMeta.RestoreLockedConfiguration();
+
+            // Check the locked state:
+            if (configMeta.IsLocked && configMeta.LockedByConfigPluginId != Guid.Empty && !IsPluginPresent(configMeta.LockedByConfigPluginId, availablePlugins, deployedConfigPluginIds))
             {
+                Log.LogInformation($"Resetting the setting '{configMeta.SettingName}': it was locked by the configuration plugin '{configMeta.LockedByConfigPluginId}', which is not available anymore.");
                 configMeta.ResetLockedConfiguration();
-                return true;
+                wasChanged = true;
+            }
+
+            // Check the editable default state:
+            if (CleanupEditableDefaultState(configMeta, availablePlugins, deployedConfigPluginIds))
+                wasChanged = true;
+
+            // Check the additive plugin contribution:
+            if (configMeta.HasPluginContribution && configMeta.PluginContributionByConfigPluginId != Guid.Empty && !IsPluginPresent(configMeta.PluginContributionByConfigPluginId, availablePlugins, deployedConfigPluginIds))
+            {
+                Log.LogInformation($"Clearing the plugin contribution for the setting '{configMeta.SettingName}': the configuration plugin '{configMeta.PluginContributionByConfigPluginId}' is not available anymore.");
+                configMeta.ClearPluginContribution();
+                wasChanged = true;
             }
         }
 
-        return CleanupEditableDefaultState(configMeta, SettingName(propertyExpression), availablePlugins);
-    }
+        // Remove persisted states which belong to settings that do not exist anymore:
+        if (RemoveUnknownManagedStates(registeredSettingNames))
+            wasChanged = true;
 
-    public static bool IsConfigurationLeftOver<TClass>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, string>> propertyExpression,
-        IReadOnlyList<IAvailablePlugin> availablePlugins)
-    {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
-
-        if (configMeta.LockedByConfigPluginId != Guid.Empty && configMeta.IsLocked)
-        {
-            var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-            if (plugin is null)
-            {
-                configMeta.ResetLockedConfiguration();
-                return true;
-            }
-        }
-
-        return CleanupEditableDefaultState(configMeta, SettingName(propertyExpression), availablePlugins);
-    }
-
-    // ReSharper disable MethodOverloadWithOptionalParameter
-    public static bool IsConfigurationLeftOver<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, TValue>> propertyExpression,
-        IReadOnlyList<IAvailablePlugin> availablePlugins,
-        ISpanParsable<TValue>? _ = null)
-        where TValue : struct, ISpanParsable<TValue>
-    {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
-
-        if (configMeta.LockedByConfigPluginId != Guid.Empty && configMeta.IsLocked)
-        {
-            var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-            if (plugin is null)
-            {
-                configMeta.ResetLockedConfiguration();
-                return true;
-            }
-        }
-
-        return CleanupEditableDefaultState(configMeta, SettingName(propertyExpression), availablePlugins);
-    }
-
-    // ReSharper restore MethodOverloadWithOptionalParameter
-
-    public static bool IsConfigurationLeftOver<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, IList<TValue>>> propertyExpression,
-        IEnumerable<IAvailablePlugin> availablePlugins)
-    {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
-
-        if (configMeta.ManagedMode is ManagedConfigurationMode.EDITABLE_DEFAULT)
-            return CleanupEditableDefaultState(configMeta, SettingName(propertyExpression), availablePlugins.ToList());
-
-        if (configMeta.LockedByConfigPluginId == Guid.Empty || !configMeta.IsLocked)
-            return false;
-
-        var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-        if (plugin is not null)
-            return false;
-
-        configMeta.ResetLockedConfiguration();
-        return true;
-    }
-
-    public static bool IsConfigurationLeftOver<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, ISet<TValue>>> propertyExpression,
-        IEnumerable<IAvailablePlugin> availablePlugins)
-    {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
-
-        if (configMeta.LockedByConfigPluginId == Guid.Empty || !configMeta.IsLocked)
-            return false;
-
-        var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-        if (plugin is null)
-        {
-            configMeta.ResetLockedConfiguration();
-            return true;
-        }
-
-        return false;
+        return wasChanged;
     }
 
     /// <summary>
-    /// Checks if a plugin contribution is left over from a configuration plugin that is no longer available.
-    /// If so, it clears the contribution and returns true.
+    /// Checks whether a configuration plugin is still present on this machine.
     /// </summary>
-    public static bool IsPluginContributionLeftOver<TClass, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, ISet<TValue>>> propertyExpression,
-        IEnumerable<IAvailablePlugin> availablePlugins)
+    /// <remarks>
+    /// A plugin counts as present when it was loaded, or when it is deployed but could not be loaded.
+    /// The latter matters for organizations: a broken configuration plugin is still in charge, so we
+    /// must not treat its settings as left over.
+    /// </remarks>
+    private static bool IsPluginPresent(Guid configPluginId, IReadOnlyCollection<IAvailablePlugin> availablePlugins, IReadOnlySet<Guid> deployedConfigPluginIds) => deployedConfigPluginIds.Contains(configPluginId) || availablePlugins.Any(x => x.Id == configPluginId);
+
+    /// <summary>
+    /// Removes persisted managed states which belong to settings that are not registered anymore.
+    /// </summary>
+    /// <remarks>
+    /// Without this, states of removed or renamed settings would stay in the settings file forever.
+    /// </remarks>
+    private static bool RemoveUnknownManagedStates(IReadOnlySet<string> registeredSettingNames)
     {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
+        var wasChanged = false;
+        var configurationData = SettingsManagerAccess.ConfigurationData;
 
-        if (!configMeta.HasPluginContribution || configMeta.PluginContributionByConfigPluginId == Guid.Empty)
-            return false;
-
-        var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.PluginContributionByConfigPluginId);
-        if (plugin is null)
+        foreach (var settingName in configurationData.ManagedLockedConfigurations.Keys.Where(x => !registeredSettingNames.Contains(x)).ToList())
         {
-            configMeta.ClearPluginContribution();
-            return true;
+            Log.LogInformation($"Removing the persisted lock of the setting '{settingName}': this setting does not exist anymore.");
+            configurationData.ManagedLockedConfigurations.Remove(settingName);
+            wasChanged = true;
         }
 
-        return false;
+        foreach (var settingName in configurationData.ManagedEditableDefaults.Keys.Where(x => !registeredSettingNames.Contains(x)).ToList())
+        {
+            Log.LogInformation($"Removing the persisted editable default of the setting '{settingName}': this setting does not exist anymore.");
+            configurationData.ManagedEditableDefaults.Remove(settingName);
+            wasChanged = true;
+        }
+
+        return wasChanged;
     }
 
-    public static bool IsConfigurationLeftOver<TClass>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, IDictionary<string, string>>> propertyExpression,
-        IEnumerable<IAvailablePlugin> availablePlugins)
-    {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
-
-        if (configMeta.LockedByConfigPluginId == Guid.Empty || !configMeta.IsLocked)
-            return false;
-
-        var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-        if (plugin is null)
-        {
-            configMeta.ResetLockedConfiguration();
-            return true;
-        }
-
-        return false;
-    }
-
-    public static bool IsConfigurationLeftOver<TClass, TKey, TValue>(
-        Expression<Func<Data, TClass>> configSelection,
-        Expression<Func<TClass, Dictionary<TKey, TValue>>> propertyExpression,
-        IEnumerable<IAvailablePlugin> availablePlugins)
-        where TKey : struct, Enum
-        where TValue : struct, Enum
-    {
-        if (!TryGet(configSelection, propertyExpression, out var configMeta))
-            return false;
-
-        if (configMeta.ManagedMode is ManagedConfigurationMode.EDITABLE_DEFAULT)
-        {
-            var plugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.EditableDefaultByConfigPluginId);
-            if (plugin is null)
-            {
-                configMeta.ClearEditableDefaultConfiguration();
-                ClearEditableDefaultState(SettingName(propertyExpression));
-                return true;
-            }
-
-            return false;
-        }
-
-        if (configMeta.LockedByConfigPluginId == Guid.Empty || !configMeta.IsLocked)
-            return false;
-
-        var lockedPlugin = availablePlugins.FirstOrDefault(x => x.Id == configMeta.LockedByConfigPluginId);
-        if (lockedPlugin is null)
-        {
-            configMeta.ResetLockedConfiguration();
-            return true;
-        }
-
-        return false;
-    }
-    
     private static string Path<TClass, TValue>(Expression<Func<Data, TClass>> configSelection, Expression<Func<TClass, TValue>> propertyExpression)
     {
         var className = typeof(TClass).Name;
@@ -507,12 +390,9 @@ public static partial class ManagedConfiguration
 
     private static bool ClearEditableDefaultState(string settingName) => SettingsManagerAccess.ConfigurationData.ManagedEditableDefaults.Remove(settingName);
 
-    private static bool CleanupEditableDefaultState<TClass, TValue>(
-        ConfigMeta<TClass, TValue> configMeta,
-        string settingName,
-        IReadOnlyList<IAvailablePlugin> availablePlugins)
+    private static bool CleanupEditableDefaultState(ConfigMetaBase configMeta, IReadOnlyCollection<IAvailablePlugin> availablePlugins, IReadOnlySet<Guid> deployedConfigPluginIds)
     {
-        if (!TryGetEditableDefaultState(settingName, out var editableDefaultState))
+        if (!TryGetEditableDefaultState(configMeta.SettingName, out var editableDefaultState))
         {
             if (configMeta.ManagedMode is not ManagedConfigurationMode.EDITABLE_DEFAULT)
                 return false;
@@ -521,11 +401,11 @@ public static partial class ManagedConfiguration
             return true;
         }
 
-        var plugin = availablePlugins.FirstOrDefault(x => x.Id == editableDefaultState.ConfigPluginId);
-        if (plugin is not null)
+        if (IsPluginPresent(editableDefaultState.ConfigPluginId, availablePlugins, deployedConfigPluginIds))
             return false;
 
+        Log.LogInformation($"Clearing the editable default of the setting '{configMeta.SettingName}': the configuration plugin '{editableDefaultState.ConfigPluginId}' is not available anymore.");
         configMeta.ClearEditableDefaultConfiguration();
-        return ClearEditableDefaultState(settingName);
+        return ClearEditableDefaultState(configMeta.SettingName);
     }
 }