Handling error messaging when PR creation fails with Invalid code error #321

avimanyum · 2022-02-25T21:20:35Z

There are cases when the PR creation fails and the exception message thrown looks like this:
{"message":"Validation Failed","errors":[{"resource":"PullRequest","field":"head","code":"invalid"}],"documentation_url":"https://docs.github.com/enterprise/3.1/rest/reference/pulls#create-a-pull-request"}

In such case, this line will throw a NullPointerException since the JsonArray does not have a field called message. Unfortunately, since this error is thrown inside the catch block, it is not handled and essentially terminates the process.

This PR checks for the existence of this key inside the Json object before trying to get it. If it does not find it there, it assumes that the error was due to the head having invalid code (error message shown above). In such a scenario, the method returns an error code of 1, so that we can proceed to safely deleting the forked repo here

codeclimate · 2022-02-25T21:22:14Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

            log.info("error: {}", error);
            if (error.startsWith("A pull request already exists")) {
                log.info("NOTE: {} New commits may have been added to the pull request.", error);
                return 0;
-            } else if (error.startsWith("No commits between")) {
+            } else if (error.startsWith("No commits between") || error.startsWith("PR head has invalid code.")) {


Line is longer than 100 characters (found 113).

codecov · 2022-02-25T21:22:18Z

Codecov Report

Merging #321 (c9bf244) into master (859c50c) will increase coverage by 0.05%.
The diff coverage is 80.00%.

❗ Current head c9bf244 differs from pull request most recent head 596a07d. Consider uploading reports for the commit 596a07d to get more accurate results

@@             Coverage Diff              @@
##             master     #321      +/-   ##
============================================
+ Coverage     81.49%   81.55%   +0.05%     
- Complexity      263      265       +2     
============================================
  Files            19       19              
  Lines           935      938       +3     
  Branches        126      127       +1     
============================================
+ Hits            762      765       +3     
  Misses          142      142              
  Partials         31       31

Impacted Files	Coverage Δ
...sforce/dockerfileimageupdate/utils/GitHubUtil.java	`72.81% <80.00%> (+0.81%)`	⬆️

afalko · 2022-03-14T17:19:30Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

+            if (errorJson.get(0).getAsJsonObject().has("message")) {
+                error = errorJson.get(0).getAsJsonObject().get("message").getAsString();
+            } else {
+                error = "PR head has invalid code.";


What does "invalid code" mean? What action should be taken to resolve if any?

The error message is trying to communicate that the field/keyword called "head" contains invalid data. When i saw this message, I manually deleted the fork and the next run was successful. This change will also do the same - deleted the fork.

afalko · 2022-03-14T17:20:10Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

            log.info("error: {}", error);
            if (error.startsWith("A pull request already exists")) {
                log.info("NOTE: {} New commits may have been added to the pull request.", error);
                return 0;
-            } else if (error.startsWith("No commits between")) {
+            } else if (error.startsWith("No commits between") || error.startsWith("PR head has invalid code.")) {


Repeated error message. Let's make it a constant in this class

afalko

LGTM, but I think we should improve the error message.

codeclimate · 2022-03-14T17:54:35Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

+            if (errorJson.get(0).getAsJsonObject().has("message")) {
+                error = errorJson.get(0).getAsJsonObject().get("message").getAsString();
+            } else {
+                // This case usually happens when the PR head has invalid data. Deleting the forked repo resolves it.


Line is longer than 100 characters (found 117).

codeclimate · 2022-03-14T17:54:35Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

            log.info("error: {}", error);
            if (error.startsWith("A pull request already exists")) {
                log.info("NOTE: {} New commits may have been added to the pull request.", error);
                return 0;
-            } else if (error.startsWith("No commits between")) {
+            } else if (error.startsWith("No commits between") || error.startsWith(PR_INVALID_CODE)) {


Line is longer than 100 characters (found 101).

afalko · 2022-03-14T18:31:44Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

@@ -29,6 +29,7 @@
 public class GitHubUtil {
    private static final Logger log = LoggerFactory.getLogger(GitHubUtil.class);
    public static final String NO_BRANCH_WARN_FORMAT = "Couldn't find branch `%s` in repo `%s`. Waiting a second...";
+    public static final String PR_INVALID_CODE = "The PR head has invalid data.";


I still don't think a user or admin will understand this error message. Can we hint at what needs to be done to fix the PR?

codeclimate · 2022-03-14T18:38:55Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

@@ -29,6 +29,8 @@
 public class GitHubUtil {
    private static final Logger log = LoggerFactory.getLogger(GitHubUtil.class);
    public static final String NO_BRANCH_WARN_FORMAT = "Couldn't find branch `%s` in repo `%s`. Waiting a second...";
+    public static final String PR_INVALID_CODE = "The PR head has invalid data. This might be because the repository was migrated to another organization." +


Line is longer than 100 characters (found 157).

codeclimate · 2022-03-14T18:38:55Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

@@ -29,6 +29,8 @@
 public class GitHubUtil {
    private static final Logger log = LoggerFactory.getLogger(GitHubUtil.class);
    public static final String NO_BRANCH_WARN_FORMAT = "Couldn't find branch `%s` in repo `%s`. Waiting a second...";
+    public static final String PR_INVALID_CODE = "The PR head has invalid data. This might be because the repository was migrated to another organization." +


'+' should be on a new line.

justinharringa · 2022-03-14T19:13:15Z

Hmm... Are there other scenarios that end up taking this path? My concern is that when you delete a fork, the API will lag by hours and may select a fork that is no longer there. Additionally, I believe this may have an effect on open PRs.

cjneasbi · 2022-03-14T20:31:15Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

            log.info("error: {}", error);
            if (error.startsWith("A pull request already exists")) {
                log.info("NOTE: {} New commits may have been added to the pull request.", error);
                return 0;
-            } else if (error.startsWith("No commits between")) {
+            } else if (error.startsWith("No commits between") || error.startsWith(PR_INVALID_CODE)) {


Create a list of error prefixes and use stream operations

List<String> prefixes = Arrays.asList(PR_INVALID_CODE, "No commits between") if(prefixes.anyMatch(prefix -> errors.startsWith(prefix))

This way if you add more prefixes the if statement doesn't change.

cjneasbi · 2022-03-14T20:33:44Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

@@ -108,12 +110,18 @@ public int createPullReq(GHRepository origRepo, String branch,
            log.warn("Handling error with pull request creation... {}", e.getMessage());
            JsonElement root = JsonParser.parseString(e.getMessage());
            JsonArray errorJson = root.getAsJsonObject().get("errors").getAsJsonArray();
-            String error = errorJson.get(0).getAsJsonObject().get("message").getAsString();
+            String error = "";
+            if (errorJson.get(0).getAsJsonObject().has("message")) {


Store the output of errorJson.get(0).getAsJsonObject() as a local variable. Call has and get on that variable. This removes the need to call get(0).getAsJsonObject() twice.

Why are we ignoring other errors if multiple were returned?

cjneasbi · 2022-03-14T20:38:27Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

+                error = errorJson.get(0).getAsJsonObject().get("message").getAsString();
+            } else {
+                // This case usually happens when the PR head has invalid data. Deleting the forked repo resolves it.
+                error = PR_INVALID_CODE;


You don't need this else. Just assign String error = PR_INVALID_CODE at 113.

cjneasbi · 2022-03-14T20:42:34Z

...le-image-update/src/test/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtilTest.java

+        GHRepository forkRepo = mock(GHRepository.class);
+        when(forkRepo.getOwnerName()).thenReturn("owner");
+        assertEquals(gitHubUtil.createPullReq(origRepo, "branch", forkRepo, "title", "body"), 1);
+        verify(origRepo, times(1)).createPullRequest(eq("title"), eq("owner:branch"), eq("master"), eq("body"));


times(1) is the default for verify, you can remove the second parameter.

avimanyum · 2022-03-14T21:43:34Z

Hmm... Are there other scenarios that end up taking this path? My concern is that when you delete a fork, the API will lag by hours and may select a fork that is no longer there. Additionally, I believe this may have an effect on open PRs.

Hmm, in that case we might just want to make sure that we catch the exception by checking if the json field actually exists before accessing it, but not return 1 -> which is what triggers the delete of the fork.

avimanyum · 2022-03-14T21:44:10Z

@cjneasbi Addressed your comments in this new PR - #331

Handling invalid code exception

4af1c38

avimanyum requested review from justinharringa and afalko as code owners February 25, 2022 21:20

github-actions bot added the tests Improvements / maintenance to tests label Feb 25, 2022

codeclimate bot reviewed Feb 25, 2022

View reviewed changes

avimanyum added bug Bug fixes and removed tests Improvements / maintenance to tests labels Mar 8, 2022

afalko requested a review from cjneasbi March 14, 2022 17:18

afalko reviewed Mar 14, 2022

View reviewed changes

afalko requested changes Mar 14, 2022

View reviewed changes

Merge branch 'master' into pr_exception

36cd47e

github-actions bot added the tests Improvements / maintenance to tests label Mar 14, 2022

Adding error message as constant

a4dd5d7

codeclimate bot reviewed Mar 14, 2022

View reviewed changes

avimanyum removed the tests Improvements / maintenance to tests label Mar 14, 2022

Empty commit

020e878

github-actions bot added the tests Improvements / maintenance to tests label Mar 14, 2022

afalko reviewed Mar 14, 2022

View reviewed changes

Improving error message

596a07d

codeclimate bot reviewed Mar 14, 2022

View reviewed changes

afalko approved these changes Mar 14, 2022

View reviewed changes

avimanyum merged commit c498297 into salesforce:master Mar 14, 2022

cjneasbi reviewed Mar 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling error messaging when PR creation fails with Invalid code error #321

Handling error messaging when PR creation fails with Invalid code error #321

avimanyum commented Feb 25, 2022

codeclimate bot Feb 25, 2022

codecov bot commented Feb 25, 2022 •

edited

Loading

afalko Mar 14, 2022

avimanyum Mar 14, 2022

afalko Mar 14, 2022

afalko left a comment

codeclimate bot Mar 14, 2022

codeclimate bot Mar 14, 2022

afalko Mar 14, 2022

codeclimate bot Mar 14, 2022

codeclimate bot Mar 14, 2022

justinharringa commented Mar 14, 2022

cjneasbi Mar 14, 2022

cjneasbi Mar 14, 2022

cjneasbi Mar 14, 2022

cjneasbi Mar 14, 2022

cjneasbi Mar 14, 2022

avimanyum commented Mar 14, 2022

avimanyum commented Mar 14, 2022

Handling error messaging when PR creation fails with Invalid code error #321

Handling error messaging when PR creation fails with Invalid code error #321

Conversation

avimanyum commented Feb 25, 2022

codeclimate bot Feb 25, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afalko left a comment

Choose a reason for hiding this comment

codeclimate bot Mar 14, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Mar 14, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 14, 2022

Choose a reason for hiding this comment

justinharringa commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimanyum commented Mar 14, 2022

avimanyum commented Mar 14, 2022

codecov bot commented Feb 25, 2022 •

edited

Loading