Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Frontmatter.Test/DecorationOnlyKeyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
87 changes: 87 additions & 0 deletions Frontmatter.Test/MergeStrategyIsolationTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright (c) 2023-2026 ktsu-dev contributors

namespace ktsu.Frontmatter.Test;

using System.Collections.Generic;

using Microsoft.VisualStudio.TestTools.UnitTesting;

/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// These tests deliberately clear nothing between calls. The bug lived in process-wide state, and a
/// test that resets that state cannot see it.
/// </remarks>
[TestClass]
public class MergeStrategyIsolationTests
{
[TestMethod]
public void ConservativeAfterAggressive_KeepsKeyThatOnlyAggressiveMerges()
{
static Dictionary<string, object> Document() => new()
{
["title"] = "T",
["custom_title"] = "C",
};

Dictionary<string, object> aggressive = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Aggressive);
Assert.IsFalse(aggressive.ContainsKey("custom_title"), "Aggressive should merge custom_title into title");

Dictionary<string, object> conservative = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Conservative);

Assert.AreEqual("T", conservative["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]
public void AggressiveAfterConservative_StillMergesKeys()
{
static Dictionary<string, object> Document() => new()
{
["summary"] = "S",
["page_summary"] = "P",
};

Dictionary<string, object> conservative = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Conservative);
Assert.HasCount(2, conservative);

Dictionary<string, object> aggressive = PropertyMerger.MergeSimilarProperties(Document(), FrontmatterMergeStrategy.Aggressive);

Assert.HasCount(1, aggressive);
}

[TestMethod]
public void KeyAloneInALaterDocument_IsNotRenamedByAnEarlierDocument()
{
PropertyMerger.MergeSimilarProperties(new Dictionary<string, object>
{
["blurb"] = "B",
["page_blurb"] = "P",
}, FrontmatterMergeStrategy.Aggressive);

Dictionary<string, object> later = PropertyMerger.MergeSimilarProperties(new Dictionary<string, object>
{
["page_blurb"] = "Only",
}, FrontmatterMergeStrategy.Aggressive);

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]
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<string, object>? frontmatter = Frontmatter.ExtractFrontmatter(result);
Assert.IsNotNull(frontmatter);
Assert.AreEqual("C", frontmatter["custom_title"]);
}
}
18 changes: 0 additions & 18 deletions Frontmatter.Test/PropertyMergerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<string, string>? cache = cacheField.GetValue(null) as ConcurrentDictionary<string, string>;
cache?.Clear();
}

Console.WriteLine("Cache cleared");
}

[TestMethod]
public void MergeSimilarProperties_NoStrategy_DoesntMergeAnything()
{
Expand Down
1 change: 0 additions & 1 deletion Frontmatter.Test/SingleCharacterKeyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
24 changes: 4 additions & 20 deletions Frontmatter/PropertyMerger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace ktsu.Frontmatter;

using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;

Expand All @@ -11,11 +10,6 @@
/// </summary>
internal static class PropertyMerger
{
/// <summary>
/// Cache for property merge mappings
/// </summary>
private static readonly ConcurrentDictionary<string, string> PropertyMergeCache = new();

/// <summary>
/// Merges properties that capture redundant information based on the specified strategy.
/// </summary>
Expand Down Expand Up @@ -55,18 +49,14 @@
return mergedFrontmatter;
}

// Not cached: the canonical name depends on the strategy and, for Aggressive and Maximum, on the
// other keys in the same document. A cache keyed on the property name alone let whichever call
// reached a key first decide its fate for every later call in the process.
private static string GetCanonicalName(string key, FrontmatterMergeStrategy strategy, string[] frontmatterKeys)
{
// Try to find in cache first
if (PropertyMergeCache.TryGetValue(key, out string? cachedName))
{
return cachedName;
}

// For None strategy or keys with special characters, preserve the original key
if (strategy == FrontmatterMergeStrategy.None || key.Any(c => !char.IsLetterOrDigit(c) && c != '_' && c != '-'))
{
PropertyMergeCache.TryAdd(key, key);
return key;
}

Expand All @@ -80,13 +70,7 @@
};

// 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) =>
Expand All @@ -103,7 +87,7 @@

return !isKnownMapping && !hasExactMatch && !isInSameCategory
? key
: PropertyMappings.All.TryGetValue(canonicalName, out string? knownName) ? knownName : canonicalName;

Check warning on line 90 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Extract this nested ternary operation into an independent statement.

Check warning on line 90 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Extract this nested ternary operation into an independent statement.

Check warning on line 90 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Extract this nested ternary operation into an independent statement.

Check warning on line 90 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Extract this nested ternary operation into an independent statement.

Check warning on line 90 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Extract this nested ternary operation into an independent statement.

Check warning on line 90 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Extract this nested ternary operation into an independent statement.
}

private static bool IsInSameCategory(string key, string canonicalName)
Expand Down Expand Up @@ -197,7 +181,7 @@
/// <param name="key">The key to analyze.</param>
/// <param name="existingKeys">All existing keys in the frontmatter.</param>
/// <returns>The canonical name for the key.</returns>
private static string FindBasicCanonicalName(string key, string[] existingKeys)

Check warning on line 184 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

Check warning on line 184 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

Check warning on line 184 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

Check warning on line 184 in Frontmatter/PropertyMerger.cs

View workflow job for this annotation

GitHub Actions / Analyze & Release

Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.
{
// First check if it's a known property
if (PropertyMappings.All.TryGetValue(key, out string? canonicalName))
Expand Down
Loading