Skip to content

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

Open
linkdotnet wants to merge 3 commits intomasterfrom
plugin-and-play-migration
Open

feat: Refactor MigrationManager to accept migrations as a parameter#503
linkdotnet wants to merge 3 commits intomasterfrom
plugin-and-play-migration

Conversation

@linkdotnet
Copy link
Owner

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the Upgrade Assistant’s migration pipeline so MigrationManager no longer hardcodes migrations, and instead accepts an injected list discovered at runtime.

Changes:

  • Update MigrationManager to accept IEnumerable<IMigration> via constructor instead of constructing migrations internally.
  • Add MigrationDiscovery to discover and instantiate all IMigration implementations via reflection.
  • Update CLI entrypoint and tests to pass migrations into MigrationManager.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/LinkDotNet.Blog.UpgradeAssistant/Program.cs Construct MigrationManager with discovered migrations.
tools/LinkDotNet.Blog.UpgradeAssistant/MigrationManager.cs Accept migrations via constructor; remove hardcoded migration list.
tools/LinkDotNet.Blog.UpgradeAssistant/MigrationDiscovery.cs New reflection-based discovery/instantiation of migrations.
tests/LinkDotNet.Blog.UpgradeAssistant.Tests/MigrationManagerTests.cs Update tests to pass migrations explicitly.
tests/LinkDotNet.Blog.UpgradeAssistant.Tests/GlobalUsings.cs Add global usings for the tool and migrations namespaces.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +7 to +12
return typeof(IMigration).Assembly
.GetTypes()
.Where(t => t is { IsClass: true, IsAbstract: false }
&& typeof(IMigration).IsAssignableFrom(t))
.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.
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.
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.

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.
Comment on lines 12 to 15
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.
Comment on lines 14 to 16
_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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants