diff --git a/collectoss/tasks/github/facade_github/contributor_interfaceable/contributor_interface.py b/collectoss/tasks/github/facade_github/contributor_interfaceable/contributor_interface.py index b1b163a2b..5dfb3ab72 100644 --- a/collectoss/tasks/github/facade_github/contributor_interfaceable/contributor_interface.py +++ b/collectoss/tasks/github/facade_github/contributor_interfaceable/contributor_interface.py @@ -1,6 +1,7 @@ from collectoss.tasks.github.util.github_task_session import * import json import time +import httpx import sqlalchemy as s from collectoss.application.db.models import * from collectoss.tasks.github.util.github_paginator import hit_api, process_dict_response @@ -30,7 +31,7 @@ def clean_dict(d): @deprecated("Please use GithubDataAcess.get_resource() instead") def request_dict_from_endpoint(logger, session, url, timeout_wait=10): """Hit the endpoint specified by the url and return the json that it returns if it returns a dict. - + NOTE: This function is being deprecated in favor of GithubDataAcess.get_resource() No functional change in this function body; logic preserved fully. This function still attempts to hit a GitHub API endpoint and return a dictionary if successful. @@ -113,7 +114,7 @@ def create_endpoint_from_email(email): # Also stopped splitting per note above. url = 'https://api.github.com/search/users?q={}+in:email+type:user'.format( email) - + return url @@ -264,9 +265,9 @@ def update_contributor(self, cntrb, max_attempts=3): def fetch_username_from_email(logger, auth, commit) -> dict | None: """Try every distinct email found within a commit for possible username resolution. Add email to garbage table if can't be resolved. - + # \param contributor is the raw database entry - + Args: logger (_type_): logging instance to use auth (_type_): Keymanager instance to use for auth @@ -368,7 +369,15 @@ def get_login_with_commit_hash(logger, auth, commit_data, repo_id): #TODO: here. # Send api request github_data_access = GithubDataAccess(auth, logger) - login_json = github_data_access.get_resource(url) + try: + login_json = github_data_access.get_resource(url) + except httpx.HTTPStatusError as e: + if e.response.status_code == 422: + logger.warning( + f"GitHub rejected commit lookup for hash {commit_data['hash']} " + f"in repo_id={repo_id} with HTTP 422." + ) + raise # TODO: Why are we returning None if 'sha' is not in response if we aren't even using it? if login_json is None or 'sha' not in login_json: diff --git a/collectoss/tasks/github/facade_github/tasks.py b/collectoss/tasks/github/facade_github/tasks.py index cc380d497..2bc1c8ce9 100644 --- a/collectoss/tasks/github/facade_github/tasks.py +++ b/collectoss/tasks/github/facade_github/tasks.py @@ -1,9 +1,12 @@ import logging - from collectoss.tasks.init.celery_app import celery_app as celery from collectoss.tasks.init.celery_app import FacadeRepoCollectionTask -from collectoss.tasks.github.util.github_data_access import GithubDataAccess, UrlNotFoundException +from collectoss.tasks.github.util.github_data_access import ( + GithubDataAccess, + UrlNotFoundException, + ResourceUnprocessable, +) from collectoss.tasks.github.util.github_random_key_auth import GithubRandomKeyAuth from collectoss.tasks.github.facade_github.core import * from collectoss.application.db.lib import execute_sql, get_contributor_aliases_by_email, get_unresolved_commit_emails_by_email, get_repo_by_repo_git, batch_insert_contributors, get_batch_size @@ -62,8 +65,19 @@ def process_commit_metadata(logger, auth, contributorQueue, repo_id, platform_id # Try to get the login from the commit sha - if login == None or login == "": - login = get_login_with_commit_hash(logger, auth, contributor, repo_id) + if login is None or login == "": + try: + login = get_login_with_commit_hash( + logger, auth, contributor, repo_id + ) + except ResourceUnprocessable: + # Ideally, find another commit from this contributor. + # That lookup is not implemented yet, so skip this commit for now. + logger.warning( + f"Skipping commit {contributor.get('hash')} " + f"because GitHub returned HTTP 422." + ) + continue if login == None or login == "": logger.warning("Failed to get login from commit hash") diff --git a/collectoss/tasks/github/util/github_data_access.py b/collectoss/tasks/github/util/github_data_access.py index a67a2b1cd..4a2c66f4b 100644 --- a/collectoss/tasks/github/util/github_data_access.py +++ b/collectoss/tasks/github/util/github_data_access.py @@ -34,10 +34,14 @@ class ResourceGoneException(Exception): def __init__(self, message="Resource returned HTTP 410 Gone. It is likely intentionally removed"): super().__init__(message) +class ResourceUnprocessable(Exception): + """Exception raised when GitHub returns HTTP 422.""" + pass + class GithubDataAccess: def __init__(self, key_manager, logger: logging.Logger, feature="rest"): - + self.logger = logger self.feature = feature self.key_client = KeyClient(f"github_{feature}", logger) @@ -119,8 +123,8 @@ def paginate_resource(self, url): yield from data - return - + return + def get_resource_page_count(self, url): response = self.make_request_with_retries(url, method="HEAD") @@ -128,12 +132,12 @@ def get_resource_page_count(self, url): if 'last' not in response.links.keys(): self.logger.warning(f"Github response without links. Headers: {response.headers}.") return 1 - + try: last_page_url = response.links['last']['url'] parsed_url = urlparse(last_page_url) - + return int(parse_qs(parsed_url.query)['page'][0]) except (KeyError, ValueError): raise Exception(f"Unable to parse 'last' url from response: {response.links['last']}") @@ -142,8 +146,8 @@ def get_resource(self, url): response = self.make_request_with_retries(url) - return response.json() - + return response.json() + # TODO: Handle timeout exceptions better def make_request(self, url, method="GET", timeout=100): @@ -161,13 +165,13 @@ def make_request(self, url, method="GET", timeout=100): self.logger.warning(f"Github rate limit exceeded. Key: {mask_key(self.key)}. Response: {response.text}") raise RatelimitException(response, self.expired_keys_for_request) - # There are cases with PR files, PR commits, and messages where the parent object is removed after - # It is collected, leading the the associated URL for those objects to return a 404. - # This is not an issue that is really an Exception. It is more of a nominal signal. - + # There are cases with PR files, PR commits, and messages where the parent object is removed after + # It is collected, leading the the associated URL for those objects to return a 404. + # This is not an issue that is really an Exception. It is more of a nominal signal. + if response.status_code == 404: raise UrlNotFoundException(f"Could not find {url}") - + if response.status_code == 401: resp_content = response.json() response_msg = resp_content.get("message") @@ -178,14 +182,18 @@ def make_request(self, url, method="GET", timeout=100): raise NotAuthorizedException(f"Could not authorize with the github api because key was invalid: {mask_key(self.key)}") else: self.logger.warning(f"Received a 401 response from github unrelated to bad credentials with message {response_msg}") - + if response.status_code == 410: response_msg = response.json().get("message") if response_msg is not None: raise ResourceGoneException(response_msg) else: raise ResourceGoneException() - + if response.status_code == 422: + raise ResourceUnprocessable( + f"GitHub returned HTTP 422 for {url}" + ) + response.raise_for_status() try: @@ -197,7 +205,7 @@ def make_request(self, url, method="GET", timeout=100): return response - + def make_request_with_retries(self, url, method="GET", timeout=100): """ What method does? 1. Catches RetryError and rethrows a nicely formatted OutOfRetriesException that includes that last exception thrown @@ -212,7 +220,7 @@ def make_request_with_retries(self, url, method="GET", timeout=100): # this suggests we should retry 401 exceptions at least once if isinstance(last_exception, NotAuthorizedException): self.expired_keys_for_request = [] - self.__handle_github_not_authorized_response() + self.__handle_github_not_authorized_response() raise last_exception def _decide_retry_policy(exception: Exception) -> bool: @@ -221,8 +229,15 @@ def _decide_retry_policy(exception: Exception) -> bool: Returns: bool: Boolean describing whether or not the request should be retried """ - return not isinstance(exception, (UrlNotFoundException, ResourceGoneException)) - + return not isinstance( + exception, + ( + UrlNotFoundException, + ResourceGoneException, + ResourceUnprocessable, + ), + ) + @retry(stop=stop_after_attempt(10), wait=wait_fixed(5), retry=retry_if_exception(_decide_retry_policy)) def __make_request_with_retries(self, url, method="GET", timeout=100): """ What method does? @@ -244,7 +259,7 @@ def __handle_github_not_authorized_response(self): self.key = self.key_client.invalidate(self.key) - + def __handle_github_ratelimit_response(self, response): headers = response.headers @@ -260,11 +275,11 @@ def __handle_github_ratelimit_response(self, response): current_epoch = int(time.time()) epoch_when_key_resets = int(headers["X-RateLimit-Reset"]) key_reset_time = epoch_when_key_resets - current_epoch - + if key_reset_time < 0: self.logger.error(f"Key reset time was less than 0 setting it to 0.\nThe current epoch is {current_epoch} and the epoch that the key resets at is {epoch_when_key_resets}") key_reset_time = 0 - + self.logger.info(f"\n\n\nAPI rate limit exceeded. Key resets in {key_reset_time} seconds. Informing key manager that key is expired") self.key = self.key_client.expire(self.key, epoch_when_key_resets) diff --git a/tests/test_classes/test_github_data_access.py b/tests/test_classes/test_github_data_access.py index 3ebd4db79..9537b81f1 100644 --- a/tests/test_classes/test_github_data_access.py +++ b/tests/test_classes/test_github_data_access.py @@ -2,8 +2,13 @@ import pytest from unittest.mock import Mock, patch -from collectoss.tasks.github.util.github_data_access import GithubDataAccess - +from collectoss.tasks.github.util.github_data_access import ( + GithubDataAccess, + ResourceUnprocessable, +) +from collectoss.tasks.github.facade_github.contributor_interfaceable.contributor_interface import ( + get_login_with_commit_hash, +) @pytest.fixture def mock_logger(): @@ -55,3 +60,35 @@ def test_path_with_existing_query_params(self, gda): assert "q=python" in result assert "per_page=10" in result assert result.startswith("https://api.github.com/search/repositories") + +@pytest.mark.unit +class TestResponseErrors: + + def test_422_response_is_not_retried(self): + exception = ResourceUnprocessable( + "GitHub returned HTTP 422" + ) + + assert not GithubDataAccess._decide_retry_policy(exception) +def test_422_commit_lookup_raises_resource_unprocessable(): + logger = Mock() + auth = Mock() + commit_data = {"hash": "1f5270431bc8556cf6dfee1905e07abcc45e20d2"} + + error = ResourceUnprocessable( + "GitHub returned HTTP 422" + ) + + with patch( + "collectoss.tasks.github.facade_github.contributor_interfaceable.contributor_interface.create_endpoint_from_commit_sha", + return_value="https://api.github.com/repos/Azure/Avere/commits/test", + ), patch( + "collectoss.tasks.github.facade_github.contributor_interfaceable.contributor_interface.GithubDataAccess" + ) as mock_gda: + mock_gda.return_value.get_resource.side_effect = error + + with pytest.raises(ResourceUnprocessable): + get_login_with_commit_hash( + logger, auth, commit_data, repo_id=1 + ) +