Skip to content
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

Testcontainers.MsSql: WithDatabase to public #855

Closed

Conversation

Jejuni
Copy link
Contributor

@Jejuni Jejuni commented Mar 29, 2023

What does this PR do?

Changed visibility of WithDatabase from private to public for the MsSqlBuilder.
Also updated WaitUntil to always target the master database.

Why is it important?

With the methods being private there is no way to use the builder to set a database.
This was easily accomplishable in v2 via new MsSqlTestcontainerConfiguration { Password = "123", Database = "MyDb" }

In case of testing with ORMs (like EF Core) it can be beneficial to be able to specify a non-existing database in the connection string so that migrations can create the database.

Alternative

Calling WithDatabase will now succeed. However, calling ExecScriptAsync will fail, unless the database was created beforehand in some capacity.

An alternative that could work as well is to update the GetConnectionString() to accept an optional parameter for the database like GetConnectionString(string? database = null) to then return a connection string targeting either the given database (if not null) or simply the database according to configuration (always master).

@netlify
Copy link

netlify bot commented Mar 29, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 4c13b28
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/642413930c0c030007431810
😎 Deploy Preview https://deploy-preview-855--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Jejuni Jejuni changed the title Testcontainers.MsSql: WithDatabase and WithUsername to public Testcontainers.MsSql: WithDatabase to public Mar 29, 2023
@HofmeisterAn
Copy link
Collaborator

As explained in the XML docs and in #828 the Microsoft SQL Server image does not support configuring a database or user by default. I am certain your changes do not what you expect.

@Jejuni
Copy link
Contributor Author

Jejuni commented Mar 30, 2023

Sorry, I didn't see there was a discussion about this already. I only checked the open issues.

The change does fix my usage scenario, but I will admit it isn't ideal, as other usage scenarios are impacted as well (i.e. ExecScriptAsync, as described above).

All this could be fixed, of course, but as stated in the discussion you linked it comes down more to what you want the libraries to represent.
I just figured since it was in v2 it would make sense in v3.
But I also feel like both approaches have merit:

  • having the libraries be simple wrappers around common infrastructure components only passing on what the original authors intended
  • providing convenience on top of what he original image allows, as all the image specific libraries are basically already convenience on top of the core library

@HofmeisterAn
Copy link
Collaborator

The change does fix my usage scenario, but I will admit it isn't ideal, as other usage scenarios are impacted as well (i.e. ExecScriptAsync, as described above).

Probably I expressed myself wrong. The changes do not provide what developers generally expect. Based on other discussions I have had, developers expect that calling WithDatabase(string) will create the database.

In case of testing with ORMs (like EF Core) it can be beneficial to be able to specify a non-existing database in the connection string so that migrations can create the database.

Is it built into EF? I have never used it in that way before and I am curious about the configuration. Typically, I use a configuration that includes two database connections - one for the master (which creates the necessary database) and one for the actual database.

I would prefer an MSSQL configuration that overrides the entrypoint and allows the execution of custom scripts on startup (similar to what I suggest in #828 (comment), but built-in into Testcontainers). Other developers can use the same mechanism to customise the startup even further.

An alternative that could work as well is to update the GetConnectionString() to accept an optional parameter [...]

Having a mechanism to override or customize the connection string would be great. Unfortunately, I have not been able to come up with a good API yet that would work for other databases too. On the other hand, developers can simply use the SqlConnectionStringBuilder to parse and override the provided connection string. We do not want to include this type in Testcontainers to avoid third-party dependency clashes and forcing developers to use a certain version of a dependency.

But I also feel like both approaches have merit:

We are highly interested in the community's opinion, which is why we have created this discussion here.

@HofmeisterAn
Copy link
Collaborator

As mentioned in the comment above, I don't think this PR does what developers usually expect. It might be more confusing since it doesn't create the database, although Entity Framework does that or is capable of it, as you mentioned and as I referred to here as well.

One suitable approach would be to modify the connection string in the web application factory and let EF deal with creating the database. Additionally, having a good mechanism to modify or override the connection string would be great.

For now, I will close this issue. I really appreciate your efforts! Please don't hesitate to reopen it again if you think there's more to discuss regarding this specific PR. I'm happy to follow up on this PR with a discussion about how we can improve the MSSQL module in general (maybe together with other database modules).

@flensrocker
Copy link

For future visitors: I use the SqlConnectionBuilder for this:

public static string GetConnectionString(this MsSqlContainer container, string databasename)
{
  var connectionString = container.GetConnectionString();
  var csBuilder = new Microsoft.Data.SqlClient.SqlConnectionStringBuilder(connectionString);
  // If you use System.Data.SqlClient:
  // var csBuilder = new System.Data.SqlClient.SqlConnectionStringBuilder(connectionString);
  csBuilder.InitialCatalog = databasename;
  return csBuilder.ConnectionString;
}

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.

3 participants