Use database function to set the value of AppliedOn

[nathanpovo]

Use a database function to set the value of the AppliedOn column of the VersionInfo table whenever the version info is updated.

Previously, the value of AppliedOn would be decided when the migrations are generated and then hardcoded into the SQL script.

This is not an issue when the database is updated directly but it is a problem when updating the database through SQL scripts.

---

Before

INSERT INTO [dbo].[VersionInfo] ([Version], [AppliedOn], [Description]) VALUES (20240101010101, '2024-07-30T09:27:42', N'MyMigrationName');

After

INSERT INTO [dbo].[VersionInfo] ([Version], [AppliedOn], [Description]) VALUES (20240101010101, GETUTCDATE(), N'MyMigrationName');

---

Fixes #1846

[jzabroski]

The extension method to GetQuoter would not work for all generators. I don't want to support that. Why do you need it on the first place?

[jzabroski]

In general, I agree we need to support this but wanted to suggest some changes.

1. This is a breaking change. Should this be hidden behind a feature flag? As it stands, the quoter will never be null since it's injected now. So, using the presence of a quoter as a feature flag is not a robust approach.

2. Remove the extension method GetQuoter. You don't need it. Use dependency injection and injection the Quoter in the constructor. (Getting the quoter through the generator is a violation of the Law of Demeter, and a sign the code will be quite brittle to extension.)

3. Not every provider supports utc time. It's not clear to me that this will work across every single database provider. But as it's connectionless, it should be easy to at least run and observe what it generates for each ProviderId.

[nathanpovo]

Thanks for taking the time to go through this.

Yesterday I wrote a lengthy comment responding to your feedback and it seems to have disappeared.

---

The extension method to GetQuoter would not work for all generators. I don't want to support that. Why do you need it on the first place?

Remove the extension method GetQuoter. You don't need it. Use dependency injection and injection the Quoter in the constructor.

The extension method is necessary because of 2 reasons:

1. Each database provider implements its own implementation of IQuoter which means that there might be multiple IQuoters in the dependency injection container.
2. Implementations of IQuoter are registered as their concrete type in the dependency injection container.

You can see that this is the case by looking at the code of AddFluentMigratorCore

fluentmigrator/src/FluentMigrator.Runner.Core/FluentMigratorServiceCollectionExtensions.cs

Lines 52 to 180 in 4026fc1

	public static IServiceCollection AddFluentMigratorCore(
	[NotNull] this IServiceCollection services)
	{
	if (services == null)
	throw new ArgumentNullException(nameof(services));
	services
	// Add support for options
	.AddOptions()
	// Add loggins support
	.AddLogging()
	// The default assembly loader factory
	.AddSingleton<AssemblyLoaderFactory>()
	// Assembly loader engines
	.AddSingleton<IAssemblyLoadEngine, AssemblyNameLoadEngine>()
	.AddSingleton<IAssemblyLoadEngine, AssemblyFileLoadEngine>()
	// Defines the assemblies that are used to find migrations, profiles, maintenance code, etc...
	.AddSingleton<IAssemblySource, AssemblySource>()
	// Configure the loader for migrations that should be executed during maintenance steps
	.AddSingleton<IMaintenanceLoader, MaintenanceLoader>()
	// Add the default embedded resource provider
	.AddSingleton<IEmbeddedResourceProvider>(sp => new DefaultEmbeddedResourceProvider(sp.GetRequiredService<IAssemblySource>().Assemblies))
	// Configure the runner conventions
	.AddSingleton<IMigrationRunnerConventionsAccessor, AssemblySourceMigrationRunnerConventionsAccessor>()
	.AddSingleton(sp => sp.GetRequiredService<IMigrationRunnerConventionsAccessor>().MigrationRunnerConventions)
	// The IStopWatch implementation used to show query timing
	.AddSingleton<IStopWatch, StopWatch>()
	// Source for migrations
	#pragma warning disable 618
	.AddScoped<IMigrationSource, MigrationSource>()
	.AddScoped(
	sp => sp.GetRequiredService<IMigrationSource>() as IFilteringMigrationSource
	?? ActivatorUtilities.CreateInstance<MigrationSource>(sp))
	#pragma warning restore 618
	// Source for profiles
	.AddScoped<IProfileSource, ProfileSource>()
	// Configure the accessor for the convention set
	.AddScoped<IConventionSetAccessor, AssemblySourceConventionSetAccessor>()
	// The default set of conventions to be applied to migration expressions
	.TryAddScoped(sp =>
	sp.GetRequiredService<IConventionSetAccessor>().GetConventionSet()
	?? ActivatorUtilities.CreateInstance<DefaultConventionSet>(sp)
	);
	services
	// Configure the accessor for the version table metadata
	.AddScoped<IVersionTableMetaDataAccessor, AssemblySourceVersionTableMetaDataAccessor>()
	// Configure the default version table metadata
	.AddScoped(sp => sp.GetRequiredService<IVersionTableMetaDataAccessor>().VersionTableMetaData ?? ActivatorUtilities.CreateInstance<DefaultVersionTableMetaData>(sp))
	// Configure the migration information loader
	.AddScoped<IMigrationInformationLoader, DefaultMigrationInformationLoader>()
	// Provide a way to get the migration generator selected by its options
	.AddScoped<IGeneratorAccessor, SelectingGeneratorAccessor>()
	// Provide a way to get the migration accessor selected by its options
	.AddScoped<IProcessorAccessor, SelectingProcessorAccessor>()
	// IQuerySchema is the base interface for the IMigrationProcessor
	.AddScoped<IQuerySchema>(sp => sp.GetRequiredService<IProcessorAccessor>().Processor)
	// The profile loader needed by the migration runner
	.AddScoped<IProfileLoader, ProfileLoader>()
	// Some services especially for the migration runner implementation
	.AddScoped<IMigrationExpressionValidator, DefaultMigrationExpressionValidator>()
	.AddScoped<MigrationValidator>()
	.AddScoped<MigrationScopeHandler>()
	// The connection string readers
	#if NETFRAMEWORK
	.AddScoped<INetConfigManager, NetConfigManager>()
	#pragma warning disable 612
	.AddScoped<IConnectionStringReader, AppConfigConnectionStringReader>()
	#pragma warning restore 612
	#endif
	.AddScoped<IConnectionStringReader, ConfigurationConnectionStringReader>()
	// The connection string accessor that evaluates the readers
	.AddScoped<IConnectionStringAccessor, ConnectionStringAccessor>()
	.AddScoped<IVersionLoader>(
	sp =>
	{
	var options = sp.GetRequiredService<IOptions<RunnerOptions>>();
	var connAccessor = sp.GetRequiredService<IConnectionStringAccessor>();
	var hasConnection = !string.IsNullOrEmpty(connAccessor.ConnectionString);
	if (options.Value.NoConnection || !hasConnection)
	{
	return ActivatorUtilities.CreateInstance<ConnectionlessVersionLoader>(sp);
	}
	return ActivatorUtilities.CreateInstance<VersionLoader>(sp);
	})
	// Configure the runner
	.AddScoped<IMigrationRunner, MigrationRunner>()
	// Configure the task executor
	.AddScoped<TaskExecutor>()
	// Migration context
	.AddTransient<IMigrationContext>(
	sp =>
	{
	var querySchema = sp.GetRequiredService<IQuerySchema>();
	var options = sp.GetRequiredService<IOptions<RunnerOptions>>();
	var connectionStringAccessor = sp.GetRequiredService<IConnectionStringAccessor>();
	var connectionString = connectionStringAccessor.ConnectionString;
	return new MigrationContext(querySchema, sp, connectionString);
	});
	return services;
	}

and the builders of the database providers:

fluentmigrator/src/FluentMigrator.Runner.SqlServer/SqlServerRunnerBuilderExtensions.cs

Lines 40 to 52 in 4026fc1

	public static IMigrationRunnerBuilder AddSqlServer(this IMigrationRunnerBuilder builder)
	{
	builder.Services.TryAddTransient<SqlServerBatchParser>();
	builder.Services.TryAddScoped<SqlServer2008Quoter>();
	builder.Services.TryAddScoped<ISqlServerTypeMap>(sp => new SqlServer2008TypeMap());
	builder.Services
	.AddScoped<SqlServer2016Processor>()
	.AddScoped<IMigrationProcessor>(sp => sp.GetRequiredService<SqlServer2016Processor>())
	.AddScoped<SqlServer2016Generator>()
	.AddScoped<IMigrationGenerator>(sp => sp.GetRequiredService<SqlServer2016Generator>());
	return builder;
	}

fluentmigrator/src/FluentMigrator.Runner.Postgres/PostgresRunnerBuilderExtensions.cs

Lines 118 to 130 in 4026fc1

	private static IMigrationRunnerBuilder AddCommonPostgresServices(this IMigrationRunnerBuilder builder)
	{
	builder.Services
	.AddScoped(
	sp =>
	{
	var processorOptions = sp.GetRequiredService<IOptionsSnapshot<ProcessorOptions>>();
	return PostgresOptions.ParseProviderSwitches(processorOptions.Value.ProviderSwitches);
	})
	.AddScoped<PostgresDbFactory>()
	.AddScoped<PostgresQuoter>();
	return builder;
	}

fluentmigrator/src/FluentMigrator.Runner.Oracle/OracleRunnerBuilderExtensions.cs

Lines 37 to 47 in 4026fc1

	private static void RegisterOracleQuoter(IMigrationRunnerBuilder builder)
	{
	builder.Services.TryAddScoped<OracleQuoterBase>(
	sp =>
	{
	var opt = sp.GetRequiredService<IOptionsSnapshot<ProcessorOptions>>();
	return opt.Value.IsQuotingForced() ?
	new OracleQuoterQuotedIdentifier() :
	new OracleQuoter();
	});
	}

This means that there are no registrations of IQuoter in the dependency injection container. And, even if there were, there would be multiple registrations of IQuoter in the dependency injection container.

This can be tested out quickly by running the example console application:

What I originally wanted to do, was add a property to IMigrationProcessor that would return the IQuoter implementation that the IMigrationProcessor uses. But I ran into issues here because the project that contains IMigrationProcessor has no reference to the project that contains IQuoter.

I did not wish to add any new references between projects, so I settled on the extension method.

---

This is a breaking change. Should this be hidden behind a feature flag?

I see this as a bug fix since the scripts were not being generated correctly.

However, I understand that users might have relied on the previous behaviour.

Could this new behaviour be based on a configuration option in RunnerOptions? It could default to false to maintain the previous behaviour. In a future release, the default could be changed to true.

---

Not every provider supports utc time. It's not clear to me that this will work across every single database provider.

Based on the database providers in the code, all IQuoter implementations that override FormatSystemMethods support SystemMethods.CurrentUTCDateTime

fluentmigrator/src/FluentMigrator.Runner.Core/Generators/Generic/GenericQuoter.cs

Lines 114 to 117 in 4026fc1

	public virtual string FormatSystemMethods(SystemMethods value)
	{
	throw new NotSupportedException($"The system method {value} is not supported.");
	}

[jzabroski]

That's a really good reason why you did it this way. I am trying to think of a way we can just inject it. I made each generator take an explicit dependency on purpose for this and type maps.

Injecting it likely has other unforeseen benefits.

What we really (probably) want is a tiny service locator that can get all generator dependencies as a GeneratorContext. (I'm almost done with a tiny PR to add an ISeparator interface to formalize the concepts of statements and batches, which will further help offline scripting)

I need to think about this more. If I can't arrive at a better solution, I guess we will use your approach.

[jzabroski]

I'm going to try to merge this tonight. Just released 6.0, will put this in 6.1. Thanks for your patience. Just had a newborn baby boy in August and just starting to get good nights sleep for the first time in about two months.

[nathanpovo]

Congrats on the baby!

Absolutely not an issue. Take all the time that you need.

I worked around the issue in my own project so there is no rush on my side. The change request was a means for me to contribute something to the library that others might be able to make use of.

[jzabroski]

@nathanpovo As an update and late change request, I believe there is already infrastructure to get the generator easily - see IGeneratorAccessor. Can you use that?

[nathanpovo]

I believe there is already infrastructure to get the generator easily - see IGeneratorAccessor. Can you use that?

I checked what code would need to be modified for this to be done, and the constructors of the ConnectionlessVersionLoader and VersionLoader classes will need to be changed with an additional parameter for IGeneratorAccessor so that it is injected.

Is it fine to modify the constructors? I have done the modification, we can reset it back if the constructors should not be modified.

[jzabroski]

I think this change makes sense, even if by some opinion its technical debt. We can always obsolete those constructors later when a better design comes along.

[jzabroski]

@nathanpovo It looks like some tests failed - can you take a look?

[nathanpovo]

Sorry, didn't notice the failing tests. They have been fixed.

[jzabroski]

Looks great. I will do one last read through the code this weekend and then merge it for 6.3.0 assuming I dont catch anything, which I doubt I will.

[jzabroski]

@nathanpovo One last thought - should IMigrationGenerator expose the IQuoter directly? I am trying to think of a reason not to and can't think of one. I'd prefer that to the cast to GeneratorBase, as someone could in theory write their own Generator without that.

[nathanpovo]

That's kind of what I had wanted to do originally (but have IMigrationProcessor expose IQuoter).

Unfortunately, it's not possible because IMigrationGenerator currently does not have access to IQuoter. We'd need to add a reference to the FluentMigrator.Runner.Core project in the FluentMigrator.Abstractions project.

Or move IQuoter to the FluentMigrator.Abstractions project.

Both of which are breaking changes.

[jzabroski]

Right, sorry, new parent brain, forgot we discussed this. I'll merge this and add a ticket to clean this up.