fix: improve Document compatibility/validation layer (#12016)

Author: anakin87

haystack/components/rankers/lost_in_the_middle.py

@@ -100,7 +100,7 @@ def run(
             return {"documents": documents_to_reorder}
 
         # Raise an error if any document is not textual
-        if any(not doc.content_type == "text" for doc in documents_to_reorder):
+        if any(doc.content is None for doc in documents_to_reorder):
             raise ValueError("Some provided documents are not textual; LostInTheMiddleRanker can process only text.")
 
         # Initialize word count and indices for the "lost in the middle" order

haystack/dataclasses/document.py

@@ -13,40 +13,23 @@
 from haystack.dataclasses.sparse_embedding import SparseEmbedding
 from haystack.utils.dataclasses import _warn_on_inplace_mutation
 
-LEGACY_FIELDS = ["content_type", "id_hash_keys", "dataframe"]
+_LEGACY_FIELDS = ["content_type", "id_hash_keys", "dataframe"]
 
 
-class _BackwardCompatible(type):
-    """
-    Metaclass that handles Document backward compatibility.
-    """
-
+class _RemoveLegacyFields(type):
     def __call__(cls, *args: Any, **kwargs: Any) -> Any:
         """
-        Called before Document.__init__, handles legacy fields.
-
-        Embedding was stored as NumPy arrays in 1.x, so we convert it to a list of floats.
-        Other legacy fields are removed.
+        Called before Document.__init__, removes the legacy fields.
         """
-        ### Conversion from 1.x Document ###
-        content = kwargs.get("content")
-        if content and not isinstance(content, str):
-            raise ValueError("The `content` field must be a string or None.")
-
-        # Embedding were stored as NumPy arrays in 1.x, so we convert it to the new type
-        if isinstance(embedding := kwargs.get("embedding"), ndarray):
-            kwargs["embedding"] = embedding.tolist()
-
-        # Remove legacy fields
-        for field_name in LEGACY_FIELDS:
+        for field_name in _LEGACY_FIELDS:
             kwargs.pop(field_name, None)
 
         return super().__call__(*args, **kwargs)
 
 
 @_warn_on_inplace_mutation
 @dataclass
-class Document(metaclass=_BackwardCompatible):  # noqa: PLW1641
+class Document(metaclass=_RemoveLegacyFields):  # noqa: PLW1641
     """
     Base data class containing some data to be queried.
 
@@ -70,6 +53,20 @@ class Document(metaclass=_BackwardCompatible):  # noqa: PLW1641
     embedding: list[float] | None = field(default=None)
     sparse_embedding: SparseEmbedding | None = field(default=None)
 
+    def __post_init__(self) -> None:
+        """
+        Checks content type, converts embedding from 1.x type and generates the ID based on the init parameters.
+        """
+        if self.content is not None and not isinstance(self.content, str):
+            raise ValueError("The `content` field must be a string or None.")
+
+        # Embeddings were stored as NumPy arrays in 1.x, so we convert them to the new type
+        if isinstance(self.embedding, ndarray):
+            self.embedding = self.embedding.tolist()
+
+        # Generate an id only if not explicitly set
+        self.id = self.id or self._create_id()
+
     def __repr__(self) -> str:
         fields = []
         if self.content is not None:
@@ -99,13 +96,6 @@ def __eq__(self, other: object) -> bool:
             return False
         return self.to_dict() == other.to_dict()
 
-    def __post_init__(self) -> None:
-        """
-        Generate the ID based on the init parameters.
-        """
-        # Generate an id only if not explicitly set
-        self.id = self.id or self._create_id()
-
     def _create_id(self) -> str:
         """
         Creates a hash of the given content that acts as the document's ID.
@@ -162,10 +152,10 @@ def from_dict(cls, data: dict[str, Any]) -> "Document":
         # ValueError later if this is the case.
         meta = data.pop("meta", {})
         # Unflatten metadata if it was flattened. We assume any keyword argument that's not
-        # a document field is a metadata key. We treat legacy fields as document fields
-        # for backward compatibility.
+        # a document field is a metadata key. We treat legacy fields as document fields, so that
+        # `_RemoveLegacyFields` drops them instead of them ending up in `meta`.
         flatten_meta = {}
-        document_fields = LEGACY_FIELDS + [f.name for f in fields(cls)]
+        document_fields = _LEGACY_FIELDS + [f.name for f in fields(cls)]
         for key in list(data.keys()):
             if key not in document_fields:
                 flatten_meta[key] = data.pop(key)

releasenotes/notes/document-validate-fields-in-post-init-6b2e9f4a1c8d375e.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    Improved the validation and compatibility layer of ``Document``:
+    - since Haystack 2.x, ``embedding`` is a list of floats, while it was a NumPy array in 1.x, so a NumPy
+      ``embedding`` is converted to a list for backward compatibility. The conversion only happened when ``embedding``
+      was passed as a keyword argument: passed positionally, the array was silently left as is, potentially causing
+      downstream issues.
+    - ``content`` type was validated only when passed as a keyword argument.
+    - ``content`` type validation was also skipped when the value was falsy, such as ``0`` or ``[]``, and failed later
+      with an unrelated error.

test/components/rankers/test_lost_in_the_middle.py

@@ -6,6 +6,7 @@
 
 from haystack import Document
 from haystack.components.rankers.lost_in_the_middle import LostInTheMiddleRanker
+from haystack.dataclasses.byte_stream import ByteStream
 
 
 class TestLostInTheMiddleRanker:
@@ -35,6 +36,12 @@ def test_lost_in_the_middle_order_two_docs(self):
         assert result["documents"][0].content == "1"
         assert result["documents"][1].content == "2"
 
+    def test_lost_in_the_middle_with_non_textual_documents(self):
+        ranker = LostInTheMiddleRanker()
+        docs = [Document(content="1"), Document(blob=ByteStream(b"some bytes"))]
+        with pytest.raises(ValueError, match="Some provided documents are not textual"):
+            ranker.run(documents=docs)
+
     def test_lost_in_the_middle_init(self):
         # tests that LostInTheMiddleRanker initializes with default values
         ranker = LostInTheMiddleRanker()

test/dataclasses/test_document.py

@@ -6,6 +6,7 @@
 from copy import deepcopy
 from dataclasses import replace
 
+import numpy
 import pytest
 
 from haystack import Document
@@ -63,47 +64,47 @@ def test_init_with_parameters():
     assert doc.sparse_embedding == sparse_embedding
 
 
-def test_init_with_legacy_fields():
-    doc = Document(
-        content="test text",
-        content_type="text",
-        id_hash_keys=["content"],
-        dataframe="placeholder",
-        score=0.812,
-        embedding=[0.1, 0.2, 0.3],  # type: ignore
-    )
-    assert doc.id == "18fc2c114825872321cf5009827ca162f54d3be50ab9e9ffa027824b6ec223af"
-    assert doc.content == "test text"
-    assert doc.blob is None
-    assert doc.meta == {}
-    assert doc.score == 0.812
-    assert doc.embedding == [0.1, 0.2, 0.3]
-    assert doc.sparse_embedding is None
+@pytest.mark.parametrize(
+    "legacy_fields,meta,expected_id",
+    [
+        (
+            {"content_type": "text", "id_hash_keys": ["content"], "dataframe": "placeholder"},
+            {},
+            "18fc2c114825872321cf5009827ca162f54d3be50ab9e9ffa027824b6ec223af",
+        ),
+        (
+            {"content_type": "text", "id_hash_keys": ["content"]},
+            {"date": "10-10-2023", "type": "article"},
+            "dcd4914f727544e89ce8082f6f2e298d244dd0803a4dc167f19d24e7d43b28ac",
+        ),
+    ],
+)
+def test_init_with_legacy_fields(legacy_fields, meta, expected_id):
+    doc = Document(content="test text", score=0.812, embedding=[0.1, 0.2, 0.3], meta=meta, **legacy_fields)
 
+    # legacy fields are dropped, so the Document is the same as if they were not passed at all
+    assert doc == Document(content="test text", score=0.812, embedding=[0.1, 0.2, 0.3], meta=meta)
+    assert doc.id == expected_id
     assert doc.content_type == "text"  # this is a property now
-
     assert not hasattr(doc, "id_hash_keys")
     assert not hasattr(doc, "dataframe")
 
 
-def test_init_with_legacy_field():
-    doc = Document(
-        content="test text",
-        content_type="text",  # type: ignore
-        id_hash_keys=["content"],
-        score=0.812,
-        embedding=[0.1, 0.2, 0.3],
-        meta={"date": "10-10-2023", "type": "article"},
-    )
-    assert doc.id == "dcd4914f727544e89ce8082f6f2e298d244dd0803a4dc167f19d24e7d43b28ac"
-    assert doc.content == "test text"
-    assert doc.meta == {"date": "10-10-2023", "type": "article"}
-    assert doc.score == 0.812
-    assert doc.embedding == [0.1, 0.2, 0.3]
-    assert doc.sparse_embedding is None
+@pytest.mark.parametrize("content", [123, 0, []])
+def test_init_with_non_string_content(content):
+    with pytest.raises(ValueError, match="must be a string or None"):
+        Document(content=content)
 
-    assert doc.content_type == "text"  # this is a property now
-    assert not hasattr(doc, "id_hash_keys")
+    with pytest.raises(ValueError, match="must be a string or None"):
+        Document("", content)
+
+
+def test_init_with_numpy_embedding():
+    expected = Document(content="test text", embedding=[0.1, 0.2, 0.3])
+
+    assert Document(content="test text", embedding=numpy.array([0.1, 0.2, 0.3])) == expected  # type: ignore[arg-type]
+
+    assert Document("", "test text", None, {}, None, numpy.array([0.1, 0.2, 0.3])) == expected  # type: ignore[arg-type]
 
 
 def test_basic_equality_type_mismatch():