RUST-2196 Require ignored bits in packed bit vector to be 0 (#586)

Author: isabelatkinson

src/binary/vector.rs

@@ -87,18 +87,30 @@ impl PackedBitVector {
     /// ```
     ///
     /// Padding must be within 0-7 inclusive. Padding must be 0 or unspecified if the provided
-    /// vector is empty.
+    /// vector is empty. The ignored bits in the vector must all be 0.
     pub fn new(vector: Vec<u8>, padding: impl Into<Option<u8>>) -> Result<Self> {
         let padding = padding.into().unwrap_or(0);
         if !(0..8).contains(&padding) {
             return Err(Error::binary(format!(
                 "vector padding must be within 0-7 inclusive, got {padding}"
             )));
         }
-        if padding != 0 && vector.is_empty() {
-            return Err(Error::binary(format!(
-                "cannot specify non-zero padding if the provided vector is empty, got {padding}",
-            )));
+        match vector.last() {
+            Some(last) => {
+                if last.trailing_zeros() < u32::from(padding) {
+                    return Err(Error::binary(
+                        "the ignored bits in a packed bit vector must all be 0",
+                    ));
+                }
+            }
+            None => {
+                if padding != 0 {
+                    return Err(Error::binary(format!(
+                        "cannot specify non-zero padding if the provided vector is empty, got \
+                         {padding}"
+                    )));
+                }
+            }
         }
         Ok(Self { vector, padding })
     }

src/tests/spec/json/bson-binary-vector/README.md

@@ -55,6 +55,69 @@ MUST assert that the input float array is the same after encoding and decoding.
 - if the canonical_bson field is present, raise an exception when attempting to deserialize it into the corresponding
     numeric values, as the field contains corrupted data.
 
+## Prose Tests
+
+### Treatment of non-zero ignored bits
+
+All drivers MUST test encoding and decoding behavior according to their design and version. For drivers that haven't
+been completed, raise exceptions in both cases. For those that have, update to this behavior according to semantic
+versioning rules, and update tests accordingly.
+
+In both cases, [255], a single byte PACKED_BIT vector of length 1 (hence padding of 7) provides a good example to use,
+as all of its bits are ones.
+
+#### 1. Encoding
+
+- Test encoding with non-zero ignored bits. Use the driver API that validates vector metadata.
+- If the driver validates ignored bits are zero (preferred), expect an error. Otherwise expect the ignored bits are
+    preserved.
+
+```python
+with pytest.raises(ValueError):
+    Binary.from_vector([0b11111111], BinaryVectorDtype.PACKED_BIT, padding=7)
+```
+
+### 2. Decoding
+
+- Test the behaviour of your driver when one attempts to decode from binary to vector.
+    - e.g. As of pymongo 4.14, a warning is raised. From 5.0, it will be an exception.
+
+```python
+b = Binary(b'\x10\x07\xff', subtype=9)
+with pytest.warns():
+    Binary.as_vector(b)
+```
+
+Drivers MAY skip this test if they choose not to implement a `Vector` type.
+
+### 3. Comparison
+
+Once we can guarantee that all ignored bits are non-zero, then equality can be tested on the binary subtype. Until then,
+equality is ambiguous, and depends on whether one compares by bits (uint1), or uint8. Drivers SHOULD test equality
+behavior according to their design and version.
+
+For example, in `pymongo < 5.0`, we define equality of a BinaryVector by matching padding, dtype, and integer. This
+means that two single bit vectors in which 7 bits are ignored do not match unless all bits match. This mirrors what the
+server does.
+
+```python
+b1 = Binary(b'\x10\x07\x80', subtype=9) # 1-bit vector with all 0 ignored bits.
+b2 = Binary(b'\x10\x07\xff', subtype=9) # 1-bit vector with all 1 ignored bits.
+b3 = Binary.from_vector([0b10000000], BinaryVectorDtype.PACKED_BIT, padding=7) # Same data as b1.
+
+v1 = Binary.as_vector(b1)
+v2 = Binary.as_vector(b2)
+v3 = Binary.as_vector(b3)
+
+assert b1 != b2  # Unequal at naive Binary level 
+assert v2 != v1  # Also chosen to be unequal at BinaryVector level as [255] != [128]
+assert b1 == b3  # Equal at naive Binary level
+assert v1 == v3  # Equal at the BinaryVector level
+```
+
+Drivers MAY skip this test if they choose not to implement a `Vector` type, or the type does not support comparison, or
+the type cannot be constructed with non-zero ignored bits.
+
 ## FAQ
 
 - What MongoDB Server version does this apply to?

src/tests/spec/json/bson-binary-vector/packed_bit.json

@@ -21,22 +21,22 @@
       "canonical_bson": "1600000005766563746F7200040000000910007F0700"
     },
     {
-      "description": "Empty Vector PACKED_BIT",
+      "description": "PACKED_BIT with padding",
       "valid": true,
-      "vector": [],
+      "vector": [127, 8],
       "dtype_hex": "0x10",
       "dtype_alias": "PACKED_BIT",
-      "padding": 0,
-      "canonical_bson": "1400000005766563746F72000200000009100000"
+      "padding": 3,
+      "canonical_bson": "1600000005766563746F7200040000000910037F0800"
     },
     {
-      "description": "PACKED_BIT with padding",
+      "description": "Empty Vector PACKED_BIT",
       "valid": true,
-      "vector": [127, 7],
+      "vector": [],
       "dtype_hex": "0x10",
       "dtype_alias": "PACKED_BIT",
-      "padding": 3,
-      "canonical_bson": "1600000005766563746F7200040000000910037F0700"
+      "padding": 0,
+      "canonical_bson": "1400000005766563746F72000200000009100000"
     },
     {
       "description": "Overflow Vector PACKED_BIT",

src/tests/spec/vector.rs

@@ -210,3 +210,23 @@ fn run_test_file(test_file: TestFile) {
 fn run_vector_tests() {
     run_spec_test(&["bson-binary-vector"], run_test_file);
 }
+
+#[test]
+fn non_zero_ignored_bits() {
+    // 1. Encoding
+    let error = PackedBitVector::new(vec![u8::MAX], 1).unwrap_err();
+    assert!(error
+        .message
+        .is_some_and(|message| message.contains("must all be 0")));
+
+    // 2. Decoding
+    let bytes = vec![PACKED_BIT, 1, u8::MAX];
+    let error = Vector::from_bytes(bytes).unwrap_err();
+    assert!(error
+        .message
+        .is_some_and(|message| message.contains("must all be 0")));
+
+    // 3. Comparison
+    // This test is not implemented because it is not possible to construct a Vector with non-zero
+    // padding bits.
+}