github-api icon indicating copy to clipboard operation
github-api copied to clipboard

401 when trying to extract GHUser#type

Open Haarolean opened this issue 1 year ago • 5 comments

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 avatar Mar 14 '24 13:03 Haarolean

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();

Haarolean avatar Mar 14 '24 13:03 Haarolean

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.

bitwiseman avatar Mar 15 '24 22:03 bitwiseman

@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 avatar Mar 16 '24 00:03 Haarolean

@bitwiseman bump?

Haarolean avatar Jul 25 '24 14:07 Haarolean

@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?

bitwiseman avatar Aug 05 '24 17:08 bitwiseman