fix(outlook): dispatch Contacts by item type and harden folder/field assumptions by Jan-Kazlouski-elastic · Pull Request #4147 · elastic/connectors

Jan-Kazlouski-elastic · 2026-07-09T08:40:08Z

Related to https://github.com/elastic/sdh-search/issues/1898

Follow-up to #4123. The last month has been a series of "the next malformed item aborts the whole sync" reports, most recently 'DistributionList' object has no attribute 'email_addresses'. This PR fixes that class of failure with explicit, visible handling, and audits the connector for the remaining assumptions of the same shape.

Root cause

The Contacts folder returns two item models — Contact and DistributionList — but the formatter assumed every item was a Contact and read per-contact fields off it, so a single contact group aborted the entire sync. The same "unhandled shape aborts everything" gap existed elsewhere: Calendar/Tasks folders that shared and resource mailboxes can legitimately lack, birthday events without a start, and an optional LDAP type key.

Changes

Contacts formatted by item type. _fetch_contacts now dispatches on type: a Contact goes through the strict contact_doc_formatter and a contact group through a dedicated distribution_list_doc_formatter that indexes it by name and its members' email addresses (typed "Distribution List"). The folder query fetches the union of both models' fields (incl. members). The Contacts folder contractually returns only these two models, so any other type raises a TypeError rather than being silently skipped — an unknown type is a broken assumption we want surfaced, not a whole category of items quietly dropped. (Thanks @artem-shelkovnikov for the review — explicit type handling over defensive getattr, and failing on unexpected types.)

Note on approach. An earlier revision of this PR wrapped all formatting in a broad _format_item guard that caught item-shape errors (AttributeError/KeyError/ValueError/TypeError) and skipped the item. That was dropped: swallowing those errors around every item can turn a systematic bug into a "successful" sync that returns 0 items and deletes previously indexed documents. Instead, known shapes are handled explicitly (type dispatch + targeted field guards) and genuine errors still fail the sync loudly — consistent with the principle established in #4085/#4123.

Remaining assumptions found in the audit, now fixed:

Calendar & Tasks folders are now skipped on ErrorFolderNotFound, mirroring the existing Contacts/mail-folder handling. Shared and resource mailboxes can legitimately lack these folders; previously that aborted the sync (same class as Fix Outlook connector crash on localized Exchange servers (contacts folder) #4065).
Birthdays calendar branch called .split("T") on the formatted datetime, which is None when calendar.start is missing → AttributeError. Now guarded.
Optional LDAP type key is read with .get(...) instead of user["type"].

Test plan

pytest tests/sources/test_outlook.py (65 tests), incl. coverage for:
- contact / DistributionList type dispatch and the group formatter
- unexpected Contacts item type → raises TypeError
- ErrorFolderNotFound skips for Calendar / child calendars / Tasks
- birthday without a start
- missing LDAP type key
ruff check / ruff format --check

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 <cursoragent@cursor.com>

artem-shelkovnikov · 2026-07-09T09:03:39Z

+        # 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.


Technically this change says different:

If an object returned by contacts does not have some attributes, skip them.

I'm wondering if there are other types of objects that can come here, and will these objects also have display_name?

Also I'm wondering if we should instead check the object type and return formatted documents based on type of incoming object - it will make it easier to comprehend why some objects do not have all fields and which fields are supported per object.

Good call, agreed on both points — I've reworked this to dispatch on item type instead.

To your questions: a Contacts-folder query in exchangelib returns exactly two item models — Contacts.supported_item_models == (Contact, DistributionList) — and both inherit display_name (plus id/last_modified_time) from the base Item, so the shared fields are always safe. DistributionList just doesn't have the per-contact fields (email_addresses, phone_numbers, company_name, birthday); instead it has members.

Changes in a9fb212:

Reverted contact_doc_formatter to strict Contact access (no more getattr), so it documents the real Contact schema.

Added a dedicated distribution_list_doc_formatter that indexes the group by name and its members' email addresses, typed as "Distribution List".

Dispatch by isinstance in _fetch_contacts, and the query now fetches the union of both models' fields (incl. members).

The general per-item _format_item guard stays as defense-in-depth for genuinely unexpected shapes, but the contact path is now explicit about which fields belong to which type.

Update:

Removed _format_item entirely rather than keeping it as defense-in-depth.

…ield 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 <cursoragent@cursor.com>

…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 <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

artem-shelkovnikov · 2026-07-09T10:28:46Z

-        ):
+        # Skip instead of aborting if the mailbox has no Calendar folder.
+        try:
+            folder = account.calendar


Sanity check - this does not produce network call, folder.all() does, right?

Hmm, actually it's the other way around:
account.calendar makes the network call. It is the same as self.root.get_default_folder(Calendar), which calls GetFolder and raises ErrorFolderNotFound.

folder.all() on the other hand is a lazy QuerySet. No network call until it gets iterated, and no ErrorFolderNotFound. So we should be good here.

Okay, then there is a problem here. exchangelib is not async, and the call to account.calendar is gonna block the whole process until it resolves. Then we'd need to wrap it into asyncio.to_thread or something similar

This then will have to be done to the methods that do network calls below unfortunately

Yep. You're right. Done.

artem-shelkovnikov · 2026-07-09T10:31:11Z

+                    distribution_list=contact,
+                    timezone=timezone,
+                )
+            else:


I personally like doing explicit type checks and warn/raise on the case when the type is unexpected.

artem-shelkovnikov · 2026-07-09T10:34:26Z

        """
        yield content

+    def _format_item(self, formatter, item, account):


This actually has tendency to actually produce syncs that are successful but return 0 items. I'm not really sure we should do this.

We've tried an approach with this class:

connectors/app/connectors_service/connectors/utils.py

Line 827 in 0b1727c

class ErrorMonitor:

Error Monitor swallows errors up to a certain threshold and then raises an exception if too many errors happen. It also has its downsides.

Continuing the train of thought from above:

I feel this method attempt to help not raise when formatting objects of unexpected types. E.g. you have an API returning types contact and distribution_list and you format them well, but then for some niche case the customer gets an error because an object of type broadcast_group (imaginary name) is returned and the method breaks.

So instead each formatter method can do type checks and warn or error if type is unexpected.

Warn or error? Not sure. It's all about expectation setting to me.

Warns are good because the sync does not crash
Errors are good because they tell what went wrong immediately, so you will know that something is not being synced.

Because of that Error Monitor will not actually help, but will just hide the problem.

All in all I think it's best to do type checks in formatter methods and warn on unexpected types. I am open to a discussion here, though

Thanks. Fixed. Now we're raising instead of silently skipping.

…e 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 <cursoragent@cursor.com>

…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 <cursoragent@cursor.com>

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 <cursoragent@cursor.com>

Jan-Kazlouski-elastic requested a review from a team as a code owner July 9, 2026 08:40

github-actions Bot added auto-backport v9.5.0 labels Jul 9, 2026

Jan-Kazlouski-elastic added bug Something isn't working team:extract-and-transform v9.3.8 v9.4.5 labels Jul 9, 2026

artem-shelkovnikov reviewed Jul 9, 2026

View reviewed changes

Jan-Kazlouski-elastic changed the title ~~fix(outlook): handle contact groups (DistributionList) in the Contacts folder~~ fix(outlook): isolate per-item failures so one malformed Exchange item can't abort the sync Jul 9, 2026

Jan-Kazlouski-elastic and others added 3 commits July 9, 2026 12:33

Merge branch 'main' into jan-kazlouski/outlook-fix

c976747

docs(outlook): tighten inline comments

0e99183

Co-authored-by: Cursor <cursoragent@cursor.com>

artem-shelkovnikov reviewed Jul 9, 2026

View reviewed changes

Jan-Kazlouski-elastic changed the title ~~fix(outlook): isolate per-item failures so one malformed Exchange item can't abort the sync~~ fix(outlook): dispatch Contacts by item type and harden folder/field assumptions Jul 9, 2026

Jan-Kazlouski-elastic requested a review from artem-shelkovnikov July 9, 2026 12:43

Jan-Kazlouski-elastic and others added 2 commits July 9, 2026 15:54

Jan-Kazlouski-elastic self-assigned this Jul 9, 2026

Jan-Kazlouski-elastic added the v9.6.0 label Jul 9, 2026

Uh oh!

Conversation

Jan-Kazlouski-elastic commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Changes

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-shelkovnikov Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Jan-Kazlouski-elastic commented Jul 9, 2026 •

edited

Loading

artem-shelkovnikov Jul 9, 2026 •

edited

Loading