Skip to content

Data source type mapper#4691

Merged
roji merged 3 commits intonpgsql:mainfrom
roji:DataSourceTypeMapper
Oct 20, 2022
Merged

Data source type mapper#4691
roji merged 3 commits intonpgsql:mainfrom
roji:DataSourceTypeMapper

Conversation

@roji
Copy link
Member

@roji roji commented Oct 3, 2022

Here we go. This PR looks massive (and was a lot of work), but a great deal is just test cleanup. The PR is based on #4684 and #4678, so look only at the last 2 commits when reviewing.

  • Connection-level type mapping is now obsolete-as-error (we point users towards the release notes, which will detail data source mapping).
  • I also obsoleted global type mapping (not as-error), since they're inferior and we want to nudge people towards data source (but see #4690). This may be a bit premature/extreme though (e.g. replication doesn't yet support data source).
  • Note that global mapping changes now only affect connections before their data source is created. Previously global mapping changes took effect when a connection was closed/open.
    • This is a breaking change. Though most people were just putting global mappings at the start of the program (this is fine), if a user globally mapped late - after their pool was already created - this will now break.
  • Since the DatabaseInfo is now held/cached by the built NpgsqlDataSource, I've removed the global cache for it. That means that if you build two data sources to the same database, we do type loading twice. Previously the caching was necessary since each individual connection open would load it otherwise, but with data sources I don't think it matters. But I did it in a separate commit in case you guys think we should keep the caching.
  • We may want to add ReloadTypes/ReloadTypeAsync to NpgsqlDataSource (since it works at the data source level). If so, we may also want to obsolete it at the connection level, though there's some utility in the specific connection also being refreshed immediately.
  • Naming-wise, things are now a bit weird: the (internal) TypeMapper does not, in fact, implement INpgsqlTypeMapper; that interface is about letting users manage mappings/plugins, and is now implemented by GlobalTypeMapper and NpgsqlDataSourceBuilder only. But it doesn't seem important.

Closes #4494
Does part of #4572

@roji roji requested a review from vonzshik as a code owner October 3, 2022 10:29
@roji roji force-pushed the DataSourceTypeMapper branch 4 times, most recently from 76b0a8f to 4a23663 Compare October 3, 2022 11:09
@roji roji force-pushed the DataSourceTypeMapper branch from 4a23663 to ea83b45 Compare October 20, 2022 10:53
@roji roji enabled auto-merge (squash) October 20, 2022 12:15
@roji roji merged commit d1e7910 into npgsql:main Oct 20, 2022
@roji roji deleted the DataSourceTypeMapper branch October 20, 2022 12:22
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.

Implement type mapping plugins at the NpgsqlDataSource level (and remove connection-level)

2 participants