Fix installation payload handling #1371

gsmet · 2022-02-02T13:39:02Z

Hey there,

Long time no see :).

The problem was initially reported in my Quarkus GitHub App project: quarkiverse/quarkus-github-app#253.

Parsing Installation event payloads with a connected GitHub instance (i.e. not an offline one) leads to:

Exception: java.lang.NullPointerException: Missing instance URL!
	at java.base/java.util.Objects.requireNonNull(Objects.java:247)
	at org.kohsuke.github.GHRepository.populate(GHRepository.java:3260)
	at org.kohsuke.github.GHEventPayload$Installation.lateBind(GHEventPayload.java:276)
	at org.kohsuke.github.GitHub.parseEventPayload(GitHub.java:944)

Installation payloads do not contain the URL of the repository so we cannot populate the data from there. You can see an example in the tests here:
https://github.com/hub4j/github-api/blob/main/src/test/resources/org/kohsuke/github/GHEventPayloadTest/installation.json#L45-L53

I think what I did is safe but better check the URL thing as I decided to always use a forged URL rather than the one provided in the JSON. I can do it in another way if you prefer and keep the JSON URL when it's provided but, given we already had a try/catch for a corner case, I thought we might as well always forge the URL.

The tests are not failing because we are testing all the payloads with an offline GitHub instance thus not populating the repositories. Maybe we should change that? But I'm not totally sure of the consequences of this change. So I thought I might as well ask.

Very motivated to get this change in as one of my users is blocked by this so eager to hear your feedback :).

In the installation payload, the repository doesn't contain a lot of information and it especially does not contain the URL. Tests are passing because we use an offline GitHub instance for testing the payloads.

gsmet · 2022-02-02T13:39:36Z

src/main/java/org/kohsuke/github/GHRepository.java

@@ -2978,7 +2977,7 @@ String getApiTailUrl(String tail) {
        if (tail.length() > 0 && !tail.startsWith("/")) {
            tail = '/' + tail;
        }
-        return "/repos/" + getOwnerName() + "/" + name + tail;
+        return "/repos/" + full_name + tail;


FWIW, the owner is not provided either in the installation token, the only info we have is the full name.

gsmet · 2022-02-02T13:40:19Z

src/main/java/org/kohsuke/github/GHRepository.java

-                throw e;
-            }
-        }
+        root().createRequest().withPreview(BAPTISTE).withPreview(NEBULA).withUrlPath(getApiTailUrl("")).fetchInto(this);


Decided to simplify that but I can add a bunch of ifs if you prefer using the URL coming from the JSON when it is around.

No, this is functional and clean. 💯

codecov · 2022-02-02T13:43:32Z

Codecov Report

Merging #1371 (c87903e) into main (0e41ccb) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1371      +/-   ##
============================================
- Coverage     78.18%   78.10%   -0.09%     
  Complexity     2045     2045              
============================================
  Files           200      200              
  Lines          6284     6270      -14     
  Branches        356      355       -1     
============================================
- Hits           4913     4897      -16     
- Misses         1162     1165       +3     
+ Partials        209      208       -1

Impacted Files	Coverage Δ
...c/main/java/org/kohsuke/github/GHEventPayload.java	`80.41% <100.00%> (ø)`
src/main/java/org/kohsuke/github/GHRepository.java	`68.04% <100.00%> (-0.39%)`	⬇️
...c/main/java/org/kohsuke/github/GitHubResponse.java	`81.39% <0.00%> (-9.31%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e41ccb...c87903e. Read the comment docs.

gsmachado · 2022-02-02T16:54:17Z

I can confirm that this PR fixes the issue on my GitHub app. 👍 💪 💯

gsmet · 2022-02-03T14:19:46Z

@bitwiseman I updated the test to make it a connected one so it's now properly tested.

gsmet · 2022-02-03T14:21:35Z

src/test/java/org/kohsuke/github/GHEventPayloadTest.java

@@ -630,7 +630,7 @@ public void checkRunEvent() throws Exception {
                GHEventPayload.CheckRun.class);
        final GHCheckRun checkRun2 = verifyBasicCheckRunEvent(event2);

-        int expectedRequestCount = mockGitHub.isUseProxy() ? 3 : 2;
+        int expectedRequestCount = 2;


I refreshed the snapshots and this is no longer needed.

bitwiseman · 2022-02-14T21:18:38Z

Sorry for the slow response. Looking now.

bitwiseman

Looks good.

src/main/java/org/kohsuke/github/GHEventPayload.java

src/main/java/org/kohsuke/github/GHRepository.java

src/main/java/org/kohsuke/github/GHEventPayload.java

gsmet · 2022-02-25T18:25:59Z

Hey @bitwiseman , any chance we could have a release with this fix? Thanks!

bitwiseman · 2022-03-02T02:16:22Z

@gsmet Released v1.302

gsmet · 2022-03-02T09:22:43Z

@bitwiseman thanks!

Fix installation payload handling

cfdfca9

In the installation payload, the repository doesn't contain a lot of information and it especially does not contain the URL. Tests are passing because we use an offline GitHub instance for testing the payloads.

gsmet commented Feb 2, 2022

View reviewed changes

gsmet mentioned this pull request Feb 2, 2022

Installation Created: event triggers a NullPointerException quarkiverse/quarkus-github-app#253

Closed

Add a test for repository population of installation event payloads

83386fc

gsmet commented Feb 3, 2022

View reviewed changes

bitwiseman closed this Feb 14, 2022

bitwiseman reopened this Feb 14, 2022

bitwiseman approved these changes Feb 14, 2022

View reviewed changes

src/main/java/org/kohsuke/github/GHEventPayload.java Outdated Show resolved Hide resolved

src/main/java/org/kohsuke/github/GHRepository.java Outdated Show resolved Hide resolved

Apply suggestions from code review

ea9cfef

bitwiseman reviewed Feb 14, 2022

View reviewed changes

src/main/java/org/kohsuke/github/GHEventPayload.java Outdated Show resolved Hide resolved

Update src/main/java/org/kohsuke/github/GHEventPayload.java

c87903e

bitwiseman merged commit 9b164ca into hub4j:main Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix installation payload handling #1371

Fix installation payload handling #1371

gsmet commented Feb 2, 2022 •

edited

Loading

gsmet Feb 2, 2022

gsmet Feb 2, 2022

bitwiseman Feb 14, 2022

codecov bot commented Feb 2, 2022 •

edited

Loading

gsmachado commented Feb 2, 2022

gsmet commented Feb 3, 2022 •

edited

Loading

gsmet Feb 3, 2022

bitwiseman commented Feb 14, 2022

bitwiseman left a comment •

edited

Loading

gsmet commented Feb 25, 2022

bitwiseman commented Mar 2, 2022

gsmet commented Mar 2, 2022

Fix installation payload handling #1371

Fix installation payload handling #1371

Conversation

gsmet commented Feb 2, 2022 • edited Loading

gsmet Feb 2, 2022

Choose a reason for hiding this comment

gsmet Feb 2, 2022

Choose a reason for hiding this comment

bitwiseman Feb 14, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2022 • edited Loading

Codecov Report

gsmachado commented Feb 2, 2022

gsmet commented Feb 3, 2022 • edited Loading

gsmet Feb 3, 2022

Choose a reason for hiding this comment

bitwiseman commented Feb 14, 2022

bitwiseman left a comment • edited Loading

Choose a reason for hiding this comment

gsmet commented Feb 25, 2022

bitwiseman commented Mar 2, 2022

gsmet commented Mar 2, 2022

gsmet commented Feb 2, 2022 •

edited

Loading

codecov bot commented Feb 2, 2022 •

edited

Loading

gsmet commented Feb 3, 2022 •

edited

Loading

bitwiseman left a comment •

edited

Loading