Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions mssql-py-core/src/async_description.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub(crate) async fn materialize(
}

fn python_type<'py>(py: Python<'py>, metadata: &ColumnMetadata) -> PyResult<Bound<'py, PyType>> {
let python_type = match metadata.data_type {
let python_type = match metadata.effective_data_type() {
TdsDataType::Int1
| TdsDataType::Int2
| TdsDataType::Int4
Expand Down Expand Up @@ -115,12 +115,12 @@ fn column_size(metadata: &ColumnMetadata) -> u64 {
return 0;
}

match metadata.data_type {
match metadata.effective_data_type() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

TdsDataType::Int1 => 3,
TdsDataType::Int2 => 5,
TdsDataType::Int4 => 10,
TdsDataType::Int8 => 19,
TdsDataType::IntN => match metadata.type_info.length {
TdsDataType::IntN => match metadata.effective_type_info().length {
1 => 3,
2 => 5,
4 => 10,
Expand All @@ -130,7 +130,7 @@ fn column_size(metadata: &ColumnMetadata) -> u64 {
TdsDataType::Bit | TdsDataType::BitN => 1,
TdsDataType::Flt4 => 7,
TdsDataType::Flt8 => 15,
TdsDataType::FltN => match metadata.type_info.length {
TdsDataType::FltN => match metadata.effective_type_info().length {
4 => 7,
8 => 15,
_ => 0,
Expand All @@ -142,7 +142,7 @@ fn column_size(metadata: &ColumnMetadata) -> u64 {
}
TdsDataType::DateTime => 23,
TdsDataType::DateTim4 => 16,
TdsDataType::DateTimeN => match metadata.type_info.length {
TdsDataType::DateTimeN => match metadata.effective_type_info().length {
8 => 23,
4 => 16,
_ => 0,
Expand All @@ -163,18 +163,18 @@ fn column_size(metadata: &ColumnMetadata) -> u64 {
| TdsDataType::Money4
| TdsDataType::MoneyN => u64::from(metadata.get_precision().unwrap_or(0)),
TdsDataType::NChar | TdsDataType::NVarChar | TdsDataType::NText => {
(metadata.type_info.length / 2) as u64
(metadata.effective_type_info().length / 2) as u64
}
_ => metadata.type_info.length as u64,
_ => metadata.effective_type_info().length as u64,
}
}

fn decimal_digits(metadata: &ColumnMetadata) -> u8 {
match metadata.data_type {
match metadata.effective_data_type() {
TdsDataType::Money | TdsDataType::Money4 | TdsDataType::MoneyN => 4,
TdsDataType::DateTime => 3,
TdsDataType::DateTim4 => 0,
TdsDataType::DateTimeN => match metadata.type_info.length {
TdsDataType::DateTimeN => match metadata.effective_type_info().length {
8 => 3,
_ => 0,
},
Expand Down
126 changes: 123 additions & 3 deletions mssql-tds/src/query/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl ColumnMetadata {
/// Returns `Some(scale)` for types that include scale information (e.g., `decimal(18,4)`, `time(7)`),
/// or `None` for types where scale is not applicable.
pub fn get_scale(&self) -> Option<u8> {
match self.type_info.type_info_variant {
match self.effective_type_info().type_info_variant {
TypeInfoVariant::VarLenScale(_, scale) => Some(scale),
TypeInfoVariant::VarLenPrecisionScale(_, _, _, scale) => Some(scale),
_ => None,
Expand All @@ -141,7 +141,7 @@ impl ColumnMetadata {
pub fn get_precision(&self) -> Option<u8> {
use crate::datatypes::sqldatatypes::{FixedLengthTypes, VariableLengthTypes};

match self.type_info.type_info_variant {
match self.effective_type_info().type_info_variant {
TypeInfoVariant::VarLenPrecisionScale(_, _, precision, _) => Some(precision),
TypeInfoVariant::FixedLen(FixedLengthTypes::Money) => Some(19),
TypeInfoVariant::FixedLen(FixedLengthTypes::Money4) => Some(10),
Expand All @@ -158,12 +158,42 @@ impl ColumnMetadata {
pub fn get_collation(&self) -> Option<SqlCollation> {
// Collation is only applicable to string types which are either VarLen strings
// Or PLP types with a collation.
match self.type_info.type_info_variant {
match self.effective_type_info().type_info_variant {
TypeInfoVariant::VarLenString(_, _, collation) => collation,
TypeInfoVariant::PartialLen(_, _, collation, _, _) => collation,
_ => None,
}
}

/// Returns the logical SQL Server data type of the column.
///
/// For an Always Encrypted column, this is the plaintext type stored in the
/// column before encryption. For non-encrypted columns, this is the same as
/// [`ColumnMetadata::data_type`].
///
/// [`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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

self.crypto_metadata
.as_ref()
.map(|c| c.base_data_type)
.unwrap_or(self.data_type)
Comment on lines +176 to +180
}

/// Returns type information for the logical value delivered to consumers.
///
/// For an Always Encrypted column, this is the plaintext type information from
/// the column's encryption metadata. For non-encrypted columns, this is the
/// same as [`ColumnMetadata::type_info`].
///
/// The wire-level [`ColumnMetadata::type_info`] remains available for decoding
/// the ciphertext internally.
pub fn effective_type_info(&self) -> &TypeInfo {
self.crypto_metadata
.as_ref()
.map(|c| &c.base_type_info)
.unwrap_or(&self.type_info)
}
}

/// Wire encoding of a PLP (partially-length-prefixed) column, returned by
Expand Down Expand Up @@ -331,6 +361,28 @@ mod tests {
}
}

fn create_encrypted_test_column_metadata(
wire_data_type: TdsDataType,
wire_type_info_variant: TypeInfoVariant,
base_type_info: TypeInfo,
) -> ColumnMetadata {
let mut metadata = create_test_column_metadata(0x0800, wire_type_info_variant);

metadata.data_type = wire_data_type;
metadata.type_info.tds_type = wire_data_type;
metadata.crypto_metadata = Some(CryptoMetadata {
cek_table_ordinal: 0,
base_data_type: base_type_info.tds_type,
base_type_info,
cipher_algorithm_id: 2,
cipher_algorithm_name: None,
encryption_type: 1,
normalization_rule_version: 1,
});

metadata
}

#[test]
fn test_is_nullable() {
let metadata =
Expand Down Expand Up @@ -740,4 +792,72 @@ mod tests {
assert!(rendered.contains("encrypted_key_len: 4"));
assert!(rendered.contains("RSA_OAEP"));
}

#[test]
fn test_effective_data_type_encrypted() {
let metadata = create_encrypted_test_column_metadata(
TdsDataType::VarBinary,
TypeInfoVariant::VarLen(VariableLengthTypes::VarBinary, 8000),
TypeInfo {
tds_type: TdsDataType::Int4,
length: 4,
type_info_variant: TypeInfoVariant::FixedLen(FixedLengthTypes::Int4),
},
);

assert!(metadata.is_encrypted());
// Wire metadata remains the ciphertext representation.
assert_eq!(metadata.data_type, TdsDataType::VarBinary);
// Effective metadata describes the decrypted value.
assert_eq!(metadata.effective_data_type(), TdsDataType::Int4);
}

#[test]
fn test_effective_data_type_unencrypted() {
let metadata =
create_test_column_metadata(0x00, TypeInfoVariant::FixedLen(FixedLengthTypes::Int4));

assert!(!metadata.is_encrypted());
assert_eq!(metadata.data_type, TdsDataType::IntN);
assert_eq!(metadata.effective_data_type(), TdsDataType::IntN);
}

#[test]
fn test_precision_and_scale_use_effective_type_info_for_encrypted_column() {
let metadata = create_encrypted_test_column_metadata(
TdsDataType::VarBinary,
TypeInfoVariant::VarLen(VariableLengthTypes::VarBinary, 8000),
TypeInfo {
tds_type: TdsDataType::DecimalN,
length: 17,
type_info_variant: TypeInfoVariant::VarLenPrecisionScale(
VariableLengthTypes::DecimalN,
17,
18,
4,
),
},
);

assert_eq!(metadata.get_precision(), Some(18));
assert_eq!(metadata.get_scale(), Some(4));
}

#[test]
fn test_effective_type_info_encrypted() {
let metadata = create_encrypted_test_column_metadata(
TdsDataType::VarBinary,
TypeInfoVariant::VarLen(VariableLengthTypes::VarBinary, 8000),
TypeInfo {
tds_type: TdsDataType::Int4,
length: 4,
type_info_variant: TypeInfoVariant::FixedLen(FixedLengthTypes::Int4),
},
);

assert!(matches!(
metadata.effective_type_info().type_info_variant,
TypeInfoVariant::FixedLen(FixedLengthTypes::Int4)
));
}
}