Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
82 changes: 80 additions & 2 deletions src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

using Aspire.Hosting;
using Aspire.Hosting.ApplicationModel;
using Microsoft.Data.SqlClient;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

namespace Aspire.Hosting;

Expand Down Expand Up @@ -45,6 +47,42 @@ public static IResourceBuilder<SqlServerServerResource> AddSqlServer(this IDistr
}
});

builder.Eventing.Subscribe<ResourceReadyEvent>(sqlServer, async (@event, ct) =>
{
if (connectionString is null)
{
throw new DistributedApplicationException($"ResourceReadyEvent was published for the '{sqlServer.Name}' resource but the connection string was null.");
}

using var sqlConnection = new SqlConnection(connectionString);
await sqlConnection.OpenAsync(ct).ConfigureAwait(false);

foreach (var sqlDatabase in sqlServer.DatabaseResources)
{
var quotedDatabaseIdentifier = new SqlCommandBuilder().QuoteIdentifier(sqlDatabase.DatabaseName);

try
{
if (sqlConnection.State != System.Data.ConnectionState.Open)
{
throw new InvalidOperationException($"Could not open connection to '{sqlServer.Name}'");
}

var scriptAnnotation = sqlDatabase.Annotations.OfType<ScriptAnnotation>().LastOrDefault();

using var command = sqlConnection.CreateCommand();
command.CommandText = scriptAnnotation?.Script ??
$"IF ( NOT EXISTS ( SELECT 1 FROM sys.databases WHERE name = @DatabaseName ) ) CREATE DATABASE {quotedDatabaseIdentifier};";
Copy link
Member

Choose a reason for hiding this comment

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

Why the difference between the name in the WHERE and the name used on the Create Database? Shouldn't they both use the same SqlParameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Parameters can't be used for object names (such as database names, table names, or column names).
It's used in the query part because in that case it's a string value.

command.Parameters.Add(new SqlParameter("@DatabaseName", sqlDatabase.DatabaseName));
await command.ExecuteNonQueryAsync(ct).ConfigureAwait(false);
}
catch (Exception e)
{
@event.Services.GetRequiredService<ILogger<SqlServerServerResource>>().LogError(e, "Failed to create database '{DatabaseName}'", sqlDatabase.DatabaseName);
Copy link
Member

Choose a reason for hiding this comment

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

Can we log this to resource's logger? Where does this currently log? To the console of the AppHost?

Copy link
Member Author

Choose a reason for hiding this comment

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

It currently shows up in the console. Would it be nice if it were in the database resource log in the dashboard? If so what is the way to do it?

}
}
});

var healthCheckKey = $"{name}_check";
builder.Services.AddHealthChecks().AddSqlServer(sp => connectionString ?? throw new InvalidOperationException("Connection string is unavailable"), name: healthCheckKey);

Expand Down Expand Up @@ -72,12 +110,33 @@ public static IResourceBuilder<SqlServerDatabaseResource> AddDatabase(this IReso
ArgumentNullException.ThrowIfNull(builder);
ArgumentException.ThrowIfNullOrEmpty(name);

Dictionary<string, string> databaseConnectionStrings = [];

// Use the resource name as the database name if it's not provided
databaseName ??= name;

builder.Resource.AddDatabase(name, databaseName);
var sqlServerDatabase = new SqlServerDatabaseResource(name, databaseName, builder.Resource);
return builder.ApplicationBuilder.AddResource(sqlServerDatabase);

builder.Resource.AddDatabase(sqlServerDatabase);

builder.ApplicationBuilder.Eventing.Subscribe<ConnectionStringAvailableEvent>(sqlServerDatabase, async (@event, ct) =>
{
var databaseConnectionString = await sqlServerDatabase.ConnectionStringExpression.GetValueAsync(ct).ConfigureAwait(false);

if (databaseConnectionString == null)
{
throw new DistributedApplicationException($"ConnectionStringAvailableEvent was published for the '{name}' resource but the connection string was null.");
}

databaseConnectionStrings[name] = databaseConnectionString;
});

var healthCheckKey = $"{name}_check";
builder.ApplicationBuilder.Services.AddHealthChecks().AddSqlServer(sp => databaseConnectionStrings[name] ?? throw new InvalidOperationException("Connection string is unavailable"), name: healthCheckKey);

return builder.ApplicationBuilder
.AddResource(sqlServerDatabase)
.WithHealthCheck(healthCheckKey);
}

/// <summary>
Expand Down Expand Up @@ -112,4 +171,23 @@ public static IResourceBuilder<SqlServerServerResource> WithDataBindMount(this I

return builder.WithBindMount(source, "/var/opt/mssql", isReadOnly);
}

/// <summary>
/// Alters the JSON configuration document used by the emulator.
/// </summary>
/// <param name="builder">The builder for the <see cref="SqlServerDatabaseResource"/>.</param>
/// <param name="script">The SQL script used to create the database.</param>
/// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns>
/// <remarks>
/// <value>Default script is <code>IF ( NOT EXISTS ( SELECT 1 FROM sys.databases WHERE name = @DatabaseName ) ) CREATE DATABASE [&lt;QUOTED_DATABASE_NAME%gt;];</code></value>
/// </remarks>
public static IResourceBuilder<SqlServerDatabaseResource> WithCreationScript(this IResourceBuilder<SqlServerDatabaseResource> builder, string script)
{
ArgumentNullException.ThrowIfNull(builder);
ArgumentNullException.ThrowIfNull(script);

builder.WithAnnotation(new ScriptAnnotation(script));

return builder;
}
}
13 changes: 11 additions & 2 deletions src/Aspire.Hosting.SqlServer/SqlServerDatabaseResource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using Microsoft.Data.SqlClient;

namespace Aspire.Hosting.ApplicationModel;

Expand All @@ -23,8 +24,16 @@ public class SqlServerDatabaseResource(string name, string databaseName, SqlServ
/// <summary>
/// Gets the connection string expression for the SQL Server database.
/// </summary>
public ReferenceExpression ConnectionStringExpression =>
ReferenceExpression.Create($"{Parent};Database={DatabaseName}");
public ReferenceExpression ConnectionStringExpression {
get
{
var connectionStringBuilder = new SqlConnectionStringBuilder();
connectionStringBuilder["Database"] = DatabaseName;
var connectionString = connectionStringBuilder.ToString();

return ReferenceExpression.Create($"{Parent};{connectionString}");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Formatting. Why the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

The database name was passed directly in the connection string, so if it contains special chars conflicting with the connection string format (=";) it could break the client.

In this PR we are executing a SQL script which will have to encode the database name. I created a test for that and obviously it will fail if it's not handled at this location too.


/// <summary>
/// Gets the database name.
Expand Down
8 changes: 6 additions & 2 deletions src/Aspire.Hosting.SqlServer/SqlServerServerResource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,18 @@ public ReferenceExpression ConnectionStringExpression
}

private readonly Dictionary<string, string> _databases = new(StringComparers.ResourceName);
private readonly List<SqlServerDatabaseResource> _databaseResources = [];

/// <summary>
/// A dictionary where the key is the resource name and the value is the database name.
/// </summary>
public IReadOnlyDictionary<string, string> Databases => _databases;

internal void AddDatabase(string name, string databaseName)
internal void AddDatabase(SqlServerDatabaseResource database)
{
_databases.TryAdd(name, databaseName);
_databases.TryAdd(database.Name, database.DatabaseName);
_databaseResources.Add(database);
}

internal IReadOnlyList<SqlServerDatabaseResource> DatabaseResources => _databaseResources;
}
25 changes: 25 additions & 0 deletions src/Aspire.Hosting/ApplicationModel/ScriptAnnotation.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Aspire.Hosting.ApplicationModel;

/// <summary>
/// Represents an annotation for defining a script to create a resource.
/// </summary>
public sealed class ScriptAnnotation : IResourceAnnotation
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should give this a more specific name. ScriptAnnotation sounds really general.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was torn between a database specific name or not. But in the end it's only a string. Could have been all the way to StringAnnotation, so I understand why a more specific one would make sense. If we tell users they can add one or change it. Another advantage of a specific name is that it won't conflict with another annotation that would use the same type for something else, and we would be screwed. So I agree with that: DatabaseCreationScriptAnnotation for instance.

NB: To go all the way meta we could have general annotation like KeyValuePairAnnotation which would also have a key/value ("databasecreationscript"/"create database [foo]"). But that's why we have custom type for ;)

Copy link
Member

Choose a reason for hiding this comment

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

Given the summary of the class "for defining a script to create a resource." I thought the class would be named more specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to CreationScriptAnnotation since there should be only on in a resource, whatever resource is using it, so there can't be a conflict.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about this, we also have a sample for how to customize the initial database:

https://github.com/dotnet/aspire-samples/blob/fc7aa13409821eb1d7b7ccd374b465c97dca5591/samples/DatabaseContainers/DatabaseContainers.AppHost/Program.cs#L38-L45

  1. Does this feature work well with this example?
  2. I wonder if we really need to support a custom create database script, and instead point people to this route. (possibly with an option for turning this feature off).

cc @DamianEdwards

Copy link
Member Author

Choose a reason for hiding this comment

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

As a user I would definitely prefer this extension method (and annotation) to customize the create database script than having to deal with bind mounts and understand how the container is configured. The same way we have WithDataVolume extensions. Or should we provide (in addition maybe) a method to configure this "/usr/config/entrypoint.sh" script in sql server container to make it easier?

Though I would understand if we say we skip this feature (customizing CREATE DATABASE) since there is already a way to do it in the container itself.

{
/// <summary>
/// Initializes a new instance of the <see cref="ScriptAnnotation"/> class.
/// </summary>
/// <param name="script">The script used to create the resource.</param>
public ScriptAnnotation(string script)
{
ArgumentNullException.ThrowIfNull(script);
Script = script;
}

/// <summary>
/// Gets the script used to create the resource.
/// </summary>
public string Script { get; }
}
14 changes: 7 additions & 7 deletions tests/Aspire.Hosting.SqlServer.Tests/AddSqlServerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ public async Task SqlServerDatabaseCreatesConnectionString()
var connectionStringResource = (IResourceWithConnectionString)sqlResource;
var connectionString = await connectionStringResource.GetConnectionStringAsync();

Assert.Equal("Server=127.0.0.1,1433;User ID=sa;Password=p@ssw0rd1;TrustServerCertificate=true;Database=mydb", connectionString);
Assert.Equal("{sqlserver.connectionString};Database=mydb", connectionStringResource.ConnectionStringExpression.ValueExpression);
Assert.Equal("Server=127.0.0.1,1433;User ID=sa;Password=p@ssw0rd1;TrustServerCertificate=true;Initial Catalog=mydb", connectionString);
Assert.Equal("{sqlserver.connectionString};Initial Catalog=mydb", connectionStringResource.ConnectionStringExpression.ValueExpression);
}

[Fact]
Expand Down Expand Up @@ -154,7 +154,7 @@ public async Task VerifyManifest()
expectedManifest = """
{
"type": "value.v0",
"connectionString": "{sqlserver.connectionString};Database=db"
"connectionString": "{sqlserver.connectionString};Initial Catalog=db"
}
""";
Assert.Equal(expectedManifest, dbManifest.ToString());
Expand Down Expand Up @@ -228,8 +228,8 @@ public void CanAddDatabasesWithDifferentNamesOnSingleServer()
Assert.Equal("customers1", db1.Resource.DatabaseName);
Assert.Equal("customers2", db2.Resource.DatabaseName);

Assert.Equal("{sqlserver1.connectionString};Database=customers1", db1.Resource.ConnectionStringExpression.ValueExpression);
Assert.Equal("{sqlserver1.connectionString};Database=customers2", db2.Resource.ConnectionStringExpression.ValueExpression);
Assert.Equal("{sqlserver1.connectionString};Initial Catalog=customers1", db1.Resource.ConnectionStringExpression.ValueExpression);
Assert.Equal("{sqlserver1.connectionString};Initial Catalog=customers2", db2.Resource.ConnectionStringExpression.ValueExpression);
}

[Fact]
Expand All @@ -246,7 +246,7 @@ public void CanAddDatabasesWithTheSameNameOnMultipleServers()
Assert.Equal("imports", db1.Resource.DatabaseName);
Assert.Equal("imports", db2.Resource.DatabaseName);

Assert.Equal("{sqlserver1.connectionString};Database=imports", db1.Resource.ConnectionStringExpression.ValueExpression);
Assert.Equal("{sqlserver2.connectionString};Database=imports", db2.Resource.ConnectionStringExpression.ValueExpression);
Assert.Equal("{sqlserver1.connectionString};Initial Catalog=imports", db1.Resource.ConnectionStringExpression.ValueExpression);
Assert.Equal("{sqlserver2.connectionString};Initial Catalog=imports", db2.Resource.ConnectionStringExpression.ValueExpression);
}
}
Loading