Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 2 additions & 0 deletions spelling.dic
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,5 @@ uninstrumented
upsert
uris
urls
Npgsql
Postgre
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,108 @@ public static partial class AspireEFPostgreSqlExtensions

configureSettings?.Invoke(settings);

if (settings.DbContextPooling)
builder.Services.AddNpgsqlDataSource(settings.ConnectionString ?? string.Empty, builder =>
Copy link
Member

Choose a reason for hiding this comment

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

You are overwriting my changes in ae11632. Can you revert this so it doesn't add back NpgsqlDataSource into DI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

{
builder.Services.AddDbContextPool<TContext>(ConfigureDbContext);
// delay validating the ConnectionString until the DataSource is requested. This ensures an exception doesn't happen until a Logger is established.
if (string.IsNullOrEmpty(settings.ConnectionString))
{
throw new InvalidOperationException($"ConnectionString is missing. It should be provided in 'ConnectionStrings:{connectionName}' or under the 'ConnectionString' key in '{DefaultConfigSectionName}' or '{typeSpecificSectionName}' configuration section.");
}

builder.UseLoggerFactory(null); // a workaround for https://github.com/npgsql/efcore.pg/issues/2821
});

builder.Services.AddDbContextPool<TContext>(options => ConfigureDbContext(settings, options, configureDbContextOptions));

ConfigureInstrumentation<TContext>(builder, settings);
}

/// <summary>
/// Configures health check, logging and telemetry for the <see cref="DbContext" />.
/// </summary>
/// <exception cref="ArgumentNullException">Thrown if mandatory <paramref name="builder"/> is null.</exception>
/// <exception cref="InvalidOperationException">Thrown when mandatory <see cref="DbContext"/> is not registered in DI.</exception>
public static void EnrichNpgsqlDbContext<[DynamicallyAccessedMembers(RequiredByEF)] TContext>(
this IHostApplicationBuilder builder,
Action<NpgsqlEntityFrameworkCorePostgreSQLSettings>? configureSettings = null) where TContext : DbContext
{
ArgumentNullException.ThrowIfNull(builder);

NpgsqlEntityFrameworkCorePostgreSQLSettings settings = new();
var typeSpecificSectionName = $"{DefaultConfigSectionName}:{typeof(TContext).Name}";
var typeSpecificConfigurationSection = builder.Configuration.GetSection(typeSpecificSectionName);
if (typeSpecificConfigurationSection.Exists()) // https://github.com/dotnet/runtime/issues/91380
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 extract this into a common method?

{
typeSpecificConfigurationSection.Bind(settings);
}
else
{
builder.Services.AddDbContext<TContext>(ConfigureDbContext);
builder.Configuration.GetSection(DefaultConfigSectionName).Bind(settings);
}

configureSettings?.Invoke(settings);

ConfigureRetry();

ConfigureInstrumentation<TContext>(builder, settings);

void ConfigureRetry()
{
if (!settings.Retry)
{
return;
}

// Resolving DbContext<TContextService> will resolve DbContextOptions<TContextImplementation>
var olDbContextOptionsDescriptor = builder.Services.FirstOrDefault(sd => sd.ServiceType == typeof(DbContextOptions<TContext>));

if (olDbContextOptionsDescriptor is null)
{
throw new InvalidOperationException($"DbContext<{nameof(TContext)}> was not registered");
}

builder.Services.Remove(olDbContextOptionsDescriptor);

ServiceDescriptor dbContextOptionsDescriptor;

dbContextOptionsDescriptor = new ServiceDescriptor(
olDbContextOptionsDescriptor.ServiceType,
olDbContextOptionsDescriptor.ServiceKey,
factory: (sp, key) =>
{
DbContextOptionsBuilder? optionsBuilder = null;

if (olDbContextOptionsDescriptor.ImplementationFactory != null)
{
var instance = olDbContextOptionsDescriptor.ImplementationFactory?.Invoke(sp) as DbContextOptions<TContext>;

if (instance == null)
{
throw new InvalidOperationException($"DbContextOptions<{nameof(TContext)}> couldn't be resolved");
}

optionsBuilder = new DbContextOptionsBuilder<TContext>(instance);
}
else
{
optionsBuilder ??= new DbContextOptionsBuilder<TContext>();
}

ConfigureDbContext(settings, optionsBuilder, null);

optionsBuilder.UseNpgsql(options => options.EnableRetryOnFailure());

return optionsBuilder.Options;
},
olDbContextOptionsDescriptor.Lifetime
);

builder.Services.Add(dbContextOptionsDescriptor);
}
}

private static void ConfigureInstrumentation<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties)] TContext>(IHostApplicationBuilder builder, NpgsqlEntityFrameworkCorePostgreSQLSettings settings) where TContext : DbContext
{
if (settings.HealthChecks)
{
// calling MapHealthChecks is the responsibility of the app, not Component
Expand Down Expand Up @@ -113,34 +206,31 @@ public static partial class AspireEFPostgreSqlExtensions
NpgsqlCommon.AddNpgsqlMetrics(meterProviderBuilder);
});
}
}

void ConfigureDbContext(DbContextOptionsBuilder dbContextOptionsBuilder)
private static void ConfigureDbContext(NpgsqlEntityFrameworkCorePostgreSQLSettings settings, DbContextOptionsBuilder dbContextOptionsBuilder, Action<DbContextOptionsBuilder>? configureDbContextOptions)
{
if (!settings.Retry)
{
// delay validating the ConnectionString until the DbContext is requested. This ensures an exception doesn't happen until a Logger is established.
if (string.IsNullOrEmpty(settings.ConnectionString))
{
throw new InvalidOperationException($"ConnectionString is missing. It should be provided in 'ConnectionStrings:{connectionName}' or under the 'ConnectionString' key in '{DefaultConfigSectionName}' or '{typeSpecificSectionName}' configuration section.");
}

// We don't register a logger factory, because there is no need to: https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontextoptionsbuilder.useloggerfactory?view=efcore-7.0#remarks
dbContextOptionsBuilder.UseNpgsql(settings.ConnectionString, builder =>
{
// Resiliency:
// 1. Connection resiliency automatically retries failed database commands: https://www.npgsql.org/efcore/misc/other.html#execution-strategy
if (settings.MaxRetryCount > 0)
{
builder.EnableRetryOnFailure(settings.MaxRetryCount);
}
// 2. "Scale proportionally: You want to ensure that you don't scale out a resource to a point where it will exhaust other associated resources."
// The pooling is enabled by default, the min pool size is 0 by default: https://www.npgsql.org/doc/connection-string-parameters.html#pooling
// There is nothing for us to set here.
// 3. "Timeout: Places limit on the duration for which a caller can wait for a response."
// The timeouts have default values, except of Internal Command Timeout, which we should ignore:
// https://www.npgsql.org/doc/connection-string-parameters.html#timeouts-and-keepalive
// There is nothing for us to set here.
});

configureDbContextOptions?.Invoke(dbContextOptionsBuilder);
return;
}

// We don't provide the connection string, it's going to use the pre-registered DataSource.
// We don't register logger factory, because there is no need to: https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontextoptionsbuilder.useloggerfactory?view=efcore-7.0#remarks
dbContextOptionsBuilder.UseNpgsql(builder =>
Copy link
Member

Choose a reason for hiding this comment

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

I think you always need to call UseNpgsql whether settings.Retry is true or false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, mistake when I refactored the code to reuse the same method for both Enrich and AddNpgsql. And with your changes I might have to split them.

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 also means we might be missing a test that should have caught that for AddNpgsql.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

It also means we might be missing a test that should have caught that for AddNpgsql.

Good call. #2136

{
// Resiliency:
// 1. Connection resiliency automatically retries failed database commands: https://www.npgsql.org/efcore/misc/other.html#execution-strategy
builder.EnableRetryOnFailure();
// 2. "Scale proportionally: You want to ensure that you don't scale out a resource to a point where it will exhaust other associated resources."
// The pooling is enabled by default, the min pool size is 0 by default: https://www.npgsql.org/doc/connection-string-parameters.html#pooling
// There is nothing for us to set here.
// 3. "Timeout: Places limit on the duration for which a caller can wait for a response."
// The timeouts have default values, except of Internal Command Timeout, which we should ignore:
// https://www.npgsql.org/doc/connection-string-parameters.html#timeouts-and-keepalive
// There is nothing for us to set here.
});

configureDbContextOptions?.Invoke(dbContextOptionsBuilder);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,25 +58,21 @@
"type": "string",
"description": "Gets or sets the connection string of the PostgreSQL database to connect to."
},
"DbContextPooling": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Pooling is enabled by default in AddNpgsql. Use Enrich otherwise.

"type": "boolean",
"description": "Gets or sets a boolean value that indicates whether the DbContext will be pooled or explicitly created every time it's requested.",
"default": true
},
"HealthChecks": {
"type": "boolean",
"description": "Gets or sets a boolean value that indicates whether the database health check is enabled or not.",
"default": true
},
"MaxRetryCount": {
"type": "integer",
"description": "Gets or sets the maximum number of retry attempts. Default value is 6, set it to 0 to disable the retry mechanism."
},
"Metrics": {
"type": "boolean",
"description": "Gets or sets a boolean value that indicates whether the OpenTelemetry metrics are enabled or not.",
"default": true
},
"Retry": {
Copy link
Member Author

Choose a reason for hiding this comment

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

bool instead of int to simplify the usage. Use a custom call in the options builder to define a custom value.

"type": "boolean",
"description": "Gets or sets whether retries should be enabled.",
"default": true
},
"Tracing": {
"type": "boolean",
"description": "Gets or sets a boolean value that indicates whether the OpenTelemetry tracing is enabled or not.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,12 @@ public sealed class NpgsqlEntityFrameworkCorePostgreSQLSettings
public string? ConnectionString { get; set; }

/// <summary>
/// Gets or sets a boolean value that indicates whether the DbContext will be pooled or explicitly created every time it's requested.
/// Gets or sets whether retries should be enabled.
/// </summary>
/// <value>
/// The default value is <see langword="true"/>.
/// </value>
/// <remarks>Should be set to false in multi-tenant scenarios.</remarks>
public bool DbContextPooling { get; set; } = true;

/// <summary>
/// <para>Gets or sets the maximum number of retry attempts.</para>
/// <para>Default value is 6, set it to 0 to disable the retry mechanism.</para>
/// </summary>
public int MaxRetryCount { get; set; } = 6;
public bool Retry { get; set; } = true;

/// <summary>
/// Gets or sets a boolean value that indicates whether the database health check is enabled or not.
Expand Down
Loading