Teams webhook message too large exception#2164
Conversation
…teams, either in the status code or inside the response text
|
👋 @GuyEshdat |
📝 WalkthroughWalkthroughRemoved Teams adaptive-card payload truncation and helper functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
elementary/messages/messaging_integrations/teams_webhook.py (1)
101-103: Consider documenting the string-based error detection.The substring check for
"HTTP error 413"is a pragmatic workaround for the old Teams webhook behavior (as noted in the comment above). A brief inline comment explaining this would help future maintainers understand why string matching is used instead of checkingresponse.status_code.Suggested comment
if response.status_code not in (HTTPStatus.OK, HTTPStatus.ACCEPTED) or ( response.status_code == HTTPStatus.OK and len(response.text) > 1 ): + # Old Teams webhook returns HTTP 200 with error message in body + # instead of proper HTTP status codes if "HTTP error 413" in response.text: raise TeamsWebhookPayloadTooLargeError(response)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/teams_webhook.py` around lines 101 - 103, Add a brief inline comment above the string check that explains why the code inspects response.text for "HTTP error 413" rather than using response.status_code: note that this is a pragmatic workaround for old Microsoft Teams webhook responses that embed the 413 error in the body instead of returning the proper status code, so the substring match is intentional to detect payload-too-large and raise TeamsWebhookPayloadTooLargeError; place this comment next to the existing check that examines response.text and references TeamsWebhookPayloadTooLargeError for clarity to future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/messages/messaging_integrations/teams_webhook.py`:
- Around line 43-45: The exception message currently uses len(response.text)
(server response size) which is wrong; update the exception constructor (the
class __init__ where super().__init__ is called) to accept a payload_size
parameter (or use an existing request_payload variable) and build the message
with that payload_size (e.g. f"Teams webhook payload size ({payload_size} bytes)
exceeds limit") or omit the size if the caller cannot provide it; then update
call sites that raise this exception to pass the actual request payload size.
---
Nitpick comments:
In `@elementary/messages/messaging_integrations/teams_webhook.py`:
- Around line 101-103: Add a brief inline comment above the string check that
explains why the code inspects response.text for "HTTP error 413" rather than
using response.status_code: note that this is a pragmatic workaround for old
Microsoft Teams webhook responses that embed the 413 error in the body instead
of returning the proper status code, so the substring match is intentional to
detect payload-too-large and raise TeamsWebhookPayloadTooLargeError; place this
comment next to the existing check that examines response.text and references
TeamsWebhookPayloadTooLargeError for clarity to future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0290272b-09fa-412e-b815-cf0557516a24
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/teams_webhook.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elementary/messages/messaging_integrations/teams_webhook.py (1)
48-65:⚠️ Potential issue | 🟠 MajorHandle direct HTTP 413 responses in the
HTTPErrorexception path.
send_adaptive_card()callsresponse.raise_for_status()at line 65, so an actual HTTP 413 response is raised asrequests.HTTPErrorand caught at line 115, where it is re-raised asTeamsWebhookHttpErrorinstead of the new typedTeamsWebhookPayloadTooLargeError. The new exception only fires on the legacy path (lines 102–105) when a 200 status with "HTTP error 413" in the response body is received.Add 413 handling to the
HTTPErrorexception block:Suggested fix
except requests.HTTPError as e: + if ( + e.response is not None + and e.response.status_code == HTTPStatus.REQUEST_ENTITY_TOO_LARGE + ): + raise TeamsWebhookPayloadTooLargeError( + e.response, payload_size=len(payload_json) + ) from e raise TeamsWebhookHttpError(e.response) from eNote: The payload size reported (line 104) also excludes the outer Teams message envelope, so it undercounts the actual request body size.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/teams_webhook.py` around lines 48 - 65, send_adaptive_card currently calls response.raise_for_status() so a real HTTP 413 surfaces as requests.HTTPError but is re-raised as TeamsWebhookHttpError; update the except requests.HTTPError handler (the block that currently raises TeamsWebhookHttpError) to detect e.response and if e.response.status_code == 413 raise TeamsWebhookPayloadTooLargeError instead (include the reported payload size and note that you must compute the full request body size including the outer Teams message envelope when populating the size field). Ensure other HTTP errors still raise TeamsWebhookHttpError and preserve the original error details.
♻️ Duplicate comments (1)
elementary/messages/messaging_integrations/teams_webhook.py (1)
48-58:⚠️ Potential issue | 🟡 MinorReport the size of the full webhook payload, not just the card body.
payload_jsononly serializescard, butsend_adaptive_card()wraps it in the Teams message envelope before posting. The byte count inTeamsWebhookPayloadTooLargeErrorwill therefore be lower than the actual request size. Either compute the size from the final payload or omit the byte count.Also applies to: 90-90, 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/teams_webhook.py` around lines 48 - 58, The code currently measures size using only the card JSON, causing TeamsWebhookPayloadTooLargeError to report a smaller byte count than the real request; in send_adaptive_card (and the other webhook-sending helpers that raise TeamsWebhookPayloadTooLargeError) serialize the full message envelope (the payload dict that includes "type" and "attachments") to bytes (e.g., json.dumps(payload).encode("utf-8")) and use len(...) for the byte count, then use that value in the exception or log; alternatively remove the byte count from TeamsWebhookPayloadTooLargeError and its raise sites—update references in send_adaptive_card and the other functions that build the final payload to compute/report the size from the final payload variable rather than from the card only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@elementary/messages/messaging_integrations/teams_webhook.py`:
- Around line 48-65: send_adaptive_card currently calls
response.raise_for_status() so a real HTTP 413 surfaces as requests.HTTPError
but is re-raised as TeamsWebhookHttpError; update the except requests.HTTPError
handler (the block that currently raises TeamsWebhookHttpError) to detect
e.response and if e.response.status_code == 413 raise
TeamsWebhookPayloadTooLargeError instead (include the reported payload size and
note that you must compute the full request body size including the outer Teams
message envelope when populating the size field). Ensure other HTTP errors still
raise TeamsWebhookHttpError and preserve the original error details.
---
Duplicate comments:
In `@elementary/messages/messaging_integrations/teams_webhook.py`:
- Around line 48-58: The code currently measures size using only the card JSON,
causing TeamsWebhookPayloadTooLargeError to report a smaller byte count than the
real request; in send_adaptive_card (and the other webhook-sending helpers that
raise TeamsWebhookPayloadTooLargeError) serialize the full message envelope (the
payload dict that includes "type" and "attachments") to bytes (e.g.,
json.dumps(payload).encode("utf-8")) and use len(...) for the byte count, then
use that value in the exception or log; alternatively remove the byte count from
TeamsWebhookPayloadTooLargeError and its raise sites—update references in
send_adaptive_card and the other functions that build the final payload to
compute/report the size from the final payload variable rather than from the
card only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 740de3f4-8ef9-4969-8531-7fae3f83e42a
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/teams_webhook.py
michael-myaskovsky
deleted the
teams-webhook-message-too-large-exception
branch
3 participants
null
Summary by CodeRabbit
New Features
Bug Fixes