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:

  1. Declare the documented params on the forks and starred projects APIs: the two use lines, the regenerated OpenAPI v3 document, and specs that false, 0 and an empty value leave the field out and that a value that is not a boolean is answered with 400, on both routes, and that a custom_attributes value that is not a hash is answered with 400 on the starred projects route. It carries the Changelog: fixed trailer and describes the whole change, so it is the right message if the MR is squashed.
  2. Cover statistics on the list forks API: nothing tested statistics on this route. This pins that a Developer of a fork gets its statistics with statistics=true and 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.
  3. 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=true and not when the parameter is left out, that a user who is not an administrator does not get them, and that the custom_attributes filter 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.rb with lib/api/projects.rb from master, 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 the custom_attributes value that is not a hash). The forks examples fail on expect(json_response.first).not_to include('statistics'), the starred projects examples on expect(json_response).to all(exclude('custom_attributes')), and the three 400 examples on expected 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.rb on the branch: 925 examples, 0 failures, 63 pending (the pending ones are skips inside shared examples, such as namespace has no top-level group).
  • On commit 1: bundle exec rake gitlab:openapi:v3:generate leaves the tree unchanged, GITLAB_SIMULATE_SAAS=false bin/rake gitlab:openapi:v3:check_docs says the document is up to date, and bin/rake gitlab:openapi:v3:validate_tag_docs says the tag content is valid.
  • RuboCop on the Ruby files of each commit: no offenses. Both commit message linters (scripts/lint/commit_linter.rb and the Danger one) on each commit: no problems. git diff --check: clean.

How to set up and validate locally

  1. Fork any project you can read. You own the fork, so the source project now has a fork whose statistics you may see.

  2. 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 statistics object; with it, none does. statistics=true still includes it.

  3. As an administrator (with a token that has the admin_mode scope when Admin Mode is enabled), add a custom attribute to a project you starred (PUT /projects/<id>/custom_attributes/foo with value=bar), then request GET /api/v4/users/<your_id>/starred_projects?with_custom_attributes=false. Before this MR the response carries custom_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.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading