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

GHRepository#isDeleteBranchOnMerge alway returns false

Open alexsuter opened this issue 2 years ago • 10 comments

Describe the bug GHRepository#isDeleteBranchOnMerge alway returns false even if it's true

To Reproduce Call GHRepository#isDeleteBranchOnMerge

Expected behavior GHRepository#isDeleteBranchOnMerge should return true if its true

alexsuter avatar Aug 19 '22 13:08 alexsuter

The curl response itself doesn't have few other attributes described in their [contract]including DeleteBranchOnMerge (https://docs.github.com/en/rest/repos/repos#get-a-repository).

javapriyan avatar Aug 27 '22 17:08 javapriyan

Is it possible that the OAuth token we generate doesn't pull repo setting related attributes.

javapriyan avatar Aug 27 '22 17:08 javapriyan

Could have someone a look into this :)

alexsuter avatar Nov 13 '23 11:11 alexsuter

@alexsuter

We have a test specifically for this: https://github.com/hub4j/github-api/blob/3d3e90588494c6a534ff793a0879a3714e159d44/src/test/java/org/kohsuke/github/GHRepositoryTest.java#L994-L1008

Can you point to a repository where you are seeing this issue?

bitwiseman avatar Nov 13 '23 20:11 bitwiseman

I checked again and it still returns always false:

https://github.com/axonivy/github-repo-manager/blob/905f94e78899c1956f760bfbb3ad594a31a27b8f/github-repo-manager/src/main/java/com/axonivy/github/GitHubRepoSettingsManager.java#L51C13-L57

alexsuter avatar Nov 17 '23 14:11 alexsuter

I did some digging.

Summary

When checking isDeleteBranchOnMerge you credentials must have at least repo:public_repo permissions.

Details

I ran the following commands:

# unauthenticated
curl -L -H "Accept: application/vnd.github+json" https://api.github.com/repos/hub4j-test-org/github-api > response_1.json
# TOKEN has no permissions checked
curl -L -H "Accept: application/vnd.github+json" -H "Authorization: Bearer <TOKEN>" https://api.github.com/repos/hub4j-test-org/github-api > response_2.json
# TOKEN has only "repo:public_repo" permission set
curl -L -H "Accept: application/vnd.github+json" -H "Authorization: Bearer <TOKEN>" https://api.github.com/repos/hub4j-test-org/github-api > response_3.json

Files response_1.json and response_2.json were missing a number of fields that were present in response_3.json, including delete_branch_on_merge. Here's this list of fields not returned unless authenticated with at least repo:public_repo permissions:

  "permissions": {
    "admin": true,
    "maintain": true,
    "push": true,
    "triage": true,
    "pull": true
  },
  "allow_squash_merge": true,
  "allow_merge_commit": true,
  "allow_rebase_merge": true,
  "allow_auto_merge": false,
  "delete_branch_on_merge": false,
  "allow_update_branch": false,
  "use_squash_pr_title_as_default": false,
  "squash_merge_commit_message": "COMMIT_MESSAGES",
  "squash_merge_commit_title": "COMMIT_OR_PR_TITLE",
  "merge_commit_message": "PR_TITLE",
  "merge_commit_title": "MERGE_MESSAGE",
  "security_and_analysis": {
    "secret_scanning": {
      "status": "disabled"
    },
    "secret_scanning_push_protection": {
      "status": "disabled"
    },
    "dependabot_security_updates": {
      "status": "disabled"
    }
  }

It seems likely that what you're seeing is an accurate representation of what is being returned from GitHub. In the short term, I think the only thing this project could do about this is document the behavior in our JavaDoc.

In the longer term, there are a number of things we might look at to make this clearer. We could add a "hasField(name)" method to allow consumers of this library to check if a field was present in the JSON that was used to create an object. Or we could switch to using objects instead of values ("Boolean" instead of bool in this case) - which would allow us to return null for missing fields instead of whatever the default value (false in this case). This would be breaking change and could make the library a bit harder to use, but would also make it clear when data is not being provided.

bitwiseman avatar Nov 17 '23 22:11 bitwiseman

My token has repo:public_repo permission. Still not working.

Can I enable a logging to see how to request and response looks like?

alexsuter avatar Nov 20 '23 08:11 alexsuter

It is likely that getting all repositories for an org (https://github.com/axonivy/github-repo-manager/blob/905f94e78899c1956f760bfbb3ad594a31a27b8f/github-repo-manager/src/main/java/com/axonivy/github/GitHubRepoSettingsManager.java#L30C25-L30C46) doesn't return records with that value...

Looking at the example response for that endpoint it doesn't show that field.

😡 The REST API is remarkable slapdash in this respect.

I think the right thing to do is similar to what I outlined above: change these fields to Boolean and then check them for null when returning them. It would look a bit like this:

    /**
     * Automatically deleting head branches when pull requests are merged. 
     *
     * This field is not present when querying from some endpoints.  Calling this method may result in network call.
     * Also, this field in not returned unless the connection credentials have {@code repo:public_repo} permission.
     * Calling this method without sufficient permission will return false. 
     *
     * @return true if delete branch on merge, false if not delete branch on merge or if insufficient permission
     */
    public boolean isDeleteBranchOnMerge() {
        if (delete_branch_on_merge == null) {
            try {
                populate();
            } catch (IOException e) {
                // Convert this to a runtime exception to avoid messy method signature
                throw new GHException("Failed to populate optional field", e);
            }
            // if this field not populated, set it to false and log a warning
            delete_branch_on_merge = Boolean.TRUE.equals(delete_branch_on_merge);
        }
        return delete_branch_on_merge;
    }

bitwiseman avatar Nov 20 '23 19:11 bitwiseman

Yes, this is the problem! Now I reload each repo again.. Not good, but it works for me now.

Thanks for your heavy investigations!

alexsuter avatar Nov 21 '23 08:11 alexsuter

@alexsuter Do you have time to do a PR for this?

bitwiseman avatar Nov 22 '23 17:11 bitwiseman