hub4j / github-api

Java API for GitHub
https://github-api.kohsuke.org/
MIT License
1.14k stars 727 forks source link

401 when trying to extract GHUser#type #1816

Open Haarolean opened 6 months ago

Haarolean commented 6 months ago

Hi, a code like this

new GitHubBuilder()
        .withJwtToken("token")
        .build()
        .getApp()
        .getInstallationById(githubInstallationId)
        .getAccount()
        .getType()

leads to 401:

Caused by: org.kohsuke.github.HttpException: {"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}
    at org.kohsuke.github.GitHubConnectorResponseErrorHandler$1.onError(GitHubConnectorResponseErrorHandler.java:62) ~[github-api-unbridged-1.318.jar:na]

There are a few issues here:

  1. The field is already initialized as "Organization" after calling getAccount(), so populating here is not necessary, perhaps.
  2. installation.getAccount().getUrl() returns https://api.github.com/users/<ORGANIZATION_LOGIN>, which perhaps is invalid, because it's an organization, not a user.
Haarolean commented 6 months ago

Unfortunately, I couldn't find a way to work around this by determining installation owner's account type, please let me know if there's something I missed

UPD: Found a workaround:

ghInstallation.getRoot().setConnector(HttpConnector.OFFLINE);
ghInstallation.getAccount().getType();
bitwiseman commented 6 months ago

Here's the problem:

https://github.com/hub4j/github-api/blob/3f9954144a6df515bd7d6c8cdc123536430e893a/src/main/java/org/kohsuke/github/GHPerson.java#L342-L344

populate() should only be called if type is null.

Haarolean commented 6 months ago

@bitwiseman got it. How should we solve this taking into consideration the fact that there are multiple fields like this? Should we track the "populated = true/false" flag for an entity or something else?

Haarolean commented 2 months ago

@bitwiseman bump?

bitwiseman commented 2 months ago

@Haarolean In this specific case, you can guard the populate() call with if (type == null).
I think some of the classes have a populate(bool doPopulate) method or populate(Object objectToCheckForNullValue) to streamline this process.

Tracking bool isPopulated; as field could also work.
I don't like the way we currently don't know what fields we have actually loaded in an instance, but I haven't had time to ponder a better solution for the range partial fields that are returned. Maybe have the base record and then use interfaces to show what fields are present.

What are your thoughts?