From 14a1bc44ed763741fc88c96ba581926d7a9ece97 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Sun, 27 Sep 2026 17:24:36 +0000 Subject: [PATCH 1/2] [patch] Stop the merge-strategy cache letting one call decide a key for every later call PropertyMerger cached each key's canonical name process-wide, keyed on the property name alone. The name also depends on the strategy and, for Aggressive and Maximum, on the other keys in the document, so an Aggressive run made a later Conservative run drop custom_title, and a key merged in one document was renamed in another that had nothing to merge it with. Drop the cache: the per-call work is a dictionary lookup and a small normalization over a handful of keys. The tests no longer clear it by reflection, and new tests run the strategies back to back without clearing anything. Fixes ktsu-dev/Frontmatter#130 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KrGyxYxnoFgAkP1ENResCJ --- Frontmatter.Test/DecorationOnlyKeyTests.cs | 1 - .../MergeStrategyIsolationTests.cs | 87 +++++++++++++++++++ Frontmatter.Test/PropertyMergerTests.cs | 18 ---- Frontmatter.Test/SingleCharacterKeyTests.cs | 1 - Frontmatter/PropertyMerger.cs | 24 +---- 5 files changed, 91 insertions(+), 40 deletions(-) create mode 100644 Frontmatter.Test/MergeStrategyIsolationTests.cs diff --git a/Frontmatter.Test/DecorationOnlyKeyTests.cs b/Frontmatter.Test/DecorationOnlyKeyTests.cs index 9c0f4af..f912aef 100644 --- a/Frontmatter.Test/DecorationOnlyKeyTests.cs +++ b/Frontmatter.Test/DecorationOnlyKeyTests.cs @@ -24,7 +24,6 @@ public class DecorationOnlyKeyTests public void ClearCaches() { ClearCache(typeof(NameStandardizer), "PropertyNameCache"); - ClearCache(typeof(PropertyMerger), "PropertyMergeCache"); } private static void ClearCache(Type type, string fieldName) diff --git a/Frontmatter.Test/MergeStrategyIsolationTests.cs b/Frontmatter.Test/MergeStrategyIsolationTests.cs new file mode 100644 index 0000000..04f6201 --- /dev/null +++ b/Frontmatter.Test/MergeStrategyIsolationTests.cs @@ -0,0 +1,87 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Frontmatter.Test; + +using System.Collections.Generic; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Regression tests for #130: the canonical name a key merges to depends on the strategy and on +/// the other keys in the same document, so one call must not decide it for a later one. +/// +/// +/// These tests deliberately clear nothing between calls. The bug lived in process-wide state, and a +/// test that resets that state cannot see it. +/// +[TestClass] +public class MergeStrategyIsolationTests +{ + [TestMethod] + public void ConservativeAfterAggressive_KeepsKeyThatOnlyAggressiveMerges() + { + static Dictionary Document() => new() + { + ["title"] = "T", + ["custom_title"] = "C", + }; + + Dictionary aggressive = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Aggressive); + Assert.IsFalse(aggressive.ContainsKey("custom_title"), "Aggressive should merge custom_title into title"); + + Dictionary conservative = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Conservative); + + Assert.AreEqual("T", conservative["title"]); + Assert.IsTrue(conservative.ContainsKey("custom_title"), "Conservative only applies predefined mappings, so custom_title must survive"); + Assert.AreEqual("C", conservative["custom_title"]); + } + + [TestMethod] + public void AggressiveAfterConservative_StillMergesKeys() + { + static Dictionary Document() => new() + { + ["summary"] = "S", + ["page_summary"] = "P", + }; + + Dictionary conservative = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Conservative); + Assert.HasCount(2, conservative); + + Dictionary aggressive = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Aggressive); + + Assert.HasCount(1, aggressive); + } + + [TestMethod] + public void KeyAloneInALaterDocument_IsNotRenamedByAnEarlierDocument() + { + PropertyMerger.MergeSimilarProperties(new Dictionary + { + ["blurb"] = "B", + ["page_blurb"] = "P", + }, FrontmatterMergeStrategy.Aggressive); + + Dictionary later = PropertyMerger.MergeSimilarProperties(new Dictionary + { + ["page_blurb"] = "Only", + }, FrontmatterMergeStrategy.Aggressive); + + Assert.IsTrue(later.ContainsKey("page_blurb"), "With no other key to merge with, page_blurb keeps its name"); + Assert.AreEqual("Only", later["page_blurb"]); + } + + [TestMethod] + public void CombineFrontmatter_ConservativeAfterAggressive_KeepsCustomTitle() + { + const string first = "---\ntitle: T\ncustom_title: C\n---\nBody1\n"; + const string second = "---\ntitle: T\ncustom_title: C\n---\nBody2\n"; + + Frontmatter.CombineFrontmatter(first, FrontmatterNaming.AsIs, FrontmatterOrder.AsIs, FrontmatterMergeStrategy.Aggressive); + string result = Frontmatter.CombineFrontmatter(second, FrontmatterNaming.AsIs, FrontmatterOrder.AsIs, FrontmatterMergeStrategy.Conservative); + + Dictionary? frontmatter = Frontmatter.ExtractFrontmatter(result); + Assert.IsNotNull(frontmatter); + Assert.AreEqual("C", frontmatter["custom_title"]); + } +} diff --git a/Frontmatter.Test/PropertyMergerTests.cs b/Frontmatter.Test/PropertyMergerTests.cs index 8a72a51..4314c03 100644 --- a/Frontmatter.Test/PropertyMergerTests.cs +++ b/Frontmatter.Test/PropertyMergerTests.cs @@ -5,9 +5,7 @@ namespace ktsu.Frontmatter.Test; using System; -using System.Collections.Concurrent; using System.Collections.Generic; -using System.Reflection; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -24,22 +22,6 @@ public class PropertyMergerTests private static readonly string[] valueArray = ["tag1", "tag2"]; private static readonly string[] valueArray0 = ["tag2", "tag3"]; - [TestInitialize] - public void ClearPropertyMergerCache() - { - // Clear the static cache between tests using reflection - FieldInfo? cacheField = typeof(PropertyMerger).GetField("PropertyMergeCache", - BindingFlags.NonPublic | BindingFlags.Static); - - if (cacheField != null) - { - ConcurrentDictionary? cache = cacheField.GetValue(null) as ConcurrentDictionary; - cache?.Clear(); - } - - Console.WriteLine("Cache cleared"); - } - [TestMethod] public void MergeSimilarProperties_NoStrategy_DoesntMergeAnything() { diff --git a/Frontmatter.Test/SingleCharacterKeyTests.cs b/Frontmatter.Test/SingleCharacterKeyTests.cs index 4ca9b14..f05344e 100644 --- a/Frontmatter.Test/SingleCharacterKeyTests.cs +++ b/Frontmatter.Test/SingleCharacterKeyTests.cs @@ -31,7 +31,6 @@ public class SingleCharacterKeyTests public void ClearCaches() { ClearCache(typeof(NameStandardizer), "PropertyNameCache"); - ClearCache(typeof(PropertyMerger), "PropertyMergeCache"); } private static void ClearCache(Type type, string fieldName) diff --git a/Frontmatter/PropertyMerger.cs b/Frontmatter/PropertyMerger.cs index 70ab447..323059a 100644 --- a/Frontmatter/PropertyMerger.cs +++ b/Frontmatter/PropertyMerger.cs @@ -2,7 +2,6 @@ namespace ktsu.Frontmatter; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; @@ -11,11 +10,6 @@ namespace ktsu.Frontmatter; /// internal static class PropertyMerger { - /// - /// Cache for property merge mappings - /// - private static readonly ConcurrentDictionary PropertyMergeCache = new(); - /// /// Merges properties that capture redundant information based on the specified strategy. /// @@ -55,18 +49,14 @@ internal static Dictionary MergeSimilarProperties(Dictionary !char.IsLetterOrDigit(c) && c != '_' && c != '-')) { - PropertyMergeCache.TryAdd(key, key); return key; } @@ -80,13 +70,7 @@ private static string GetCanonicalName(string key, FrontmatterMergeStrategy stra }; // If no mapping was found, preserve the original key - if (string.IsNullOrEmpty(canonicalName) || canonicalName == key) - { - canonicalName = key; - } - - PropertyMergeCache.TryAdd(key, canonicalName); - return canonicalName; + return string.IsNullOrEmpty(canonicalName) ? key : canonicalName; } private static string GetConservativeCanonicalName(string key) => From f412408da5f61cfacdc97b18f5b009a25034d0a2 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Sun, 27 Sep 2026 17:27:12 +0000 Subject: [PATCH 2/2] Use TryGetValue instead of ContainsKey plus indexer in merge-isolation tests Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KrGyxYxnoFgAkP1ENResCJ --- Frontmatter.Test/MergeStrategyIsolationTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Frontmatter.Test/MergeStrategyIsolationTests.cs b/Frontmatter.Test/MergeStrategyIsolationTests.cs index 04f6201..01133f4 100644 --- a/Frontmatter.Test/MergeStrategyIsolationTests.cs +++ b/Frontmatter.Test/MergeStrategyIsolationTests.cs @@ -32,8 +32,8 @@ public void ConservativeAfterAggressive_KeepsKeyThatOnlyAggressiveMerges() Dictionary conservative = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Conservative); Assert.AreEqual("T", conservative["title"]); - Assert.IsTrue(conservative.ContainsKey("custom_title"), "Conservative only applies predefined mappings, so custom_title must survive"); - Assert.AreEqual("C", conservative["custom_title"]); + Assert.IsTrue(conservative.TryGetValue("custom_title", out object? customTitle), "Conservative only applies predefined mappings, so custom_title must survive"); + Assert.AreEqual("C", customTitle); } [TestMethod] @@ -67,8 +67,8 @@ public void KeyAloneInALaterDocument_IsNotRenamedByAnEarlierDocument() ["page_blurb"] = "Only", }, FrontmatterMergeStrategy.Aggressive); - Assert.IsTrue(later.ContainsKey("page_blurb"), "With no other key to merge with, page_blurb keeps its name"); - Assert.AreEqual("Only", later["page_blurb"]); + Assert.IsTrue(later.TryGetValue("page_blurb", out object? pageBlurb), "With no other key to merge with, page_blurb keeps its name"); + Assert.AreEqual("Only", pageBlurb); } [TestMethod]