Conversation
…hods
- sailthru_response.py: use response.text instead of response.content so
simplejson (which rejects bytes) receives a str; also catch TypeError in
addition to ValueError for robustness
- sailthru_client.py: remove redundant json.loads() in receive_verify_post
since get_body() already returns a parsed dict
- sailthru_client.py: fix get_signature_string() returning bytes instead of
str by moving .encode('utf-8') into get_signature_hash() where hashlib needs it
- test: update receive_verify_post mock to return a dict matching real get_body() behavior
- setup.py: bump version to 2.3.6
Closes INT-1681. Resolves the same issue identified in PR #63 (open since 2017).
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
Fixes Python 3 bytes/str incompatibilities that surface when simplejson is installed (as required), and aligns postback verification logic with the actual types returned by SailthruResponse.get_body().
Changes:
- Parse API responses via
response.textand broaden JSON parse error handling to includeTypeError. - Fix signature generation to keep
get_signature_string()asstrand move UTF-8 encoding to the MD5 hashing step. - Correct
receive_verify_post()to use the dict returned byget_body()(and update the related unit test mock), plus bump package version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
sailthru/sailthru_response.py |
Switch JSON parsing to use response.text and catch (ValueError, TypeError) to avoid bytes inputs with simplejson. |
sailthru/sailthru_client.py |
Make signature generation consistently str until hashing; simplify verify postback by using parsed response dict directly. |
test/test_sailthru_client.py |
Update receive_verify_post test double to return a dict from get_body() to match real behavior. |
setup.py |
Bump package version to 2.3.6. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_receive_verify_post(self): | ||
| mock_http_request = MagicMock() | ||
| mock_http_request.return_value.get_body.return_value = '{"email":"menglander@sailthru.com"}' | ||
| mock_http_request.return_value.get_body.return_value = {"email": "menglander@sailthru.com"} |
There was a problem hiding this comment.
This test now mocks get_body() as a dict (matching SailthruResponse.get_body(as_dictionary=True)), but other tests in this file still mock get_body() as a JSON string. Updating those mocks to return dicts as well would prevent tests from accidentally passing due to string-substring checks and better reflect real behavior.
There was a problem hiding this comment.
This does seem like an unrelated change.
There was a problem hiding this comment.
keeping this change as it is directly required by the code fix. The old test returned a JSON string from get_body() because the old code called json.loads() on it. Since we removed that redundant json.loads() call (the real get_body() already returns a parsed dict), the mock must also return a dict — otherwise the test would be testing behaviour that no longer exists in production code.
There was a problem hiding this comment.
Shouldn't all other tests also be updated then? i.e. test_hardbounce_valid_json etc
There was a problem hiding this comment.
Good catch — updated in cc949ff. The only other test using a JSON string mock was test_hardbounce_valid_json. The other two hardbounce tests (test_hardbounce_invalid_json and test_hardbounce_blast_invalid_json) already return None which is appropriate for their error-case scenarios, so those are fine as-is.
- receive_verify_post: use isinstance(send_json, dict) guard and .get() instead of direct key access, as suggested by reviewers - sailthru/__init__.py: sync __version__ to 2.3.6 (was stale at 2.3.1) - sailthru/sailthru_http.py: update hardcoded version in User-Agent to 2.3.6 - CHANGES: add 2.3.6 changelog entry
…et_body() behaviour
Summary
Fixes INT-1681. Resolves the same bytes/str issue originally identified in PR #63 (open since 2017).
Root cause:
simplejson(a required dependency) does not acceptbytesin Python 3, unlike stdlibjsonwhich does. The code was passingresponse.content(bytes) directly tojson.loads, causing:This error was silently masked for users running without simplejson installed (stdlib json handles bytes fine since Python 3.6), but surfaces when simplejson is present — which it always is since it's in
install_requires.Changes
sailthru_response.py: Useresponse.text(str) instead ofresponse.content(bytes) when callingjson.loads. Also broadened the exception handler to catchTypeErrorin addition toValueErrorfor robustness.sailthru_client.py—receive_verify_post: Removed redundantjson.loads()call.get_body()already returns a parsed dict — callingjson.loadson a dict fails. Now uses the dict directly.sailthru_client.py—get_signature_string: Was returningbytesdue to.encode('utf-8'). Moved the encoding intoget_signature_hashwherehashlib.md5actually needs bytes.get_signature_stringnow correctly returns astr.test/test_sailthru_client.py: Updatedreceive_verify_postmock to return adict(matching realget_body()behaviour) instead of a JSON string.setup.py: Bumped version to2.3.6.