Fix uptade protected branch

[bmsareias]

Adds missing elements for the full API functionality of UpdateProtectedBranch() method.

As consequence of this, it's mandatory by API to ship this data via body to the API, therefore I've added the case to the already existing switch-case.

This should have more extensive tests via the testing suit. I've only tested this for my own needs.

This PR fixes #1657

[svanharmelen]

Did you try to get it working without using the body (so with using query params instead)? Can you post the result/error when you tried that?

[svanharmelen]

FYI I'm asking because if you add http.MethodPatch to the list of calls that should use the request body instead of query params, you change this for all calls that currently use PATH requests.

So to avoid potential issues with other calls I prefer to not change this. Unless you can show that you tested all other impacted calls and they do not break...

[MUlt1mate]

This fix works for me as well. @bmsareias thank you

As for using query params - it doesn't work. It will generate RawQuery="allowed_to_merge={<nil> <nil> <nil> <nil> 0xc000032bb0 <nil>}&allowed_to_push={<nil> <nil> <nil> <nil> 0xc000032bb8 <nil>}"

[bmsareias]

Hey @svanharmelen,

Based on my (very narrowed tests) all the calls with any Update/Delete do fail with error "400 {error: allowed_to_[push, merge, unprotect] is invalid}" even thou they are correctly formatted as per API specs, furthermore a simple change from PATH to Body Requests fixes the issue all together with the exact same calls.

What I believe it's going on is: most of the calls to this endpoint work with PATH requests, however and since the 3 properties in question are array lists, I believe Gitlab doesn't allow them to be called via PATH, as it would be very easy for such a request to exhaust the PATH size limits for a very small amount of data pushed to the endpoint.

Yes, I do agree this is potentially dangerous to the overall stability of the entire LIB as this change provokes a design change to a large amount of endpoint calls, and given this i'm only seeing a very narrow group of solutions:

• have a "beta branch/tag" so people can test it more extensively
• a large test suit for all the calls (which should be the best course of action, but it's very time consuming)
• have an endpoint switch case for just this call or any particular ones that come in the future with such issues

Final note, i've created this PR to raise awareness of the fix/issue/solution that resulted from my own tests and needs, and with that i'm all open to suggestions and discuss better approaches to the fix/solution that better aligns to the community overall. With that said, if you see it more fit to mark this as WIP, just let me know :)

[bmsareias]

This fix works for me as well. @bmsareias thank you

As for using query params - it doesn't work. It will generate RawQuery="allowed_to_merge={<nil> <nil> <nil> <nil> 0xc000032bb0 <nil>}&allowed_to_push={<nil> <nil> <nil> <nil> 0xc000032bb8 <nil>}"

@MUlt1mate glad to hear! Yes from my tests all calls that want to UPDATE/DELETE (and to some extent CREATE) new entries on any of those 3 array/list/slices results in 400 - Invalid <Attribute_Name>.

[svanharmelen]

Thanks for the responses! Funny enough I don't have access to a GitLab instance myself, but looking through the code I see 3 other API's (implemented by this package) that currently use MethodPatch:

• https://docs.gitlab.com/ee/api/users.html#disable-two-factor-authentication
• https://docs.gitlab.com/ee/api/managed_licenses.html#edit-an-existing-managed-license
• https://docs.gitlab.com/ee/api/error_tracking.html#enable-or-disable-the-error-tracking-project-settings

If you can verify that these 3 calls also work when using the request body, then I am good to merge this PR...

[MUlt1mate]

The first method has no parameters, so it should be fine. I don't have administrator permissions to check it.
The second one, I tried but it doesn't work for me. I can't create license with api and project license is not listed in GET method. I don't know why. Also this method has body data in the documentation example, so looks good either way.
And the last one requires sentry instance, I couldn't check it as well.

[bmsareias]

The Users API only applies to Self-Managed Instances (which I don't have one accessible at this point)
The Managed License API is already marked as deprecated so I don't see any benefit in really testing it.
The Error Tracking I do not have access to a project that has this feature configured in it.

Can try to test the Users API if I manage to have the time over the weekend, and will ask around for a project that has the Error Tracking feature that I (or proxied through someone else) has maintainer access on it.

[svanharmelen]

OK... Well I guess in this case I'm good with moving this one forward and fixing things forward when it turns out one of those 3 APIs does end up having issues. Potentially not the most user friendly approach, but we are actually fixing a bug here so I guess in this case it can be justified.

[svanharmelen]

I'll take it... Thanks @bmsareias and @MUlt1mate 👍🏻