-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
base: master
Are you sure you want to change the base?
Conversation
…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; } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
orgeometry
. 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 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 |
This fixes Marten issue JasperFx/marten#2515