-
-
Notifications
You must be signed in to change notification settings - Fork 14
fix(webhooks): send payload as JSON object instead of double-encoded string - #623
Open
MateoLostanlen wants to merge 5 commits into
Open
fix(webhooks): send payload as JSON object instead of double-encoded string #623MateoLostanlen wants to merge 5 commits into
MateoLostanlen wants to merge 5 commits into
Conversation
...string model_dump_json() returns a str, which httpx json= encodes a second time, so consumers received a quoted JSON string instead of an object (Slack rejects it with 400 invalid_payload). Use model_dump(mode="json") and let httpx do the encoding. Also log connection/timeout errors instead of letting them bubble up to callers.
Codecov Report
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.68%. Comparing base (3cf4d52) to head (d84fa4f).
Additional details and impacted files
@@ Coverage Diff @@ ## main #623 +/- ## ========================================== + Coverage 90.97% 93.68% +2.71% ========================================== Files 3 59 +56 Lines 133 3074 +2941 ========================================== + Hits 121 2880 +2759 - Misses 12 194 +182
☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
Bug
dispatch_webhookpassedmodel_dump_json()(astr) to httpx'sjson=, which JSON-encoded it a second time. Every generic webhook consumer received a double-encoded body: a quoted JSON string like"{\"id\": 123, ...}"instead of the object{"id": 123, ...}, so any parser got astrinstead of adict. Present since the mechanism was introduced (#367).Why Slack notifications were never affected
The Slack messages we receive go through the dedicated
organization.slack_hookpath (slack_client.notify, #478), which formats a proper Slack message. The recurring400 invalid_payloaderrors in the prod logs came from the same Slack URL being also registered in the genericwebhooktable, a leftover from before #478 that never worked and only produced log noise. That row has been removed from the prod table.Why fix the payload anyway
Removing the bad row only stops the log noise: the generic webhook mechanism itself stays broken for its intended use. Any legitimate consumer registered in the
webhooktable (partner system, internal service expecting the detection JSON) would still receive a double-encoded string. This fixes the mechanism for the next consumer.Changes
model_dump(mode="json")and let httpx do the encoding.httpx.RequestError(timeouts, connection failures) instead of letting them bubble up, consistent with best-effort dispatch.