Skip to content

ConfigurationManager only adds, doesn't remove from Dictionary-based items #2952

Description

@tig

When I coded this, I failed to consider what happens if an item is removed from a dictionary.

To repro:

  • Run UI Catalog
  • While running, add a new Theme named "UI Catalog Theme 2" to ./.tui/config.json
  • Note Themes menu now also shows "UI Catalog Theme 2"
  • Now remove that added Theme and save the config file

Expected Behavior:
"UI Catalog Theme 2" disappears

Actual Behavior:
"UI Catalog Theme 2" is still in the Themes dictionary.

The problem is in ConfigProperty.cs where Update does a deep memberwise copy:

	internal object? UpdateValueFrom (object source)
	{
		if (source == null) {
			return PropertyValue;
		}

		var ut = Nullable.GetUnderlyingType (PropertyInfo!.PropertyType);
		if (source.GetType () != PropertyInfo!.PropertyType && (ut != null && source.GetType () != ut)) {
			throw new ArgumentException ($"The source object ({PropertyInfo!.DeclaringType}.{PropertyInfo!.Name}) is not of type {PropertyInfo!.PropertyType}.");
		}
		if (PropertyValue != null && source != null) {
			PropertyValue = ConfigurationManager.DeepMemberwiseCopy (source, PropertyValue);
		} else {
			PropertyValue = source;
		}

		return PropertyValue;
	}

DeepMemberwiseCopy is really an "append" operation:

	/// <summary>
	/// System.Text.Json does not support copying a deserialized object to an existing instance.
	/// To work around this, we implement a 'deep, memberwise copy' method. 
	/// </summary>
	/// <remarks>
	/// TOOD: When System.Text.Json implements `PopulateObject` revisit
	///	https://github.com/dotnet/corefx/issues/37627
	/// </remarks>
	/// <param name="source"></param>
	/// <param name="destination"></param>
	/// <returns><paramref name="destination"/> updated from <paramref name="source"/></returns>
	internal static object? DeepMemberwiseCopy (object? source, object? destination)
	{
		if (destination == null) {
			throw new ArgumentNullException (nameof (destination));
		}

		if (source == null) {
			return null!;
		}

		if (source.GetType () == typeof (SettingsScope)) {
			return ((SettingsScope)destination).Update ((SettingsScope)source);
		}
		if (source.GetType () == typeof (ThemeScope)) {
			return ((ThemeScope)destination).Update ((ThemeScope)source);
		}
		if (source.GetType () == typeof (AppScope)) {
			return ((AppScope)destination).Update ((AppScope)source);
		}

		// If value type, just use copy constructor.
		if (source.GetType ().IsValueType || source.GetType () == typeof (string)) {
			return source;
		}

		// Dictionary
		if (source.GetType ().IsGenericType && source.GetType ().GetGenericTypeDefinition ().IsAssignableFrom (typeof (Dictionary<,>))) {
			foreach (var srcKey in ((IDictionary)source).Keys) {
				if (srcKey is string) {

				}
				if (((IDictionary)destination).Contains (srcKey))
					((IDictionary)destination) [srcKey] = DeepMemberwiseCopy (((IDictionary)source) [srcKey], ((IDictionary)destination) [srcKey]);
				else {
					((IDictionary)destination).Add (srcKey, ((IDictionary)source) [srcKey]);
				}
			}
			return destination;
		}

		// ALl other object types
		var sourceProps = source?.GetType ().GetProperties ().Where (x => x.CanRead).ToList ();
		var destProps = destination?.GetType ().GetProperties ().Where (x => x.CanWrite).ToList ()!;
		foreach (var (sourceProp, destProp) in
			from sourceProp in sourceProps
			where destProps.Any (x => x.Name == sourceProp.Name)
			let destProp = destProps.First (x => x.Name == sourceProp.Name)
			where destProp.CanWrite
			select (sourceProp, destProp)) {

			var sourceVal = sourceProp.GetValue (source);
			var destVal = destProp.GetValue (destination);
			if (sourceVal != null) {
				if (destVal != null) {
					// Recurse
					destProp.SetValue (destination, DeepMemberwiseCopy (sourceVal, destVal));
				} else {
					destProp.SetValue (destination, sourceVal);
				}
			}
		}
		return destination!;
	}

It really should be a "merge" operation, but my brain hurts when thinking how to make that work.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    No status

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions