From fff7d54af72e9e685f48a08f1e1547c316222946 Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 11:39:42 +0300 Subject: [PATCH 1/7] fix(outlook): handle contact groups in the Contacts folder Relates to elastic/sdh-search#1898 The Exchange Contacts folder can also contain DistributionList (contact group) items, which do not carry the per-contact fields the formatter reads (email_addresses, phone_numbers, company_name, birthday). Accessing those attributes raised 'DistributionList' object has no attribute 'email_addresses' and aborted the entire sync. Read the per-contact fields defensively so a single contact group is indexed by name instead of crashing the sync. Co-authored-by: Cursor --- .../connectors/sources/outlook/datasource.py | 11 ++++--- .../tests/sources/test_outlook.py | 30 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index 77280007f..2d3f73936 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -145,6 +145,9 @@ def task_doc_formatter(self, task, timezone): } def contact_doc_formatter(self, contact, timezone): + # The Contacts folder can also return contact groups (DistributionList), + # which lack the per-contact fields below. Read them defensively so a + # single group no longer aborts the whole sync with an AttributeError. return { "_id": contact.id, "type": "Contact", @@ -154,17 +157,17 @@ def contact_doc_formatter(self, contact, timezone): "name": contact.display_name, "email_addresses": [ email.email - for email in (contact.email_addresses or []) + for email in (getattr(contact, "email_addresses", None) or []) if email and email.email ], "contact_numbers": [ number.phone_number - for number in (contact.phone_numbers or []) + for number in (getattr(contact, "phone_numbers", None) or []) if number and number.phone_number ], - "company_name": contact.company_name, + "company_name": getattr(contact, "company_name", None), "birthday": ews_format_to_datetime( - source_datetime=contact.birthday, timezone=timezone + source_datetime=getattr(contact, "birthday", None), timezone=timezone ), } diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 8896d5a44..432e73b9b 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -246,6 +246,18 @@ def __init__(self): self.birthday = "2023-12-12T01:01:01Z" +class DistributionListDocument: + """Mimics a DistributionList (contact group) item that Exchange can return + from the Contacts folder. It carries only the shared item fields and lacks + the per-contact fields (email_addresses, phone_numbers, company_name, + birthday).""" + + def __init__(self): + self.id = "distribution_list_1" + self.last_modified_time = "2023-12-12T01:01:01Z" + self.display_name = "Dummy Group" + + class CalendarDocument: def __init__(self): organizer = MagicMock() @@ -1232,6 +1244,24 @@ def test_contact_doc_formatter_handles_missing_email_and_phone_entries(): assert document["contact_numbers"] == [] +def test_contact_doc_formatter_handles_distribution_list(): + # A DistributionList (contact group) lacks the per-contact fields; the + # formatter must not crash with an AttributeError and abort the sync. + distribution_list = DistributionListDocument() + + document = OutlookDocFormatter().contact_doc_formatter( + contact=distribution_list, + timezone=TIMEZONE, + ) + + assert document["_id"] == "distribution_list_1" + assert document["name"] == "Dummy Group" + assert document["email_addresses"] == [] + assert document["contact_numbers"] == [] + assert document["company_name"] is None + assert document["birthday"] is None + + @pytest.mark.asyncio async def test_fetch_attachments_skips_attachment_without_id(): async with create_outlook_source() as source: From 7d92b8607948585c11d087a3d78f7bb4bcc8743e Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 12:11:04 +0300 Subject: [PATCH 2/7] fix(outlook): isolate per-item failures and harden remaining folder/field assumptions Relates to elastic/sdh-search#1898 Instead of hardening one field at a time, isolate item-shape failures so a single malformed Exchange object no longer aborts the whole sync: - Add a per-item guard (_format_item) around mail, contact, task, calendar, and attachment formatting that skips and logs an item on shape errors (AttributeError/KeyError/ValueError/TypeError) while letting connection-wide errors (transport/SSL/rate-limit/missing mailbox) still fail the sync loudly. - Skip Calendar and Tasks folders on ErrorFolderNotFound, mirroring the existing Contacts and mail-folder handling (shared/resource mailboxes may lack these folders). - Guard the Birthdays calendar branch so a missing start no longer raises on the .split() of a None datetime. - Read the optional LDAP "type" key defensively. Co-authored-by: Cursor --- .../connectors/sources/outlook/client.py | 36 +++++- .../connectors/sources/outlook/datasource.py | 112 ++++++++++++++---- .../tests/sources/test_outlook.py | 106 +++++++++++++++++ 3 files changed, 228 insertions(+), 26 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/client.py b/app/connectors_service/connectors/sources/outlook/client.py index 505b18522..49fee6013 100644 --- a/app/connectors_service/connectors/sources/outlook/client.py +++ b/app/connectors_service/connectors/sources/outlook/client.py @@ -226,7 +226,7 @@ async def get_user_accounts(self): ) async for user in self.get_users(): - if "searchResRef" in user["type"]: + if "searchResRef" in user.get("type", ""): continue mail = _extract_ldap_mail(user.get("attributes", {})) @@ -423,20 +423,44 @@ async def get_mails(self, account): yield mail, mail_type async def get_calendars(self, account): - for calendar in await asyncio.to_thread( - account.calendar.all().only, *CALENDAR_FIELDS - ): + # account.calendar uses a distinguished folder ID (locale-agnostic), but + # a mailbox may still lack a Calendar folder; skip it instead of aborting. + try: + folder = account.calendar + except ErrorFolderNotFound: + self._logger.warning( + f"Could not resolve Calendar folder for {account.primary_smtp_address}, skipping." + ) + return + for calendar in await asyncio.to_thread(folder.all().only, *CALENDAR_FIELDS): yield calendar async def get_child_calendars(self, account): - for child_calendar in account.calendar.children: + try: + child_calendars = account.calendar.children + except ErrorFolderNotFound: + self._logger.warning( + f"Could not resolve Calendar folder for {account.primary_smtp_address}, " + "skipping child calendars." + ) + return + for child_calendar in child_calendars: for calendar in await asyncio.to_thread( child_calendar.all().only, *CALENDAR_FIELDS ): yield calendar, child_calendar async def get_tasks(self, account): - for task in await asyncio.to_thread(account.tasks.all().only, *TASK_FIELDS): + # account.tasks uses a distinguished folder ID (locale-agnostic), but + # shared/resource mailboxes may lack a Tasks folder; skip if absent. + try: + folder = account.tasks + except ErrorFolderNotFound: + self._logger.warning( + f"Could not resolve Tasks folder for {account.primary_smtp_address}, skipping." + ) + return + for task in await asyncio.to_thread(folder.all().only, *TASK_FIELDS): yield task async def get_contacts(self, account): diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index 2d3f73936..bff274679 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -34,6 +34,14 @@ ) from connectors.utils import html_to_text +# Errors that mean a single Exchange item has an unexpected shape (a missing +# attribute, an unexpected item type, a bad field value, etc.). These are +# isolated per item so one malformed object cannot abort the whole sync. +# Connection-wide failures (transport, SSL, rate limiting, missing mailbox) are +# deliberately excluded so they still fail the sync loudly instead of silently +# emptying the index. +ITEM_SHAPE_ERRORS = (AttributeError, KeyError, ValueError, TypeError) + class OutlookDocFormatter: """Format Outlook object documents to Elasticsearch document""" @@ -90,11 +98,14 @@ def calendar_doc_formatter(self, calendar, child_calendar, timezone): } if child_calendar in ["Folder (Birthdays)", "Birthdays (Birthdays)"]: + # calendar.start can be absent on malformed items; guard the split so + # ews_format_to_datetime returning None does not raise. + birthday = ews_format_to_datetime( + source_datetime=calendar.start, timezone=timezone + ) document.update( { - "date": ews_format_to_datetime( - source_datetime=calendar.start, timezone=timezone - ).split("T", 1)[0], + "date": birthday.split("T", 1)[0] if birthday else None, } ) else: @@ -491,6 +502,31 @@ async def download_func(self, content): """ yield content + def _format_item(self, formatter, item, account): + """Run a document formatter for a single item, isolating item-shape + errors so one malformed Exchange object is skipped (with a warning) + instead of aborting the whole sync. + + Args: + formatter (callable): Zero-argument callable that formats the item + (typically a functools.partial around a doc formatter). + item: The Exchange item being formatted, used for logging context. + account: The account the item belongs to, used for logging context. + + Returns: + The formatted document, or None if the item was malformed and + skipped. Connection-wide errors are not caught here and propagate. + """ + try: + return formatter() + except ITEM_SHAPE_ERRORS as error: + self._logger.warning( + f"Skipping malformed {type(item).__name__} item " + f"{getattr(item, 'id', 'unknown')} for " + f"{account.primary_smtp_address}: {error}" + ) + return None + async def _fetch_attachments( self, attachment_type, outlook_object, timezone, account ): @@ -500,11 +536,18 @@ async def _fetch_attachments( f"Skipping attachment without an ID on item {outlook_object.id}" ) continue - document = self.doc_formatter.attachment_doc_formatter( - attachment=attachment, - attachment_type=attachment_type, - timezone=timezone, + document = self._format_item( + partial( + self.doc_formatter.attachment_doc_formatter, + attachment=attachment, + attachment_type=attachment_type, + timezone=timezone, + ), + item=attachment, + account=account, ) + if document is None: + continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -516,11 +559,18 @@ async def _fetch_attachments( async def _fetch_mails(self, account, timezone): async for mail, mail_type in self.client.get_mails(account=account): - document = self.doc_formatter.mails_doc_formatter( - mail=mail, - mail_type=mail_type, - timezone=timezone, + document = self._format_item( + partial( + self.doc_formatter.mails_doc_formatter, + mail=mail, + mail_type=mail_type, + timezone=timezone, + ), + item=mail, + account=account, ) + if document is None: + continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -540,10 +590,17 @@ async def _fetch_mails(self, account, timezone): async def _fetch_contacts(self, account, timezone): self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") async for contact in self.client.get_contacts(account=account): - document = self.doc_formatter.contact_doc_formatter( - contact=contact, - timezone=timezone, + document = self._format_item( + partial( + self.doc_formatter.contact_doc_formatter, + contact=contact, + timezone=timezone, + ), + item=contact, + account=account, ) + if document is None: + continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -554,9 +611,17 @@ async def _fetch_contacts(self, account, timezone): async def _fetch_tasks(self, account, timezone): self._logger.debug(f"Fetching tasks for {account.primary_smtp_address}") async for task in self.client.get_tasks(account=account): - document = self.doc_formatter.task_doc_formatter( - task=task, timezone=timezone + document = self._format_item( + partial( + self.doc_formatter.task_doc_formatter, + task=task, + timezone=timezone, + ), + item=task, + account=account, ) + if document is None: + continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -600,11 +665,18 @@ async def _fetch_child_calendars(self, account, timezone): yield doc async def _enqueue_calendars(self, calendar, child_calendar, timezone, account): - document = self.doc_formatter.calendar_doc_formatter( - calendar=calendar, - child_calendar=str(child_calendar), - timezone=timezone, + document = self._format_item( + partial( + self.doc_formatter.calendar_doc_formatter, + calendar=calendar, + child_calendar=str(child_calendar), + timezone=timezone, + ), + item=calendar, + account=account, ) + if document is None: + return yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 432e73b9b..3dd0ea2fd 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -258,6 +258,17 @@ def __init__(self): self.display_name = "Dummy Group" +class BrokenContact: + """Mimics a malformed item whose formatting raises an item-shape error.""" + + id = "broken_contact" + + @property + def last_modified_time(self): + msg = "malformed contact" + raise AttributeError(msg) + + class CalendarDocument: def __init__(self): organizer = MagicMock() @@ -1174,6 +1185,88 @@ async def test_get_contacts_skips_when_folder_not_found(): assert contacts == [] +@pytest.mark.asyncio +async def test_get_calendars_skips_when_folder_not_found(): + async with create_outlook_source() as source: + account = MagicMock() + account.primary_smtp_address = "alex.wilber@gmail.com" + type(account).calendar = mock.PropertyMock( + side_effect=ErrorFolderNotFound("no") + ) + + calendars = [ + calendar async for calendar in source.client.get_calendars(account) + ] + assert calendars == [] + + +@pytest.mark.asyncio +async def test_get_child_calendars_skips_when_folder_not_found(): + async with create_outlook_source() as source: + account = MagicMock() + account.primary_smtp_address = "alex.wilber@gmail.com" + type(account).calendar = mock.PropertyMock( + side_effect=ErrorFolderNotFound("no") + ) + + calendars = [ + calendar async for calendar in source.client.get_child_calendars(account) + ] + assert calendars == [] + + +@pytest.mark.asyncio +async def test_get_tasks_skips_when_folder_not_found(): + async with create_outlook_source() as source: + account = MagicMock() + account.primary_smtp_address = "alex.wilber@gmail.com" + type(account).tasks = mock.PropertyMock(side_effect=ErrorFolderNotFound("no")) + + tasks = [task async for task in source.client.get_tasks(account)] + assert tasks == [] + + +@pytest.mark.asyncio +async def test_fetch_contacts_skips_malformed_item_and_continues(): + # A single malformed item must be skipped with a warning, not abort the sync. + async with create_outlook_source() as source: + source._logger = MagicMock() + source.client.get_contacts = AsyncIterator([BrokenContact(), ContactDocument()]) + account = MockAccount() + + documents = [ + document + async for document, _ in source._fetch_contacts( + account=account, timezone=TIMEZONE + ) + ] + + assert [document["_id"] for document in documents] == ["contact_1"] + source._logger.warning.assert_called_once() + warning_message = source._logger.warning.call_args.args[0] + assert "broken_contact" in warning_message + assert "BrokenContact" in warning_message + + +@pytest.mark.asyncio +@patch("connectors.sources.outlook.client.Account", return_value="account") +async def test_exchange_get_user_accounts_handles_missing_type_key( + mock_account, reset_http_adapter_cls +): + # Some LDAP entries may not carry a "type" key; that must not raise KeyError. + user_without_type = {"attributes": {"mail": ["user@example.com"]}} + async with create_outlook_source() as source: + source.client.is_cloud = False + source.client._get_user_instance.get_users = AsyncIterator([user_without_type]) + + accounts = [ + account + async for account in source.client._get_user_instance.get_user_accounts() + ] + + assert accounts == ["account"] + + def test_mails_doc_formatter_handles_missing_sender(): mail = MailDocument() mail.sender = None @@ -1215,6 +1308,19 @@ def test_calendar_doc_formatter_handles_occurrence_without_recurrence(): assert document["meeting_type"] == "Occurrence" +def test_calendar_doc_formatter_handles_birthday_without_start(): + calendar = CalendarDocument() + calendar.start = None + + document = OutlookDocFormatter().calendar_doc_formatter( + calendar=calendar, + child_calendar="Birthdays (Birthdays)", + timezone=TIMEZONE, + ) + + assert document["date"] is None + + def test_calendar_doc_formatter_skips_attendees_without_mailbox(): calendar = CalendarDocument() attendee_without_mailbox = MagicMock() From a9fb2124f5feb1c00639ed8bab40c9c238046383 Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 12:33:37 +0300 Subject: [PATCH 3/7] refactor(outlook): format contacts by item type instead of defensive getattr Addresses review feedback on #4147: rather than reading contact fields defensively (which silently tolerated any missing attribute), dispatch on the item type returned by the Contacts folder, which exchangelib guarantees is one of Contact or DistributionList. - Restore the strict Contact formatter (documents the real Contact schema). - Add a dedicated distribution_list_doc_formatter that indexes a contact group by name and its members' email addresses, typed as "Distribution List". - Dispatch by isinstance in _fetch_contacts; the general _format_item guard remains as defense-in-depth for genuinely unexpected shapes. - Fetch the union of Contact and DistributionList fields for the folder query. Co-authored-by: Cursor --- .../connectors/sources/outlook/client.py | 6 +- .../connectors/sources/outlook/constants.py | 8 +++ .../connectors/sources/outlook/datasource.py | 49 +++++++++++----- .../tests/sources/test_outlook.py | 58 +++++++++++++++---- 4 files changed, 95 insertions(+), 26 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/client.py b/app/connectors_service/connectors/sources/outlook/client.py index 49fee6013..ef752494d 100644 --- a/app/connectors_service/connectors/sources/outlook/client.py +++ b/app/connectors_service/connectors/sources/outlook/client.py @@ -28,7 +28,7 @@ from connectors.sources.outlook.constants import ( API_SCOPE, CALENDAR_FIELDS, - CONTACT_FIELDS, + CONTACT_FOLDER_FIELDS, EWS_ENDPOINT, MAIL_FIELDS, MAIL_TYPES, @@ -473,5 +473,7 @@ async def get_contacts(self, account): f"Could not resolve Contacts folder for {account.primary_smtp_address}, skipping." ) return - for contact in await asyncio.to_thread(folder.all().only, *CONTACT_FIELDS): + for contact in await asyncio.to_thread( + folder.all().only, *CONTACT_FOLDER_FIELDS + ): yield contact diff --git a/app/connectors_service/connectors/sources/outlook/constants.py b/app/connectors_service/connectors/sources/outlook/constants.py index 1d7db5bd8..57e21e518 100644 --- a/app/connectors_service/connectors/sources/outlook/constants.py +++ b/app/connectors_service/connectors/sources/outlook/constants.py @@ -71,6 +71,14 @@ "company_name", "birthday", ] +DISTRIBUTION_LIST_FIELDS = [ + "last_modified_time", + "display_name", + "members", +] +# The Contacts folder holds both Contact and DistributionList items, so the +# query must request the union of the fields each formatter needs. +CONTACT_FOLDER_FIELDS = list(dict.fromkeys(CONTACT_FIELDS + DISTRIBUTION_LIST_FIELDS)) TASK_FIELDS = [ "last_modified_time", "due_date", diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index bff274679..fa764826b 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -14,6 +14,7 @@ iso_utc, ) from exchangelib.errors import ErrorNonExistentMailbox +from exchangelib.items import DistributionList from connectors.access_control import ACCESS_CONTROL, es_access_control_query from connectors.sources.outlook.client import OutlookClient, _extract_ldap_mail @@ -156,9 +157,6 @@ def task_doc_formatter(self, task, timezone): } def contact_doc_formatter(self, contact, timezone): - # The Contacts folder can also return contact groups (DistributionList), - # which lack the per-contact fields below. Read them defensively so a - # single group no longer aborts the whole sync with an AttributeError. return { "_id": contact.id, "type": "Contact", @@ -168,20 +166,39 @@ def contact_doc_formatter(self, contact, timezone): "name": contact.display_name, "email_addresses": [ email.email - for email in (getattr(contact, "email_addresses", None) or []) + for email in (contact.email_addresses or []) if email and email.email ], "contact_numbers": [ number.phone_number - for number in (getattr(contact, "phone_numbers", None) or []) + for number in (contact.phone_numbers or []) if number and number.phone_number ], - "company_name": getattr(contact, "company_name", None), + "company_name": contact.company_name, "birthday": ews_format_to_datetime( - source_datetime=getattr(contact, "birthday", None), timezone=timezone + source_datetime=contact.birthday, timezone=timezone ), } + def distribution_list_doc_formatter(self, distribution_list, timezone): + # A DistributionList is a contact group. It carries only the shared item + # fields plus its members, not the per-contact fields, so it has a + # dedicated formatter rather than being forced into the Contact schema. + return { + "_id": distribution_list.id, + "type": "Distribution List", + "_timestamp": ews_format_to_datetime( + source_datetime=distribution_list.last_modified_time, + timezone=timezone, + ), + "name": distribution_list.display_name, + "email_addresses": [ + member.mailbox.email_address + for member in (distribution_list.members or []) + if member and member.mailbox and member.mailbox.email_address + ], + } + def attachment_doc_formatter(self, attachment, attachment_type, timezone): attachment_id = ( attachment.attachment_id.id if attachment.attachment_id else None @@ -590,15 +607,21 @@ async def _fetch_mails(self, account, timezone): async def _fetch_contacts(self, account, timezone): self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") async for contact in self.client.get_contacts(account=account): - document = self._format_item( - partial( + # The Contacts folder holds both individual contacts and contact + # groups (DistributionList); each has its own document shape. + if isinstance(contact, DistributionList): + formatter = partial( + self.doc_formatter.distribution_list_doc_formatter, + distribution_list=contact, + timezone=timezone, + ) + else: + formatter = partial( self.doc_formatter.contact_doc_formatter, contact=contact, timezone=timezone, - ), - item=contact, - account=account, - ) + ) + document = self._format_item(formatter, item=contact, account=account) if document is None: continue yield ( diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 3dd0ea2fd..4fbfa51b3 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -19,6 +19,7 @@ ErrorNonExistentMailbox, TransportError, ) +from exchangelib.items import DistributionList from exchangelib.protocol import BaseProtocol, NoVerifyHTTPAdapter from connectors.sources.outlook import OutlookDataSource @@ -248,14 +249,18 @@ def __init__(self): class DistributionListDocument: """Mimics a DistributionList (contact group) item that Exchange can return - from the Contacts folder. It carries only the shared item fields and lacks - the per-contact fields (email_addresses, phone_numbers, company_name, + from the Contacts folder. It carries the shared item fields plus its members, + not the per-contact fields (email_addresses, phone_numbers, company_name, birthday).""" def __init__(self): + member = MagicMock() + member.mailbox.email_address = "group.member@gmail.com" + self.id = "distribution_list_1" self.last_modified_time = "2023-12-12T01:01:01Z" self.display_name = "Dummy Group" + self.members = [member] class BrokenContact: @@ -1248,6 +1253,39 @@ async def test_fetch_contacts_skips_malformed_item_and_continues(): assert "BrokenContact" in warning_message +@pytest.mark.asyncio +async def test_fetch_contacts_routes_distribution_list_to_group_formatter(): + # A DistributionList must be dispatched to the group formatter, while a + # regular contact still goes through the contact formatter. + async with create_outlook_source() as source: + member = MagicMock() + member.mailbox.email_address = "group.member@gmail.com" + distribution_list = MagicMock(spec=DistributionList) + distribution_list.id = "distribution_list_1" + distribution_list.last_modified_time = "2023-12-12T01:01:01Z" + distribution_list.display_name = "Dummy Group" + distribution_list.members = [member] + + source.client.get_contacts = AsyncIterator( + [distribution_list, ContactDocument()] + ) + account = MockAccount() + + documents = [ + document + async for document, _ in source._fetch_contacts( + account=account, timezone=TIMEZONE + ) + ] + + by_id = {document["_id"]: document for document in documents} + assert by_id["distribution_list_1"]["type"] == "Distribution List" + assert by_id["distribution_list_1"]["email_addresses"] == [ + "group.member@gmail.com" + ] + assert by_id["contact_1"]["type"] == "Contact" + + @pytest.mark.asyncio @patch("connectors.sources.outlook.client.Account", return_value="account") async def test_exchange_get_user_accounts_handles_missing_type_key( @@ -1350,22 +1388,20 @@ def test_contact_doc_formatter_handles_missing_email_and_phone_entries(): assert document["contact_numbers"] == [] -def test_contact_doc_formatter_handles_distribution_list(): - # A DistributionList (contact group) lacks the per-contact fields; the - # formatter must not crash with an AttributeError and abort the sync. +def test_distribution_list_doc_formatter(): + # A DistributionList (contact group) is formatted by its own formatter and + # indexed by name plus its members' email addresses. distribution_list = DistributionListDocument() - document = OutlookDocFormatter().contact_doc_formatter( - contact=distribution_list, + document = OutlookDocFormatter().distribution_list_doc_formatter( + distribution_list=distribution_list, timezone=TIMEZONE, ) assert document["_id"] == "distribution_list_1" + assert document["type"] == "Distribution List" assert document["name"] == "Dummy Group" - assert document["email_addresses"] == [] - assert document["contact_numbers"] == [] - assert document["company_name"] is None - assert document["birthday"] is None + assert document["email_addresses"] == ["group.member@gmail.com"] @pytest.mark.asyncio From 0e9918315c2129f5ee94c71a3d6bc973dc2965b7 Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 13:15:10 +0300 Subject: [PATCH 4/7] docs(outlook): tighten inline comments Co-authored-by: Cursor --- .../connectors/sources/outlook/client.py | 6 ++---- .../connectors/sources/outlook/constants.py | 3 +-- .../connectors/sources/outlook/datasource.py | 19 ++++++------------- .../tests/sources/test_outlook.py | 6 ++---- 4 files changed, 11 insertions(+), 23 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/client.py b/app/connectors_service/connectors/sources/outlook/client.py index ef752494d..a87332a8b 100644 --- a/app/connectors_service/connectors/sources/outlook/client.py +++ b/app/connectors_service/connectors/sources/outlook/client.py @@ -423,8 +423,7 @@ async def get_mails(self, account): yield mail, mail_type async def get_calendars(self, account): - # account.calendar uses a distinguished folder ID (locale-agnostic), but - # a mailbox may still lack a Calendar folder; skip it instead of aborting. + # Skip instead of aborting if the mailbox has no Calendar folder. try: folder = account.calendar except ErrorFolderNotFound: @@ -451,8 +450,7 @@ async def get_child_calendars(self, account): yield calendar, child_calendar async def get_tasks(self, account): - # account.tasks uses a distinguished folder ID (locale-agnostic), but - # shared/resource mailboxes may lack a Tasks folder; skip if absent. + # Skip instead of aborting if the mailbox has no Tasks folder. try: folder = account.tasks except ErrorFolderNotFound: diff --git a/app/connectors_service/connectors/sources/outlook/constants.py b/app/connectors_service/connectors/sources/outlook/constants.py index 57e21e518..0333ac315 100644 --- a/app/connectors_service/connectors/sources/outlook/constants.py +++ b/app/connectors_service/connectors/sources/outlook/constants.py @@ -76,8 +76,7 @@ "display_name", "members", ] -# The Contacts folder holds both Contact and DistributionList items, so the -# query must request the union of the fields each formatter needs. +# Contacts folder holds both item types, so query the union of their fields. CONTACT_FOLDER_FIELDS = list(dict.fromkeys(CONTACT_FIELDS + DISTRIBUTION_LIST_FIELDS)) TASK_FIELDS = [ "last_modified_time", diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index fa764826b..e380014d8 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -35,12 +35,9 @@ ) from connectors.utils import html_to_text -# Errors that mean a single Exchange item has an unexpected shape (a missing -# attribute, an unexpected item type, a bad field value, etc.). These are -# isolated per item so one malformed object cannot abort the whole sync. -# Connection-wide failures (transport, SSL, rate limiting, missing mailbox) are -# deliberately excluded so they still fail the sync loudly instead of silently -# emptying the index. +# Per-item errors: a single malformed item is skipped, not fatal. Connection-wide +# failures (transport, SSL, rate limit, missing mailbox) are excluded on purpose +# so they still abort the sync instead of silently emptying the index. ITEM_SHAPE_ERRORS = (AttributeError, KeyError, ValueError, TypeError) @@ -99,8 +96,7 @@ def calendar_doc_formatter(self, calendar, child_calendar, timezone): } if child_calendar in ["Folder (Birthdays)", "Birthdays (Birthdays)"]: - # calendar.start can be absent on malformed items; guard the split so - # ews_format_to_datetime returning None does not raise. + # calendar.start may be missing; guard against splitting a None. birthday = ews_format_to_datetime( source_datetime=calendar.start, timezone=timezone ) @@ -181,9 +177,7 @@ def contact_doc_formatter(self, contact, timezone): } def distribution_list_doc_formatter(self, distribution_list, timezone): - # A DistributionList is a contact group. It carries only the shared item - # fields plus its members, not the per-contact fields, so it has a - # dedicated formatter rather than being forced into the Contact schema. + # A contact group has members, not per-contact fields, so it needs its own shape. return { "_id": distribution_list.id, "type": "Distribution List", @@ -607,8 +601,7 @@ async def _fetch_mails(self, account, timezone): async def _fetch_contacts(self, account, timezone): self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") async for contact in self.client.get_contacts(account=account): - # The Contacts folder holds both individual contacts and contact - # groups (DistributionList); each has its own document shape. + # Contacts folder holds both contacts and groups; each has its own shape. if isinstance(contact, DistributionList): formatter = partial( self.doc_formatter.distribution_list_doc_formatter, diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 4fbfa51b3..318addb3e 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -1255,8 +1255,7 @@ async def test_fetch_contacts_skips_malformed_item_and_continues(): @pytest.mark.asyncio async def test_fetch_contacts_routes_distribution_list_to_group_formatter(): - # A DistributionList must be dispatched to the group formatter, while a - # regular contact still goes through the contact formatter. + # DistributionList routes to the group formatter; a contact to the contact one. async with create_outlook_source() as source: member = MagicMock() member.mailbox.email_address = "group.member@gmail.com" @@ -1389,8 +1388,7 @@ def test_contact_doc_formatter_handles_missing_email_and_phone_entries(): def test_distribution_list_doc_formatter(): - # A DistributionList (contact group) is formatted by its own formatter and - # indexed by name plus its members' email addresses. + # A contact group is indexed by name plus its members' emails. distribution_list = DistributionListDocument() document = OutlookDocFormatter().distribution_list_doc_formatter( From 09a7de2f78a5ba6a9d80290ebbf5cb76296a374a Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 15:28:18 +0300 Subject: [PATCH 5/7] refactor(outlook): drop broad per-item guard for explicit contact type checks Per review feedback on #4147, the broad _format_item guard swallowed item-shape errors (AttributeError/KeyError/ValueError/TypeError) around every formatter call. That can turn a systematic formatting bug into a "successful" sync that returns 0 items and deletes previously indexed documents. Remove it in favor of explicit, visible handling: - Dispatch _fetch_contacts on item type: Contact -> contact_doc_formatter, DistributionList -> distribution_list_doc_formatter, and warn + skip any unexpected type instead of forcing it through a formatter. - Call the mail/task/calendar/attachment formatters directly again, so a genuine formatting error fails loudly rather than silently emptying the index. The folder guards (ErrorFolderNotFound on Calendar/Tasks), field-level null guards (sender/organizer/attendees/birthday), the optional LDAP type key, and the distribution list formatter remain. Co-authored-by: Cursor --- .../connectors/sources/outlook/datasource.py | 111 +++++------------- .../tests/sources/test_outlook.py | 53 ++++----- 2 files changed, 51 insertions(+), 113 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index e380014d8..d3f84a699 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -14,7 +14,7 @@ iso_utc, ) from exchangelib.errors import ErrorNonExistentMailbox -from exchangelib.items import DistributionList +from exchangelib.items import Contact, DistributionList from connectors.access_control import ACCESS_CONTROL, es_access_control_query from connectors.sources.outlook.client import OutlookClient, _extract_ldap_mail @@ -35,11 +35,6 @@ ) from connectors.utils import html_to_text -# Per-item errors: a single malformed item is skipped, not fatal. Connection-wide -# failures (transport, SSL, rate limit, missing mailbox) are excluded on purpose -# so they still abort the sync instead of silently emptying the index. -ITEM_SHAPE_ERRORS = (AttributeError, KeyError, ValueError, TypeError) - class OutlookDocFormatter: """Format Outlook object documents to Elasticsearch document""" @@ -513,31 +508,6 @@ async def download_func(self, content): """ yield content - def _format_item(self, formatter, item, account): - """Run a document formatter for a single item, isolating item-shape - errors so one malformed Exchange object is skipped (with a warning) - instead of aborting the whole sync. - - Args: - formatter (callable): Zero-argument callable that formats the item - (typically a functools.partial around a doc formatter). - item: The Exchange item being formatted, used for logging context. - account: The account the item belongs to, used for logging context. - - Returns: - The formatted document, or None if the item was malformed and - skipped. Connection-wide errors are not caught here and propagate. - """ - try: - return formatter() - except ITEM_SHAPE_ERRORS as error: - self._logger.warning( - f"Skipping malformed {type(item).__name__} item " - f"{getattr(item, 'id', 'unknown')} for " - f"{account.primary_smtp_address}: {error}" - ) - return None - async def _fetch_attachments( self, attachment_type, outlook_object, timezone, account ): @@ -547,18 +517,11 @@ async def _fetch_attachments( f"Skipping attachment without an ID on item {outlook_object.id}" ) continue - document = self._format_item( - partial( - self.doc_formatter.attachment_doc_formatter, - attachment=attachment, - attachment_type=attachment_type, - timezone=timezone, - ), - item=attachment, - account=account, + document = self.doc_formatter.attachment_doc_formatter( + attachment=attachment, + attachment_type=attachment_type, + timezone=timezone, ) - if document is None: - continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -570,18 +533,11 @@ async def _fetch_attachments( async def _fetch_mails(self, account, timezone): async for mail, mail_type in self.client.get_mails(account=account): - document = self._format_item( - partial( - self.doc_formatter.mails_doc_formatter, - mail=mail, - mail_type=mail_type, - timezone=timezone, - ), - item=mail, - account=account, + document = self.doc_formatter.mails_doc_formatter( + mail=mail, + mail_type=mail_type, + timezone=timezone, ) - if document is None: - continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -601,21 +557,23 @@ async def _fetch_mails(self, account, timezone): async def _fetch_contacts(self, account, timezone): self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") async for contact in self.client.get_contacts(account=account): - # Contacts folder holds both contacts and groups; each has its own shape. - if isinstance(contact, DistributionList): - formatter = partial( - self.doc_formatter.distribution_list_doc_formatter, + # Contacts folder holds contacts and groups; dispatch on type and + # skip anything unexpected instead of forcing it through a formatter. + if isinstance(contact, Contact): + document = self.doc_formatter.contact_doc_formatter( + contact=contact, + timezone=timezone, + ) + elif isinstance(contact, DistributionList): + document = self.doc_formatter.distribution_list_doc_formatter( distribution_list=contact, timezone=timezone, ) else: - formatter = partial( - self.doc_formatter.contact_doc_formatter, - contact=contact, - timezone=timezone, + self._logger.warning( + f"Skipping unexpected Contacts item type " + f"{type(contact).__name__} for {account.primary_smtp_address}" ) - document = self._format_item(formatter, item=contact, account=account) - if document is None: continue yield ( self._decorate_with_access_control( @@ -627,17 +585,9 @@ async def _fetch_contacts(self, account, timezone): async def _fetch_tasks(self, account, timezone): self._logger.debug(f"Fetching tasks for {account.primary_smtp_address}") async for task in self.client.get_tasks(account=account): - document = self._format_item( - partial( - self.doc_formatter.task_doc_formatter, - task=task, - timezone=timezone, - ), - item=task, - account=account, + document = self.doc_formatter.task_doc_formatter( + task=task, timezone=timezone ) - if document is None: - continue yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] @@ -681,18 +631,11 @@ async def _fetch_child_calendars(self, account, timezone): yield doc async def _enqueue_calendars(self, calendar, child_calendar, timezone, account): - document = self._format_item( - partial( - self.doc_formatter.calendar_doc_formatter, - calendar=calendar, - child_calendar=str(child_calendar), - timezone=timezone, - ), - item=calendar, - account=account, + document = self.doc_formatter.calendar_doc_formatter( + calendar=calendar, + child_calendar=str(child_calendar), + timezone=timezone, ) - if document is None: - return yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 318addb3e..c87621fb6 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -19,7 +19,7 @@ ErrorNonExistentMailbox, TransportError, ) -from exchangelib.items import DistributionList +from exchangelib.items import Contact, DistributionList from exchangelib.protocol import BaseProtocol, NoVerifyHTTPAdapter from connectors.sources.outlook import OutlookDataSource @@ -232,19 +232,22 @@ def __init__(self): self.attachments = [MOCK_ATTACHMENT] -class ContactDocument: - def __init__(self): - contact = MagicMock() - contact.email = "dummy.user@gmail.com" - contact.phone_number = 99887776655 +def ContactDocument(): + # A real Contact instance so _fetch_contacts' isinstance dispatch routes it + # to the contact formatter (spec mock keeps isinstance(..., Contact) True). + contact = MagicMock() + contact.email = "dummy.user@gmail.com" + contact.phone_number = 99887776655 - self.id = "contact_1" - self.last_modified_time = "2023-12-12T01:01:01Z" - self.display_name = "Dummy User" - self.email_addresses = [contact] - self.phone_numbers = [contact] - self.company_name = "ABC" - self.birthday = "2023-12-12T01:01:01Z" + document = MagicMock(spec=Contact) + document.id = "contact_1" + document.last_modified_time = "2023-12-12T01:01:01Z" + document.display_name = "Dummy User" + document.email_addresses = [contact] + document.phone_numbers = [contact] + document.company_name = "ABC" + document.birthday = "2023-12-12T01:01:01Z" + return document class DistributionListDocument: @@ -263,17 +266,6 @@ def __init__(self): self.members = [member] -class BrokenContact: - """Mimics a malformed item whose formatting raises an item-shape error.""" - - id = "broken_contact" - - @property - def last_modified_time(self): - msg = "malformed contact" - raise AttributeError(msg) - - class CalendarDocument: def __init__(self): organizer = MagicMock() @@ -1232,11 +1224,13 @@ async def test_get_tasks_skips_when_folder_not_found(): @pytest.mark.asyncio -async def test_fetch_contacts_skips_malformed_item_and_continues(): - # A single malformed item must be skipped with a warning, not abort the sync. +async def test_fetch_contacts_skips_unexpected_item_type_with_warning(): + # An item that is neither a Contact nor a DistributionList is skipped with a + # warning; a known Contact is still indexed. async with create_outlook_source() as source: source._logger = MagicMock() - source.client.get_contacts = AsyncIterator([BrokenContact(), ContactDocument()]) + unexpected = MagicMock() + source.client.get_contacts = AsyncIterator([unexpected, ContactDocument()]) account = MockAccount() documents = [ @@ -1249,8 +1243,9 @@ async def test_fetch_contacts_skips_malformed_item_and_continues(): assert [document["_id"] for document in documents] == ["contact_1"] source._logger.warning.assert_called_once() warning_message = source._logger.warning.call_args.args[0] - assert "broken_contact" in warning_message - assert "BrokenContact" in warning_message + assert "unexpected" in warning_message.lower() + assert type(unexpected).__name__ in warning_message + assert account.primary_smtp_address in warning_message @pytest.mark.asyncio From cf5231a6398b1a1b5b2bdf9e6a95a77bcfa5b132 Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 15:54:05 +0300 Subject: [PATCH 6/7] refactor(outlook): raise on unexpected Contacts item type instead of skipping The Contacts folder contractually returns only Contact or DistributionList, so an unknown type is a systematic broken-assumption condition, not per-item data variance. Warning and skipping would silently drop a whole category of items; raise a TypeError instead so the sync fails loudly and the gap is fixed in code. Co-authored-by: Cursor --- .../connectors/sources/outlook/datasource.py | 13 +++++----- .../tests/sources/test_outlook.py | 25 ++++++------------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index d3f84a699..e3b5bd302 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -557,8 +557,8 @@ async def _fetch_mails(self, account, timezone): async def _fetch_contacts(self, account, timezone): self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") async for contact in self.client.get_contacts(account=account): - # Contacts folder holds contacts and groups; dispatch on type and - # skip anything unexpected instead of forcing it through a formatter. + # Contacts folder returns only Contact or DistributionList; any other + # type breaks that assumption, so fail loudly instead of guessing. if isinstance(contact, Contact): document = self.doc_formatter.contact_doc_formatter( contact=contact, @@ -570,11 +570,12 @@ async def _fetch_contacts(self, account, timezone): timezone=timezone, ) else: - self._logger.warning( - f"Skipping unexpected Contacts item type " - f"{type(contact).__name__} for {account.primary_smtp_address}" + msg = ( + f"Unexpected Contacts item type {type(contact).__name__} for " + f"{account.primary_smtp_address}; expected Contact or " + "DistributionList" ) - continue + raise TypeError(msg) yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index c87621fb6..3b7eb993e 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -1224,28 +1224,17 @@ async def test_get_tasks_skips_when_folder_not_found(): @pytest.mark.asyncio -async def test_fetch_contacts_skips_unexpected_item_type_with_warning(): - # An item that is neither a Contact nor a DistributionList is skipped with a - # warning; a known Contact is still indexed. +async def test_fetch_contacts_raises_on_unexpected_item_type(): + # The Contacts folder should only yield Contact or DistributionList items; + # anything else breaks that assumption and must fail loudly. async with create_outlook_source() as source: - source._logger = MagicMock() unexpected = MagicMock() - source.client.get_contacts = AsyncIterator([unexpected, ContactDocument()]) + source.client.get_contacts = AsyncIterator([unexpected]) account = MockAccount() - documents = [ - document - async for document, _ in source._fetch_contacts( - account=account, timezone=TIMEZONE - ) - ] - - assert [document["_id"] for document in documents] == ["contact_1"] - source._logger.warning.assert_called_once() - warning_message = source._logger.warning.call_args.args[0] - assert "unexpected" in warning_message.lower() - assert type(unexpected).__name__ in warning_message - assert account.primary_smtp_address in warning_message + with pytest.raises(TypeError, match="Unexpected Contacts item type"): + async for _ in source._fetch_contacts(account=account, timezone=TIMEZONE): + pass @pytest.mark.asyncio From 3cd23fc09e7ba21ecd7608177770936d6c6f979a Mon Sep 17 00:00:00 2001 From: Jan-Kazlouski-elastic Date: Thu, 9 Jul 2026 17:22:35 +0300 Subject: [PATCH 7/7] fix(outlook): resolve folders off the event loop exchangelib is synchronous, so resolving a distinguished folder (e.g. account.calendar) issues a blocking GetFolder network call. Running it directly in the async get_* methods blocked the event loop until it returned. Wrap the folder resolution in asyncio.to_thread in get_mails, get_calendars, get_child_calendars, get_tasks and get_contacts so it runs off the event loop. ErrorFolderNotFound raised in the thread still propagates out of the await, so the existing skip-and-continue handling is unchanged. Also add tests asserting resolution is offloaded and that the Contacts query requests DistributionList fields (members). Co-authored-by: Cursor --- .../connectors/sources/outlook/client.py | 28 ++++--- .../tests/sources/test_outlook.py | 79 +++++++++++++++++++ 2 files changed, 97 insertions(+), 10 deletions(-) diff --git a/app/connectors_service/connectors/sources/outlook/client.py b/app/connectors_service/connectors/sources/outlook/client.py index a87332a8b..b522d72a2 100644 --- a/app/connectors_service/connectors/sources/outlook/client.py +++ b/app/connectors_service/connectors/sources/outlook/client.py @@ -406,12 +406,17 @@ async def get_mails(self, account): f"Fetching {mail_type['folder']} mails for {account.primary_smtp_address}" ) try: + # Resolve folders off the event loop (blocking exchangelib call). if mail_type["folder"] == "archive": # msg_folder_root is locale-agnostic; the "Archive" leaf has no # distinguished ID, so resolve it by name and skip if absent. - folder_object = account.msg_folder_root / "Archive" + folder_object = await asyncio.to_thread( + lambda: account.msg_folder_root / "Archive" + ) else: - folder_object = getattr(account, mail_type["folder"]) + folder_object = await asyncio.to_thread( + getattr, account, mail_type["folder"] + ) except ErrorFolderNotFound: self._logger.warning( f"Could not resolve {mail_type['folder']} folder for " @@ -423,9 +428,9 @@ async def get_mails(self, account): yield mail, mail_type async def get_calendars(self, account): - # Skip instead of aborting if the mailbox has no Calendar folder. + # Resolve the folder off the event loop (blocking call); skip if absent. try: - folder = account.calendar + folder = await asyncio.to_thread(getattr, account, "calendar") except ErrorFolderNotFound: self._logger.warning( f"Could not resolve Calendar folder for {account.primary_smtp_address}, skipping." @@ -435,8 +440,11 @@ async def get_calendars(self, account): yield calendar async def get_child_calendars(self, account): + # Resolve folder and children off the event loop; skip if absent. try: - child_calendars = account.calendar.children + child_calendars = await asyncio.to_thread( + lambda: list(account.calendar.children) + ) except ErrorFolderNotFound: self._logger.warning( f"Could not resolve Calendar folder for {account.primary_smtp_address}, " @@ -450,9 +458,9 @@ async def get_child_calendars(self, account): yield calendar, child_calendar async def get_tasks(self, account): - # Skip instead of aborting if the mailbox has no Tasks folder. + # Resolve the folder off the event loop (blocking call); skip if absent. try: - folder = account.tasks + folder = await asyncio.to_thread(getattr, account, "tasks") except ErrorFolderNotFound: self._logger.warning( f"Could not resolve Tasks folder for {account.primary_smtp_address}, skipping." @@ -462,10 +470,10 @@ async def get_tasks(self, account): yield task async def get_contacts(self, account): - # account.contacts uses a distinguished folder ID, which is locale-agnostic - # unlike name-based paths that break on non-English Exchange servers. + # account.contacts uses a locale-agnostic distinguished folder ID; resolve + # it off the event loop (blocking call); skip if absent. try: - folder = account.contacts + folder = await asyncio.to_thread(getattr, account, "contacts") except ErrorFolderNotFound: self._logger.warning( f"Could not resolve Contacts folder for {account.primary_smtp_address}, skipping." diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 3b7eb993e..bc00eb4af 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -5,6 +5,7 @@ # """Tests the Outlook source class methods""" +import asyncio import ssl from contextlib import asynccontextmanager from unittest import mock @@ -1223,6 +1224,84 @@ async def test_get_tasks_skips_when_folder_not_found(): assert tasks == [] +@pytest.mark.asyncio +@pytest.mark.parametrize( + "method_name, folder_attr", + [ + ("get_calendars", "calendar"), + ("get_tasks", "tasks"), + ("get_contacts", "contacts"), + ], +) +async def test_get_methods_resolve_folder_off_event_loop(method_name, folder_attr): + # exchangelib is synchronous, so resolving the distinguished folder must be + # offloaded via asyncio.to_thread instead of blocking the event loop. + async with create_outlook_source() as source: + account = MockAccount() + method = getattr(source.client, method_name) + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [item async for item in method(account)] + + assert any( + call.args[:3] == (getattr, account, folder_attr) + for call in to_thread.call_args_list + ) + + +@pytest.mark.asyncio +async def test_get_mails_resolve_folder_off_event_loop(): + async with create_outlook_source() as source: + account = MockAccount() + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [item async for item in source.client.get_mails(account)] + + # Named folders resolve via getattr, off the loop. + assert any( + call.args[:3] == (getattr, account, "inbox") + for call in to_thread.call_args_list + ) + # The Archive leaf resolves via a lambda, also off the loop. + archive_calls = [c for c in to_thread.call_args_list if len(c.args) == 1] + assert archive_calls + assert archive_calls[0].args[0]().object_type == MAIL + + +@pytest.mark.asyncio +async def test_get_child_calendars_resolve_folder_off_event_loop(): + async with create_outlook_source() as source: + account = MockAccount() + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [item async for item in source.client.get_child_calendars(account)] + + # The first offloaded call resolves the child-calendar list. + resolved = to_thread.call_args_list[0].args[0]() + assert resolved == list(account.calendar.children) + + +@pytest.mark.asyncio +async def test_get_contacts_queries_distribution_list_fields(): + # The Contacts folder query must include DistributionList fields (members), + # or contact groups come back with no email addresses. + async with create_outlook_source() as source: + account = MockAccount() + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [contact async for contact in source.client.get_contacts(account)] + + assert any("members" in call.args[1:] for call in to_thread.call_args_list) + + @pytest.mark.asyncio async def test_fetch_contacts_raises_on_unexpected_item_type(): # The Contacts folder should only yield Contact or DistributionList items;