fix(models): coerce Contact.birthday + relax Table.owner_display_name
Two upstream Pydantic ValidationErrors that took down whole list responses. #704: Contact.birthday is declared str, but vobject parses BDAY as a datetime.date — any contact with a populated BDAY broke nc_contacts_list_contacts entirely. Add a field_validator(mode="before") that coerces date / datetime to ISO strings. Strings and None pass through unchanged. Defense in depth: existing call sites already coerce, but the model is now correct on its own so any future code path that constructs Contact from raw vobject output stays safe. #728: Tables app v2.0.1 stopped emitting owner_display_name on the top-level table payload (still present inside views via get_schema), so list_tables failed for every user with a Pydantic ValidationError. Make the field Optional[str] = None — captures the value when present, won't blow up when missing. Six new direct-construction unit tests in tests/unit/test_response_models.py pin both fixes (date / datetime / str / None for birthday; with / without owner_display_name for Table) so the regressions can't recur silently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
4e669781ee
commit
06f895f5b9
@@ -1,8 +1,9 @@
|
||||
"""Pydantic models for Contacts app responses."""
|
||||
|
||||
from datetime import date, datetime
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
from pydantic import BaseModel, Field
|
||||
from pydantic import BaseModel, Field, field_validator
|
||||
|
||||
from .base import BaseResponse, StatusResponse
|
||||
|
||||
@@ -57,6 +58,21 @@ class Contact(BaseModel):
|
||||
)
|
||||
etag: Optional[str] = Field(None, description="ETag for versioning")
|
||||
|
||||
@field_validator("birthday", mode="before")
|
||||
@classmethod
|
||||
def _coerce_birthday(cls, value: Any) -> Any:
|
||||
"""Accept ``date``/``datetime`` from vobject and serialise to ISO string.
|
||||
|
||||
vobject parses BDAY into ``datetime.date`` (or ``datetime.datetime``);
|
||||
the Pydantic model stores it as ``str``. Without this coercion any
|
||||
contact with a populated BDAY blew up the entire ``list_contacts``
|
||||
response (#704, #672). Strings and ``None`` pass through unchanged so
|
||||
callers can still construct contacts with raw ISO input.
|
||||
"""
|
||||
if isinstance(value, (date, datetime)):
|
||||
return value.isoformat()
|
||||
return value
|
||||
|
||||
@property
|
||||
def primary_email(self) -> Optional[str]:
|
||||
"""Get the primary email address."""
|
||||
|
||||
@@ -72,7 +72,12 @@ class Table(BaseModel):
|
||||
title: str = Field(description="Table title")
|
||||
emoji: Optional[str] = Field(None, description="Table emoji")
|
||||
ownership: str = Field(description="Table ownership")
|
||||
owner_display_name: str = Field(description="Display name of table owner")
|
||||
# Tables app v2.0.1 stopped emitting owner_display_name at the top level
|
||||
# (still present inside views via get_schema). Optional avoids a 100% failure
|
||||
# rate on list_tables — see #728.
|
||||
owner_display_name: Optional[str] = Field(
|
||||
None, description="Display name of table owner"
|
||||
)
|
||||
created_by: Optional[str] = Field(None, description="User who created the table")
|
||||
created_at: Optional[str] = Field(None, description="Table creation timestamp")
|
||||
last_edit_by: Optional[str] = Field(
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
"""Unit tests for Pydantic response models."""
|
||||
|
||||
from datetime import date
|
||||
from datetime import date, datetime
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -18,6 +18,7 @@ from nextcloud_mcp_server.models.semantic import (
|
||||
SamplingSearchResponse,
|
||||
SemanticSearchResult,
|
||||
)
|
||||
from nextcloud_mcp_server.models.tables import Table
|
||||
from nextcloud_mcp_server.server.calendar import _event_dict_to_summary
|
||||
from nextcloud_mcp_server.server.contacts import _raw_contact_to_model
|
||||
|
||||
@@ -677,3 +678,76 @@ def test_event_dict_to_summary_calendar_name_without_display_name():
|
||||
|
||||
assert summary.calendar_name == "personal"
|
||||
assert summary.calendar_display_name == "personal"
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------------
|
||||
# Direct Contact() construction with date-typed birthday — pins #704 / #672
|
||||
# ----------------------------------------------------------------------------
|
||||
# These bypass _raw_contact_to_model and hit the Pydantic validator directly,
|
||||
# so any future code path that constructs Contact from raw vobject output is
|
||||
# covered without depending on the upstream coercion in server/contacts.py.
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_contact_model_coerces_date_birthday_to_iso():
|
||||
"""Regression for #704: a datetime.date birthday must coerce to ISO str
|
||||
rather than raise a Pydantic ValidationError.
|
||||
"""
|
||||
contact = Contact(uid="c1", fn="Date BDay", birthday=date(2000, 1, 1))
|
||||
assert contact.birthday == "2000-01-01"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_contact_model_coerces_datetime_birthday_to_iso():
|
||||
"""A datetime.datetime input is also valid in vobject output and must coerce."""
|
||||
contact = Contact(
|
||||
uid="c2", fn="DateTime BDay", birthday=datetime(2000, 1, 1, 12, 30)
|
||||
)
|
||||
assert contact.birthday == "2000-01-01T12:30:00"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_contact_model_string_birthday_passes_through():
|
||||
"""ISO strings must round-trip unchanged — the validator should be a no-op."""
|
||||
contact = Contact(uid="c3", fn="String BDay", birthday="1990-05-15")
|
||||
assert contact.birthday == "1990-05-15"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_contact_model_none_birthday_is_allowed():
|
||||
contact = Contact(uid="c4", fn="No BDay")
|
||||
assert contact.birthday is None
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------------
|
||||
# Table model parses without owner_display_name — pins #728
|
||||
# ----------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_table_parses_without_owner_display_name():
|
||||
"""Tables app v2.0.1 dropped owner_display_name from the top-level payload.
|
||||
Parsing must succeed (#728) — anything else 100%-fails nc_tables_list_tables.
|
||||
"""
|
||||
raw = {
|
||||
"id": 1,
|
||||
"title": "Welcome to Nextcloud Tables!",
|
||||
"ownership": "alice",
|
||||
# owner_display_name intentionally absent
|
||||
}
|
||||
table = Table(**raw)
|
||||
assert table.id == 1
|
||||
assert table.owner_display_name is None
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_table_parses_with_owner_display_name():
|
||||
"""When the field is present we still capture it — Optional doesn't drop data."""
|
||||
raw = {
|
||||
"id": 2,
|
||||
"title": "Old API Table",
|
||||
"ownership": "bob",
|
||||
"owner_display_name": "Bob the Builder",
|
||||
}
|
||||
table = Table(**raw)
|
||||
assert table.owner_display_name == "Bob the Builder"
|
||||
|
||||
Reference in New Issue
Block a user