Skip to content

Fix Wave Analysis issues #3403

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 11, 2025
Merged

Fix Wave Analysis issues #3403

merged 3 commits into from
Jun 11, 2025

Conversation

paulmedynski
Copy link
Contributor

Description

  • Updated .NET to explicitly use System.Text.Json 8.0.5 or 9.0.5 to avoid transitive vulnerabilities.
  • Updated test tools to use Microsoft.SqlServer.SqlManagementObjects 172.76.0 to avoid transitive vulnerabilities.
  • Sorted PackageReference entries alphabetically by package name.
  • Updated .NET Framework project to build on Linux.

Issues

ADO 37675

Testing

  • Checked that all affected projects build.
  • CI will find any regressions.

@paulmedynski paulmedynski added this to the 6.1-preview2 milestone Jun 9, 2025
@Copilot Copilot AI review requested due to automatic review settings June 9, 2025 09:37
@paulmedynski paulmedynski requested a review from a team as a code owner June 9, 2025 09:37
Copilot
Copilot AI reviewed Jun 9, 2025
View reviewed changes
Copy link
Contributor

@Copilot 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

This PR pins explicit versions for System.Text.Json and other dependencies to address transitive vulnerabilities, reorders and updates package references, and adjusts project files to improve Linux build support.

  • Add System.Text.Json dependencies and bump SqlManagementObjects to mitigate vulnerabilities
  • Sort and standardize <PackageReference> entries and version props
  • Update netfx csproj (SDK name, code analysis rule, remove COM reference, adjust compile includes)

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tools/specs/Microsoft.Data.SqlClient.nuspec Add System.Text.Json dependency for net7.0, net9.0, netstandard2.0
tools/props/VersionsNet9OrLater.props Introduce SystemTextJsonVersion and fix property ordering
tools/props/Versions.props Relocate SystemTextJsonVersion and bump SqlManagementObjects
src/Microsoft.Data.SqlClient/netfx/...csproj Change SDK, update code analysis rule condition, rename files, remove COM reference, reorder packages
src/Microsoft.Data.SqlClient/netcore/...csproj Rename SqlMetadataFactory to SqlMetaDataFactory, reorder packages
src/Microsoft.Data.SqlClient/netcore/ref/...csproj Reorder package references to match new standard
Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj:951

  • Verify that the source file and its class declaration have been renamed to "DBConnectionString.cs" (uppercase 'DB') on disk; otherwise the case-sensitive Linux build may fail due to a mismatch.
<Compile Include="Microsoft\Data\Common\DBConnectionString.cs" />

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj:663

  • Ensure the file and class name have been updated to "SqlMetaDataFactory.cs" (capital 'D') so the project reference matches the actual file on a case-sensitive filesystem.
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlMetaDataFactory.cs">

paulmedynski
paulmedynski commented Jun 9, 2025
View reviewed changes
Copy link
Contributor Author

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Leaving some notes for reviewers.

@codecov Codecov
Copy link

codecov bot commented Jun 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.34%. Comparing base (e037c8a) to head (e82dd8b).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3403   +/-   ##
=======================================
  Coverage   65.34%   65.34%           
=======================================
  Files         301      301           
  Lines       65625    65625           
=======================================
+ Hits        42880    42882    +2     
+ Misses      22745    22743    -2
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 68.57% <ø> (+<0.01%) ⬆️
netfx 66.77% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

apoorvdeshmukh
apoorvdeshmukh reviewed Jun 9, 2025
View reviewed changes
@cheenamalhotra
Copy link
Member

I have this addressed in #3333

@paulmedynski
User Story 37675: Fix Wave Analysis issues
541300e 
- Updated .NET to explicitly use System.Text.Json 8.0.5 or 9.0.5 to avoid transitive vulnerabilities.
- Sorted PackageReference entries alphabetically by package name.
- Updated test tools to use Microsoft.SqlServer.SqlManagementObjects 172.76.0 to avoid transitive vulnerabilities.
- Updated .NET Framework project to build on Linux.
@paulmedynski paulmedynski force-pushed the dev/paul/wave-analysis branch from 427b65a to 541300e Compare June 10, 2025 10:46
@paulmedynski
Copy link
Contributor Author

Merging in #3333 was a mess, so I had to rebase instead - sorry! The diffs are pretty small now though.

@paulmedynski paulmedynski requested a review from a team June 10, 2025 10:54
@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 10, 2025

LGTM

cheenamalhotra
cheenamalhotra reviewed Jun 10, 2025
View reviewed changes
@paulmedynski
User Story 37675: Fix Wave Analysis issues
df89296 
- Added System.Text.Json 9.0.5 to .NET 9.0.
- Fixed System.Formats.Asn1 transitive vulnerability.
@paulmedynski
User Story 37675: Fix Wave Analysis issues
e82dd8b 
- Removed unnecessary PackageReference Version attributes.
@cheenamalhotra cheenamalhotra merged commit 260940b into main Jun 11, 2025
251 checks passed
@cheenamalhotra cheenamalhotra deleted the dev/paul/wave-analysis branch June 11, 2025 20:39
Frulfump
Frulfump reviewed Jun 28, 2025
View reviewed changes
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
<PackageReference Include="Microsoft.Bcl.Cryptography" />

<!-- Transitive dependencies that would otherwise bring in older, vulnerable versions. -->
<PackageReference Include="System.Text.Json" />
Copy link

@Frulfump Frulfump Jun 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@paulmedynski What references are pulling in the vulnerable version? Might make sense to add a comment so it will be easier to check when this can be removed. Especially since the reference will likely be pruned or raise NU1510 when using the .NET 10 SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoorvdeshmukh apoorvdeshmukh apoorvdeshmukh left review comments

Copilot code review Copilot Copilot left review comments

@benrr101 benrr101 benrr101 approved these changes

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

+2 more reviewers

@ErikEJ ErikEJ ErikEJ left review comments

@Frulfump Frulfump Frulfump left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.
6 participants
@paulmedynski @cheenamalhotra @ErikEJ @benrr101 @apoorvdeshmukh @Frulfump