Feature | Add scaffolding for SSRP response parsing #3741
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces test infrastructure and placeholder unit tests for SSRP (SQL Server Resolution Protocol) packet processing functionality. The changes lay the groundwork for testing both SQL Data Source responses and DAC (Dedicated Admin Connection) responses.
- Adds comprehensive test data generation utilities for creating and validating SSRP packet structures
- Implements utility classes for working with
ReadOnlySequence<byte>to parse binary network packets - Creates placeholder test methods marked with
ActiveIssuethat reference a planned implementation (issue #3700)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SsrpPacketTestData.cs | Provides test data generators for various valid and invalid SSRP packet scenarios, including SVR_RESP messages, DAC responses, and edge cases |
| SqlDataSourceResponseProcessorTest.cs | Placeholder unit tests for SQL Data Source response processing, covering empty buffers, invalid responses, and valid response data |
| DacResponseProcessorTest.cs | Placeholder unit tests for DAC response processing with similar coverage patterns |
| CodeAnalysis.cs | Adds NotNullWhenAttribute for better null-state analysis in conditional scenarios |
| ReadOnlySequenceUtilities.cs | Extension methods for reading bytes and little-endian ushort values from ReadOnlySequence<byte> |
| PacketBuffer.cs | Linked list node implementation for building ReadOnlySequence<byte> from multiple buffers |
| netfx/.../Microsoft.Data.SqlClient.csproj | Includes new common source files in .NET Framework project |
| netcore/.../Microsoft.Data.SqlClient.csproj | Includes new common source files in .NET Core project |
| /// <see cref="DacResponseProcessorTest.Process_EmptyBuffer_ReturnsFalse"/> | ||
| /// <see cref="SqlDataSourceResponseProcessorTest.Process_EmptyBuffer_ReturnsFalse"/> |
There was a problem hiding this comment.
The <see cref> tags reference test methods, but the standard pattern is to reference types or members that are part of the code contract, not test methods. Consider using plain text or comments instead of <see cref> for test method references, as these create documentation dependencies that can break if test method names change.
There was a problem hiding this comment.
I'd personally describe the test method and the test's input to be part of the same overall test contract. <see cref> is appropriate here.
| public static bool ReadByte(this ref ReadOnlySequence<byte> sequence, ref ReadOnlySpan<byte> currSpan, ref long currPos, out byte value) | ||
| { | ||
| if (sequence.Length < sizeof(byte)) |
There was a problem hiding this comment.
The currPos parameter is incremented but its updated value may not reflect the actual state if the sequence is too short (line 23-27). Consider moving the increment after the length check succeeds, or document that callers should not rely on currPos when the method returns false.
There was a problem hiding this comment.
currPos is the current position in the ReadOnlySequence<byte>. If there's not enough space in the sequence, the method doesn't advance and it's thus appropriate to leave currPos untouched.
| public static bool ReadLittleEndian(this ref ReadOnlySequence<byte> sequence, ref ReadOnlySpan<byte> currSpan, ref long currPos, out ushort value) | ||
| { | ||
| if (sequence.Length < sizeof(ushort)) |
There was a problem hiding this comment.
Similar to the ReadByte method, the currPos parameter is incremented (line 69) before the actual read operation, which could leave it in an inconsistent state if the sequence length check fails. Consider moving the increment after successful validation or documenting this behavior.
There was a problem hiding this comment.
As per the ReadByte method - currPos is always in a consistent state. Once execution reaches line 69, there's guaranteed to be enough space in the ReadOnlySpan<byte> - the only question is whether we can directly read it from the current span, or need to reassemble it because byte 1 is in the current span and byte 2 is in the next span.
|
This pull request has been marked as stale due to inactivity for more than 30 days. If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days. |
github-actions
bot
added
the
Stale
|
This PR is not stale. |
|
@edwardneal Can you rebase to main (no merge commit) to pickup the latest pipeline and CodeQL configs? That should eliminate the unnecessary CI checks and unblock the Code scanning. |
|
Since this is a new feature, we will need to prioritize our review of it against existing priorities, so this may not see much attention until early 2026. @edwardneal Do you think you could have all of the work for SSRP ready for the 7.0 GA in March? |
paulmedynski
added
Approval Needed
This consists of: * ReadOnlySequenceSegment derivative, * Utilities to read data from a ReadOnlySequence<byte> * Test cases
edwardneal
force-pushed
the
feat/ssrp/01-scaffolding
branch
from
3e5d56f to
c8f6688
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Thanks @paulmedynski. I tried rebasing, but that went badly wrong and the CodeQL error persisted. I think I've fixed it now, and the build looks like it's triggered properly. March is enough time to implement the feature, and I have a local branch with the SSRP response parsing logic and test cases. I definitely wouldn't prioritise SqlDataSourceEnumerator above the other changes in 7.0 though, and I think it's likely to be a little more sensitive to review (since the only way it can work is by parsing untrusted network broadcasts.) I'm happy enough for this series of PRs to lie dormant until post-GA. Implementation doesn't introduce a breaking change, it just means that clients can enumerate SQL Server instances on the local LAN rather than having GetDataSources throw an exception. |
3 participants
Description
This is the first of four or five PRs which introduce an SSRP client to the managed SNI. This PR doesn't actually introduce any parsing code - it just adds test cases and lays some scaffolding/infrastructure down for the next one.
Key points of interest here:
ReadOnlySequence<byte>to store packet buffers, then read the SSRP responses from them. This introduces the use ofPacketBufferandReadOnlySequenceUtilities.ReadOnlySequenceUtilitiesis a workaround; netcore has aSequenceReader<T>type in-box. This isn't available to netfx though, and I didn't think it was worth maintaining two sets of fairly simple logic. We could alternatively bring theSequenceReader<T>andSequenceReaderExtensionssource code in-tree, as a few other libraries have done (examples 1 and 2.)Issues
Contributes to #3700.
Testing
There's nothing to test in this PR, so CI should validate it.