Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PATCH endpoint for branch protections does not clear contexts array #26333

Closed
Infinoid opened this issue Aug 4, 2023 · 0 comments · Fixed by #26351
Closed

PATCH endpoint for branch protections does not clear contexts array #26333

Infinoid opened this issue Aug 4, 2023 · 0 comments · Fixed by #26351
Labels
modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Milestone

Comments

@Infinoid
Copy link
Contributor

Infinoid commented Aug 4, 2023

Description

The PATCH /api/v1/repos/<org>/<repo>/branch_protections/<branch> API endpoint edits an existing branch protection rule for the given repo and branch.

When using the web interface to disable status checks in a branch protection rule, the lists of contexts (CICD job names) is automatically cleared.

When using the API endpoint to do the same, you can pass status_check_contexts=[], but this value is ignored because enable_status_check is false.

To clear out that array, you have to make two separate PATCH calls: once to empty the array, and once to unset the boolean flag. There is a command in the below reproducer, where it says "UNCOMMENT THIS", which demonstrates the workaround of calling PATCH twice.

Ideally the status_check_contexts array would be emptied automatically when the enable_status_check boolean flag is set to false, like the UI does.

Reproducer

  1. Use the PATCH endpoint, setting enable_status_check=true, and add a string to the status_check_contexts array.
  2. Use PATCH again, setting enable_status_check=false, and status_check_contexts=[].
  3. See in the response that the status_check_contexts array was not actually emptied. It still has the value from step 1.
  4. Do another GET, see that it's still not empty.

The below shell script does this, using a try.gitea.io account. After each call, the status_check_contexts array from the response is printed.

Expected output:

[
  "infinoid/test-branch-protection-rules/pipeline/pr-main"
]
[]
[]

Actual output:

[
  "infinoid/test-branch-protection-rules/pipeline/pr-main"
]
[
  "infinoid/test-branch-protection-rules/pipeline/pr-main"
]
[
  "infinoid/test-branch-protection-rules/pipeline/pr-main"
]

Output when the "UNCOMMENT THIS" line is uncommented:

[
  "infinoid/test-branch-protection-rules/pipeline/pr-main"
]
[]
[]
[]

Shell script:

#!/bin/sh

GITEA_API="`grep try.gitea.io ~/.git-credentials`/api/v1"
REPO=$USER/test-branch-protection-rules
BRANCH=main

# add status check
echo '{"enable_status_check": true, "status_check_contexts": [ "'$REPO'/pipeline/pr-main"]}' \
    | curl -s -X 'PATCH' "$GITEA_API/repos/$REPO/branch_protections/$BRANCH" -H 'accept: application/json' -H 'Content-Type: application/json' -d @- \
    | jq '.status_check_contexts'

# UNCOMMENT THIS to empty status context array first
# echo '{"enable_status_check": true, "status_check_contexts": []}' | curl -s -X 'PATCH' "$GITEA_API/repos/$REPO/branch_protections/$BRANCH" -H 'accept: application/json' -H 'Content-Type: application/json' -d @- | jq '.status_check_contexts'

# remove status check
echo '{"enable_status_check": false, "status_check_contexts": []}' \
    | curl -s -X 'PATCH' "$GITEA_API/repos/$REPO/branch_protections/$BRANCH" -H 'accept: application/json' -H 'Content-Type: application/json' -d @- \
    | jq '.status_check_contexts'

# query status check
curl -s -X 'GET' "$GITEA_API/repos/$REPO/branch_protections/$BRANCH" -H 'accept: application/json' \
    | jq '.status_check_contexts'

Reproducer assumptions

  • There are login credentials for your try.gitea.io account in your ~/.git-credentials file, which have API access
  • Your username on try.gitea.io is the same as your $USER variable
  • You have a repo on try.gitea.io called test-branch-protection-rules
  • It has a main branch (just a template README is fine)
  • It has a branch protection rule for the main branch (defaults are fine)

Gitea Version

1.20.2, 1.21.0+dev-463-g6a7a5ea32 (the version of try.gitea.io at time of writing)

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

image

This screenshot is from loading the branch protection edit page after running the above reproducer. It shows the stale context string in the text area.

Git Version

No response

Operating System

No response

How are you running Gitea?

I was able to reproduce it using try.gitea.io.

Database

None

Infinoid added a commit to Infinoid/gitea that referenced this issue Aug 5, 2023
@lunny lunny added this to the 1.21.0 milestone Aug 24, 2023
@lunny lunny added type/enhancement An improvement of existing functionality modifies/api This PR adds API routes or modifies them and removed type/bug labels Aug 24, 2023
lunny pushed a commit that referenced this issue Aug 24, 2023
…led (#26351)

Fixes: #26333.

Previously, this endpoint only updates the `StatusCheckContexts` field
when `EnableStatusCheck==true`, which makes it impossible to clear the
array otherwise.

This patch uses slice `nil`-ness to decide whether to update the list of
checks. The field is ignored when either the client explicitly passes in
a null, or just omits the field from the json ([which causes
`json.Unmarshal` to leave the struct field
unchanged](https://go.dev/play/p/Z2XHOILuB1Q)). I think this is a better
measure of intent than whether the `EnableStatusCheck` flag was set,
because it matches the semantics of other field types.

Also adds a test case. I noticed that [`testAPIEditBranchProtection`
only checks the branch
name](https://github.com/go-gitea/gitea/blob/c1c83dbaec840871c1247f4bc3f875309b0de6bb/tests/integration/api_branch_test.go#L68)
and no other fields, so I added some extra `GET` calls and specific
checks to make sure the fields are changing properly.

I added those checks the existing integration test; is that the right
place for it?
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants