Double database connexion validation in SetupService

[TFleury]

It seems to me that there is a double database connexion validation in SetupService.

There is the IDbConnectionValidator.ValidateAsync method

OrchardCore/src/OrchardCore/OrchardCore.Setup.Core/SetupService.cs

Lines 170 to 184 in 2b1771f

	switch (await _dbConnectionValidator.ValidateAsync(shellSettings["DatabaseProvider"], shellSettings["ConnectionString"], shellSettings["TablePrefix"], shellSettings.Name))
	{
	case DbConnectionValidatorResult.NoProvider:
	context.Errors.Add(String.Empty, S["DatabaseProvider setting is required."]);
	break;
	case DbConnectionValidatorResult.UnsupportedProvider:
	context.Errors.Add(String.Empty, S["The provided database provider is not supported."]);
	break;
	case DbConnectionValidatorResult.InvalidConnection:
	context.Errors.Add(String.Empty, S["The provided connection string is invalid or server is unreachable."]);
	break;
	case DbConnectionValidatorResult.DocumentTableFound:
	context.Errors.Add(String.Empty, S["The provided database and table prefix are already in use."]);
	break;
	}

And also this piece of code :

OrchardCore/src/OrchardCore/OrchardCore.Setup.Core/SetupService.cs

Lines 201 to 223 in 2b1771f

	using (var shellContext = await _shellContextFactory.CreateDescribedContextAsync(shellSettings, shellDescriptor))
	{
	await shellContext.CreateScope().UsingServiceScopeAsync(async scope =>
	{
	IStore store;
	try
	{
	store = scope.ServiceProvider.GetRequiredService<IStore>();
	}
	catch (Exception e)
	{
	// Tables already exist or database was not found
	// The issue is that the user creation needs the tables to be present,
	// if the user information is not valid, the next POST will try to recreate the
	// tables. The tables should be rolled back if one of the steps is invalid,
	// unless the recipe is executing?
	_logger.LogError(e, "An error occurred while initializing the datastore.");
	context.Errors.Add(String.Empty, S["An error occurred while initializing the datastore: {0}", e.Message]);
	return;
	}

Isn't this the same verification ?

The idea behind is to remove the dependency over YesSql.

[Skrypt]

I think the second one will throw an exception instead of just testing if it actually works before moving forward.

[TFleury]

They both add an error and return.

[Skrypt]

True, sorry I've read that code too quickly. Then we need to move the validations all in one place. I believe that the issue is that our DBConnectionValidator will not test if the Store was properly instantiated. This is why it is doing GetRequiredService.