Skip to content

Conversation

@paulomorgado
Copy link
Contributor

Following up on dotnet/aspire#8237, removed the mandatory name parameter from the AddDbGate method across various builder extension classes, simplifying the method signature to only require the port parameter. The default name "dbgate" is now used internally.

Updated tests to reflect the new method signature and default naming convention, ensuring assertions check for the new default name. Adjusted environment variable handling in ConfigureDbGateContainer methods to accommodate these changes.

These updates streamline the API for adding a DbGate container resource while maintaining functionality for different database types.

Closes #<ISSUE_NUMBER>

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • New integration
    • Docs are written
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

Other information

Copilot AI review requested due to automatic review settings November 25, 2025 19:03
Copilot finished reviewing on behalf of paulomorgado November 25, 2025 19:09
Copy link
Contributor

Copilot AI left a 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 PR refactors the AddDbGate method to use a default name of "dbgate" instead of requiring a mandatory name parameter. The changes update the method signature to make the name parameter optional with a default value, and refactor how database resources configure the shared DbGate container to use resource names directly instead of numbered suffixes.

Key changes:

  • Modified AddDbGate to accept an optional name parameter with default value "dbgate"
  • Refactored MongoDB and Redis extensions to configure DbGate on a per-resource basis using resource names
  • Updated all test files to expect the new default name and environment variable patterns
  • Added WaitFor dependency to ensure DbGate waits for database resources to be ready

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/CommunityToolkit.Aspire.Hosting.DbGate/DbGateBuilderExtensions.cs Modified AddDbGate method signature to make name parameter optional with default value "dbgate"
src/CommunityToolkit.Aspire.Hosting.MongoDB.Extensions/MongoDBBuilderExtensions.cs Refactored to use new AddDbGate signature and configure environment per-resource, but contains type mismatch and syntax errors
src/CommunityToolkit.Aspire.Hosting.Redis.Extensions/RedisBuilderExtensions.cs Attempted refactor with critical bugs: incomplete method body, type mismatch, and syntax errors
tests/CommunityToolkit.Aspire.Hosting.DbGate.Tests/DbGatePublicApiTests.cs Removed null name parameter test and updated calls to use parameterless AddDbGate
tests/CommunityToolkit.Aspire.Hosting.DbGate.Tests/AddDbGateTests.cs Updated tests to use new AddDbGate signature, but AddDbGate(9090) call won't compile
tests/CommunityToolkit.Aspire.Hosting.DbGate.Tests/AppHostTests.cs Updated resource name assertion to expect "dbgate"
tests/CommunityToolkit.Aspire.Hosting.MongoDB.Extensions.Tests/ResourceCreationTests.cs Updated assertions to expect "dbgate" name and resource-name-based environment variable keys
tests/CommunityToolkit.Aspire.Hosting.MongoDB.Extensions.Tests/AppHostTests.cs Updated resource name assertion to expect "dbgate"
tests/CommunityToolkit.Aspire.Hosting.Redis.Extensions.Tests/ResourceCreationTests.cs Updated assertions to expect "dbgate" name and resource-name-based environment variable keys
tests/CommunityToolkit.Aspire.Hosting.Redis.Extensions.Tests/AppHostTests.cs Updated resource name assertion to expect "dbgate"
tests/CommunityToolkit.Aspire.Hosting.PostgreSQL.Extensions.Tests/ResourceCreationTests.cs Updated assertions for future refactoring (source code not yet updated in this PR)
tests/CommunityToolkit.Aspire.Hosting.PostgreSQL.Extensions.Tests/AppHostTests.cs Updated resource name assertion to expect "dbgate"
tests/CommunityToolkit.Aspire.Hosting.SqlServer.Extensions.Tests/ResourceCreationTests.cs Updated assertions for future refactoring (source code not yet updated in this PR)
tests/CommunityToolkit.Aspire.Hosting.SqlServer.Extensions.Tests/AppHostTests.cs Updated resource name assertion to expect "dbgate"

var appBuilder = DistributedApplication.CreateBuilder();

var dbgate = appBuilder.AddDbGate("dbgate", 9090);
var dbgate = appBuilder.AddDbGate(9090);
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

This call will not compile. The method signature AddDbGate(string name = "dbgate", int? port = null) requires the first parameter to be a string (name), but 9090 is an int.

To support the pattern AddDbGate(9090) for specifying just the port, you need to either:

  1. Add a separate overload: public static IResourceBuilder<DbGateContainerResource> AddDbGate(this IDistributedApplicationBuilder builder, int port)
  2. Or change this test to use named parameters: appBuilder.AddDbGate(port: 9090)

The first approach is cleaner for API consumers.

Suggested change
var dbgate = appBuilder.AddDbGate(9090);
var dbgate = appBuilder.AddDbGate(port: 9090);

Copilot uses AI. Check for mistakes.
/// </summary>
/// <param name="builder">The resource builder.</param>
/// <param name="name">The name of the resource. This name will be used as the connection string name when referenced in a dependency.</param>
/// <param name="name">The name of the resource. This name will be used as the connection string name when referenced in a dependency. Optional; defaults to <c>dbgate</c>.</param>
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The XML documentation for the name parameter is inconsistent with the actual behavior. The documentation states the parameter is "Optional; defaults to dbgate", but it's actually a required parameter in the sense that it must be explicitly provided if you want to specify the port (due to parameter ordering).

Consider either:

  1. Adding an overload that takes only int port to make the name truly optional when specifying a port
  2. Updating the documentation to clarify that when providing a port, the name must be specified using named parameters: builder.AddDbGate(port: 9090)

Additionally, given the default value and the [ResourceName] attribute, the ArgumentNullException.ThrowIfNull(name) check at line 69 is unnecessary since the parameter can never be null.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parameter is mandatory, but the argument is optional on invocation.
It's false that the parameter can never be null.

public static IResourceBuilder<DbGateContainerResource> AddDbGate(this IDistributedApplicationBuilder builder, [ResourceName] string name = "dbgate", int? port = null)
{
ArgumentNullException.ThrowIfNull(builder);
ArgumentNullException.ThrowIfNull(name);
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The ArgumentNullException.ThrowIfNull(name) check is unnecessary because the name parameter has a default value of "dbgate" and can never be null. This validation should be removed.

Suggested change
ArgumentNullException.ThrowIfNull(name);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does not prevent a null value to be sent.

@paulomorgado paulomorgado force-pushed the feature/dbgate branch 3 times, most recently from 01e4ca9 to 11ef778 Compare November 25, 2025 20:44
@paulomorgado paulomorgado marked this pull request as draft November 26, 2025 08:51
@paulomorgado paulomorgado force-pushed the feature/dbgate branch 2 times, most recently from 0ea12df to 1844f0f Compare November 26, 2025 11:30
Removed the `name` parameter from the `AddDbGate` method across various builder extension classes, simplifying the method signature to only require the `port` parameter. The default name "dbgate" is now used internally.

Updated tests to reflect the new method signature and default naming convention, ensuring assertions check for the new default name. Adjusted environment variable handling in `ConfigureDbGateContainer` methods to accommodate these changes.

These updates streamline the API for adding a DbGate container resource while maintaining functionality for different database types.
@paulomorgado
Copy link
Contributor Author

Hi @Alirexaa, failing tests seem to be related to timeouts. Can you check?

@Alirexaa
Copy link
Member

Thanks @paulomorgado for your contributions. I've rerun the failed jobs and will review the PR later.

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.

2 participants