From 723aee9134e3b7fe63e4672c54283e4d16dcbc74 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 23 Apr 2026 07:28:05 +0200 Subject: [PATCH] fix(contacts): persist all documented fields on create (fixes #716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create_contact previously read only fn/email/tel from contact_data and silently dropped org, organization, note, title, nickname, bday, categories, url — and didn't accept phone as an alias for tel, so the reporter's exact call lost every field except fn and email. Introduce _build_contact_from_data, share it with update_contact's fallback, and normalise str→list inputs so pythonvCard4 doesn't iterate bare strings character-by-character for list-typed properties. Closes #716 Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/contacts.py | 117 ++++++++++++++++-- nextcloud_mcp_server/server/contacts.py | 18 ++- .../contacts/test_contacts_operations.py | 44 +++++++ tests/server/test_contacts_mcp.py | 11 +- 4 files changed, 174 insertions(+), 16 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index a9a8e457..256a8c16 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -11,6 +11,108 @@ from .base import BaseNextcloudClient logger = logging.getLogger(__name__) +# Keys that _build_contact_from_data consumes. Used to warn (not error) on unknown keys. +_SUPPORTED_CONTACT_KEYS = frozenset( + { + "fn", + "email", + "tel", + "phone", + "org", + "organization", + "note", + "title", + "nickname", + "bday", + "categories", + "url", + } +) + + +def _wrap_contact_field(value) -> list[dict]: + """Normalize an email/tel/url 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). + """ + if value is None or value == "": + return [] + items = value if isinstance(value, list) else [value] + out: list[dict] = [] + for item in items: + if isinstance(item, dict) and item.get("value"): + types = item.get("type") or ["HOME"] + out.append({"value": item["value"], "type": list(types)}) + elif isinstance(item, str) and item: + out.append({"value": item, "type": ["HOME"]}) + return out + + +def _build_contact_from_data(contact_data: dict, 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. + """ + + # 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": contact_data.get("fn"), "uid": uid} + + emails = _wrap_contact_field(contact_data.get("email")) + if emails: + kwargs["email"] = emails + + tels = _wrap_contact_field(contact_data.get("tel") or contact_data.get("phone")) + 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 contact_data.get("note"): + kwargs["note"] = contact_data["note"] + + if contact_data.get("title"): + kwargs["title"] = contact_data["title"] + + if contact_data.get("nickname"): + kwargs["nickname"] = _as_list(contact_data["nickname"]) + + if contact_data.get("categories"): + kwargs["categories"] = _as_list(contact_data["categories"]) + + if contact_data.get("url"): + kwargs["url"] = _as_list(contact_data["url"]) + + bday = contact_data.get("bday") + if bday: + if isinstance(bday, date): + kwargs["bday"] = bday + elif isinstance(bday, str): + try: + kwargs["bday"] = date.fromisoformat(bday) + except ValueError: + logger.warning("Ignoring non-ISO bday value: %r", bday) + + unknown = set(contact_data) - _SUPPORTED_CONTACT_KEYS + if unknown: + logger.debug("Ignoring unknown contact_data keys: %s", sorted(unknown)) + + return Contact(**kwargs) # type: ignore[arg-type] + + class ContactsClient(BaseNextcloudClient): """Client for NextCloud CardDAV contact operations.""" @@ -127,13 +229,7 @@ class ContactsClient(BaseNextcloudClient): 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() + vcard = _build_contact_from_data(contact_data, uid).to_vcard() headers = { "Content-Type": "text/vcard; charset=utf-8", @@ -177,12 +273,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", diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 2becd064..90935b05 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -191,7 +191,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( diff --git a/tests/client/contacts/test_contacts_operations.py b/tests/client/contacts/test_contacts_operations.py index 85628dc9..63317174 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 f391fa82..c464ff83 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,13 @@ 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 # 5. Delete contact via MCP logger.info(f"Deleting contact {contact_uid} via MCP")