Skip to content

Methods checking for 204 status handle 404 incorrectly #2760

@jorge-morse

Description

@jorge-morse

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions