Skip to content

Fix API break when contents not found #3181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
EnricoMi merged 2 commits into from
Jan 30, 2025
Merged

Fix API break when contents not found #3181

EnricoMi merged 2 commits into from
Jan 30, 2025

Conversation

@skinitimski
Copy link
Contributor

@skinitimski skinitimski commented Jan 27, 2025 •
edited
Loading

Alternative to #3180 that fixes #3179, but tries to preserve existing behavior.

UnknownObjectException is raised for all kinds of calls that may return a 404, for example get_repo. Those API calls still return a "Not Found" message:

>>> from github import Github
>>> from os import environ
>>> Github(environ['GITHUB_TOKEN']).get_organization('asdfqwertyjkl')
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    Github(environ['GITHUB_TOKEN']).get_organization('asdfqwertyjkl')
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.13/site-packages/github/MainClass.py", line 408, in get_organization
    headers, data = self.__requester.requestJsonAndCheck("GET", f"/orgs/{login}")
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.13/site-packages/github/Requester.py", line 586, in requestJsonAndCheck
    return self.__check(*self.requestJson(verb, url, parameters, headers, input, self.__customConnection(url)))
           ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.13/site-packages/github/Requester.py", line 744, in __check
    raise self.createException(status, responseHeaders, data)
github.GithubException.UnknownObjectException: 404 {"message": "Not Found", "documentation_url": "https://docs.github.com/rest/orgs/orgs#get-an-organization", "status": "404"}

So, in this PR we honor both kinds of messages, but still no other kinds of messages.

Tests are updated to reflect the new case.

@skinitimski skinitimski changed the title Fix https://github.com/PyGithub/PyGithub/issues/3179 Fix API break when contents not found Jan 27, 2025
@skinitimski skinitimski marked this pull request as ready for review January 27, 2025 19:49
This was referenced Jan 27, 2025
Closed
Closed
@github-actions github-actions bot added the ⭐ top pull request Top pull request. label Jan 28, 2025
@github-actions github-actions bot mentioned this pull request Jan 28, 2025
Open
Felixoid
Felixoid approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@Felixoid Felixoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'd like to have it merged to have reliable UnknownObjectException

Felixoid
Felixoid reviewed Jan 29, 2025
View reviewed changes
github/Requester.py
elif status == 403 and cls.isRateLimitError(message):
exc = GithubException.RateLimitExceededException
elif status == 404 and message == "not found":
elif status == 404 and (message == "not found" or "no object found" in message):
Copy link
Contributor

@Felixoid Felixoid Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elif status == 404 and (message == "not found" or "no object found" in message):
elif status == 404 and (message == "not found" or "no object found" in message.lower()):

It will fail for the cases when message is "No object found for the path <path>"

Copy link
Collaborator

@EnricoMi EnricoMi Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

message is lowercase already, see above, the test uses "No object found ..." and passes.

@nielsole
Copy link

nielsole commented Jan 30, 2025 •
edited
Loading

EDIT: Just noticed that the sibling PR is taking this exact route
I wonder if depending on the message at all is the right approach. Looking at the docs there is no schema for the response in case of a 404 at all. So maybe only the error code can be checked, and maybe validated that the response is valid JSON:
https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#get-repository-content

EnricoMi
EnricoMi approved these changes Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@EnricoMi EnricoMi added this pull request to the merge queue Jan 30, 2025
Merged via the queue into PyGithub:main with commit d90323f Jan 30, 2025
14 checks passed
@skinitimski skinitimski deleted the fix-3179 branch January 31, 2025 19:45
@jens-create
Copy link

jens-create commented Feb 4, 2025

Will a new tag be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EnricoMi EnricoMi EnricoMi approved these changes

+1 more reviewer

@Felixoid Felixoid Felixoid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix ⭐ top pull request Top pull request.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Change to Github API message returned with 404 leads to UnknownObjectException not being raised

5 participants

@skinitimski @nielsole @jens-create @Felixoid @EnricoMi