diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 8badded9..88553f11 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -3,6 +3,7 @@ import logging import xml.etree.ElementTree as ET from datetime import date +from typing import Any from pythonvCard4.vcard import Contact @@ -11,6 +12,184 @@ from .base import BaseNextcloudClient logger = logging.getLogger(__name__) +# Canonical keys accepted by _build_contact_from_data. Callers normalise aliases +# (``phone``→``tel``, ``organization``→``org``) via _normalize_contact_data beforehand +# so the set never needs to list them. +_SUPPORTED_CONTACT_KEYS = frozenset( + { + "fn", + "email", + "tel", + "org", + "note", + "title", + "nickname", + "bday", + "categories", + "url", + } +) + + +def _normalize_contact_data(contact_data: dict[str, Any]) -> dict[str, Any]: + """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[str, Any] | list[str | dict[str, Any]] | None, +) -> list[dict[str, Any]]: + """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 and dicts without a ``value`` key are dropped. Always + returns a list (possibly empty). + """ + if value is None or value == "": + return [] + items = value if isinstance(value, list) else [value] + out: list[dict[str, Any]] = [] + for item in items: + if isinstance(item, dict) and item.get("value"): + types = item.get("type") or ["HOME"] + # Wrap a bare string so ``list("WORK")`` doesn't iterate it into + # ``["W", "O", "R", "K"]`` — same char-iteration footgun this whole + # helper exists to avoid for the outer ``value``. + if isinstance(types, str): + types = [types] + out.append({"value": item["value"], "type": list(types)}) + elif isinstance(item, str) and item: + out.append({"value": item, "type": ["HOME"]}) + return out + + +def _as_str_list(value: str | list[str]) -> 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[str]) -> 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 _parse_bday(value: str | date | None) -> date | None: + """Parse a BDAY input to a ``date``. Logs and returns ``None`` if unparseable. + + Shared by the create path (``_build_contact_from_data``) and the update path + (``_merge_vcard_properties``) so a non-ISO BDAY is rejected consistently + instead of being written raw on update. + """ + if value is None or value == "": + return None + if isinstance(value, date): + return value + if isinstance(value, str): + try: + return date.fromisoformat(value) + except ValueError: + logger.warning("Ignoring non-ISO bday value: %r", value) + return None + + +def _safe_vcard_value(value: Any) -> Any: + """Escape newlines in a value so it can't inject additional vCard properties. + + Per RFC 6350 §3.4 newlines inside a property value are encoded as ``\\n``. + Unfolding this on the read side is pythonvCard4's job; we only need to make + sure ``contact_data`` strings don't terminate the line on the way out. + """ + if isinstance(value, str): + return value.replace("\r\n", "\\n").replace("\n", "\\n").replace("\r", "\\n") + return value + + +def _build_contact_from_data(contact_data: dict[str, Any], uid: str) -> Contact: + """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict. + + Maps every key documented on ``nc_contacts_create_contact`` onto the underlying + library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char + iteration of bare strings — see issue #716. + + Callers must pre-normalise aliases via ``_normalize_contact_data`` before + invoking this helper; it assumes canonical keys only. + """ + data = contact_data + + if not data.get("fn"): + logger.warning( + "contact_data missing required 'fn' field; pythonvCard4 may reject or " + "produce an invalid vCard" + ) + + kwargs: dict[str, Any] = {"fn": data.get("fn"), "uid": uid} + + emails = _wrap_contact_field(data.get("email")) + if emails: + kwargs["email"] = emails + + tels = _wrap_contact_field(data.get("tel")) + if tels: + kwargs["tel"] = tels + + if data.get("org"): + kwargs["org"] = _as_str_list(data["org"]) + + if data.get("note"): + kwargs["note"] = data["note"] + + if data.get("title"): + kwargs["title"] = data["title"] + + if data.get("nickname"): + kwargs["nickname"] = _as_str_list(data["nickname"]) + + if data.get("categories"): + kwargs["categories"] = _split_categories(data["categories"]) + + if data.get("url"): + kwargs["url"] = _as_str_list(data["url"]) + + bday = _parse_bday(data.get("bday")) + if bday is not None: + kwargs["bday"] = bday + + 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] + + class ContactsClient(BaseNextcloudClient): """Client for NextCloud CardDAV contact operations.""" @@ -122,18 +301,16 @@ class ContactsClient(BaseNextcloudClient): url = f"{carddav_path}/{name}/" await self._make_request("DELETE", url) - async def create_contact(self, *, addressbook: str, uid: str, contact_data: dict): + async def create_contact( + self, *, addressbook: str, uid: str, contact_data: dict[str, Any] + ): """Create a new contact.""" carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{uid}.vcf" - contact = Contact(fn=contact_data.get("fn"), uid=uid) # type: ignore - if "email" in contact_data: - contact.email = [{"value": contact_data["email"], "type": ["HOME"]}] - if "tel" in contact_data: - contact.tel = [{"value": contact_data["tel"], "type": ["HOME"]}] - - vcard = contact.to_vcard() + # Normalise aliases here so the helper's invariant (canonical keys only) holds. + contact_data = _normalize_contact_data(contact_data) + vcard = _build_contact_from_data(contact_data, uid).to_vcard() headers = { "Content-Type": "text/vcard; charset=utf-8", @@ -149,12 +326,20 @@ class ContactsClient(BaseNextcloudClient): await self._make_request("DELETE", url) async def update_contact( - self, *, addressbook: str, uid: str, contact_data: dict, etag: str = "" + self, + *, + addressbook: str, + uid: str, + contact_data: dict[str, Any], + etag: str = "", ): """Update an existing contact while preserving all existing properties.""" 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: @@ -177,12 +362,7 @@ class ContactsClient(BaseNextcloudClient): ) else: # Fallback to creating new vCard if we couldn't get existing - contact = Contact(fn=contact_data.get("fn"), uid=uid) # type: ignore - if "email" in contact_data: - contact.email = [{"value": contact_data["email"], "type": ["HOME"]}] - if "tel" in contact_data: - contact.tel = [{"value": contact_data["tel"], "type": ["HOME"]}] - vcard_content = contact.to_vcard() + vcard_content = _build_contact_from_data(contact_data, uid).to_vcard() headers = { "Content-Type": "text/vcard; charset=utf-8", @@ -300,9 +480,31 @@ class ContactsClient(BaseNextcloudClient): raise def _merge_vcard_properties( - self, raw_vcard: str, contact_data: dict, uid: str + self, raw_vcard: str, contact_data: dict[str, Any], uid: str ) -> str: - """Merge new contact data into existing raw vCard while preserving all properties.""" + """Merge new contact data into existing raw vCard while preserving all properties. + + Limitation: dict / list-form ``email`` and ``tel`` inputs are not applied + by this text-merge path. Existing EMAIL/TEL lines are preserved unchanged, + and no new lines are written for the dict/list inputs. Pass plain strings + to update EMAIL/TEL here, or recreate via ``create_contact`` for full + multi-entry support with TYPE annotations. + """ + # Surface dict/list email/tel up front rather than silently no-op in the + # add-new loop below (where the isinstance(value, str) guard skips them). + for _key in ("email", "tel"): + _value = contact_data.get(_key) + if _value is not None and not isinstance(_value, str): + logger.warning( + "update_contact: %s=%r (dict/list shape) is not applied via " + "the text-merge update path; existing %s lines are preserved " + "unchanged. Use a plain string to update %s here, or recreate " + "via create_contact for multi-entry support.", + _key, + _value, + _key.upper(), + _key.upper(), + ) try: # Instead of using pythonvCard4 which has formatting issues, # let's do a simple text-based merge to preserve exact formatting @@ -327,21 +529,27 @@ class ContactsClient(BaseNextcloudClient): # Handle updates for specific properties if property_name == "FN" and "fn" in contact_data: - updated_lines.append(f"FN:{contact_data['fn']}") + updated_lines.append(f"FN:{_safe_vcard_value(contact_data['fn'])}") updated_properties.add("fn") elif property_name == "EMAIL" and "email" in contact_data: # Replace first email with new one, preserve others if "email" not in updated_properties: if isinstance(contact_data["email"], str): + email_value = _safe_vcard_value(contact_data["email"]) # Try to preserve the original format as much as possible if ";TYPE=" in line: type_part = line.split(";TYPE=")[1].split(":")[0] updated_lines.append( - f"EMAIL;TYPE={type_part}:{contact_data['email']}" + f"EMAIL;TYPE={type_part}:{email_value}" ) else: - updated_lines.append(f"EMAIL:{contact_data['email']}") - updated_properties.add("email") + updated_lines.append(f"EMAIL:{email_value}") + updated_properties.add("email") + else: + # Dict / list inputs aren't translatable to a single + # text-merge replacement; keep the original line so we + # don't silently drop the contact's email. + updated_lines.append(line) else: # Keep additional emails unchanged updated_lines.append(line) @@ -349,46 +557,79 @@ class ContactsClient(BaseNextcloudClient): # Similar handling for phone numbers if "tel" not in updated_properties: if isinstance(contact_data["tel"], str): + tel_value = _safe_vcard_value(contact_data["tel"]) if ";TYPE=" in line: type_part = line.split(";TYPE=")[1].split(":")[0] updated_lines.append( - f"TEL;TYPE={type_part}:{contact_data['tel']}" + f"TEL;TYPE={type_part}:{tel_value}" ) else: - updated_lines.append(f"TEL:{contact_data['tel']}") - updated_properties.add("tel") + updated_lines.append(f"TEL:{tel_value}") + updated_properties.add("tel") + else: + # Same reasoning as the EMAIL branch above: don't drop. + updated_lines.append(line) else: # Keep additional phone numbers unchanged updated_lines.append(line) elif property_name == "NOTE" and "note" in contact_data: - updated_lines.append(f"NOTE:{contact_data['note']}") + updated_lines.append( + f"NOTE:{_safe_vcard_value(contact_data['note'])}" + ) updated_properties.add("note") elif property_name == "NICKNAME" and "nickname" in contact_data: nickname_value = contact_data["nickname"] if isinstance(nickname_value, list): nickname_value = ",".join(nickname_value) - updated_lines.append(f"NICKNAME:{nickname_value}") + updated_lines.append( + f"NICKNAME:{_safe_vcard_value(nickname_value)}" + ) updated_properties.add("nickname") elif property_name == "BDAY" and "bday" in contact_data: - updated_lines.append(f"BDAY:{contact_data['bday']}") - updated_properties.add("bday") + parsed_bday = _parse_bday(contact_data["bday"]) + if parsed_bday is not None: + updated_lines.append(f"BDAY:{parsed_bday.isoformat()}") + updated_properties.add("bday") + else: + # Invalid input — keep the existing BDAY rather than + # writing a malformed line or silently dropping it. + updated_lines.append(line) elif property_name == "CATEGORIES" and "categories" in contact_data: categories_value = contact_data["categories"] if isinstance(categories_value, list): categories_value = ",".join(categories_value) - updated_lines.append(f"CATEGORIES:{categories_value}") - updated_properties.add("categories") - elif property_name == "ORG" and ( - "org" in contact_data or "organization" in contact_data - ): - org_value = contact_data.get("org") or contact_data.get( - "organization" + updated_lines.append( + f"CATEGORIES:{_safe_vcard_value(categories_value)}" ) - updated_lines.append(f"ORG:{org_value}") + updated_properties.add("categories") + elif property_name == "ORG" and "org" in contact_data: + org_value = contact_data["org"] + # ORG is structured (Company;Department;…) per RFC 6350 §6.6.4; + # join list components with ';' so callers using the same shape + # ``_build_contact_from_data`` accepts don't get a Python repr. + if isinstance(org_value, list): + org_value = ";".join(org_value) + updated_lines.append(f"ORG:{_safe_vcard_value(org_value)}") updated_properties.add("org") elif property_name == "TITLE" and "title" in contact_data: - updated_lines.append(f"TITLE:{contact_data['title']}") + updated_lines.append( + f"TITLE:{_safe_vcard_value(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"] + # Only the first URL from a list is written; multi-URL + # contacts are rare and this text merge doesn't attempt + # position-stable mapping to existing URL lines. + if isinstance(url_value, list): + url_value = url_value[0] if url_value else "" + if url_value: + updated_lines.append(f"URL:{_safe_vcard_value(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) @@ -397,29 +638,47 @@ class ContactsClient(BaseNextcloudClient): for key, value in contact_data.items(): if key not in updated_properties: if key == "fn": - updated_lines.append(f"FN:{value}") + updated_lines.append(f"FN:{_safe_vcard_value(value)}") elif key == "email" and isinstance(value, str): - updated_lines.append(f"EMAIL:{value}") + updated_lines.append(f"EMAIL:{_safe_vcard_value(value)}") elif key == "tel" and isinstance(value, str): - updated_lines.append(f"TEL:{value}") + updated_lines.append(f"TEL:{_safe_vcard_value(value)}") elif key == "note": - updated_lines.append(f"NOTE:{value}") + updated_lines.append(f"NOTE:{_safe_vcard_value(value)}") elif key == "nickname": nickname_value = ( value if isinstance(value, str) else ",".join(value) ) - updated_lines.append(f"NICKNAME:{nickname_value}") + updated_lines.append( + f"NICKNAME:{_safe_vcard_value(nickname_value)}" + ) elif key == "bday": - updated_lines.append(f"BDAY:{value}") + parsed_bday = _parse_bday(value) + if parsed_bday is not None: + updated_lines.append(f"BDAY:{parsed_bday.isoformat()}") elif key == "categories": categories_value = ( value if isinstance(value, str) else ",".join(value) ) - updated_lines.append(f"CATEGORIES:{categories_value}") - elif key in ["org", "organization"]: - updated_lines.append(f"ORG:{value}") + updated_lines.append( + f"CATEGORIES:{_safe_vcard_value(categories_value)}" + ) + elif key == "org": + # See ORG note in update-existing branch above. + org_value = ( + ";".join(value) if isinstance(value, list) else value + ) + updated_lines.append(f"ORG:{_safe_vcard_value(org_value)}") elif key == "title": - updated_lines.append(f"TITLE:{value}") + updated_lines.append(f"TITLE:{_safe_vcard_value(value)}") + elif key == "url": + # Only the first URL is written on add-new; see note in the + # update-existing branch above. + url_value = ( + value[0] if isinstance(value, list) and value else value + ) + if url_value: + updated_lines.append(f"URL:{_safe_vcard_value(url_value)}") # Add the END:VCARD line updated_lines.append("END:VCARD") diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index ffa35bcf..d74b88db 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -273,7 +273,23 @@ def configure_contacts_tools(mcp: FastMCP): not the display name. Use nc_contacts_list_addressbooks to find available URI slugs. uid: The unique ID for the contact. - contact_data: A dictionary with the contact's details, e.g. {"fn": "John Doe", "email": "john.doe@example.com"}. + contact_data: A dictionary with the contact's details. Supported keys: + + - ``fn`` (str, required): Formatted full name. + - ``email`` (str or list of str/dicts): Email address(es). + - ``tel`` / ``phone`` (str or list): Phone number(s). + - ``org`` / ``organization`` (str or list of str): Organization. + Lists become semicolon-separated ORG components per RFC 6350. + - ``title`` (str): Job title. + - ``note`` (str): Free-form note. + - ``nickname`` (str or list of str). + - ``bday`` (ISO date str ``"YYYY-MM-DD"`` or ``datetime.date``). + - ``categories`` (list of str, or comma-separated str). + - ``url`` (str or list of str). + + Unknown keys are ignored. Example: + ``{"fn": "John Doe", "email": "john@example.com", + "organization": "Acme", "note": "Met at conference"}``. """ client = await get_client(ctx) return await client.contacts.create_contact( @@ -316,7 +332,30 @@ def configure_contacts_tools(mcp: FastMCP): not the display name. Use nc_contacts_list_addressbooks to find available URI slugs. uid: The unique ID of the contact to update. - contact_data: A dictionary with the contact's updated details, e.g. {"fn": "Jane Doe", "email": "jane.doe@example.com"}. + contact_data: A dictionary with the contact's updated details. Supported + keys mirror nc_contacts_create_contact: + + - ``fn`` (str): Formatted full name. + - ``email`` (str): Email address. **Update path supports plain + strings only**; dict / list-form inputs are not applied — the + existing EMAIL line is preserved unchanged and a warning is + logged. Use create_contact for multi-entry support with TYPE + annotations. + - ``tel`` / ``phone`` (str): Phone number. Same single-string + limitation as ``email`` above. + - ``org`` / ``organization`` (str or list of str): Organization. + Lists become semicolon-separated ORG components per RFC 6350. + - ``title`` (str): Job title. + - ``note`` (str): Free-form note. + - ``nickname`` (str or list of str). + - ``bday`` (ISO date str ``"YYYY-MM-DD"`` or ``datetime.date``). + Non-ISO strings are rejected with a warning; the existing + BDAY line is preserved. + - ``categories`` (list of str, or comma-separated str). + - ``url`` (str or list of str). Only the first URL is written + on update; multi-URL contacts should use create_contact. + + Example: ``{"fn": "Jane Doe", "email": "jane.doe@example.com"}``. etag: Optional ETag for optimistic concurrency control. """ client = await get_client(ctx) diff --git a/tests/client/contacts/test_contacts_operations.py b/tests/client/contacts/test_contacts_operations.py index 9a496a80..c894a52b 100644 --- a/tests/client/contacts/test_contacts_operations.py +++ b/tests/client/contacts/test_contacts_operations.py @@ -86,3 +86,47 @@ async def test_full_contact_workflow( contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name) contact_uids = [c["vcard_id"] for c in contacts] assert contact_uid not in contact_uids + + +async def test_create_contact_persists_all_documented_fields( + nc_client: NextcloudClient, temporary_addressbook: str +): + """Regression for issue #716: org/note/phone/organization must persist to the vCard. + + Historically ``create_contact`` only handled fn/email/tel and silently dropped every + other key. Inspect the raw server-side vCard (not just the parsed list response) to + confirm each documented field round-trips. + """ + addressbook_name = temporary_addressbook + contact_uid = f"test-full-{uuid.uuid4().hex[:8]}" + contact_data = { + "fn": "Full Field User", + "email": "full@example.com", + "phone": "555-0716", # alias for tel + "organization": "Acme Corp", # alias for org + "note": "Issue 716 regression", + "title": "Engineer", + "url": "https://example.com", + } + + await nc_client.contacts.create_contact( + addressbook=addressbook_name, + uid=contact_uid, + contact_data=contact_data, + ) + try: + raw_vcard, _etag = await nc_client.contacts._get_raw_vcard( + addressbook_name, contact_uid + ) + assert "FN:Full Field User" in raw_vcard + assert "EMAIL" in raw_vcard and "full@example.com" in raw_vcard + assert "TEL" in raw_vcard and "555-0716" in raw_vcard + assert "ORG:Acme Corp" in raw_vcard + assert "NOTE:Issue 716 regression" in raw_vcard + assert "TITLE:Engineer" in raw_vcard + # Sabre rewrites bare URL: to URL;VALUE=URI: on PUT + assert "URL" in raw_vcard and "https://example.com" in raw_vcard + finally: + await nc_client.contacts.delete_contact( + addressbook=addressbook_name, uid=contact_uid + ) diff --git a/tests/server/test_contacts_mcp.py b/tests/server/test_contacts_mcp.py index 9f93e34f..6f842d33 100644 --- a/tests/server/test_contacts_mcp.py +++ b/tests/server/test_contacts_mcp.py @@ -24,6 +24,9 @@ async def test_mcp_contacts_workflow( "fn": f"MCP Contact {unique_suffix}", "email": f"mcp.contact.{unique_suffix}@example.com", "tel": "1234567890", + # Regression for issue #716 — these were silently dropped before + "organization": "MCP Test Corp", + "note": f"Created by test {unique_suffix}", } try: @@ -51,9 +54,31 @@ async def test_mcp_contacts_workflow( ) assert create_c_result.isError is False - # 4. Verify contact creation + # 4. Verify contact creation (and that all fields — #716 — actually persisted) contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name) - assert any(c["vcard_id"] == contact_uid for c in contacts) + created = next((c for c in contacts if c["vcard_id"] == contact_uid), None) + assert created is not None + raw_vcard = created.get("addressdata", "") + 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("Deleting contact %s via MCP", contact_uid) diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py new file mode 100644 index 00000000..a5deebb0 --- /dev/null +++ b/tests/unit/client/test_contacts.py @@ -0,0 +1,411 @@ +"""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, + _wrap_contact_field, +) + +pytestmark = pytest.mark.unit + + +def _vcard(**kwargs) -> str: + """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text. + + Mirrors ``create_contact``'s real call chain: normalise aliases first, then hand + canonical keys to ``_build_contact_from_data``. + """ + data = _normalize_contact_data(kwargs) + return _build_contact_from_data(data, 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 + + +def test_dict_form_email_with_bare_string_type(): + """Regression: a dict with ``type="WORK"`` (bare string) used to be + char-iterated into ``["W","O","R","K"]`` because of an unguarded ``list()`` + call inside ``_wrap_contact_field``. + """ + vcard = _vcard( + fn="Alice", + email={"value": "work@example.com", "type": "WORK"}, + ) + assert "EMAIL;TYPE=WORK:work@example.com" in vcard + # The bug would emit something like ``EMAIL;TYPE=W,O,R,K:`` — guard against it. + assert "TYPE=W," not in vcard + + +def test_wrap_field_dict_without_value_is_dropped(): + """Dict inputs lacking the ``value`` key are silently dropped so malformed + payloads don't emit an EMAIL/TEL line pointing at nothing. + """ + assert _wrap_contact_field({"type": ["WORK"]}) == [] + # Mixed list: the valid entry survives, the value-less dict is omitted. + out = _wrap_contact_field( + [{"value": "ok@example.com", "type": ["WORK"]}, {"type": ["HOME"]}] + ) + assert out == [{"value": "ok@example.com", "type": ["WORK"]}] + + +def test_missing_fn_logs_warning(caplog): + """A missing ``fn`` should log a warning so operators notice malformed payloads.""" + import logging + + with caplog.at_level( + logging.WARNING, logger="nextcloud_mcp_server.client.contacts" + ): + try: + _build_contact_from_data({"email": "x@example.com"}, uid="no-fn-uid") + except Exception: + # pythonvCard4 may raise on missing fn; we only care about the warning log. + pass + assert any("fn" in r.message.lower() for r in caplog.records) + + +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"} + + +class TestMergeVcardProperties: + """Direct tests for ``_merge_vcard_properties`` — the primary update path. + + Written in response to PR #719 review claiming NICKNAME/BDAY/CATEGORIES are not + updatable via this function. These tests pin the actual behaviour so future + regressions (or claims) can be answered in one line. + """ + + @staticmethod + def _merge(raw: str, data: dict) -> str: + from nextcloud_mcp_server.client.contacts import ContactsClient + + client = ContactsClient.__new__(ContactsClient) # no HTTP / no __init__ + return client._merge_vcard_properties(raw, data, uid="merge-test") + + def test_nickname_overwrites_existing_line(self): + """Existing NICKNAME must be replaced with the new value, not preserved.""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nNICKNAME:Bob\nEND:VCARD\n" + result = self._merge(existing, {"nickname": "Robert"}) + assert "NICKNAME:Robert" in result + assert "NICKNAME:Bob" not in result + + def test_bday_overwrites_existing_line(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nBDAY:1990-05-01\nEND:VCARD\n" + result = self._merge(existing, {"bday": "1991-06-02"}) + assert "BDAY:1991-06-02" in result + assert "BDAY:1990-05-01" not in result + + def test_categories_overwrites_existing_line(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nCATEGORIES:old,stale\nEND:VCARD\n" + result = self._merge(existing, {"categories": ["vip", "new"]}) + assert "CATEGORIES:vip,new" in result + assert "old,stale" not in result + + def test_nickname_added_when_not_in_existing_vcard(self): + """If the existing vCard has no NICKNAME line, update must append one.""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"nickname": "Bob"}) + assert "NICKNAME:Bob" in result + + def test_bday_added_when_not_in_existing_vcard(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"bday": "1990-05-01"}) + assert "BDAY:1990-05-01" in result + + def test_categories_added_when_not_in_existing_vcard(self): + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"categories": "a,b,c"}) + assert "CATEGORIES:a,b,c" in result + + def test_url_update_preserves_unrelated_properties(self): + """A URL update must not clobber ORG / NOTE / TEL from the existing vCard.""" + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "ORG:Acme\nTEL:555-1234\nNOTE:keep me\nEND:VCARD\n" + ) + result = self._merge(existing, {"url": "https://example.com"}) + assert "URL:https://example.com" in result + assert "ORG:Acme" in result + assert "TEL:555-1234" in result + assert "NOTE:keep me" in result + + def test_dict_email_input_preserves_existing_line(self): + """Regression: a dict-form email on update used to consume the existing + EMAIL: line and write nothing, silently deleting the contact's email. + Now the original line is preserved when the input shape isn't a plain str. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "EMAIL;TYPE=HOME:alice@example.com\nEND:VCARD\n" + ) + result = self._merge( + existing, {"email": {"value": "work@example.com", "type": ["WORK"]}} + ) + assert "EMAIL;TYPE=HOME:alice@example.com" in result + + def test_list_tel_input_preserves_existing_line(self): + """Same regression as the email branch but for TEL — a list-shaped tel + input must not silently drop the existing phone number. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "TEL;TYPE=HOME:555-0001\nEND:VCARD\n" + ) + result = self._merge( + existing, {"tel": [{"value": "555-9999", "type": ["WORK"]}]} + ) + assert "TEL;TYPE=HOME:555-0001" in result + + def test_invalid_bday_on_update_preserves_existing_line(self): + """A non-ISO BDAY string must not produce a malformed vCard line on update. + We share validation with the create path; invalid → keep the existing line. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n" + "BDAY:1990-05-01\nEND:VCARD\n" + ) + result = self._merge(existing, {"bday": "not-a-date"}) + assert "BDAY:1990-05-01" in result + assert "BDAY:not-a-date" not in result + + def test_invalid_bday_on_add_new_is_dropped(self): + """No existing BDAY + invalid input → no BDAY line appended (vs. raw write).""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"bday": "not-a-date"}) + assert "BDAY" not in result + + def test_newline_in_note_does_not_inject_property(self): + """Regression: a literal newline in a value must not terminate the line + and inject a fresh vCard property. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge( + existing, {"note": "harmless\nEMAIL:attacker@evil.example"} + ) + # The injected property must not appear as a real EMAIL line. + lines = result.splitlines() + assert "EMAIL:attacker@evil.example" not in lines + # The note value is preserved with newlines escaped per RFC 6350. + assert any(line.startswith("NOTE:") and "\\n" in line for line in lines) + + def test_list_org_overwrites_with_semicolon_join(self): + """Regression: list-form ORG used to fall through ``_safe_vcard_value`` + unchanged and emit a Python ``repr`` like ``ORG:['Acme', 'Engineering']``. + Per RFC 6350 §6.6.4 components are ``;``-separated. + """ + existing = ( + "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nORG:OldCo\nEND:VCARD\n" + ) + result = self._merge(existing, {"org": ["Acme", "Engineering"]}) + assert "ORG:Acme;Engineering" in result + assert "ORG:OldCo" not in result + assert "[" not in result and "'Acme'" not in result + + def test_list_org_added_with_semicolon_join(self): + """Add-new branch: list ORG without an existing line still serialises + with ``;`` rather than as a Python list repr. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + result = self._merge(existing, {"org": ["Acme", "Engineering"]}) + assert "ORG:Acme;Engineering" in result + assert "[" not in result + + def test_dict_email_on_no_existing_line_warns(self, caplog): + """No existing EMAIL + dict input is a known limitation of the text-merge + path. Surface it as a warning so the silent no-op is at least observable. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + with caplog.at_level("WARNING"): + result = self._merge( + existing, {"email": {"value": "alice@work.com", "type": ["WORK"]}} + ) + # The dict input is not applied; no EMAIL line is added. + assert "EMAIL" not in result + # A warning specifically calls out the dict/list shape and recommends + # plain str / create_contact as alternatives. + assert any( + "email" in r.message and "dict/list shape" in r.message + for r in caplog.records + ) + + def test_list_tel_on_no_existing_line_warns(self, caplog): + """Same warning behaviour for TEL — list input on a contact without an + existing TEL line must not silently disappear. + """ + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + with caplog.at_level("WARNING"): + result = self._merge( + existing, {"tel": [{"value": "555-9999", "type": ["WORK"]}]} + ) + assert "TEL" not in result + assert any( + "tel" in r.message and "dict/list shape" in r.message + for r in caplog.records + ) + + def test_str_email_does_not_warn(self, caplog): + """Plain string email is the supported shape — no warning should fire.""" + existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n" + with caplog.at_level("WARNING"): + result = self._merge(existing, {"email": "alice@work.com"}) + assert "EMAIL:alice@work.com" in result + assert not any("dict/list shape" in r.message for r in caplog.records)