Skip to content

Add ToString() to SqlJson #3427

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 6 commits into from
Jun 25, 2025
Merged

Add ToString() to SqlJson #3427

merged 6 commits into from
Jun 25, 2025

Conversation

paulmedynski
Copy link
Contributor

@paulmedynski paulmedynski commented Jun 17, 2025 •
edited
Loading

Description

  • Added a ToString() method to SqlJson that returns the serialized JSON string or null.
  • Added StringSyntax JSON to the string constructor's argument.
  • Updated XML documentation to include descriptive summaries, parameters, and exceptions.
  • Simplified the SqlJson implementation.
  • Moved existing tests to the UnitTests project and improved them.

Issues

Addresses #3424 and #3107 .

Testing

Unit tests were run manually, and CI will cover regressions.

@paulmedynski
Issue 37739: GH 3424 Add SqlJson ToString()
7c4e0ff 
- Added ToString() to SqlJson.
- Added missing commentary to public API docs.
- Simplified implementation.
- Improved tests and moved them to the UnitTests project.
@paulmedynski
Issue 37739: GH 3424 Add SqlJson ToString()
b30375a 
- Fixed ManualTests project.
@Copilot Copilot AI review requested due to automatic review settings June 17, 2025 18:21
@paulmedynski paulmedynski requested a review from a team as a code owner June 17, 2025 18:21
Copilot
Copilot AI reviewed Jun 17, 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 adds a ToString() override to SqlJson, refactors constructors to validate JSON via JsonDocument.Parse, migrates manual tests into unit tests, and updates XML documentation.

  • Implement ToString() to return the underlying JSON string or null.
  • Simplify null-handling logic in constructors and validate inputs with JsonDocument.Parse.
  • Replace manual test file with comprehensive unit tests and update documentation snippets.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlJson.cs Added ToString(), removed legacy null field, streamlined constructors
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlTypes/SqlJsonTest.cs New unit tests covering constructors, Value, and ToString()
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj Removed manual test entry for SqlJsonTest
src/Microsoft.Data.SqlClient/tests/ManualTests/Json/SqlJsonTest.cs Deleted legacy manual test file
doc/snippets/Microsoft.Data.SqlTypes/SqlJson.xml Updated XML docs for constructors, added <ToString> docs
Comments suppressed due to low confidence (1)

doc/snippets/Microsoft.Data.SqlTypes/SqlJson.xml:63

  • [nitpick] Consider adding a <returns> element under <ToString> to explicitly describe the return value (including its null semantics).
    <ToString>

@paulmedynski paulmedynski added Public API 🆕 Issues/PRs that introduce new APIs to the driver. Area\Json Use this for issues that are targeted for the Json feature in the driver. P2 Use to label moderate priority issue - impacts atleast more than 1 customer. labels Jun 17, 2025
@paulmedynski paulmedynski added this to the 6.1-preview2 milestone Jun 17, 2025
@paulmedynski paulmedynski linked an issue Jun 17, 2025 that may be closed by this pull request
Microsoft.DataSqlTypes.SqlJson is missing ToString #3424
Closed
paulmedynski
paulmedynski commented Jun 17, 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.

Added commentary for reviewers.

apoorvdeshmukh
apoorvdeshmukh reviewed Jun 18, 2025
View reviewed changes
Copy link
Contributor

@apoorvdeshmukh apoorvdeshmukh left a comment

Choose a reason for hiding this comment

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

Added some minor comments.

cheenamalhotra
cheenamalhotra previously approved these changes Jun 18, 2025
View reviewed changes
Copy link
Member

@cheenamalhotra cheenamalhotra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems integration (manual) tests using SqlDataReader to test SqlJson are not running, which should be running now against Azure for a more comprehensive code coverage around Json feature.

Since its out of scope for this PR, can you create an issue and we can try assigning to Copilot to configure the same (after PR #3429 is merged) - and we should be able to run those tests against Azure SQL DBs now.

@codecov Codecov
Copy link

codecov bot commented Jun 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.83%. Comparing base (78dec95) to head (c0791a8).
Report is 12 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3427      +/-   ##
==========================================
+ Coverage   63.59%   69.83%   +6.23%     
==========================================
  Files         299      280      -19     
  Lines       65418    62169    -3249     
==========================================
+ Hits        41601    43414    +1813     
+ Misses      23817    18755    -5062
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 72.92% <100.00%> (+6.04%) ⬆️
netfx 69.33% <100.00%> (+3.30%) ⬆️

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.

@paulmedynski paulmedynski linked an issue Jun 18, 2025 that may be closed by this pull request
[API Proposal] Add StringSyntaxAttributes to SqlJson #3107
Closed
@paulmedynski
Issue 37739: GH 3424 Add SqlJson ToString()
75f87e6 
- Added StringSyntax JSON attribute to the string constructor.
- Disposing of JsonDocument used for validation.
- Enabled CA2000 analyzer as a suggestion.
paulmedynski
paulmedynski commented Jun 18, 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.

Move commentary for reviewers.

benrr101
benrr101 requested changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Mostly just looking for changes to the tests.

benrr101
benrr101 previously approved these changes Jun 19, 2025
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

A few nits, but I won't hold it up on this. My tolerance level for test code is a lot higher than prod code.

campersau
campersau reviewed Jun 20, 2025
View reviewed changes
Copy link

@campersau campersau left a comment

Choose a reason for hiding this comment

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

Test using nits.

@paulmedynski
Issue 37739: GH 3424 Add SqlJson ToString()
4ceb419 
- Added some missing usings.
cheenamalhotra
cheenamalhotra reviewed Jun 24, 2025
View reviewed changes
Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

Rest everything looks good!

@paulmedynski
Issue 37739: GH 3424 Add SqlJson ToString()
c0791a8 
- Added ToString() to SqlJson in the ref projects.
@cheenamalhotra cheenamalhotra merged commit 195de03 into main Jun 25, 2025
251 checks passed
@cheenamalhotra cheenamalhotra deleted the dev/paul/json-tostring branch June 25, 2025 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@campersau campersau campersau left review comments

@edwardneal edwardneal edwardneal left review comments

Copilot code review Copilot Copilot left review comments

@benrr101 benrr101 benrr101 approved these changes

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@apoorvdeshmukh apoorvdeshmukh Awaiting requested review from apoorvdeshmukh

Assignees
No one assigned
Labels
Area\Json Use this for issues that are targeted for the Json feature in the driver. P2 Use to label moderate priority issue - impacts atleast more than 1 customer. Public API 🆕 Issues/PRs that introduce new APIs to the driver.
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

Microsoft.DataSqlTypes.SqlJson is missing ToString [API Proposal] Add StringSyntaxAttributes to SqlJson
6 participants
@paulmedynski @benrr101 @campersau @apoorvdeshmukh @cheenamalhotra @edwardneal