bugfix: always encrypted result metadata for logical types - #519
bugfix: always encrypted result metadata for logical types#519Lakshay Chauhan (nos1dot618) wants to merge 2 commits into
Conversation
Always Encrypted columns currently expose their ciphertext/wire type through ColumnMetadata::data_type and type_info, even though mssql-tds already knows the plaintext type from CryptoMetadata. Add effective_data_type() and effective_type_info() to expose the logical plaintext metadata to higher-level consumers. Also update get_precision(), get_scale(), and get_collation() to use the effective type information so encrypted columns report metadata for their underlying SQL type. Keep the existing data_type/type_info fields unchanged for TDS wire decoding.
Use the logical column type and type information when constructing Python result metadata for Always Encrypted columns. Previously, mssql-py-core used the wire/ciphertext data type and type information exposed by mssql-tds, which could cause encrypted columns to be reported as binary values even though the decrypted result was returned as the underlying SQL type. Use ColumnMetadata::effective_data_type() and ColumnMetadata::effective_type_info() when determining Python types, column sizes, and decimal digits so result metadata matches the decrypted values.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate metadata mismatches remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes Always Encrypted result metadata by exposing logical plaintext types while preserving wire-level metadata for TDS decoding.
Changes:
- Adds effective data-type and type-info accessors.
- Updates metadata helpers and Python result descriptions.
- Adds encrypted metadata tests.
Required fixes:
- Make effective metadata conditional on result decryption being enabled.
- Use effective PLP metadata when calculating
column_size.
File summaries
| File | Description |
|---|---|
mssql-tds/src/query/metadata.rs |
Adds logical metadata accessors and tests; must handle disabled per-command decryption. |
mssql-py-core/src/async_description.rs |
Uses logical metadata for Python descriptions; PLP sizing remains incorrect for bounded encrypted columns. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| match metadata.data_type { | ||
| match metadata.effective_data_type() { |
| pub fn effective_data_type(&self) -> TdsDataType { | ||
| self.crypto_metadata | ||
| .as_ref() | ||
| .map(|c| c.base_data_type) | ||
| .unwrap_or(self.data_type) |
Saurabh Singh (saurabh500)
left a comment
Saurabh Singh (saurabh500)
left a comment
There was a problem hiding this comment.
🤖 Unattended review sweep. This review was generated by an automated sweep and posted without a human checking the findings first. Treat it as reviewer input, not a gate. Event is COMMENT only.
Summary
Exposes the logical (plaintext) type for Always Encrypted result columns via new effective_data_type()/effective_type_info() accessors, and routes get_scale/get_precision/get_collation plus the mssql-py-core cursor description through them. Fixes the #469 mismatch for the primary path (connection CE enabled + decryption active) — the only path reachable through the modified py-core consumer — and the lower-layer accessors have mutation-verified unit coverage.
What I verified: the decrypt path decodes plaintext in security/encryption/cell.rs from base_type_info directly, so it is unaffected by the get_scale/get_precision switch; the wire row decoder reads get_scale() only in temporal/decimal branches keyed on the wire data_type, and an encrypted column is a binary ciphertext type on the wire, so those branches never see the effective values — no decode regression; mssql-odbc describe_col keys on the wire data_type and is unchanged for encrypted columns. Reverting the two accessors to ignore crypto_metadata fails 3 of the 4 new tests, so they are not vacuous.
Verdict
Approve-leaning COMMENT. No blocking findings. A few suggestions about the effective-metadata contract's edges and test coverage; none must block merge, but the doc-contract one (Suggestion 1) is worth resolving in this PR since it touches a public API.
Blocking
None.
Suggestion
- Effective-metadata accessors are unconditional, but the doc claims they are delivery-scoped. Inline at
mssql-tds/src/query/metadata.rs:176. column_sizestill gates on the wireis_plp(), leaving large encrypted columns at size 0. Inline atmssql-py-core/src/async_description.rs:118.- The consumer-facing changes are untested. All 4 new tests live in
mssql-tdsand cover the accessors + precision/scale. The mssql-py-core changes (python_type/column_size/decimal_digits) have no added test, and no new fixture uses a string base type — so theget_collation()→ effective switch (which also changes mssql-js, see Nit 1) is unguarded. Add a py-core description test for an encrypted column and a string-baseget_collationcase.
Nit
- The description scopes the change to mssql-py-core, but switching
get_collation()to effective also changes mssql-js (ffidatatypes.rs:323) for encrypted string columns. Worth naming in the description. - mssql-py-core is excluded from the cargo workspace, so the checklist's
cargo bclippy/cargo bfmt(workspace-only) do not cover it. Confirm they were run against mssql-py-core separately — the note thatcargo btestcould not fully run locally is fine and honest.
Rubber-duck pass changed three things before posting, each re-verified against the tree: narrowed "well-tested" to the lower layer only (Suggestion 3), broadened Suggestion 1 to the ResultSetOnly/return-value path and reframed it as a contract choice, and softened Suggestion 2 to flag the outbound-vs-inbound evidence gap.
| /// | ||
| /// [`ColumnMetadata::data_type`] describes the type used for the value on the | ||
| /// TDS wire, which may be a ciphertext/binary type for encrypted columns. | ||
| pub fn effective_data_type(&self) -> TdsDataType { |
There was a problem hiding this comment.
Suggestion. effective_data_type()/effective_type_info() (and the get_scale/get_precision/get_collation switch just above) select plaintext whenever crypto_metadata is Some, unconditionally. But the doc says "type information for the logical value delivered to consumers", and that only holds when the row was actually decrypted.
Under a per-command ExecutionColumnEncryptionSetting::Disabled, resolve_cell_decryptor returns None and decode_or_decrypt_column returns the raw ciphertext varbinary (io/token_stream.rs, the (true, None) arm), while these accessors still report the plaintext type — the same metadata/value mismatch this PR fixes, reversed. Return values have the same gap under Disabled and ResultSetOnly (see the finalize_return_value doc at connection/tds_client.rs:4761). Issue #469 itself scopes the fix to "whenever column decryption is active."
Not reachable through mssql-py-core today (it does not expose the per-command CE setting, and a connection without CE never populates crypto_metadata), so no current consumer is wrong — this is about the mssql-tds public contract. Please pick one contract and make the docs match:
- Schema contract — these describe the underlying column regardless of what this execution delivered. Reword the doc (drop "delivered to consumers") and note that callers wanting the raw representation use
data_type/type_info. Then this PR is fine as-is. - Delivery contract — carry whether decryption happened and gate the selection on it.
Either is defensible; today the doc wording asserts the delivery contract while the code implements the schema contract.
| } | ||
|
|
||
| match metadata.data_type { | ||
| match metadata.effective_data_type() { |
There was a problem hiding this comment.
Suggestion. The early if metadata.is_plp() { return 0; } just above tests the wire type_info. Encrypted columns can be PLP on the wire — the codebase already recognizes this case (io/token_stream.rs:592, "Always Encrypted paused PLP streaming"). So a bounded encrypted column whose ciphertext is transmitted as varbinary(max) returns column_size == 0 even though its effective type has a finite length, leaving this part of #469 unfixed for large encrypted string/binary columns while the rest of the function was converted to effective_*.
Evidence gap worth closing first: the only citation for "encrypted ciphertext is PLP" is the outbound RPC path (rpc_parameters.rs); I could not confirm from here that inbound COLMETADATA advertises a bounded-plaintext encrypted column as varbinary(max) without a live AE server. If it does, add an effective/logical PLP predicate and use it here — keep the existing wire is_plp() for the decoder/streaming callers that need wire semantics:
if metadata.effective_is_plp() {
return 0;
}and test both: wire-PLP + bounded logical (e.g. nvarchar(4000)) → 4000, and wire-PLP + logical nvarchar(max) → 0. Not a regression — pre-PR also returned 0.
Description
Fix result-set metadata for Always Encrypted columns.
mssql-tdscurrently retains the plaintext SQL type inCryptoMetadata, but the publicColumnMetadata::data_typeandtype_infodescribe the ciphertext representation used on the TDS wire. This caused higher-level consumers such asmssql-py-coreto report encrypted columns as binary values even though the decrypted values were returned using their underlying SQL types.This change adds
effective_data_type()andeffective_type_info()toColumnMetadatafor accessing the logical plaintext metadata while preserving the existing wire-level metadata for TDS decoding.The metadata helpers for precision, scale, and collation now use the effective type information.
mssql-py-corealso uses the effective metadata when constructing Python result descriptions, including Python type, column size, and decimal digits.Tests were added to cover encrypted and unencrypted metadata as well as precision and scale handling for encrypted columns.
Related Issues
Checklist
cargo bfmtpassescargo bclippypassescargo btestpassescargo btestwas run locally, but the full suite did not complete successfully. The failures were environment-related: the local test environment was missing the required SQL Server/certificate setup. In particular, connectivity tests failed withConnectionRefused, and certificate/TLS tests reported failures related to the unavailable certificate fixtures/environment.