Fix case insensitive dictionary problem (Lombiq Technologies: OCORE-197)

[sarahelsaig]

Fixes #16585

Easiest way to test:

1. Create a new site with Blog recipe
2. Go to Admin > Design > Templates
3. Click "Add Template"
4. Name it "Menu", "MENU", or "mEnU", set some custom content.
5. Load the frontend and observe the overridden menu.

[MikeAlhayek]

@sarahelsaig what line fixed the referenced issue?

[MikeAlhayek]

I am not sure this is needed. Can you explain how this prevents multiple enumerations?

[sarahelsaig]

Here you had itemIds.Contains(x.Key). This means itemIds was enumerated templatesDocument.Templates.Count times, which is wasteful if the object is not already IList<string>.

[MikeAlhayek]

How about this instead?
The following code is more readable, does reduce enumeration, does not use as much memory and does not waste resources when Remove is not being request.

        if (itemIds?.Any() == true)
        {
            switch (options.BulkAction)
            {
                case ContentsBulkAction.None:
                    break;
                case ContentsBulkAction.Remove:

                    var templatesDocument = options.AdminTemplates
                        ? await _adminTemplatesManager.LoadTemplatesDocumentAsync()
                        : await _templatesManager.LoadTemplatesDocumentAsync();

                    foreach (var itemId in itemIds)
                    {
                        if (!templatesDocument.Templates.ContainsKey(itemId))
                        {
                            continue;
                        }

                        if (options.AdminTemplates)
                        {
                            await _adminTemplatesManager.RemoveTemplateAsync(itemId);

                            continue;
                        }

                        await _templatesManager.RemoveTemplateAsync(itemId);
                    }
                    await _notifier.SuccessAsync(H["Templates successfully removed."]);
                    break;
                default:
                    return BadRequest();
            }
        }

[sarahelsaig]

The if (itemIds?.Any() == true) also does as an enumeration on its own. I guess it can be replaced with if (itemIds != null) without too much performance hit (probably even a perf gain on average as it should be very rare that an empty ID list is posted.

Also it occurred to me that this code compares keys. If we use emplatesDocument.Templates.Keys, then we are just looking for the union of two string sets. Linq already has an extension method for that. see commit.

[MikeAlhayek]

yes .Any() iterates over the IEnumerable checking if there is at least one item. This is okay because it's fast since it does not iterate over every item. May be better to use itemIds != null && itemIds.Any() this way you don't bother loading the document or applying the union logic where there is no work to be done.

[sarahelsaig]

sarahelsaig what line fixed the referenced issue?

The changes in the file src/OrchardCore.Modules/OrchardCore.Templates/Models/TemplatesDocument.cs fixed the issue.

I have made the same changes for all other dictionary properties with a StringComparer.OrdinalIgnoreCase type default value, because the problem I have described here is applicable to any such property.

[gvkries]

Instead of hard-coding this into the documents, shouldn't we use a JsonConverter instead if this applies to all dictonaries?

[sarahelsaig]

Instead of hard-coding this into the documents, shouldn't we use a JsonConverter instead if this applies to all dictonaries?

That would be a converter that checks every object for dictionary properties with a particular default value. So some kind of a partial converter. Is that actually possible in STJ?

[gvkries]

IIRC, you can apply a converter to a property, via the JsonConverterAttribute. The converter would be something like public class CaseInsensitiveDictionaryConverter<TKey, TValue> : JsonConverter<Dictionary<TKey, TValue>> and returns a case-insensitive dictionary instead of a default one.

[gvkries]

Oh, here you go 🤭

https://stackoverflow.com/questions/67307699/deserialize-into-a-case-insensitive-dictionary-using-system-text-json

[sarahelsaig]

IIRC, you can apply a converter to a property, via the JsonConverterAttribute. The converter would be something like public class CaseInsensitiveDictionaryConverter<TKey, TValue> : JsonConverter<Dictionary<TKey, TValue>> and returns a case-insensitive dictionary instead of a default one.

Ok, I did that. I was confused, because this still involves some code you have to add to each dictionary prop.

[sebastienros]

You see how these properties already had a dictionary with the correct casing? I believed that's a setting on the JSON options to prefer using the existing instance instead of creating a new one (which is what the attribute does).

[MikeAlhayek]

https://github.com/dotnet/runtime/blob/main/src/tasks/MonoTargetsTasks/JsonToItemsTaskFactory/JsonToItemsTaskFactory.cs#L344

[sarahelsaig]

@MikeAlhayek Nice find. Unfortunately it's only for Dictionary<string, string> instead of Dictionary<string, TValue>.

[sebastienros]

@sarahelsaig do you think https://learn.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.preferredobjectcreationhandling?view=net-8.0#system-text-json-jsonserializeroptions-preferredobjectcreationhandling would be the setting to set to get parity with the previous behavior? Which is a perfectly fine default IMO.

[sarahelsaig]

That's already set in JOptions.Base. So looks like it's not enough by itself.

[MikeAlhayek]

@sarahelsaig Please remove EnumerableExtensions since it is no longer used.

[sarahelsaig]

sarahelsaig Please remove EnumerableExtensions since it is no longer used.

Ok, removed.

[sarahelsaig]

I called Rider's "Organize imports" feature to get rid of dead using namespaces and it did this. I think it looks better, but comment here if I should revert them.

[gvkries]

LGTM, but let's see what @sebastienros and @MikeAlhayek have to say.