- 
                Notifications
    You must be signed in to change notification settings 
- Fork 715
Create database for Sql Server resource #8022
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds support for creating a database for the Sql Server resource with the option to run a custom creation script. Key changes include:
- Adding a new ScriptAnnotation class used to attach custom creation scripts.
- Updating tests to verify database creation and custom script execution.
- Refactoring extension methods to support improved database resource registration.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description | 
|---|---|
| src/Aspire.Hosting/ApplicationModel/ScriptAnnotation.cs | Introduces the ScriptAnnotation class to encapsulate custom script logic. | 
| tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs | Updates tests to use explicit database names and verify custom script functionality. | 
| src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs | Refactors the AddDatabase extension to use a database resource instance; however, a faulty dictionary initialization is observed. | 
| src/Aspire.Hosting.SqlServer/SqlServerServerResource.cs | Adjusts database resource addition to use a list and dictionary; the list initialization syntax appears incorrect. | 
| playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/Program.cs | Comments out a setup block related to an external project dependency. | 
        
          
                tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      |  | ||
| var sqlserver1 = builder1.AddSqlServer("sqlserver"); | ||
| var masterdb1 = sqlserver1.AddDatabase("master"); | ||
| var db1 = sqlserver1.AddDatabase(databaseName); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using master updates this test to ensure the database is created. master was used since it's a default one in sql server.
| [RequiresDocker] | ||
| public async Task AddDatabaseCreatesDatabaseWithSpecialNames() | ||
| { | ||
| const string databaseName = "!'][\""; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt this one might itch a bit. This was to ensure the name is correctly encoded in the default script. But I needed to change the connection string creation to encode it too.
        
          
                tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | public ReferenceExpression ConnectionStringExpression { | ||
| get | ||
| { | ||
| var connectionStringBuilder = new SqlConnectionStringBuilder(); | ||
| connectionStringBuilder["Database"] = DatabaseName; | ||
| var connectionString = connectionStringBuilder.ToString(); | ||
|  | ||
| return ReferenceExpression.Create($"{Parent};{connectionString}"); | ||
| } | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting. Why the change?
There was a problem hiding this comment.
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.
| This generally looks good; I wonder if we should also think about publish mode (maybe not in this PR). | 
| 
 What does this mean exactly? For Azure SQL, we will create the DB with bicep. Are you thinking of some sort of "startup" command that runs on publish of a SQL container? | 
| /// <summary> | ||
| /// Represents an annotation for defining a script to create a resource. | ||
| /// </summary> | ||
| public sealed class ScriptAnnotation : IResourceAnnotation | 
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|  | ||
| using var command = sqlConnection.CreateCommand(); | ||
| command.CommandText = scriptAnnotation?.Script ?? | ||
| $"IF ( NOT EXISTS ( SELECT 1 FROM sys.databases WHERE name = @DatabaseName ) ) CREATE DATABASE {quotedDatabaseIdentifier};"; | 
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
| catch (Exception e) | ||
| { | ||
| @event.Services.GetRequiredService<ILogger<SqlServerServerResource>>().LogError(e, "Failed to create database '{DatabaseName}'", sqlDatabase.DatabaseName); | 
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also update the playground app to no longer ensure the DB is created?
aspire/playground/SqlServerEndToEnd/SqlServerEndToEnd.ApiService/Program.cs
Lines 19 to 23 in 8d2aac4
| // You wouldn't normally do this on every call, | |
| // but doing it here just to make this simple. | |
| await db1Context.Database.EnsureCreatedAsync(); | |
| await db2Context.Database.EnsureCreatedAsync(); | 
| /// <summary> | ||
| /// Represents an annotation for defining a script to create a resource. | ||
| /// </summary> | ||
| public sealed class ScriptAnnotation : IResourceAnnotation | 
There was a problem hiding this comment.
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:
- Does this feature work well with this example?
- 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).
There was a problem hiding this comment.
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.
| Currently adding  -- Create the AddressBook database
IF NOT EXISTS (SELECT * FROM sys.databases WHERE name = N'AddressBook')
BEGIN
  CREATE DATABASE AddressBook;
END;
GO
USE AddressBook;
GO
-- Create the Contacts table
IF OBJECT_ID(N'Contacts', N'U') IS NULL
BEGIN
    CREATE TABLE Contacts
    (
        Id        INT PRIMARY KEY IDENTITY(1,1) ,
        FirstName VARCHAR(255) NOT NULL,
        LastName  VARCHAR(255) NOT NULL,
        Email     VARCHAR(255) NULL,
        Phone     VARCHAR(255) NULL
    );
END;
GO | 
Co-authored-by: Dan Moseley <[email protected]>
| } | ||
| catch (Exception e) | ||
| { | ||
| var logger = serviceProvider.GetRequiredService<ILogger<DistributedApplicationBuilder>>(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to update this to write the log to the resource instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just got taught how to, yes I will change that right away.
| @sebastienros This is amazing, love the script support. Together with a SQL project, This allows full lifecycle support for a persisted local database. | 
| @sebastienros @davidfowl When will this be in the latest "daily" ??  Not in  | 
| It should be there. | 
| @davidfowl Found it - I will show myself out! | 
Description
Support database creation for
AddDatabasein Sql Server container resource. If the database already exists nothing is done. Custom scripts can be provided, using an annotation internally.Part of #7101
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):