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

Conversation

mysticmind
Copy link
Member

@mysticmind mysticmind commented Dec 4, 2023

  • Add functionality to auto reload Npgsql types for PG extensions added via schema objects
  • Add unit test
  • Few refactoring changes.

This fixes Marten issue JasperFx/marten#2515

…sions

- Add funtionality to auto reload Npgsql types for PG extensions added via schema objects
- Add unit test

This fixes Marten issue JasperFx/marten#2515
/// 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.

@mysticmind mysticmind marked this pull request as draft December 4, 2023 14:47
@jeremydmiller
Copy link
Member

@mysticmind I'd say just to make that one property internal, then use InternalsVisibleTo for the test project. Other than that, I say let's rebase and get this in soon

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