fix: fall back when payload compression fails#700
Conversation
|
Reviews (1): Last reviewed commit: "fix: fall back when payload compression ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| except OSError as exc: | ||
| log.warning("failed to gzip request body, sending uncompressed: %s", exc) |
There was a problem hiding this comment.
except OSError is narrower than needed. GzipFile.write() delegates to zlib.Compress.compress(), which raises zlib.error — a plain Exception subclass, not an OSError — if the zlib library itself fails. That exception would bypass this handler, propagate uncaught, and drop the batch instead of falling back to uncompressed. Broadening the catch to Exception covers all realistic in-process compression failures without swallowing truly unexpected errors like MemoryError.
| except OSError as exc: | |
| log.warning("failed to gzip request body, sending uncompressed: %s", exc) | |
| except Exception as exc: | |
| log.warning("failed to gzip request body, sending uncompressed: %s", exc) |
| def test_post_falls_back_to_uncompressed_payload_when_gzip_fails(self): | ||
| mock_response = requests.Response() | ||
| mock_response.status_code = 200 | ||
| mock_session = mock.MagicMock() | ||
| mock_session.post.return_value = mock_response | ||
|
|
||
| with mock.patch.object(request_module, "GzipFile", side_effect=OSError("boom")): | ||
| request_module.post( | ||
| TEST_API_KEY, | ||
| host="https://test.posthog.com", | ||
| path="/batch/", | ||
| gzip=True, | ||
| session=mock_session, | ||
| batch=[], | ||
| ) | ||
|
|
||
| data = mock_session.post.call_args.kwargs["data"] | ||
| headers = mock_session.post.call_args.kwargs["headers"] | ||
| self.assertIsInstance(data, str) | ||
| self.assertNotIn("Content-Encoding", headers) |
There was a problem hiding this comment.
The team's style guide says "We always prefer parameterised tests." This test exercises a single hard-coded OSError scenario, but if the catch is widened (e.g. to Exception) there are now multiple distinct exception types worth covering (e.g. OSError, zlib.error). Converting to @pytest.mark.parametrize (or subTest) with multiple exception types would both express the intent more clearly and guard against future regressions on each path.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
posthog-python Compliance ReportDate: 2026-06-27 13:32:59 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
1 participant
💡 Motivation and Context
When
gzip=True, local gzip failures should not prevent the SDK from sending the batch. The SDK should fall back to the original uncompressed JSON payload without aContent-Encodingheader.This catches local gzip errors during request construction and keeps the upload path uncompressed. A unit test covers the fallback.
💚 How did you test it?
uv run --extra test pytest posthog/test/test_request.py -q📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented as part of a cross-SDK consistency pass for client-side compression failure fallback. Kept the change scoped to request construction and added a focused regression test.