From 0186b494f14210947fc0a256170f28800688cc05 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 23 Apr 2026 07:44:05 +0200 Subject: [PATCH] test(contacts): address PR #719 review feedback - Type-annotate _wrap_contact_field signature; drop stale "url" mention from its docstring (url is handled by the list-coercion helper, not this one). - Split the shape-coercion helper so comma-splitting only applies to categories: _as_str_list (no split) for org/nickname/url, _split_categories (comma split) for CATEGORIES. Fixes the case where organization="Smith, Jones & Associates" was mangled into a two- component ORG. - Share _normalize_contact_data between create and update so _merge_vcard_properties only sees canonical keys; add URL handlers in both update branches so the primary update path no longer drops URL silently. - Annotate the Contact(**kwargs) type:ignore with the reason (pythonvCard4 typeshed doesn't accept **dict[str, Any]). - Add tests/unit/client/test_contacts.py (pure unit, no HTTP) covering the #716 round-trip, comma-in-org regression, invalid-bday warning, tel/phone precedence, categories string-vs-list behaviour, and direct _normalize_contact_data cases. - Extend the MCP workflow test with an update-with-url step asserting the URL handler in _merge_vcard_properties. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 117 ++++++++++++---- tests/server/test_contacts_mcp.py | 18 +++ tests/unit/client/test_contacts.py | 179 ++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 31 deletions(-) create mode 100644 tests/unit/client/test_contacts.py diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 256a8c16..d037afb5 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -11,7 +11,8 @@ from .base import BaseNextcloudClient logger = logging.getLogger(__name__) -# Keys that _build_contact_from_data consumes. Used to warn (not error) on unknown keys. +# Canonical keys that _build_contact_from_data consumes. Aliases (``phone``, ``organization``) +# are normalised to their canonical form by _normalize_contact_data before lookup. _SUPPORTED_CONTACT_KEYS = frozenset( { "fn", @@ -30,8 +31,27 @@ _SUPPORTED_CONTACT_KEYS = frozenset( ) -def _wrap_contact_field(value) -> list[dict]: - """Normalize an email/tel/url input into pythonvCard4's list-of-dicts shape. +def _normalize_contact_data(contact_data: dict) -> dict: + """Map documented aliases to canonical keys. + + ``phone`` → ``tel``, ``organization`` → ``org``. The canonical key wins if both + are supplied, so callers who set ``tel`` don't lose it to a stray ``phone`` entry. + Returns a new dict — does not mutate the caller's argument. + """ + normalised = dict(contact_data) + if "phone" in normalised and "tel" not in normalised: + normalised["tel"] = normalised.pop("phone") + else: + normalised.pop("phone", None) + if "organization" in normalised and "org" not in normalised: + normalised["org"] = normalised.pop("organization") + else: + normalised.pop("organization", None) + return normalised + + +def _wrap_contact_field(value: str | dict | list | None) -> list[dict]: + """Normalize an email/tel input into pythonvCard4's list-of-dicts shape. Accepts a plain string, a dict already in ``{value, type}`` form, or a list of either. Empty strings are dropped. Always returns a list (possibly empty). @@ -49,6 +69,29 @@ def _wrap_contact_field(value) -> list[dict]: return out +def _as_str_list(value: str | list) -> list[str]: + """Wrap a bare string in a list. Does NOT split on commas. + + Used for ORG/NICKNAME/URL where commas are part of the value (e.g. + ``"Smith, Jones & Associates"``) and only the list wrapper is needed to + prevent pythonvCard4 from iterating the string character-by-character. + """ + return value if isinstance(value, list) else [value] + + +def _split_categories(value: str | list) -> list[str]: + """Normalise CATEGORIES input: a comma-separated string is split into a list. + + Unlike ORG/NICKNAME, CATEGORIES is canonically comma-separated in vCards + (``CATEGORIES:a,b,c``) so splitting a bare string is the expected shape. + Lists pass through unchanged — callers that already provide ``["a,b"]`` keep + their exact item, no double-splitting. + """ + if isinstance(value, list): + return value + return [v.strip() for v in value.split(",") if v.strip()] + + def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict. @@ -56,47 +99,37 @@ def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char iteration of bare strings — see issue #716. """ + data = _normalize_contact_data(contact_data) - # pythonvCard4 iterates bare strings character-by-character for list-typed fields - # (ORG, NICKNAME, CATEGORIES, URL), producing garbage like ``ORG:A;c;m;e``. Wrap - # single strings in a list to keep the vCard well-formed. - def _as_list(value): - if isinstance(value, list): - return value - if isinstance(value, str) and "," in value: - return [v.strip() for v in value.split(",") if v.strip()] - return [value] + kwargs: dict = {"fn": data.get("fn"), "uid": uid} - kwargs: dict = {"fn": contact_data.get("fn"), "uid": uid} - - emails = _wrap_contact_field(contact_data.get("email")) + emails = _wrap_contact_field(data.get("email")) if emails: kwargs["email"] = emails - tels = _wrap_contact_field(contact_data.get("tel") or contact_data.get("phone")) + tels = _wrap_contact_field(data.get("tel")) if tels: kwargs["tel"] = tels - org_value = contact_data.get("org") or contact_data.get("organization") - if org_value: - kwargs["org"] = _as_list(org_value) + if data.get("org"): + kwargs["org"] = _as_str_list(data["org"]) - if contact_data.get("note"): - kwargs["note"] = contact_data["note"] + if data.get("note"): + kwargs["note"] = data["note"] - if contact_data.get("title"): - kwargs["title"] = contact_data["title"] + if data.get("title"): + kwargs["title"] = data["title"] - if contact_data.get("nickname"): - kwargs["nickname"] = _as_list(contact_data["nickname"]) + if data.get("nickname"): + kwargs["nickname"] = _as_str_list(data["nickname"]) - if contact_data.get("categories"): - kwargs["categories"] = _as_list(contact_data["categories"]) + if data.get("categories"): + kwargs["categories"] = _split_categories(data["categories"]) - if contact_data.get("url"): - kwargs["url"] = _as_list(contact_data["url"]) + if data.get("url"): + kwargs["url"] = _as_str_list(data["url"]) - bday = contact_data.get("bday") + bday = data.get("bday") if bday: if isinstance(bday, date): kwargs["bday"] = bday @@ -106,10 +139,12 @@ def _build_contact_from_data(contact_data: dict, uid: str) -> Contact: except ValueError: logger.warning("Ignoring non-ISO bday value: %r", bday) - unknown = set(contact_data) - _SUPPORTED_CONTACT_KEYS + unknown = set(data) - _SUPPORTED_CONTACT_KEYS if unknown: logger.debug("Ignoring unknown contact_data keys: %s", sorted(unknown)) + # kwargs built dynamically from contact_data; pythonvCard4's Contact typeshed + # has specific typed params and doesn't accept **dict[str, Any]. return Contact(**kwargs) # type: ignore[arg-type] @@ -251,6 +286,9 @@ class ContactsClient(BaseNextcloudClient): carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{uid}.vcf" + # Canonicalise aliases up front so both code paths (merge + fallback) agree. + contact_data = _normalize_contact_data(contact_data) + # Get raw vCard content to preserve all properties including extended ones raw_vcard_content = "" if not etag: @@ -480,6 +518,17 @@ class ContactsClient(BaseNextcloudClient): elif property_name == "TITLE" and "title" in contact_data: updated_lines.append(f"TITLE:{contact_data['title']}") updated_properties.add("title") + elif property_name == "URL" and "url" in contact_data: + if "url" not in updated_properties: + url_value = contact_data["url"] + if isinstance(url_value, list): + url_value = url_value[0] if url_value else "" + if url_value: + updated_lines.append(f"URL:{url_value}") + updated_properties.add("url") + else: + # Keep additional URLs unchanged + updated_lines.append(line) else: # Keep all other properties unchanged (preserves all extended/custom fields) updated_lines.append(line) @@ -511,6 +560,12 @@ class ContactsClient(BaseNextcloudClient): updated_lines.append(f"ORG:{value}") elif key == "title": updated_lines.append(f"TITLE:{value}") + elif key == "url": + url_value = ( + value[0] if isinstance(value, list) and value else value + ) + if url_value: + updated_lines.append(f"URL:{url_value}") # Add the END:VCARD line updated_lines.append("END:VCARD") diff --git a/tests/server/test_contacts_mcp.py b/tests/server/test_contacts_mcp.py index c464ff83..8b5d587d 100644 --- a/tests/server/test_contacts_mcp.py +++ b/tests/server/test_contacts_mcp.py @@ -62,6 +62,24 @@ async def test_mcp_contacts_workflow( assert "ORG:MCP Test Corp" in raw_vcard assert f"NOTE:Created by test {unique_suffix}" in raw_vcard + # 4b. Update with a URL — regression guard for PR #719 review: + # _merge_vcard_properties previously had no URL handler, silently dropping it. + update_result = await nc_mcp_client.call_tool( + "nc_contacts_update_contact", + { + "addressbook": addressbook_name, + "uid": contact_uid, + "contact_data": {"url": "https://mcp-test.example.com"}, + }, + ) + assert update_result.isError is False + contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name) + updated = next(c for c in contacts if c["vcard_id"] == contact_uid) + updated_vcard = updated.get("addressdata", "") + assert "mcp-test.example.com" in updated_vcard + # Prior properties must not have been clobbered by the merge. + assert "ORG:MCP Test Corp" in updated_vcard + # 5. Delete contact via MCP logger.info(f"Deleting contact {contact_uid} via MCP") delete_c_result = await nc_mcp_client.call_tool( diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py new file mode 100644 index 00000000..b72756ca --- /dev/null +++ b/tests/unit/client/test_contacts.py @@ -0,0 +1,179 @@ +"""Unit tests for the contacts client vCard builder. + +These exercise ``_build_contact_from_data`` in isolation — no HTTP, no fixtures — +so they cover the issue #716 regression surface and the edge cases flagged in +PR #719 review without standing up the compose stack. +""" + +from datetime import date + +import pytest + +from nextcloud_mcp_server.client.contacts import ( + _build_contact_from_data, + _normalize_contact_data, +) + +pytestmark = pytest.mark.unit + + +def _vcard(**kwargs) -> str: + """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text.""" + return _build_contact_from_data(kwargs, uid="unit-test-uid").to_vcard() + + +def test_issue_716_minimal_payload_keeps_all_fields(): + """Reporter's exact payload from issue #716: every field must survive.""" + vcard = _vcard( + fn="Repro User", + email="repro@example.com", + phone="555-0716", + organization="Acme Corp", + note="Issue 716", + ) + assert "FN:Repro User" in vcard + assert "EMAIL" in vcard and "repro@example.com" in vcard + assert "TEL" in vcard and "555-0716" in vcard + assert "ORG:Acme Corp" in vcard + assert "NOTE:Issue 716" in vcard + + +def test_org_preserves_comma_in_company_name(): + """Regression: ``_as_list`` used to comma-split ORG, mangling names like + "Smith, Jones & Associates" into a two-component ORG. After the fix the whole + string is a single ORG component (with the comma RFC-6350-escaped as ``\\,``). + """ + vcard = _vcard(fn="Alice", organization="Smith, Jones & Associates") + org_line = next(line for line in vcard.splitlines() if line.startswith("ORG")) + # Single component: no unescaped semicolon separator. + payload = org_line.split(":", 1)[1] + assert ";" not in payload + # Comma is escaped per RFC 6350 but the logical value is preserved. + assert payload.replace(r"\,", ",") == "Smith, Jones & Associates" + + +def test_org_list_input_produces_structured_org(): + """A list input is the opt-in shape for multi-component ORG (Company;Department).""" + vcard = _vcard(fn="Alice", org=["Acme", "Engineering"]) + assert "ORG:Acme;Engineering" in vcard + + +def test_invalid_bday_is_dropped_not_raised(caplog): + """An unparseable BDAY must warn and be omitted, not crash the call.""" + import logging + + with caplog.at_level( + logging.WARNING, logger="nextcloud_mcp_server.client.contacts" + ): + vcard = _vcard(fn="Alice", bday="not-a-date") + assert "BDAY" not in vcard + assert any("bday" in r.message.lower() for r in caplog.records) + + +def test_valid_iso_bday_is_persisted(): + vcard = _vcard(fn="Alice", bday="1990-05-01") + assert "BDAY:1990-05-01" in vcard + + +def test_date_object_bday_is_persisted(): + vcard = _vcard(fn="Alice", bday=date(1985, 12, 24)) + assert "BDAY:1985-12-24" in vcard + + +def test_tel_takes_precedence_over_phone_alias(): + """When the caller supplies both canonical and alias, canonical wins. Documents + the precedence so future callers aren't surprised. + """ + vcard = _vcard(fn="Alice", tel="111-1111", phone="222-2222") + assert "111-1111" in vcard + assert "222-2222" not in vcard + + +def test_organization_alias_fills_in_when_org_absent(): + vcard = _vcard(fn="Alice", organization="Acme") + assert "ORG:Acme" in vcard + + +def test_categories_string_is_split_on_commas(): + vcard = _vcard(fn="Alice", categories="friends,work,vip") + cat_line = next( + line for line in vcard.splitlines() if line.startswith("CATEGORIES") + ) + assert cat_line == "CATEGORIES:friends,work,vip" + + +def test_categories_list_passes_through_unchanged(): + """A caller that already supplied a list shouldn't have their entries split again + — ``["friends,work"]`` stays as one item (with the comma RFC-6350-escaped), not + two categories ``friends`` + ``work``. + """ + vcard = _vcard(fn="Alice", categories=["friends,work"]) + cat_line = next( + line for line in vcard.splitlines() if line.startswith("CATEGORIES") + ) + payload = cat_line.split(":", 1)[1] + assert payload == r"friends\,work" # one item, comma escaped + + +def test_nickname_bare_string_is_not_char_iterated(): + """Regression: pythonvCard4 iterates bare strings; we wrap to prevent that.""" + vcard = _vcard(fn="Alice", nickname="Bob") + nick_line = next(line for line in vcard.splitlines() if line.startswith("NICKNAME")) + assert nick_line == "NICKNAME:Bob" + + +def test_url_bare_string_is_not_char_iterated(): + vcard = _vcard(fn="Alice", url="https://example.com") + # Must appear as a single URL, not one URL: per character. + url_lines = [line for line in vcard.splitlines() if line.startswith("URL")] + assert url_lines == ["URL:https://example.com"] + + +def test_unknown_keys_are_ignored_without_error(caplog): + """Future-compat: callers sending unknown keys shouldn't blow up.""" + import logging + + with caplog.at_level(logging.DEBUG, logger="nextcloud_mcp_server.client.contacts"): + vcard = _vcard(fn="Alice", totally_made_up_field="ignored") + assert "FN:Alice" in vcard + assert "totally_made_up_field" not in vcard + # A debug log is expected but not required — main guarantee is that no exception is raised. + + +def test_empty_email_is_skipped(): + """An empty string for email must not emit an EMAIL: line.""" + vcard = _vcard(fn="Alice", email="") + assert "EMAIL" not in vcard + + +def test_dict_form_email_preserves_custom_type(): + vcard = _vcard( + fn="Alice", + email={"value": "work@example.com", "type": ["WORK"]}, + ) + assert "EMAIL;TYPE=WORK:work@example.com" in vcard + + +class TestNormalizeContactData: + """Direct tests for the alias helper — it's load-bearing for update_contact too.""" + + def test_phone_maps_to_tel(self): + assert _normalize_contact_data({"phone": "123"}) == {"tel": "123"} + + def test_organization_maps_to_org(self): + assert _normalize_contact_data({"organization": "Acme"}) == {"org": "Acme"} + + def test_canonical_wins_when_both_present(self): + """Caller intent: they set ``tel`` deliberately. A stray ``phone`` entry + must not clobber the canonical value. + """ + out = _normalize_contact_data({"tel": "canonical", "phone": "alias"}) + assert out == {"tel": "canonical"} + + def test_does_not_mutate_input(self): + original = {"phone": "123", "organization": "Acme"} + _normalize_contact_data(original) + assert original == {"phone": "123", "organization": "Acme"} + + def test_passthrough_for_unknown_keys(self): + assert _normalize_contact_data({"foo": "bar"}) == {"foo": "bar"}