Fix #829 by adding the default encoding by seratch · Pull Request #833 · slackapi/python-slack-sdk

seratch · 2020-10-06T02:25:58Z

Summary

This pull request fixes #829 by adding default values for the charset that is supposed to be used for parsing response body from Slack. The situation may arise either when the content-type header is missing or content-type header doesn't contain charset information in it.

We can safely assume "utf-8" is the one it can fallback in the case where the server-side cannot return a valid response.

Category (place an `x` in each of the `[ ]`)

slack.web.WebClient (Web API client)
slack.webhook.WebhookClient (Incoming Webhook, response_url sender)
slack.web.classes (UI component builders)
slack.rtm.RTMClient (RTM client)
Documents
Others

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run python setup.py validate after making the changes.

codecov · 2020-10-06T02:35:12Z

Codecov Report

Merging #833 into main will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #833      +/-   ##
==========================================
- Coverage   87.29%   87.23%   -0.06%     
==========================================
  Files          30       30              
  Lines        3761     3761              
  Branches      337      337              
==========================================
- Hits         3283     3281       -2     
- Misses        329      331       +2     
  Partials      149      149

Impacted Files	Coverage Δ
slack/web/base_client.py	`80.35% <100.00%> (ø)`
slack/webhook/client.py	`88.70% <100.00%> (ø)`
slack/web/async_internal_utils.py	`79.77% <0.00%> (-2.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3053d6...9a50442. Read the comment docs.

seratch

A few comments just for clarifying the intention

seratch · 2020-10-06T02:34:10Z

tests/web/mock_web_api_server.py

                if pattern == "html_response":
                    self.send_response(404)
-                    self.set_common_headers()
+                    self.send_header("content-type", "text/html;charset=utf-8")


I've changed this content-type that had been unintentionally application/json; charset=utf-8 from the beginning.

seratch · 2020-10-06T02:34:44Z

tests/web/test_async_web_client.py

        except err.SlackApiError as e:
-            self.assertTrue(
-                str(e).startswith("Failed to parse the response body: Expecting value: line 1 column 1 (char 0)"), e)
+            self.assertEqual(


This is the expected behavior for async clients in this case.

seratch · 2020-10-06T02:34:53Z

tests/web/test_web_client.py

        except err.SlackApiError as e:
-            self.assertTrue(
-                str(e).startswith("Failed to parse the response body: Expecting value: line 1 column 1 (char 0)"), e)
+            self.assertEqual(


same as above

seratch · 2020-10-07T05:27:13Z

To reviewers, please let me know if you have any comments on this. I will be merging this within a few days.

mwbrooks

Very nice @seratch!

mwbrooks · 2020-10-08T00:17:16Z

tests/web/mock_web_api_server.py


    html_response_body = '<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">\n<html><head>\n<title>404 Not Found</title>\n</head><body>\n<h1>Not Found</h1>\n<p>The requested URL /api/team.info was not found on this server.</p>\n</body></html>\n'

+    error_html_response_body = '<!DOCTYPE html>\n<html lang="en">\n<head>\n\t<meta charset="utf-8">\n\t<title>Server Error | Slack</title>\n\t<meta name="author" content="Slack">\n\t<style></style>\n</head>\n<body>\n\t<nav class="top persistent">\n\t\t<a href="https://status.slack.com/" class="logo" data-qa="logo"></a>\n\t</nav>\n\t<div id="page">\n\t\t<div id="page_contents">\n\t\t\t<h1>\n\t\t\t\t<svg width="30px" height="27px" viewBox="0 0 60 54" class="warning_icon"><path d="" fill="#D94827"/></svg>\n\t\t\t\tServer Error\n\t\t\t</h1>\n\t\t\t<div class="card">\n\t\t\t\t<p>It seems like there’s a problem connecting to our servers, and we’re investigating the issue.</p>\n\t\t\t\t<p>Please <a href="https://status.slack.com/">check our Status page for updates</a>.</p>\n\t\t\t</div>\n\t\t</div>\n\t</div>\n\t<script type="text/javascript">\n\t\tif (window.desktop) {\n\t\t\tdocument.documentElement.className = \'desktop\';\n\t\t}\n\n\t\tvar FIVE_MINS = 5 * 60 * 1000;\n\t\tvar TEN_MINS = 10 * 60 * 1000;\n\n\t\tfunction randomBetween(min, max) {\n\t\t\treturn Math.floor(Math.random() * (max - (min + 1))) + min;\n\t\t}\n\n\t\twindow.setTimeout(function () {\n\t\t\twindow.location.reload(true);\n\t\t}, randomBetween(FIVE_MINS, TEN_MINS));\n\t</script>\n</body>\n</html>'


Nice, I was trying to find the HTML response from the server! Super happy to see you have a copy.

The original one is a bit long so that I removed the CSS in <style> tag from it 😺

mwbrooks · 2020-10-08T00:18:06Z

tests/web/mock_web_api_server.py

                    return

+                if pattern == "error_html_response":
+                    self.send_response(503)


Nice, this was also the HTTP status code of the error that we experienced earlier this week. 👌🏻

adamchainz

Great, thanks for the fast fix

adamchainz · 2020-10-08T11:21:56Z

slack/web/base_client.py

                        req, context=self.ssl, timeout=self.timeout
                    )
-                charset = resp.headers.get_content_charset()
+                charset = resp.headers.get_content_charset() or "utf-8"


Why not add the type annotation?

charset: str = resp.headers.get_content_charset() or "utf-8"

Good point. I'll update it later for consistency.

seratch added 2 commits October 6, 2020 11:19

Fix #829 by adding the default encoding

3d7fc0f

Update the mock server

b7952d1

seratch added Version: 2x bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client labels Oct 6, 2020

seratch added this to the 2.9.2 milestone Oct 6, 2020

seratch requested review from aoberoi, misscoded, mwbrooks and stevengill October 6, 2020 02:25

seratch self-assigned this Oct 6, 2020

Fix tests

9a50442

seratch commented Oct 6, 2020

View reviewed changes

seratch mentioned this pull request Oct 6, 2020

Pick #833 for v3 branch #834

Merged

9 tasks

seratch added a commit that referenced this pull request Oct 6, 2020

Pick #833 for v3 branch

5317c81

mwbrooks approved these changes Oct 8, 2020

View reviewed changes

seratch merged commit 99ad72d into slackapi:main Oct 8, 2020

seratch deleted the issue-829 branch October 8, 2020 00:32

adamchainz reviewed Oct 8, 2020

View reviewed changes


		html_response_body = '<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">\n<html><head>\n<title>404 Not Found</title>\n</head><body>\n<h1>Not Found</h1>\n<p>The requested URL /api/team.info was not found on this server.</p>\n</body></html>\n'

		error_html_response_body = '<!DOCTYPE html>\n<html lang="en">\n<head>\n\t<meta charset="utf-8">\n\t<title>Server Error \| Slack</title>\n\t<meta name="author" content="Slack">\n\t<style></style>\n</head>\n<body>\n\t<nav class="top persistent">\n\t\t<a href="https://status.slack.com/" class="logo" data-qa="logo"></a>\n\t</nav>\n\t<div id="page">\n\t\t<div id="page_contents">\n\t\t\t<h1>\n\t\t\t\t<svg width="30px" height="27px" viewBox="0 0 60 54" class="warning_icon"><path d="" fill="#D94827"/></svg>\n\t\t\t\tServer Error\n\t\t\t</h1>\n\t\t\t<div class="card">\n\t\t\t\t<p>It seems like there’s a problem connecting to our servers, and we’re investigating the issue.</p>\n\t\t\t\t<p>Please <a href="https://status.slack.com/">check our Status page for updates</a>.</p>\n\t\t\t</div>\n\t\t</div>\n\t</div>\n\t<script type="text/javascript">\n\t\tif (window.desktop) {\n\t\t\tdocument.documentElement.className = \'desktop\';\n\t\t}\n\n\t\tvar FIVE_MINS = 5 * 60 * 1000;\n\t\tvar TEN_MINS = 10 * 60 * 1000;\n\n\t\tfunction randomBetween(min, max) {\n\t\t\treturn Math.floor(Math.random() * (max - (min + 1))) + min;\n\t\t}\n\n\t\twindow.setTimeout(function () {\n\t\t\twindow.location.reload(true);\n\t\t}, randomBetween(FIVE_MINS, TEN_MINS));\n\t</script>\n</body>\n</html>'

Conversation

seratch commented Oct 6, 2020

Summary

Category (place an x in each of the [ ])

Requirements (place an x in each [ ])

Uh oh!

codecov bot commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch commented Oct 7, 2020

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamchainz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

codecov bot commented Oct 6, 2020 •

edited

Loading