-
Notifications
You must be signed in to change notification settings - Fork 228
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
Major work around DbDataSource management, enum handling and plugins #3167
Conversation
src/EFCore.PG/Design/Internal/NpgsqlCSharpRuntimeAnnotationCodeGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.PG/Metadata/Conventions/NpgsqlPostgresModelFinalizingConvention.cs
Outdated
Show resolved
Hide resolved
d8db176
to
d4ae264
Compare
Fixes npgsql#2891 Fixes npgsql#3063 Fixes npgsql#1026 Co-authored-by: Nino Floris <mail@ninofloris.com>
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.
Looks like a great upgrade! My comments mostly consist of nits and questions.
src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlStringMethodTranslator.cs
Show resolved
Hide resolved
|
||
// If the user hasn't configured anything in UseNpgsql (no data source, no connection, no connection string), check the | ||
// application service provider to see if a data source is registered there, and return that. | ||
{ ConnectionString: null } when applicationServiceProvider?.GetService<NpgsqlDataSource>() is DbDataSource dataSource |
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.
Follow-up to this approach could be the user providing a serviceKey to some new option on UseNpgsql.
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.
Nice idea, can absolutely see that happening.
test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestStore.cs
Outdated
Show resolved
Hide resolved
_sqlGenerationHelper.DelimitIdentifier(name, schema), | ||
schema is null ? name : schema + "." + name, | ||
enumDefinition.ClrType, | ||
enumDefinition.Labels); |
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.
What was the conclusion around whether to sort these alphabetically?
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.
Yeah, this is done in the convention which "copies" the definitions from the context options to the model (code), because that's the part that ends up forming the migration. In the type mapping source I don't think there should be any significance to the ordering, we only use the labels when we need to stick a label constant in the SQL.
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.
Also adding sorting in NpgsqlMigrationsSqlGenerator, when detecting new labels (this is an improvement regardless of the other changes in this PR).
} | ||
else | ||
{ | ||
// TODO: Not sure what to do about quoting. Is the user expected to configure properties |
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.
TODO
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.
Do you think the current code as-is is good enough for merging for the moment (e.g. until we do npgsql/npgsql#5710)?
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.
Yeah as long as users can pass quoted names - as I understand they can - they should always be able to make EF map things correctly when mappingInfo.StoreTypeName is quoted.
@NinoFloris can I get another quick review from you on the added changes etc.? Especially on the new logic adding the ConcurrentDictionary for multiple data sources in NpgsqlDataSourceManager, and the concurrency handling around that? |
throw new InvalidOperationException(NpgsqlStrings.DataSourceWithMultipleConnectionStrings(dataSourceFeature)); | ||
newDataSource.Dispose(); | ||
} | ||
else if (_isDisposed == 1) |
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.
Not sure what the infra looks like - in terms of full disposal blocking this method from being called later on - but you might want this check to be done before as well. Just so you don't keep adding more items to the dictionary when things are already disposed.
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.
Not sure what the infra looks like
Me neither... My logic here is that it's OK if a data source gets added to the dictionary during/after Dispose (after all Dispose doesn't clear the dictionary, we don't really care) - the important bit is that the data sources always get disposed, which I think this achieves...
Will go ahead and merge, we can always revisit if needed (unlikely).
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.
LGTM
Following npgsql/efcore.pg#3167, which removes EF's logic for extracting enum mappings from Npgsql.
Following npgsql/efcore.pg#3167, which removes EF's logic for extracting enum mappings from Npgsql.
Following npgsql/efcore.pg#3167, which removes EF's logic for extracting enum mappings from Npgsql.
we throwwe create new data sources, cached by the connection string.This means it's no longer possible to do both enums and switch connection strings (breaking change).Future task: add NpgsqlDataSourceBuilder to NpgsqlDbContextOptionsBuilder, to allow easy configuration of Npgsql via the EF API (#2542). Accessing the DataSourceBuilder would also count as "requires internal data source", just like enums/plugins.
Fixes #2891
Fixes #3063
Fixes #1026
See comments below for last remaining tasks.
/cc @NinoFloris @ajcvickers