Improve NameList robustness and flexibility (#320)

* Improve `NameList` robustness and flexibility

* Fix intentionally broken test

* Add logging of values with no `"name"` key

mreg_cli/api/fields.py

@@ -3,15 +3,18 @@
 from __future__ import annotations
 
 import ipaddress
+import logging
 from typing import Annotated, Any, Self
 
-from pydantic import BeforeValidator, ValidationError
+from pydantic import AfterValidator, BeforeValidator, ValidationError
 from pydantic_extra_types.mac_address import MacAddress
 
 from mreg_cli.api.abstracts import FrozenModel
 from mreg_cli.exceptions import InputFailure
 from mreg_cli.types import IP_AddressT
 
+logger = logging.getLogger(__name__)
+
 
 class MACAddressField(FrozenModel):
     """Represents a MAC address."""
@@ -87,13 +90,33 @@ def __hash__(self):
         return hash(self.address)
 
 
-def _extract_name(value: dict[str, Any]) -> str:
-    """Extract the name from the dictionary.
+def _extract_name(value: Any) -> str:
+    """Extract the "name" value from a dictionary.
 
     :param v: Dictionary containing the name.
     :returns: Extracted name as a string.
     """
-    return value["name"]
+    if isinstance(value, dict):
+        try:
+            return str(value["name"])  # pyright: ignore[reportUnknownArgumentType]
+        except KeyError:
+            logger.error("No 'name' key in %s", value)  # pyright: ignore[reportUnknownArgumentType]
+            return ""
+    return value
+
+
+def _remove_falsy_list_items(value: Any) -> Any:
+    """Remove falsy items from a list.
+
+    For use in validators only.
+    """
+    if isinstance(value, list):
+        return [i for i in value if i]  # pyright: ignore[reportUnknownVariableType]
+    return value
 
 
-NameList = list[Annotated[str, BeforeValidator(_extract_name)]]
+NameList = Annotated[
+    list[Annotated[str, BeforeValidator(_extract_name)]],
+    AfterValidator(_remove_falsy_list_items),
+]
+"""List of names extracted from a list of dicts."""

tests/api/test_fields.py

@@ -1,8 +1,10 @@
 from __future__ import annotations
 
 import pytest
+from inline_snapshot import snapshot
+from pydantic import BaseModel, ValidationError
 
-from mreg_cli.api.fields import MACAddressField
+from mreg_cli.api.fields import MACAddressField, NameList
 from mreg_cli.exceptions import InputFailure
 
 MacAddresValidationFailure = pytest.mark.xfail(raises=InputFailure, strict=True)
@@ -47,3 +49,96 @@ def test_mac_address_field(inp: str, expect: str) -> None:
     """Test the MAC address field."""
     res = MACAddressField.validate(inp)
     assert str(res) == expect
+
+
+def test_name_list_basic():
+    """Test NameList field with basic input."""
+    inp = {
+        "hosts": [
+            {"name": "test1", "value": 1},
+            {"name": "test2"},
+            {"name": "test3", "value": 3},
+        ]
+    }
+
+    class TestModel(BaseModel):
+        hosts: NameList
+
+    m = TestModel.model_validate(inp)
+
+    assert m.model_dump(mode="json") == snapshot({"hosts": ["test1", "test2", "test3"]})
+
+
+def test_name_list_with_invalid_item(caplog: pytest.LogCaptureFixture):
+    """Test NameList field with an item without a name.
+
+    Should log an error and skip the item."""
+    inp = {
+        "hosts": [
+            {"name": "test1", "value": 1},
+            {"value": 2},
+            {"name": "test3", "value": 3},
+        ]
+    }
+
+    class TestModel(BaseModel):
+        hosts: NameList
+
+    m = TestModel.model_validate(inp)
+
+    assert m.model_dump(mode="json") == snapshot({"hosts": ["test1", "test3"]})
+
+    assert caplog.record_tuples == snapshot(
+        [("mreg_cli.api.fields", 40, "No 'name' key in {'value': 2}")]
+    )
+
+
+def test_name_list_invalid_type():
+    """Test NameList field with the wrong type (dict instead of list of dicts)."""
+    # hosts is not a list
+    inp = {"hosts": {"name": "test1", "value": 1}}
+
+    class TestModel(BaseModel):
+        hosts: NameList
+
+    with pytest.raises(ValidationError) as exc_info:
+        TestModel.model_validate(inp)
+
+    assert exc_info.value.errors(include_url=False) == snapshot(
+        [
+            {
+                "type": "list_type",
+                "loc": ("hosts",),
+                "msg": "Input should be a valid list",
+                "input": {"name": "test1", "value": 1},
+            }
+        ]
+    )
+
+
+def test_name_list_with_list() -> None:
+    """Test NameList field with a list of strings. Should return the same list."""
+    inp = {"hosts": ["test1", "test2", "test3"]}
+
+    class TestModel(BaseModel):
+        hosts: NameList
+
+    m = TestModel.model_validate(inp)
+
+    assert m.model_dump(mode="json") == snapshot({"hosts": ["test1", "test2", "test3"]})
+
+
+def test_name_list_with_empty_name() -> None:
+    """Test NameList field with a list of strings, where one name is an empty string."""
+    inp = {"hosts": ["test1", "test2", "", "test3"]}
+
+    class TestModel(BaseModel):
+        hosts: NameList
+
+    m = TestModel.model_validate(inp)
+
+    # NOTE: this is a special case where the empty string is removed,
+    # just like with the list of dictionaries. Whether or not this is
+    # desirable is up for debate.
+    # This test ensures that any change to that behavior is caught.
+    assert m.model_dump(mode="json") == snapshot({"hosts": ["test1", "test2", "test3"]})