Skip to content

Use newest Npgsql when targeting net8.0 and above#13724

Merged
roji merged 4 commits intomicrosoft:mainfrom
roji:PgVector
Mar 31, 2026
Merged

Use newest Npgsql when targeting net8.0 and above#13724
roji merged 4 commits intomicrosoft:mainfrom
roji:PgVector

Conversation

@roji
Copy link
Copy Markdown
Member

@roji roji commented Mar 31, 2026

We are currently using Npgsql 8.0 because that's the last version that's still compatible with .NET Standard 2.0/netfx; but it seems wrong to force an older version of Npgsql on modern .NET users just because netfx users require it

So for the versions of the PgVector MEVD provider targeting net8.0/net10.0, use the latest Npgsql 10.0; netstandard2.0/net462 still use Npgsql 8.0. It's not ideal to use different versions of Npgsql across different TFM targets of the same library, but it seems like the lesser of the possible evils here.

This incidentally also takes care of the ReloadTypesAsync problem in #13706.

@roji roji requested review from adamsitnik and westey-m March 31, 2026 15:35
@roji roji requested a review from a team as a code owner March 31, 2026 15:35
Copilot AI review requested due to automatic review settings March 31, 2026 15:35
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 4 | Confidence: 83%

✓ Correctness

The diff splits the Npgsql PackageVersion entry into two conditional entries: Npgsql 10.0.2 for net8.0-compatible TFMs and Npgsql 8.0.7 for older ones (netstandard2.0, net462). The condition logic using IsTargetFrameworkCompatible is correct for the PgVector project's target frameworks (net10.0;net8.0;netstandard2.0;net462). However, this is the first use of conditional PackageVersion entries in this Directory.Packages.props file, and there is a risk that NuGet Central Package Management may not correctly resolve duplicate PackageVersion items with the same Include name even when conditions are mutually exclusive — this depends on the NuGet version and can produce NU1009 warnings or restore failures in some configurations. When $(TargetFramework) is empty during the outer multi-target build evaluation, the negated condition silently falls back to 8.0.7, which is benign since package resolution occurs in inner builds.

✓ Security Reliability

This change conditionally splits the Npgsql package version based on target framework: 10.0.2 for net8.0+ and 8.0.7 for older targets (netstandard2.0, net462). The PgVector project multi-targets net10.0;net8.0;netstandard2.0;net462, so this correctly provides the newer Npgsql for compatible frameworks while maintaining the older version for legacy ones. The IsTargetFrameworkCompatible conditions are mutually exclusive, and $(TargetFramework) is properly available during inner-build evaluation in Directory.Packages.props. No security or reliability concerns identified.

✓ Test Coverage

This PR bumps Npgsql from 8.0.7 to 10.0.2 for net8.0+ target frameworks while keeping 8.0.7 for older frameworks (netstandard2.0, net462) via MSBuild conditions in Directory.Packages.props. This is a dependency version configuration change with no new application code. The existing unit tests (net10.0), conformance tests (net10.0;net472), and integration tests (net10.0) will all resolve Npgsql 10.0.2 since their target frameworks are net8.0-compatible. The fallback path to Npgsql 8.0.7 for netstandard2.0/net462 is not directly exercised by any test project, but this is expected since test projects typically don't target those older frameworks. No new behavioral code was introduced, so no new tests are needed.

✗ Design Approach

The change introduces conditional PackageVersion entries in Directory.Packages.props that select between Npgsql 10.0.2 and 8.0.7 based on $(TargetFramework) at central-package-management evaluation time. The intent is sound — Npgsql 9+ dropped netstandard2.0/net462 support, so the split is necessary for PgVector.csproj which still targets those TFMs. However, the mechanism is fragile: for multi-targeted projects (PgVector targets net10.0;net8.0;netstandard2.0;net462), Directory.Packages.props is first evaluated in the outer-build context where $(TargetFramework) is empty. IsTargetFrameworkCompatible(', 'net8.0') returns false, making the negated condition true and selecting 8.0.7 in that outer pass — the opposite of what net8.0/net10.0 inner builds need. While inner per-TFM builds typically re-evaluate and get the correct version, this is an undocumented CPM pattern that behaves unexpectedly in design-time builds, IDE evaluation, and tooling that uses the outer-build context. The correct CPM mechanism for this scenario is VersionOverride in the individual project files that need the newer version, or simply dropping the legacy TFMs from PgVector.csproj (since Npgsql 9+ already does not support them).

Flagged Issues

  • Using conditional PackageVersion entries keyed on $(TargetFramework) in Directory.Packages.props is unreliable for multi-TFM projects. In the outer-build pass, $(TargetFramework) is empty, so IsTargetFrameworkCompatible(', 'net8.0') = false and the negated condition selects Npgsql 8.0.7 globally. Design-time builds and IDE evaluation often use this outer-build context, so net8.0/net10.0 targets in PgVector.csproj may silently resolve to 8.0.7 instead of 10.0.2. Prefer keeping the global PackageVersion at 8.0.7 and using <PackageReference Include="Npgsql" VersionOverride="10.0.2" /> in projects that exclusively target net10.0, or drop netstandard2.0/net462 from PgVector.csproj and upgrade Npgsql uniformly to 10.0.2.

Suggestions

  • Verify that Pgvector 0.3.2 is compatible with Npgsql 10.0.2, since Pgvector has a transitive dependency on Npgsql and may have an upper-bound version constraint.
  • Consider whether netstandard2.0 and net462 targets are still required in PgVector.csproj. Npgsql itself dropped those TFMs in v9; removing them would allow a clean global upgrade to 10.0.2 without any conditional logic.
  • Test a full restore and build of the PgVector project across all four target frameworks (net10.0, net8.0, netstandard2.0, net462) to confirm NuGet CPM handles the conditional entries without NU1009 warnings.
  • Verify that the PgVector conformance tests (targeting net472, which resolves to Npgsql 8.0.7) and the existing unit/integration tests (targeting net10.0, resolving to 10.0.2) all pass end-to-end before merging, especially if Npgsql 10.0.2 introduces breaking API changes.
  • Add an XML comment above the conditional entries explaining why the split is needed (Npgsql 10.x dropped netstandard2.0/net462 support) to help future maintainers.

Automated review by roji's agents

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates centralized NuGet package management so projects targeting net8.0 or later consume a newer Npgsql version, while legacy targets continue using the older Npgsql version required for compatibility. This aligns the PgVector connector and any other Npgsql consumers with modern .NET runtime expectations without breaking netstandard2.0 and net462 consumers.

Changes:

  • Introduce conditional central package versions for Npgsql, selecting 10.0.2 for net8.0 compatible TFMs.
  • Keep Npgsql at 8.0.7 for TFMs that are not compatible with net8.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@roji roji marked this pull request as draft March 31, 2026 15:41
Move the conditional Npgsql version logic from Directory.Packages.props
to VersionOverride in the consuming project files. Conditional
PackageVersion in Directory.Packages.props is unreliable for multi-TFM
projects because TargetFramework is empty during the outer build pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Set the latest Npgsql 10.0.2 as the default in Directory.Packages.props
so only PgVector.csproj needs a VersionOverride (for its older TFMs).
IntegrationTests and Concepts get 10.0.2 automatically.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@roji roji marked this pull request as ready for review March 31, 2026 15:49
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 4 | Confidence: 89%

✓ Correctness

The PR correctly upgrades Npgsql from 8.0.7 to 10.0.2 in Central Package Management while using VersionOverride in PgVector.csproj to pin older target frameworks (netstandard2.0, net462) to 8.0.7. This is necessary because Npgsql 10.x dropped support for netstandard2.0 and net462. The conditional PackageReference pattern with mutually exclusive MSBuild conditions is the standard CPM approach, and the VersionOverride correctly addresses the concern raised in the previously resolved review comment. Pgvector 0.3.2 has a minimum Npgsql dependency of >= 8.0.5 with no upper bound, so it's compatible with both versions. Other Npgsql consumers in the repo target net10.0 only, so they're unaffected.

✓ Security Reliability

This PR bumps Npgsql from 8.0.7 to 10.0.2 in the central package management (CPM) and uses conditional VersionOverride in PgVector.csproj to pin older TFMs (netstandard2.0, net462) to 8.0.7 while letting net8.0+ consume 10.0.2. This correctly implements the VersionOverride approach recommended in the previously resolved review comment, and is the standard CPM pattern for per-TFM version pinning. The mutually exclusive conditions ensure each inner build gets exactly one Npgsql reference. No security or reliability concerns were identified—upgrading Npgsql is generally beneficial for security patches, and the version-pining strategy is sound. Other Npgsql consumers in the repo (Concepts.csproj, IntegrationTests.csproj) are single-TFM net10.0 projects that will correctly resolve 10.0.2 from CPM.

✓ Test Coverage

This PR bumps Npgsql from 8.0.7 to 10.0.2 for net8.0+ target frameworks and preserves 8.0.7 for older TFMs (netstandard2.0, net462) via VersionOverride. From a test coverage perspective, the existing unit tests (targeting net10.0) directly use real Npgsql types like NpgsqlCommand, NpgsqlBatch, and NpgsqlDbType — so they will validate API compatibility with 10.0.2 at both compile and runtime. The conformance tests target net10.0 and net472, providing cross-TFM integration coverage. No new code logic or behavioral changes are introduced, so no additional tests are needed for this change.

✓ Design Approach

The diff upgrades Npgsql to 10.0.2 for net8.0+ TFMs while keeping 8.0.7 for older targets (netstandard2.0, net462). The resolved comment correctly pointed out that using conditional PackageVersion in Directory.Packages.props is unreliable in outer-build context, and this PR addresses that by moving the version pin to VersionOverride in the project file. However, the chosen approach introduces two PackageReference entries for the same package ID with mutually exclusive conditions, rather than a single entry with a conditional metadata child element. In the outer build (where $(TargetFramework) is empty), IsTargetFrameworkCompatible(', 'net8.0') returns false, so only the 8.0.7 entry is active — meaning any tooling or IDE that reads the outer-build item list will silently miss that 10.0.2 is needed for net8.0/net10.0. The single-entry pattern with a conditional <VersionOverride> child eliminates the duplicate-entry risk and is idiomatic MSBuild.

Suggestions

  • Consider adding net8.0 to the unit test project's TargetFrameworks to verify Npgsql 10.0.2 compatibility on that TFM as well (pre-existing gap, not introduced by this PR).
  • Replace the two conditional PackageReference entries with a single entry using a conditional <VersionOverride> child element. This avoids a duplicate package ID in the project's item list and is more robust in outer-build evaluation where $(TargetFramework) is empty: <PackageReference Include="Npgsql"><VersionOverride Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0')">8.0.7</VersionOverride></PackageReference>.

Automated review by roji's agents

…ement

Replace two conditional PackageReference entries for Npgsql with a single
entry using a conditional <VersionOverride> child element, as recommended
by review. This avoids duplicate package IDs in the project item list and
is more robust in outer-build evaluation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@roji roji enabled auto-merge March 31, 2026 16:44
@roji roji added this pull request to the merge queue Mar 31, 2026
Merged via the queue into microsoft:main with commit 134e52e Mar 31, 2026
18 checks passed
@roji roji deleted the PgVector branch March 31, 2026 17:08
@roji roji added this to the MEVD Finalization milestone Mar 31, 2026
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.

4 participants