Changed database info cache key#3183
Changed database info cache key#3183YohDeadfall merged 2 commits intonpgsql:mainfrom YohDeadfall:database-info-caching
Conversation
|
Looks like it's a breaking change, @roji. |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Look good, thanks @YohDeadfall.
Apart from the nullability issue below, we should have a couple tests for this. One test could connect to two connection strings with the same host/port/database, but different application paths - and then assert that the connectors' DatabaseInfo property points to the same reference. Another test would do the opposite, asserting that the DatabaseInfo properties are different if the database is different (the test would have to create and drop a temporary database).
Check out TestUtil.CreateTempPool for when you need to play around with connection strings.
src/Npgsql/NpgsqlDatabaseInfo.cs
Outdated
There was a problem hiding this comment.
The strings here should be non-nullable, no? It makes no sense to have a dictionary entry without a host/database, and at the point where we're loading the types, we should have already verified that we have a host and a database..
There was a problem hiding this comment.
Unfortunately, NpgsqlConnectionStringBuilder says that these fields are nullable. Do you want some bangs here? (:
There was a problem hiding this comment.
I hate bangs as much as you do :)
The thing to do here, is refactor things a bit. We already validate that Host is never null, and we even have a bang'ed non-nullable property because of this which you can use. It would be cleaner to remove the bang from that property by make it a non-nullable auto-property, and just assign the host after we've validated it.
For Database, we know it can't be null because we null-coalesce Database to the user. We could use the same kind of technique - have a bang'ed property, or more cleanly by having a non-nullable auto-property. Another option is to pass the coalesced, non-nullable database through the stack (this avoids adding another property), but it makes the code a bit nastier.
IMHO this is a good example of how the nullable references features forces us to clean up code to be better :)
There was a problem hiding this comment.
Will fix it, but there is a bigger problem as said above. It's more important problem than bangs.
There was a problem hiding this comment.
It's because of test run parallelization.
|
@YohDeadfall this is waiting for you, right? Where do you see a breaking change here (unless you mean that the tests don't pass)? |
|
@YohDeadfall just a reminder on this, when you get some time. |
|
@YohDeadfall should we move this out of 5.0, or do you want me to take a look? I still don't know what breaking change you were referring to above... |
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <TargetFramework Condition="'$(DeveloperBuild)' == 'True'">net5.0</TargetFramework> | ||
| <TargetFrameworks>net5.0</TargetFrameworks> | ||
| <TargetFrameworks Condition="'$(DeveloperBuild)' != 'True'">net461;netstandard2.0</TargetFrameworks> |
There was a problem hiding this comment.
Why this change, why are we targeting net461 again?
|
|
||
| internal static readonly ConcurrentDictionary<string, NpgsqlDatabaseInfo> Cache | ||
| = new ConcurrentDictionary<string, NpgsqlDatabaseInfo>(); | ||
| internal static readonly ConcurrentDictionary<(string Host, int Port, string Database, bool NoTypeLoading), NpgsqlDatabaseInfo> Cache |
There was a problem hiding this comment.
Maybe a readonly struct (DatabaseInfoCacheKey)? Big tuples are hard to parse...
| @@ -0,0 +1,16 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
VS is behaving badly on rebases. Will cleanup.
2 participants
Fixes #3069