Skip to content

fix(provider-microsoft): cast contentId on polymorphic attachment $select (fixes #59)#60

Merged
stevenobiajulu merged 2 commits intomainfrom
fix/graph-attachment-select-cast
Apr 28, 2026
Merged

fix(provider-microsoft): cast contentId on polymorphic attachment $select (fixes #59)#60
stevenobiajulu merged 2 commits intomainfrom
fix/graph-attachment-select-cast

Conversation

@stevenobiajulu
Copy link
Copy Markdown
Member

Summary

Fix the regression introduced by #56getMessage() throws HTTP 400 on every Outlook message because contentId is rejected by $select against the polymorphic microsoft.graph.attachment base. Cast the field to microsoft.graph.fileAttachment/contentId and rebuild the broken tests on a tiny schema-validating mock.

Fixes #59.

What changed

Source (packages/provider-microsoft/src/email-graph-provider.ts)

  • One-line fix: ATTACHMENT_SELECT now uses microsoft.graph.fileAttachment/contentId. No other source edits — the existing 400 fallback stays as defensive infra for unrelated 400s and now uses a valid select string.
  • Doc-link comment block above the constant pointing at the Graph attachment and fileAttachment resource docs.

Tests (packages/provider-microsoft/src/email-graph-provider.test.ts)

  • Added an inlined createSchemaValidatingClient helper that validates $select fragments against an explicit allow-list (base attachment props + approved type casts) and throws a Graph-realistic GraphApiError(400) for invalid fields. Narrow allow-list (not a regex) so typos like fileAttachment/notARealProp fail fast.
  • Converted the three getMessage/getThread tests that hard-coded the broken URL (lines 99, 159/164, and 793 on main) to use the validating client. PR fix(provider-microsoft): populate attachments on getMessage (fixes #55) #56's bare-contentId string would now fail at unit-test time.
  • grep -n 'isInline,contentId' email-graph-provider.test.ts returns zero hits.

Why this would have caught #56

PR #56's tests asserted the literal broken URL with vi.fn().mockResolvedValue(...). The mock returned whatever object the test handed it — no OData validation — so the wrong-string-as-spec passed. The new validating client checks every URL passed to .get() against the allow-list before delegating to the response queue, so the same regression now throws an unhandled GraphApiError 400 in unit tests.

Test plan

  • npm run build
  • npm run lint --workspaces --if-present
  • npm run check:spec-coverage (186/186 scenarios)
  • npm run test:run (538 tests pass across 4 workspaces)
  • Live Graph smokeGET /me/messages/{id}?$expand=attachments($select=…microsoft.graph.fileAttachment/contentId) returns HTTP 200 with contentId populated.
  • Built-provider end-to-end — imported dist/email-graph-provider.js, ran getMessage() against a real Outlook message with attachments, confirmed attachments[0].contentId is a non-empty string.

Out of scope (follow-up)

  • A CI-runnable contract test that sweeps every $select constructed by GraphEmailProvider through an extended schema validator.
  • A live-Graph integration job in CI.

Refs

…lect (#59)

Microsoft Graph rejects `contentId` in `$select` against the polymorphic
`microsoft.graph.attachment` base type — the property is declared on
`microsoft.graph.fileAttachment`. PR #56 shipped the bare field, so every
Outlook `getMessage()` call (read_email, list_attachments, get_thread,
reply flows) throws HTTP 400.

Source: cast the field — `microsoft.graph.fileAttachment/contentId`.
Verified live against real Graph (HTTP 200, contentId populated).

Tests: add a tiny inlined schema-validating mock with explicit allow-lists
for base attachment props and approved type casts. Convert the three
`getMessage`/`getThread` tests that hard-coded the broken URL to use it.
PR #56's broken string would now fail unit tests instead of passing.

Doc-link comments added to the source constant and the test allow-lists
so future readers can verify the schema against the canonical Graph docs.

Fixes #59.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

Codex peer review on #60 surfaced two optional improvements; both applied:

- Drop `microsoft.graph.fileAttachment/contentLocation` from
  ALLOWED_ATTACHMENT_CASTS. The provider never selects it and Graph
  documents it as unsupported on fileAttachment, so pre-whitelisting
  weakened the "future casts must be deliberate" guard.

- Pin Graph Learn URLs with ?view=graph-rest-1.0 so the doc-link
  comments stop redirecting (and so the version lined up against the
  schema is explicit).
@stevenobiajulu stevenobiajulu enabled auto-merge (squash) April 28, 2026 14:56
@stevenobiajulu stevenobiajulu merged commit 816df6c into main Apr 28, 2026
13 checks passed
@stevenobiajulu stevenobiajulu deleted the fix/graph-attachment-select-cast branch April 28, 2026 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Microsoft Graph 'contentId' $select rejected by polymorphic base type — getMessage() throws on every Outlook message (regression from #56)

1 participant