Feature | Add scaffolding for SSRP response parsing #3741
Conversation
This consists of: * ReadOnlySequenceSegment derivative, * Utilities to read data from a ReadOnlySequence<byte> * Test cases
|
/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.
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.