Declare the documented params on the forks and starred projects APIs
What does this MR do and why?
GET /projects/:id/forks documents a statistics boolean and GET /users/:user_id/starred_projects documents with_custom_attributes, but neither route declared the parameter. Grape passes an undeclared query parameter through as the raw string, and the helpers that read these two test the value for truthiness: apply_filters and present_projects pass params[:statistics] on to the entity, and with_custom_attributes reads params[:with_custom_attributes]. The strings "false", "0" and "" are all truthy in Ruby, so statistics=false listed every fork with its statistics, and with_custom_attributes=false listed the custom attributes of every starred project to an administrator. Only leaving the parameter out worked.
This MR declares statistics_params on the forks route and with_custom_attributes on the starred projects route, as GET /projects and GET /users/:user_id/projects already do. Both values are now booleans that default to false, so false, 0 and an empty value omit the field, true includes it as before, and a value that is not a boolean is answered with 400 (statistics is invalid, with_custom_attributes is invalid), which is what the other project lists answer. use :with_custom_attributes also declares the custom_attributes filter, which the starred projects route already read from the raw params through project_finder_params. A hash keeps working, and commit 3 pins that it still narrows the list for an administrator only; a value that is not a hash (custom_attributes=foo), which the route used to ignore, is now answered with 400 (custom_attributes is invalid), as on GET /projects. The regenerated doc/api/openapi/openapi_v3.yaml now lists statistics for the forks route and with_custom_attributes and custom_attributes for the starred projects route. The Markdown pages already described both parameters, so they are unchanged.
A status code change for the maintainer to confirm. The REST API style guide counts a change of any status code other than 500 as a breaking change. With this MR, three kinds of request that were answered with 200 are answered with 400, for any caller: a statistics value that is not a boolean on GET /projects/:id/forks (the 200 included statistics where the caller may see them), a with_custom_attributes value that is not a boolean on GET /users/:user_id/starred_projects (the 200 included custom_attributes for an administrator), and a custom_attributes value that is not a hash on the same route, such as custom_attributes=foo (the 200 ignored the filter). Grape 3.2.1, through dry-types 1.8.3, reads 1, t, true, y, yes and on as true and 0, f, false, n, no and off as false, each also in upper case or with a capital first letter (TRUE, True, T), and an empty value as the parameter left out; any other value, such as TrUe or 2, is the 400. Grape checks these parameters before the route body runs, so these values also get the 400 where the route answered 401 (an invalid token), 403 (a token whose scopes do not cover the route), 404 (a project or user that does not exist, or a project the caller cannot see) or, on the starred projects route, 429 (the rate limit). GET /projects and GET /users/:user_id/projects declare the same three parameters and already answer these values with the same 400, and so did these two routes for the parameters they declared before this MR, ahead of those other statuses too. The Markdown pages of the two routes document statistics and with_custom_attributes as booleans; custom_attributes is on neither page, and the GET /projects page documents it as a hash.
The commits:
- Declare the documented params on the forks and starred projects APIs: the two
uselines, the regenerated OpenAPI v3 document, and specs thatfalse,0and an empty value leave the field out and that a value that is not a boolean is answered with400, on both routes, and that acustom_attributesvalue that is not a hash is answered with400on the starred projects route. It carries theChangelog: fixedtrailer and describes the whole change, so it is the right message if the MR is squashed. - Cover statistics on the list forks API: nothing tested
statisticson this route. This pins that a Developer of a fork gets its statistics withstatistics=trueand not when the parameter is left out, and that a Guest of the fork does not get them even when asking. These pass with the parameter undeclared and with it declared. - Cover custom attributes on the starred projects API: nothing tested custom attributes on this route. This pins that an administrator gets them with
with_custom_attributes=trueand not when the parameter is left out, that a user who is not an administrator does not get them, and that thecustom_attributesfilter narrows the list for an administrator only. These pass with the parameters undeclared and with them declared.
I picked this up because my gitlab-mcp-server exposes the forks list as its project.list_forks action, which takes a statistics boolean.
Not in this MR: GET /users/:user_id/contributed_projects goes through the same present_projects but documents neither parameter, so it is left as it is. The custom_attributes filter is on the Markdown page of neither route (the forks route already declared it), and I have left the pages as they are. doc/api/openapi/openapi_v2.yaml is not regenerated: on this base, bin/rake gitlab:openapi:v2:generate rewrites about 12,800 lines of it, and all of them except the few that add statistics and with_custom_attributes are unrelated to this change. No CI job checks that file.
References
Closes #633318
Screenshots or screen recordings
Not a UI change. What the two lists answer, from the request specs of this MR run against master and against this branch:
| Request | Before | After |
|---|---|---|
Forks list, statistics=false, 0 or empty |
statistics included |
omitted |
Forks list, statistics=true |
included | included |
Forks list, statistics=not-a-boolean |
200, included |
400 statistics is invalid |
Starred projects, administrator, with_custom_attributes=false, 0 or empty |
custom_attributes included |
omitted |
Starred projects, administrator, with_custom_attributes=true |
included | included |
Starred projects, with_custom_attributes=not-a-boolean |
200 |
400 with_custom_attributes is invalid |
Starred projects, custom_attributes=foo (not a hash) |
200, filter ignored |
400 custom_attributes is invalid |
Checks I ran
All on top of master at 8538a2b2c17, in the test environment, with bundle exec rspec:
- The final
spec/requests/api/projects_spec.rbwithlib/api/projects.rbfrommaster, filtered to the forks and starred projects examples: 40 examples, 9 failures, all of them the new examples of commit 1 (the three false values and the non-boolean value on each route, and thecustom_attributesvalue that is not a hash). The forks examples fail onexpect(json_response.first).not_to include('statistics'), the starred projects examples onexpect(json_response).to all(exclude('custom_attributes')), and the three400examples onexpected the response to have status code :bad_request but it was 200. The examples of commits 2 and 3 pass there, which is what makes them characterization. - Each commit, filtered to the forks and starred projects examples: commit 1, 32 examples, 0 failures; commit 2, 35 examples, 0 failures; commit 3, 40 examples, 0 failures (3 pending in each, skips inside the granular token shared examples such as
namespace has no top-level group). - The whole
spec/requests/api/projects_spec.rbon the branch: 925 examples, 0 failures, 63 pending (the pending ones are skips inside shared examples, such asnamespace has no top-level group). - On commit 1:
bundle exec rake gitlab:openapi:v3:generateleaves the tree unchanged,GITLAB_SIMULATE_SAAS=false bin/rake gitlab:openapi:v3:check_docssays the document is up to date, andbin/rake gitlab:openapi:v3:validate_tag_docssays the tag content is valid. - RuboCop on the Ruby files of each commit: no offenses. Both commit message linters (
scripts/lint/commit_linter.rband the Danger one) on each commit: no problems.git diff --check: clean.
How to set up and validate locally
-
Fork any project you can read. You own the fork, so the source project now has a fork whose statistics you may see.
-
Request the forks list with
statistics=false:curl --header "PRIVATE-TOKEN: <your_access_token>" \ --url "http://gdk.test:3000/api/v4/projects/<id>/forks?statistics=false"Before this MR every fork carries a
statisticsobject; with it, none does.statistics=truestill includes it. -
As an administrator (with a token that has the
admin_modescope when Admin Mode is enabled), add a custom attribute to a project you starred (PUT /projects/<id>/custom_attributes/foowithvalue=bar), then requestGET /api/v4/users/<your_id>/starred_projects?with_custom_attributes=false. Before this MR the response carriescustom_attributes; with it, it does not.
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.