Skip to content

feat: Refactor MigrationManager to accept migrations as a parameter #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
linkdotnet merged 3 commits into from
Mar 14, 2026
Merged

feat: Refactor MigrationManager to accept migrations as a parameter #503

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5aaf222
feat: Refactor MigrationManager to accept migrations as a parameter
linkdotnet Mar 11, 2026
faf910b
fix: Styling
linkdotnet Mar 11, 2026
821f98e
fix: Explicit version
linkdotnet Mar 11, 2026
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
2 changes: 2 additions & 0 deletions tests/LinkDotNet.Blog.UpgradeAssistant.Tests/GlobalUsings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
global using System;
global using System.IO;
global using System.Threading.Tasks;
global using LinkDotNet.Blog.UpgradeAssistant;
global using LinkDotNet.Blog.UpgradeAssistant.Migrations;
global using Shouldly;
global using Xunit;
12 changes: 6 additions & 6 deletions tests/LinkDotNet.Blog.UpgradeAssistant.Tests/MigrationManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public async Task Should_Migrate_From_11_To_12()
}
""";
await File.WriteAllTextAsync(testFile, json, TestContext.Current.CancellationToken);
var manager = new MigrationManager();
var manager = new MigrationManager([new Migration11To12()]);
var backupDir = Path.Combine(testDirectory, "backups");

// Act
Expand Down Expand Up @@ -53,7 +53,7 @@ public async Task Should_Not_Modify_Already_Migrated_File()
}
""";
await File.WriteAllTextAsync(testFile, json, TestContext.Current.CancellationToken);
var manager = new MigrationManager();
var manager = new MigrationManager([new Migration11To12()]);
var backupDir = Path.Combine(testDirectory, "backups");

// Act
Expand All @@ -76,7 +76,7 @@ public async Task Should_Skip_Version_Controlled_Appsettings_Json()
}
""";
await File.WriteAllTextAsync(testFile, json, TestContext.Current.CancellationToken);
var manager = new MigrationManager();
var manager = new MigrationManager([new Migration11To12()]);
var backupDir = Path.Combine(testDirectory, "backups");

// Act
Expand All @@ -95,7 +95,7 @@ public async Task Should_Handle_Invalid_Json()
// Arrange
var testFile = Path.Combine(testDirectory, "appsettings.Invalid.json");
await File.WriteAllTextAsync(testFile, "{ invalid json }", TestContext.Current.CancellationToken);
var manager = new MigrationManager();
var manager = new MigrationManager([new Migration11To12()]);
var backupDir = Path.Combine(testDirectory, "backups");

// Act
Expand All @@ -110,7 +110,7 @@ public async Task Should_Handle_Missing_File()
{
// Arrange
var testFile = Path.Combine(testDirectory, "nonexistent.json");
var manager = new MigrationManager();
var manager = new MigrationManager([new Migration11To12()]);
var backupDir = Path.Combine(testDirectory, "backups");

// Act
Expand All @@ -131,7 +131,7 @@ public async Task DryRun_Should_Not_Modify_File()
}
""";
await File.WriteAllTextAsync(testFile, json, TestContext.Current.CancellationToken);
var manager = new MigrationManager();
var manager = new MigrationManager([new Migration11To12()]);
var backupDir = Path.Combine(testDirectory, "backups");

// Act
Expand Down
14 changes: 14 additions & 0 deletions tools/LinkDotNet.Blog.UpgradeAssistant/MigrationDiscovery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
namespace LinkDotNet.Blog.UpgradeAssistant;

public static class MigrationDiscovery
{
public static IReadOnlyList<IMigration> DiscoverAll()
{
return typeof(IMigration).Assembly
.GetTypes()
.Where(t => t is { IsClass: true, IsAbstract: false }
&& typeof(IMigration).IsAssignableFrom(t))
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiscoverAll() currently returns migrations in whatever order Assembly.GetTypes() happens to yield. If you ever end up with multiple migrations that share the same FromVersion (or you rely on deterministic ordering for display/logging), this can lead to nondeterministic behavior across runtimes/builds. Consider ordering the discovered migrations deterministically (e.g., by parsed FromVersion then ToVersion, or by an explicit Order property).

Suggested change
&& typeof(IMigration).IsAssignableFrom(t))
&& typeof(IMigration).IsAssignableFrom(t))
.OrderBy(t => t.FullName, StringComparer.Ordinal)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior is introduced in MigrationDiscovery.DiscoverAll() (reflection-based migration enumeration), but there’s no unit test asserting which migrations are discovered and in what order. Since the rest of this tool has test coverage around migrations and the manager, consider adding a test that verifies discovery returns the expected migration set (and any ordering guarantees you choose).

Suggested change
&& typeof(IMigration).IsAssignableFrom(t))
&& typeof(IMigration).IsAssignableFrom(t))
.OrderBy(t => t.FullName)

Copilot uses AI. Check for mistakes.
.Select(t => (IMigration)Activator.CreateInstance(t)!)
.ToList();
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiscoverAll() instantiates every IMigration type via reflection without filtering for a public parameterless constructor. If an IMigration implementation is internal or requires ctor parameters, Activator.CreateInstance will throw at runtime and the tool won’t start. Consider filtering to types with an accessible parameterless constructor (or using the overload that allows non-public) and surfacing a clear error message when instantiation fails.

Suggested change
return typeof(IMigration).Assembly
.GetTypes()
.Where(t => t is { IsClass: true, IsAbstract: false }
&& typeof(IMigration).IsAssignableFrom(t))
.Select(t => (IMigration)Activator.CreateInstance(t)!)
.ToList();
var migrationTypes = typeof(IMigration).Assembly
.GetTypes()
.Where(t => t is { IsClass: true, IsAbstract: false }
&& typeof(IMigration).IsAssignableFrom(t)
&& t.GetConstructor(Type.EmptyTypes) is not null)
.ToList();
var migrations = new List<IMigration>(migrationTypes.Count);
foreach (var type in migrationTypes)
{
try
{
var instance = Activator.CreateInstance(type);
if (instance is IMigration migration)
{
migrations.Add(migration);
}
else
{
throw new InvalidOperationException(
$"Type '{type.FullName}' was expected to implement '{typeof(IMigration).FullName}' but did not.");
}
}
catch (Exception ex)
{
throw new InvalidOperationException(
$"Failed to create an instance of migration type '{type.FullName}'. " +
"Ensure it has a public parameterless constructor and that its constructor does not throw.",
ex);
}
}
return migrations;

Copilot uses AI. Check for mistakes.
}
}
9 changes: 2 additions & 7 deletions tools/LinkDotNet.Blog.UpgradeAssistant/MigrationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using System.Globalization;
using System.Text.Json;
using System.Text.Json.Nodes;
using LinkDotNet.Blog.UpgradeAssistant.Migrations;

namespace LinkDotNet.Blog.UpgradeAssistant;

Expand All @@ -10,13 +9,9 @@ public sealed class MigrationManager
private readonly List<IMigration> _migrations;
private readonly string _currentVersion;

public MigrationManager()
public MigrationManager(IEnumerable<IMigration> migrations)
{
_migrations = new List<IMigration>
{
new Migration11To12()
};

_migrations = migrations.ToList();
_currentVersion = DetermineCurrentVersionFromMigrations();
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MigrationManager now accepts an external migrations sequence, but the constructor doesn’t validate migrations for null. That will currently throw a NullReferenceException at migrations.ToList(). Consider adding an ArgumentNullException guard so callers get a clear error and the failure mode is consistent.

Copilot uses AI. Check for mistakes.
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DetermineCurrentVersionFromMigrations() uses Max(m => m.ToVersion) on strings, which is a lexicographic comparison and can produce an incorrect “latest” version once you have multiple migrations (e.g., "9.0" sorts after "12.0"). Since this constructor now takes an arbitrary set of migrations, consider parsing versions (e.g., Version/semver parsing) and taking the max of the parsed values instead.

Copilot uses AI. Check for mistakes.

Expand Down
2 changes: 1 addition & 1 deletion tools/LinkDotNet.Blog.UpgradeAssistant/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ static async Task<int> RunWithOptions(CommandLineOptions options)
ConsoleOutput.WriteWarning("Running in DRY RUN mode - no changes will be saved.");
}

var manager = new MigrationManager();
var manager = new MigrationManager(MigrationDiscovery.DiscoverAll());
var files = GetAppsettingsFiles(targetPath);

if (files.Count == 0)
Expand Down
Loading