From fa135eb53b278148fb411a2b1282c2fb28efad10 Mon Sep 17 00:00:00 2001 From: Jerrie-Aries Date: Tue, 7 Jun 2022 00:52:22 +0000 Subject: [PATCH 1/2] Improve and error handling for update and autoupdate features. --- bot.py | 65 ++++++++++++++++++--------------- cogs/utility.py | 38 +++++++++++++------- core/clients.py | 96 +++++++++++++++++++++++++++++++++++++------------ 3 files changed, 136 insertions(+), 63 deletions(-) diff --git a/bot.py b/bot.py index a506addfd9..5db1aab550 100644 --- a/bot.py +++ b/bot.py @@ -17,7 +17,7 @@ import discord import isodate -from aiohttp import ClientSession +from aiohttp import ClientSession, ClientResponseError from discord.ext import commands, tasks from discord.ext.commands.view import StringView from emoji import UNICODE_EMOJI @@ -630,6 +630,8 @@ async def on_ready(self): ) logger.warning("If the external servers are valid, you may ignore this message.") + self.post_metadata.start() + self.autoupdate.start() self._started = True async def convert_emoji(self, name: str) -> str: @@ -1581,6 +1583,7 @@ async def before_post_metadata(self): await self.wait_for_connected() if not self.config.get("data_collection") or not self.guild: self.post_metadata.cancel() + return logger.debug("Starting metadata loop.") logger.line("debug") @@ -1591,44 +1594,50 @@ async def autoupdate(self): latest = changelog.latest_version if self.version < parse_version(latest.version): - if self.hosting_method == HostingMethod.HEROKU: + try: + # update fork if gh_token exists data = await self.api.update_repository() + except InvalidConfigError: + data = {} + except ClientResponseError as exc: + logger.error(f"Autoupdate failed! Status {exc.status}.") + logger.error(f"Message: {exc.message}") + self.autoupdate.cancel() + return + if self.hosting_method == HostingMethod.HEROKU: + commit_data = data.get("data") + if not commit_data: + return - embed = discord.Embed(color=self.main_color) + logger.info("Bot has been updated.") + + if not self.config["update_notifications"]: + return - commit_data = data["data"] + embed = discord.Embed(color=self.main_color) + message = commit_data["commit"]["message"] + html_url = commit_data["html_url"] + short_sha = commit_data["sha"][:6] user = data["user"] + embed.add_field( + name="Merge Commit", + value=f"[`{short_sha}`]({html_url}) " f"{message} - {user['username']}", + ) embed.set_author( name=user["username"] + " - Updating Bot", icon_url=user["avatar_url"], url=user["url"], ) - embed.set_footer(text=f"Updating Modmail v{self.version} " f"-> v{latest.version}") + embed.set_footer(text=f"Updating Modmail v{self.version} -> v{latest.version}") embed.description = latest.description for name, value in latest.fields.items(): embed.add_field(name=name, value=value) - if commit_data: - message = commit_data["commit"]["message"] - html_url = commit_data["html_url"] - short_sha = commit_data["sha"][:6] - embed.add_field( - name="Merge Commit", - value=f"[`{short_sha}`]({html_url}) " f"{message} - {user['username']}", - ) - logger.info("Bot has been updated.") - channel = self.log_channel - if self.config["update_notifications"]: - await channel.send(embed=embed) + channel = self.update_channel + await channel.send(embed=embed) else: - try: - # update fork if gh_token exists - await self.api.update_repository() - except InvalidConfigError: - pass - command = "git pull" proc = await asyncio.create_subprocess_shell( command, @@ -1642,7 +1651,7 @@ async def autoupdate(self): if err and not res: logger.warning(f"Autoupdate failed: {err}") - self.autoupdate_loop.cancel() + self.autoupdate.cancel() return elif res != "Already up to date.": @@ -1659,7 +1668,7 @@ async def autoupdate(self): description="If you do not have an auto-restart setup, please manually start the bot.", color=self.main_color, ) - embed.set_footer(text=f"Updating Modmail v{self.version} " f"-> v{latest.version}") + embed.set_footer(text=f"Updating Modmail v{self.version} -> v{latest.version}") if self.config["update_notifications"]: await channel.send(embed=embed) return await self.close() @@ -1671,16 +1680,16 @@ async def before_autoupdate(self): if self.config.get("disable_autoupdates"): logger.warning("Autoupdates disabled.") - self.autoupdate_loop.cancel() + self.autoupdate.cancel() if self.hosting_method == HostingMethod.DOCKER: logger.warning("Autoupdates disabled as using Docker.") - self.autoupdate_loop.cancel() + self.autoupdate.cancel() if not self.config.get("github_token") and self.hosting_method == HostingMethod.HEROKU: logger.warning("GitHub access token not found.") logger.warning("Autoupdates disabled.") - self.autoupdate_loop.cancel() + self.autoupdate.cancel() def format_channel_name(self, author, exclude_channel=None, force_null=False): """Sanitises a username for use with text channel names diff --git a/cogs/utility.py b/cogs/utility.py index 0e86934af2..5d35a20211 100644 --- a/cogs/utility.py +++ b/cogs/utility.py @@ -1931,7 +1931,7 @@ async def github(self, ctx): async def update(self, ctx, *, flag: str = ""): """ Update Modmail. - To stay up-to-date with the latest commit rom GitHub, specify "force" as the flag. + To stay up-to-date with the latest commit from GitHub, specify "force" as the flag. """ changelog = await Changelog.from_url(self.bot) @@ -1939,7 +1939,7 @@ async def update(self, ctx, *, flag: str = ""): desc = ( f"The latest version is [`{self.bot.version}`]" - "(https://github.com/kyb3r/modmail/blob/master/bot.py#L25)" + "(https://github.com/kyb3r/modmail/blob/master/bot.py#L1)" ) if self.bot.version >= parse_version(latest.version) and flag.lower() != "force": @@ -1951,16 +1951,35 @@ async def update(self, ctx, *, flag: str = ""): embed.set_author(name=user["username"], icon_url=user["avatar_url"], url=user["url"]) await ctx.send(embed=embed) else: - if self.bot.hosting_method == HostingMethod.HEROKU: + try: + # update fork if gh_token exists data = await self.bot.api.update_repository() + except InvalidConfigError: + data = {} + except ClientResponseError as exc: + embed = discord.Embed( + title="Update failed", + description=f"Error status {exc.status}. {exc.message}", + color=self.bot.error_color, + ) + return await ctx.send(embed=embed) + + if self.bot.hosting_method == HostingMethod.HEROKU: + if not data: + # invalid gh_token + embed = discord.Embed( + title="Update failed", + description="Invalid Github token.", + color=self.bot.error_color, + ) + return await ctx.send(embed=embed) commit_data = data["data"] user = data["user"] - if commit_data and commit_data.get("html_url"): embed = discord.Embed(color=self.bot.main_color) - embed.set_footer(text=f"Updating Modmail v{self.bot.version} " f"-> v{latest.version}") + embed.set_footer(text=f"Updating Modmail v{self.bot.version} -> v{latest.version}") embed.set_author( name=user["username"] + " - Updating bot", @@ -1978,21 +1997,14 @@ async def update(self, ctx, *, flag: str = ""): else: embed = discord.Embed( title="Already up to date", - description="No further updates required", + description="No further updates required.", color=self.bot.main_color, ) embed.set_footer(text="Force update") embed.set_author(name=user["username"], icon_url=user["avatar_url"], url=user["url"]) await ctx.send(embed=embed) else: - # update fork if gh_token exists - try: - await self.bot.api.update_repository() - except InvalidConfigError: - pass - command = "git pull" - proc = await asyncio.create_subprocess_shell( command, stderr=PIPE, diff --git a/core/clients.py b/core/clients.py index a9722776f2..862c665310 100644 --- a/core/clients.py +++ b/core/clients.py @@ -1,7 +1,7 @@ import secrets import sys from json import JSONDecodeError -from typing import Union, Optional +from typing import Any, Dict, Union, Optional import discord from discord import Member, DMChannel, TextChannel, Message @@ -19,6 +19,7 @@ class GitHub: """ The client for interacting with GitHub API. + Parameters ---------- bot : Bot @@ -31,6 +32,7 @@ class GitHub: URL to the avatar in GitHub. url : str, optional URL to the GitHub profile. + Attributes ---------- bot : Bot @@ -43,6 +45,7 @@ class GitHub: URL to the avatar in GitHub. url : str URL to the GitHub profile. + Class Attributes ---------------- BASE : str @@ -77,7 +80,7 @@ def __init__(self, bot, access_token: str = "", username: str = "", **kwargs): self.headers = {"Authorization": "token " + str(access_token)} @property - def BRANCH(self): + def BRANCH(self) -> str: return "master" if not self.bot.version.is_prerelease else "development" async def request( @@ -85,11 +88,13 @@ async def request( url: str, method: str = "GET", payload: dict = None, - return_response: bool = False, headers: dict = None, - ) -> Union[ClientResponse, dict, str]: + return_response: bool = False, + read_before_return: bool = True, + ) -> Union[ClientResponse, Dict[str, Any], str]: """ Makes a HTTP request. + Parameters ---------- url : str @@ -98,16 +103,20 @@ async def request( The HTTP method (POST, GET, PUT, DELETE, FETCH, etc.). payload : Dict[str, Any] The json payload to be sent along the request. - return_response : bool - Whether the `ClientResponse` object should be returned. headers : Dict[str, str] Additional headers to `headers`. + return_response : bool + Whether the `ClientResponse` object should be returned. + read_before_return : bool + Whether to perform `.read()` method before returning the `ClientResponse` object. + Only valid if `return_response` is set to `True`. + Returns ------- ClientResponse or Dict[str, Any] or List[Any] or str `ClientResponse` if `return_response` is `True`. - `dict` if the returned data is a json object. - `list` if the returned data is a json list. + `Dict[str, Any]` if the returned data is a json object. + `List[Any]` if the returned data is a json list. `str` if the returned data is not a valid json data, the raw response. """ @@ -117,19 +126,24 @@ async def request( headers = self.headers async with self.session.request(method, url, headers=headers, json=payload) as resp: if return_response: + if read_before_return: + await resp.read() return resp + try: return await resp.json() except (JSONDecodeError, ClientResponseError): return await resp.text() - def filter_valid(self, data): + def filter_valid(self, data) -> Dict[str, Any]: """ Filters configuration keys that are accepted. + Parameters ---------- data : Dict[str, Any] The data that needs to be cleaned. + Returns ------- Dict[str, Any] @@ -138,42 +152,73 @@ def filter_valid(self, data): valid_keys = self.bot.config.valid_keys.difference(self.bot.config.protected_keys) return {k: v for k, v in data.items() if k in valid_keys} - async def update_repository(self, sha: str = None) -> Optional[dict]: + async def update_repository(self, sha: str = None) -> Dict[str, Any]: """ Update the repository from Modmail main repo. + Parameters ---------- - sha : Optional[str], optional - The commit SHA to update the repository. + sha : Optional[str] + The commit SHA to update the repository. If `None`, the latest + commit SHA will be fetched. + Returns ------- - Optional[dict] - If the response is a dict. + Dict[str, Any] + A dictionary that contains response data. """ if not self.username: raise commands.CommandInvokeError("Username not found.") if sha is None: - resp: dict = await self.request(self.REPO + "/git/refs/heads/" + self.BRANCH) + resp = await self.request(self.REPO + "/git/refs/heads/" + self.BRANCH) sha = resp["object"]["sha"] payload = {"base": self.BRANCH, "head": sha, "commit_message": "Updating bot"} merge_url = self.MERGE_URL.format(username=self.username) - resp = await self.request(merge_url, method="POST", payload=payload) - if isinstance(resp, dict): - return resp + resp = await self.request( + merge_url, + method="POST", + payload=payload, + return_response=True, + read_before_return=True, + ) + + repo_url = self.BASE + f"/repos/{self.username}/modmail" + status_map = { + 201: "Successful response.", + 204: "Already merged.", + 403: "Forbidden.", + 404: f"Repository '{repo_url}' not found.", + 409: "There is a merge conflict.", + 422: "Validation failed.", + } + # source https://docs.github.com/en/rest/branches/branches#merge-a-branch + + status = resp.status + if status in (201, 204): + try: + return await resp.json() + except (JSONDecodeError, ClientResponseError): + return await resp.text() + + args = (resp.request_info, resp.history) + kwargs = {"status": status, "message": status_map.get(status)} + # just raise + raise ClientResponseError(*args, **kwargs) async def fork_repository(self) -> None: """ Forks Modmail's repository. """ - await self.request(self.FORK_URL, method="POST") + await self.request(self.FORK_URL, method="POST", return_response=True) async def has_starred(self) -> bool: """ Checks if shared Modmail. + Returns ------- bool @@ -187,23 +232,30 @@ async def star_repository(self) -> None: """ Stars Modmail's repository. """ - await self.request(self.STAR_URL, method="PUT", headers={"Content-Length": "0"}) + await self.request( + self.STAR_URL, + method="PUT", + headers={"Content-Length": "0"}, + return_response=True, + ) @classmethod async def login(cls, bot) -> "GitHub": """ Logs in to GitHub with configuration variable information. + Parameters ---------- bot : Bot The Modmail bot. + Returns ------- GitHub The newly created `GitHub` object. """ self = cls(bot, bot.config.get("github_token")) - resp: dict = await self.request("https://api.github.com/user") + resp: Dict[str, Any] = await self.request(self.BASE + "/user") if resp.get("login"): self.username = resp["login"] self.avatar_url = resp["avatar_url"] @@ -666,7 +718,7 @@ async def update_repository(self) -> dict: "data": data, "user": { "username": user.username, - "avatar_url": user.display_avatar.url, + "avatar_url": user.avatar_url, "url": user.url, }, } From 23221a2fd691733ede405a44b9da5f593e2d74c7 Mon Sep 17 00:00:00 2001 From: Jerrie-Aries Date: Tue, 7 Jun 2022 14:36:09 +0000 Subject: [PATCH 2/2] Fix previous commit. - Return only if hosting method is HEROKU. - Try to get the response error message first if any. --- bot.py | 18 +++++++++++++----- cogs/utility.py | 18 +++++++++++------- core/clients.py | 32 +++++++++++++++++++++++--------- 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/bot.py b/bot.py index 5db1aab550..f3dc071fab 100644 --- a/bot.py +++ b/bot.py @@ -1594,17 +1594,22 @@ async def autoupdate(self): latest = changelog.latest_version if self.version < parse_version(latest.version): + error = None + data = {} try: # update fork if gh_token exists data = await self.api.update_repository() except InvalidConfigError: - data = {} + pass except ClientResponseError as exc: - logger.error(f"Autoupdate failed! Status {exc.status}.") - logger.error(f"Message: {exc.message}") - self.autoupdate.cancel() - return + error = exc if self.hosting_method == HostingMethod.HEROKU: + if error is not None: + logger.error(f"Autoupdate failed! Status: {error.status}.") + logger.error(f"Error message: {error.message}") + self.autoupdate.cancel() + return + commit_data = data.get("data") if not commit_data: return @@ -1681,15 +1686,18 @@ async def before_autoupdate(self): if self.config.get("disable_autoupdates"): logger.warning("Autoupdates disabled.") self.autoupdate.cancel() + return if self.hosting_method == HostingMethod.DOCKER: logger.warning("Autoupdates disabled as using Docker.") self.autoupdate.cancel() + return if not self.config.get("github_token") and self.hosting_method == HostingMethod.HEROKU: logger.warning("GitHub access token not found.") logger.warning("Autoupdates disabled.") self.autoupdate.cancel() + return def format_channel_name(self, author, exclude_channel=None, force_null=False): """Sanitises a username for use with text channel names diff --git a/cogs/utility.py b/cogs/utility.py index 5d35a20211..ba60253370 100644 --- a/cogs/utility.py +++ b/cogs/utility.py @@ -1951,20 +1951,24 @@ async def update(self, ctx, *, flag: str = ""): embed.set_author(name=user["username"], icon_url=user["avatar_url"], url=user["url"]) await ctx.send(embed=embed) else: + error = None + data = {} try: # update fork if gh_token exists data = await self.bot.api.update_repository() except InvalidConfigError: - data = {} + pass except ClientResponseError as exc: - embed = discord.Embed( - title="Update failed", - description=f"Error status {exc.status}. {exc.message}", - color=self.bot.error_color, - ) - return await ctx.send(embed=embed) + error = exc if self.bot.hosting_method == HostingMethod.HEROKU: + if error is not None: + embed = discord.Embed( + title="Update failed", + description=f"Error status: {error.status}.\nError message: {error.message}", + color=self.bot.error_color, + ) + return await ctx.send(embed=embed) if not data: # invalid gh_token embed = discord.Embed( diff --git a/core/clients.py b/core/clients.py index 862c665310..df7e1b7b56 100644 --- a/core/clients.py +++ b/core/clients.py @@ -130,10 +130,18 @@ async def request( await resp.read() return resp - try: - return await resp.json() - except (JSONDecodeError, ClientResponseError): - return await resp.text() + return await self._get_response_data(resp) + + @staticmethod + async def _get_response_data(response: ClientResponse) -> Union[Dict[str, Any], str]: + """ + Internal method to convert the response data to `dict` if the data is a + json object, or to `str` (raw response) if the data is not a valid json. + """ + try: + return await response.json() + except (JSONDecodeError, ClientResponseError): + return await response.text() def filter_valid(self, data) -> Dict[str, Any]: """ @@ -198,14 +206,20 @@ async def update_repository(self, sha: str = None) -> Dict[str, Any]: # source https://docs.github.com/en/rest/branches/branches#merge-a-branch status = resp.status + data = await self._get_response_data(resp) if status in (201, 204): - try: - return await resp.json() - except (JSONDecodeError, ClientResponseError): - return await resp.text() + return data args = (resp.request_info, resp.history) - kwargs = {"status": status, "message": status_map.get(status)} + try: + # try to get the response error message if any + message = data.get("message") + except AttributeError: + message = None + kwargs = { + "status": status, + "message": message if message else status_map.get(status), + } # just raise raise ClientResponseError(*args, **kwargs)