PyGithub/PyGithub

Methods checking for 204 status handle 404 incorrectly

Offen

#2.760 geöffnet am 15.09.2023

 (11 Kommentare) (0 Reaktionen) (0 zugewiesene Personen)Python (1.715 Forks)batch import
Call for Contributiongood first issue

Repository-Metriken

Stars
 (6.482 Sterne)
PR-Merge-Metriken
 (Durchschn. Merge 5T 20h) (21 gemergte PRs in 30 T)

Beschreibung

Context

I am trying to build some automation using PyGithub and a Github App to authenticate to our organization. The token I can get has an expiration of 1 hour.

The idea of my workflow is to do the following steps:

  1. Get a token from Github App
  2. Create pull request
  3. Notify reviewers
  4. Wait for PR to be merged with a flexible timeout to allow reviewers time to review.
  5. Once PR is merged, do more steps

The problem arises in 4 if the timeout exceeds 1 hour.

Problem

In the scenario above I noticed that even after the authentication token has expired, calling pr.is_merged() returns False.

Looking at the code the behavior I was seeing makes sense:

    def is_merged(self) -> bool:
        """
        :calls: `GET /repos/{owner}/{repo}/pulls/{number}/merge <https://docs.github.com/en/rest/reference/pulls>`_
        """
        status, headers, data = self._requester.requestJson("GET", f"{self.url}/merge")
        return status == 204

since the status of the request with expired token would be 401.

I see two issues with this:

  1. False negatives - a PR could have been merged but is_merged returns False.
  2. Consumer is not notified in any way that their authentication has expired.

This also differs from other methods that I have been using like pr.get_commits().get_page(0) or pr.get_labels().get_page(0) which fail with the expected github.BadCredentialsException as soon as the token expires.

I also tested with a token that has no access to my organization, but in that case an expected 403 was raised which seems to be handled by github.GithubRetry.

Steps to Reproduce

  1. Create a Github Token on your account with repo access
  2. Use token to create a client and get a pull request pr
  3. Remove Github token from your account 4 . Run pr.is_merged .

For example:

import github
github_client = github.Github("<YOUR TOKEN>")

upstream_repo = github_client.get_repo("PyGithub/PyGithub")
pr = upstream_repo.get_pull(2757)
pr.is_merged()
False
pr.get_commits().get_page(0)
[Commit(sha="7569ac6c33e3682fa42ddf7aee1640198461a557"), Commit(sha="626a0435d063b12f9a9501a282e7371fe8783494")]
#
# Remove token from account
#
pr.is_merged()
False
pr.get_commits().get_page(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../site-packages/github/PaginatedList.py", line 258, in get_page
    headers, data = self.__requester.requestJsonAndCheck(
  File ".../site-packages/github/Requester.py", line 442, in requestJsonAndCheck
    return self.__check(
  File ".../site-packages/github/Requester.py", line 487, in __check
    raise self.__createException(status, responseHeaders, data)
github.GithubException.BadCredentialsException: 401 {"message": "Bad credentials", "documentation_url": "https://docs.github.com/rest"}

Workaround

I call pr.get_commits().get_page(0) as well to ensure I can catch the token expiration scenario and handle it.

Contributor Guide