-
Notifications
You must be signed in to change notification settings - Fork 723
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.
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
Fix GHFileNotFoundException
when getting commits from PR found in search
#1779
Fix GHFileNotFoundException
when getting commits from PR found in search
#1779
Conversation
Previously, when a pull request had been fetched, it lacked an `owner`, and thus was following a specific codepath inside `GHPullRequest.GetApiRoute` that was returning a URL that corresponded to an `issue`. When that URL was then appended on for further queries, say, with `/commits`, then Github would return a 404. This fixes the issue by manipulating the URL manually and replacing the wrong reference to `/issues/` with `/pulls/`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1779 +/- ##
============================================
+ Coverage 80.44% 80.64% +0.20%
- Complexity 2316 2322 +6
============================================
Files 219 219
Lines 7026 7027 +1
Branches 371 371
============================================
+ Hits 5652 5667 +15
+ Misses 1141 1128 -13
+ Partials 233 232 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me and has test coverage. Thanks!
I think this issue might still linger? I can't retrieve |
Please provide details. Maybe sample code that reproduces this. Are you on the most recent version. |
Yeah, the problem is that we are using My understanding is that we should use |
So the pattern of using refresh() is fine, it's just the usage of getUrl, should be replaced with getApiRoute? Unclear if that shadowcat preview is still required. |
Yes, that's what I would do. Would you be able to prepare a PR? Ideally with a test case? |
When a PR was loaded from a search, the refresh() method was reloading information from the issues API, which would lead to some information not being refreshed properly, typically the mergeable state. Related to hub4j#1779 (comment)
@daniel-b2c2 I created #1810 to fix it. Thanks for providing all the details. |
…#1810) * Make sure we refresh the PRs with the PR API (and not the issues API) When a PR was loaded from a search, the refresh() method was reloading information from the issues API, which would lead to some information not being refreshed properly, typically the mergeable state. Related to #1779 (comment) * Update GHPullRequestTest.java * Update GHPullRequestTest.java * Update GHPullRequestTest.java * Update GHPullRequestTest.java --------- Co-authored-by: Liam Newman <bitwiseman@gmail.com>
Description
Fixes #1778.
The issue summarily was that a compensation mechanism for sourcing a URL for a query was generating the wrong URL, that was then being used by other parts of the library to create further queries that the Github API would then reject.
This fixes the issue by doing a string search and replace after the string has been first sourced. It has been observed locally that this is an effective fix for the issue described.
Please consider the above a proposed change. If the project maintainers think that this is not an appropriate fix, I'm happy to rework this (if minor changes are needed) or close the PR (if major changes are needed, that are better to be done by someone more familiar with the codebase than myself).
Thank you,
Before submitting a PR:
@link
JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: