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

Add functionality to auto reload Npgsql types for added PG extensions #111

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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: 1 addition & 1 deletion src/Weasel.Core/Migrations/Database.cs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@
return await SchemaMigration.DetermineAsync(conn, ct, objects).ConfigureAwait(false);
}

public Task<SchemaPatchDifference> ApplyAllConfiguredChangesToDatabaseAsync(
public virtual Task<SchemaPatchDifference> ApplyAllConfiguredChangesToDatabaseAsync(
AutoCreate? @override = null,
ReconnectionOptions? reconnectionOptions = null,
CancellationToken ct = default
Expand Down Expand Up @@ -354,7 +354,7 @@

public virtual IFeatureSchema FindFeature(Type featureType)
{
return null; // TODO - could get smarter and try to create by type

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres ionx/postgres-plv8:12.2 Case Sensitive true

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres ionx/postgres-plv8:12.2 Case Sensitive true

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres ionx/postgres-plv8:12.2 Case Sensitive true

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / MSSQL mcr.microsoft.com/mssql/server:2022-latest

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / MSSQL mcr.microsoft.com/mssql/server:2022-latest

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / MSSQL mcr.microsoft.com/mssql/server:2022-latest

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres ionx/postgres-plv8:12.2 Case Sensitive false

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres ionx/postgres-plv8:12.2 Case Sensitive false

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres ionx/postgres-plv8:12.2 Case Sensitive false

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / MSSQL mcr.microsoft.com/mssql/server:2019-latest

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / MSSQL mcr.microsoft.com/mssql/server:2019-latest

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / MSSQL mcr.microsoft.com/mssql/server:2019-latest

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres postgres:15.3-alpine Case Sensitive false

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres postgres:15.3-alpine Case Sensitive false

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres postgres:15.3-alpine Case Sensitive false

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres postgres:15.3-alpine Case Sensitive true

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres postgres:15.3-alpine Case Sensitive true

Possible null reference return.

Check warning on line 357 in src/Weasel.Core/Migrations/Database.cs

View workflow job for this annotation

GitHub Actions / Postgres postgres:15.3-alpine Case Sensitive true

Possible null reference return.
}

public void MarkAllFeaturesAsChecked()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ public async Task can_build_databases_once()
(await theDatabases.FindOrCreateDatabase("three")).ShouldBeSameAs(three);
}

public class Databases: SingleServerDatabaseCollection<DatabaseWithTables>
public class Databases: SingleServerDatabaseCollection<TestDatabase>
{
public Databases() : base(ConnectionSource.ConnectionString)
{
}

protected override DatabaseWithTables buildDatabase(string databaseName, string connectionString)
protected override TestDatabase buildDatabase(string databaseName, string connectionString)
{
return new DatabaseWithTables(databaseName, connectionString);
return new TestDatabase(databaseName, connectionString);
}
}
}
Expand Down
51 changes: 43 additions & 8 deletions src/Weasel.Postgresql.Tests/Migrations/migration_scenario_tests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Npgsql;
using Shouldly;
Expand All @@ -16,11 +17,11 @@ namespace Weasel.Postgresql.Tests.Migrations;
[Collection("migrations")]
public class SchemaMigrationTests : IntegrationContext, IAsyncLifetime
{
private readonly DatabaseWithTables theDatabase;
private readonly TestDatabase theDatabase;

public SchemaMigrationTests() : base("migrations")
{
theDatabase = new DatabaseWithTables(AutoCreate.None, "Migrations");
theDatabase = new TestDatabase(AutoCreate.None, "Migrations");
}

public override Task InitializeAsync()
Expand Down Expand Up @@ -116,6 +117,14 @@ await theDatabase.ApplyAllConfiguredChangesToDatabaseAsync(
connectionGlobalLock.Failed.ShouldBeTrue();
connectionGlobalLock.Retried.ShouldBeFalse();
}

[Fact]
public async Task test_auto_reload_of_npgsql_types()
{
theDatabase.ExtensionFeatures["Extensions"].AddExtension("pgcrypto");
await theDatabase.ApplyAllConfiguredChangesToDatabaseAsync();
theDatabase.HasNpgsqlTypesReloaded.ShouldBeTrue();
}
}

public class NamedTable: Table
Expand Down Expand Up @@ -149,22 +158,42 @@ protected override IEnumerable<ISchemaObject> schemaObjects()
}
}

public class DatabaseWithTables: PostgresqlDatabase
public class ExtensionFeature: FeatureSchemaBase
{
public static DatabaseWithTables ForConnectionString(string connectionString)
public Dictionary<string, Extension> ExtensionObjects { get; } = new();

public ExtensionFeature(string identifier, Migrator migrator) : base(identifier, migrator)
{
}

protected override IEnumerable<ISchemaObject> schemaObjects()
{
return ExtensionObjects.Values;
}

public void AddExtension(string extensionName)
{
var extension = new Extension(extensionName);
ExtensionObjects[extensionName] = extension;
}
}

public class TestDatabase: PostgresqlDatabase
{
public static TestDatabase ForConnectionString(string connectionString)
{
var builder = new NpgsqlConnectionStringBuilder(connectionString);
var identifier = builder.Database;

return new DatabaseWithTables(identifier, connectionString);
return new TestDatabase(identifier, connectionString);
}

public DatabaseWithTables(AutoCreate autoCreate, string identifier)
public TestDatabase(AutoCreate autoCreate, string identifier)
: base(new DefaultMigrationLogger(), autoCreate, new PostgresqlMigrator(), identifier, ConnectionSource.ConnectionString)
{
}

public DatabaseWithTables(string identifier, string connectionString)
public TestDatabase(string identifier, string connectionString)
: base(new DefaultMigrationLogger(), AutoCreate.All, new PostgresqlMigrator(), identifier, connectionString)
{
}
Expand All @@ -173,9 +202,15 @@ public DatabaseWithTables(string identifier, string connectionString)
new LightweightCache<string, NamedTableFeature>(name =>
new NamedTableFeature(name, new PostgresqlMigrator()));

public LightweightCache<string, ExtensionFeature> ExtensionFeatures { get; } =
new LightweightCache<string, ExtensionFeature>(name =>
new ExtensionFeature(name, new PostgresqlMigrator()));

public override IFeatureSchema[] BuildFeatureSchemas()
{
return Features.OfType<IFeatureSchema>().ToArray();
var featureSchemas = Features.OfType<IFeatureSchema>()
.Append(ExtensionFeatures.OfType<IFeatureSchema>()).ToArray();
return featureSchemas;
}
}

Expand Down
31 changes: 31 additions & 0 deletions src/Weasel.Postgresql/PostgresqlDatabase.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using JasperFx.Core;
using Npgsql;
using Weasel.Core;
using Weasel.Core.Migrations;
Expand All @@ -7,6 +8,12 @@ namespace Weasel.Postgresql;

public abstract class PostgresqlDatabase: DatabaseBase<NpgsqlConnection>
{
/// <summary>
/// Provide status of whether the Npgsql types were reloaded
/// Currently used for assertion in unit tests
/// </summary>
public bool HasNpgsqlTypesReloaded { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

SUGGESTION: I don't think that we should expose public property just for the sake of tests. I think that we should install in the test project some extension that requires a type reload and ensure that it's actually working. The test just checks if the code was run. In the worst case, we should make this property internal and allow access for test project to internal code, still, I'd treat this as a last resort if we cannot easily run tests against extensions.

Copy link
Member Author

@mysticmind mysticmind Dec 4, 2023

Choose a reason for hiding this comment

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

I am quite aware that one of you will raise a concern, that is why I added a clear description in the summary. You have provided a general suggestion, may be provide a clear cut way to solve this, will amend the code accordingly. I am okay with making it internal for now but still it has to be available in the test project.

Copy link
Member Author

@mysticmind mysticmind Dec 4, 2023

Choose a reason for hiding this comment

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

I think that we should install in the test project some extension that requires a type reload and ensure that it's actually working.

May be I can look at this with a real test using PostGIS extension then we don't need that property.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mysticmind PostGis will require using their image, but that's, of course, an option. I'd suggest trying to search for something easier to test first.

Copy link
Member Author

@mysticmind mysticmind Dec 4, 2023

Choose a reason for hiding this comment

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

Quick update:

  • I tried with citext and Npgsql looks to be loading the types by default so it is working fine without refresh of Npgsql types. This looks to be case of all built-in PG extensions.
  • Further went and tried to test with PostGis in my local, just refreshing types didn't work to fetch data, say of type point or geometry. Code something like below:
[Fact]
    public async Task test_auto_reload_of_npgsql_types()
    {
        var theDatabase = TestDatabase.ForConnectionString(ConnectionSource.ConnectionString);
        theDatabase.ExtensionFeatures["Extensions"].AddExtension("postgis");
        await theDatabase.ApplyAllConfiguredChangesToDatabaseAsync(AutoCreate.CreateOrUpdate);
        var conn = this.theDatabase.CreateConnection();
        await conn.OpenAsync();
        var cmd = conn.CreateCommand("select $1");
        cmd.Parameters.Add(new() { Value = new Point(new Coordinate(1d, 2d)) });
        var reader = await cmd.ExecuteReaderAsync();
        await reader.ReadAsync();
        var val = reader.GetFieldValue<Point>(0);
    }

Getting an exception as below:

System.InvalidCastException
Writing values of 'NetTopologySuite.Geometries.Point' is not supported for parameters having no NpgsqlDbType or DataTypeName. Try setting one of these values to the expected database type..
   at Npgsql.Internal.AdoSerializerHelpers.<GetTypeInfoForWriting>g__ThrowWritingNotSupported|1_0(Type type, PgSerializerOptions options, Nullable`1 pgTypeId, Nullable`1 npgsqlDbType, Exception inner)
   at Npgsql.Internal.AdoSerializerHelpers.GetTypeInfoForWriting(Type type, Nullable`1 pgTypeId, PgSerializerOptions options, Nullable`1 npgsqlDbType)
   at Npgsql.NpgsqlParameter.ResolveTypeInfo(PgSerializerOptions options)
   at Npgsql.NpgsqlParameterCollection.ProcessParameters(PgSerializerOptions options, Boolean validateValues, CommandType commandType)
   at Npgsql.NpgsqlCommand.ExecuteReader(Boolean async, CommandBehavior behavior, CancellationToken cancellationToken)
   at Npgsql.NpgsqlCommand.ExecuteReader(Boolean async, CommandBehavior behavior, CancellationToken cancellationToken)
   at Weasel.Postgresql.Tests.Migrations.SchemaMigrationTests.test_auto_reload_of_npgsql_types() in /Users/babuannamalai/oss_contrib/weasel_main_repo/src/Weasel.Postgresql.Tests/Migrations/migration_scenario_tests.cs:line 134
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_0.<<InvokeTestMethodAsync>b__1>d.MoveNext() in /_/src/xunit.execution/Sdk/Frameworks/Runners/TestInvoker.cs:line 285
--- End of stack trace from previous location ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in /_/src/xunit.execution/Sdk/Frameworks/ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in /_/src/xunit.core/Sdk/ExceptionAggregator.cs:line 90

In summary, there looks to be more to this than just refresh the Npgsql types. I will investigate further on this.


protected PostgresqlDatabase(
IMigrationLogger logger,
AutoCreate autoCreate,
Expand Down Expand Up @@ -50,4 +57,28 @@ public async Task<IReadOnlyList<DbObjectName>> SchemaTables(CancellationToken ct

return await conn.ExistingTablesAsync(schemas: schemaNames, ct: ct).ConfigureAwait(false);
}

public override async Task<SchemaPatchDifference> ApplyAllConfiguredChangesToDatabaseAsync(AutoCreate? @override = null,
ReconnectionOptions? reconnectionOptions = null, CancellationToken ct = default)
{
HasNpgsqlTypesReloaded = false;
var schemaPatchDiff = await base.ApplyAllConfiguredChangesToDatabaseAsync(@override, reconnectionOptions, ct).ConfigureAwait(false);

// If there were extensions installed, automatically reload Npgsql types cache.
var hasExtensions = AllObjects().OfType<Extension>().Any();
if (hasExtensions)
{
await ReloadNpgsqlTypes(ct).ConfigureAwait(false);
}

return schemaPatchDiff;
}

private async Task ReloadNpgsqlTypes(CancellationToken ct = default)
{
await using var conn = CreateConnection();
await conn.OpenAsync(ct).ConfigureAwait(false);
await conn.ReloadTypesAsync().ConfigureAwait(false);
HasNpgsqlTypesReloaded = true;
}
}
Loading