NullPointerException when retrieve a review's user if it's deleted

[ghost]

Describe the bug
This line throws an NPE if the reviewer's user account is deleted in github

To Reproduce
Steps to reproduce the behavior:

1. Create a Pull Request in a Github repo
2. Register a new github user
3. Use the new user to add a review (e.g. comment + approval) of the pull request
4. Delete that new github user account
5. Try use this API to fetch the review user (i.e. call review.getUser(); on that GHPullRequestReview object.
6. There would an uncaught NullPointerException.

Expected behavior
Perhaps returns a null user?

Desktop (please complete the following information):

• OS: MacOS
• Browser: N/A
• Version: 1.1.06

Additional context
Add any other context about the problem here.

[sahansera]

Hi @bitwiseman I had a look into this issue and found that this might not be the case anymore. However, I investigated a bit further and I observed the following behaviours during my initial tests.

1. User A requests a PR review from a User B. User B gets deleted. GitHub removes the requested reviewer from the PR.
   • If we do a pr.getRequestedReviewers() on the PR the list that gets returned will be empty.
   • Sample PR can be found here

GitHub github = new GitHubBuilder().withOAuthToken("PAT").build();
var repo = github.getRepository("sahansera/TestRepo");
var pr = repo.getPullRequest(1);
var reviewer = pr.getRequestedReviewers();

2. User A requests a PR review from a User B. User A gets deleted. GitHub assigns a ghost account to User A.
   • If we do a pr.getUser() it returns the details of the ghost user account. But it will not be null
   • Sample PR can be found here

GitHub github = new GitHubBuilder().withOAuthToken("PAT").build();
var repo = github.getRepository("sahansera/TestRepo");
var pr = repo.getPullRequest(2);
var reviewer = pr.getUser();

• Could you kindly assign this task to me?
• Do you think it's a good idea to add some unit tests around this to validate it? I am happy to work on any test cases cover these scenarios.

Thank you!

[sahansera]

@bitwiseman

Thanks again for assigning me this task and adding me to the test org.

I initially went down the path trying to create a PR then assign/remove reviewers. However, it's going to be tricky since we can't assign a non-existing user to a PR programmatically. What I would suggest is, we create two PRs in the hub4j-test-org to simulate the above two scenarios and use their Ids in the unit tests and perform the assertions.

If this sounds like a good plan, would you be able to create two PRs in hub4j-test-org for me (since I don't have admin rights I can't assign people to PRs).

1. PR 1 - Author gets deleted.
2. PR 2 - Reviewer gets deleted. Additionally, we could add a comment from the reviewer too.

I have two unit tests targeting my sample repo that are ready to go. I can perform a WireMock snapshot and update my changes and raise a PR.

[bitwiseman]

Thanks for working on this.
You might have to write custom wiremock stubs, or record different stubs and then mix and match them to get the set of responses that will exercise this scenario.

[sahansera]

@bitwiseman No problem! Thanks a lot for the suggestion. I have followed your approach and created a PR here.

In summary, I first ran the WireMock snapshots for my test repo and edited them manually so that they cover all the scenarios and comply with the getRepository() method.