Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The parser-wide refactor touches protocol-critical encryption, authentication, bulk-copy, and reader paths requiring final human and CI validation.
Pull request overview
Normalizes internal TDS metadata and token models without intended behavioral changes.
Changes:
- Replaces legacy metadata types with consistently named
Tds*types. - Converts eligible fields to properties and adds documentation.
- Updates parser, authentication, encryption, bulk-copy, reader, and tests.
File summaries
| File | Description |
|---|---|
SqlMetaDataSetTest.cs |
Updates metadata clone tests. |
SqlEnvChange.cs |
Uses renamed routing metadata. |
SqlDbColumn.cs |
Uses normalized column properties. |
SqlDataReader.cs |
Migrates reader metadata usage. |
SqlConnection.cs |
Updates UDT metadata handling. |
SqlCommand.cs |
Updates cached and return metadata. |
SqlCachedBuffer.cs |
Uses TdsTypeInfo. |
SqlBulkCopy.cs |
Migrates bulk-copy metadata. |
TdsTypeInfo.cs |
Adds normalized type metadata model. |
TdsTableName.cs |
Renames multipart table metadata. |
TdsReturnValueToken.cs |
Adds return-value token model. |
TdsFedAuthInfoToken.cs |
Adds federated-auth token model. |
TdsEnvChangeRoutingInfo.cs |
Adds routing metadata model. |
TdsColumnMetadataToken.cs |
Adds column metadata token. |
TdsColumnMetadata.cs |
Adds per-column metadata model. |
TdsAltMetadataCollection.cs |
Adds alternate metadata collection. |
SqlReturnValue.cs |
Removes superseded return model. |
SqlMetaDataPriv.cs |
Removes legacy type metadata. |
SqlFedAuthInfo.cs |
Removes legacy auth metadata. |
RoutingInfo.cs |
Removes legacy routing model. |
_SqlMetaDataSetCollection.cs |
Removes legacy alternate collection. |
_SqlMetaDataSet.cs |
Removes legacy metadata set. |
_SqlMetaData.cs |
Removes legacy column metadata. |
TdsParserStateObject.cs |
Updates parser snapshot metadata. |
TdsParser.cs |
Migrates token processing and serialization. |
SqlConnectionInternal.cs |
Updates routing and authentication consumers. |
ServerInfo.cs |
Uses normalized routing information. |
Review details
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsTableName.cs:42
- This TODO has duplicated comparative wording and writes “every time” as one word.
- Files reviewed: 27/27 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
65fd7dc to
e94650e
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The broad core-parser refactor spans metadata, encryption, routing, and authentication paths that require CI and final human validation.
Review details
Suppressed comments (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsReturnValueToken.cs:25
- Correct the type name in this constructor documentation.
/// Constructs a new instance of TddsReturnValueToken with the value initialized to a new
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsColumnMetadata.cs:12
- Correct the typo in the token description.
/// single column withing a SQLCOLMETADATA/SQLALTMETADATA token.
- Files reviewed: 27/27 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
+Comments
+Comments
e94650e to
7646a44
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The refactor preserves existing behavior; remaining feedback concerns minor documentation accuracy and spelling.
Review details
Suppressed comments (5)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsColumnMetadata.cs:12
- “withing” is a typo in the new type summary.
/// single column withing a SQLCOLMETADATA/SQLALTMETADATA token.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsReturnValueToken.cs:25
- The constructor documentation misspells the type name as
TddsReturnValueToken.
/// Constructs a new instance of TddsReturnValueToken with the value initialized to a new
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsTableName.cs:42
- This sentence contains both a duplicated comparison (“versus rather than”) and the misspelling “everytime”.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsFedAuthInfoToken.cs:37 - The SPN from FEDAUTHINFO is used as the token resource/audience (
resource: fedAuthInfoToken.SpninSqlConnectionInternal.cs), not specifically as a Kerberos service identifier. The new documentation should describe its actual use in this path.
/// Gets the Service Principal Name (SPN) associated with the federated authentication token.
/// The SPN is used in authentication processes to uniquely identify a service instance
/// in the context of Kerberos-based security mechanisms.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsEnvChangeRoutingInfo.cs:34
Protocolis populated for both regular routing and enhanced routing ENVCHANGE tokens, so describing it as enhanced-routing-only is inaccurate.
/// Represents the protocol used for routing in an EnhancedRouting ENVCHANGE token.
- Files reviewed: 27/27 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It rewires metadata types throughout core TDS parsing and execution paths, warranting human review and full CI validation.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsTableName.cs:42
- Correct the duplicated comparison phrase and spell “every time” as two words.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsColumnMetadataToken.cs:122
- This is not a deep copy:
CekTable,DbColumnSchema,SchemaTable, and_visibleColumnMapare retained by reference, while each column clone also sharesBaseTypeInfoandCipherMetadata. Please describe the selective cloning so callers do not assume mutations to all nested metadata are isolated.
/// Creates a new instance of TdsColumnMetadataToken that is a deep copy of the current
/// instance.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsAltMetadataCollection.cs:28
- “Deep copy” is also inaccurate here because each token clone retains shared schema, CEK, base-type, and cipher-metadata objects. Describe only the collection/token cloning that actually occurs.
/// Creates a deep copy of the current TdsAltMetadataCollection instance, including all its elements.
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It spans protocol parsing, encryption, routing, and bulk-copy paths, warranting final human review and full CI validation.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsTableName.cs:42
- The TODO has a duplicated comparison phrase and writes “every time” as one word.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsColumnMetadata.cs:30 UpdateableUnknownmisspells “updatable” and is inconsistent with the adjacentUpdatablemember. Rename it toUpdatableUnknownin both the declaration and mask.
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Description
Normalizes internal TDS token metadata names so each type clearly identifies the protocol token or metadata concept it represents.
Sql*and underscore-prefixed metadata types to consistentTds*names.This is layer 2 of 3 in the TDS parser organization stack. It targets
dev/russellben/tds-groups, so review should focus on this PR's token-model changes rather than the parser moves in the preceding layer.Issues
No linked issue; this is an internal naming and maintainability refactor.
Testing
No behavior change is intended. Existing metadata unit tests were updated with the renamed token types; CI will exercise the complete dependent branch.
Guidelines