Return a 400 when a board name fails validation in the boards API

What does this MR do and why?

Board validates a changed name at 255 characters at most (app/models/board.rb:14), and nothing else checks it: the routes declare name a plain String, the column is an unbounded character varying, and neither API page states a limit. A longer name therefore fails only in the model, and every REST route that sets it answers as if the write had been saved. An empty name, which the routes accept as a String too, fails the model's presence check and is answered the same way. This MR makes those routes answer 400 Bad Request with the model errors, and saves nothing, as before.

  • PUT /projects/:id/boards/:board_id and PUT /groups/:id/boards/:board_id run update_board, which hands board to Boards::UpdateService#execute (a board.update, which returns false and saves nothing), then asks board.valid? and presents board. But the board helper is board_parent.boards.find(params[:board_id]), not memoized, so each call loads a new record: the check and the answer both read the stored board, which is valid because its name did not change. The route answers 200 OK with the board as it was, and every other attribute of the request is dropped with the name, since the update is one save. The bad_request!("Failed to save board ...") branch is reached by nothing the route accepts.
  • POST /projects/:id/boards and POST /groups/:id/boards run create_board, which presents response.payload[:board] whatever the response says. Boards::CreateService#create_board! returns ServiceResponse.error with the unsaved board in its payload when the create did not persist it, so the route answers 201 Created with a board whose id is null, and nothing is created.

The change, in lib/api/boards_responses.rb:

  • The board helper is memoized (@board ||=, as lib/api/freeze_periods.rb:147 does for its record), so update_board presents and reports on the record the service updated. update_board now branches on the result of the update rather than validating the record a second time.
  • Both helpers answer a failed write with render_validation_error!, so the body is {"message":{"name":["is too long (maximum is 255 characters)"]}}, the shape other API routes use for a model that does not validate. The update route used to build its message by interpolating board.errors.messages into a string, whose text depends on the Ruby version's Hash#inspect. That branch ran only when the board as stored failed validation, never because of anything a request sent, so I do not expect a client to depend on its wording.
  • render_validation_error! renders nothing for a model without errors, so each helper follows it with bad_request!: create_board with the service's message and update_board with Failed to save board, the words the update route's old message started with. Nothing in the current code fails a board write without errors (the service's error without a board sits behind the forbidden! guard, and Board has no callback that aborts a save), so this only keeps a later change from answering such a failure with a 201 or with a 200 whose body is null. The automated review on this MR suggested it.

In doc/api/boards.md and doc/api/group_boards.md, the name rows of the create and update sections now state the 255-character limit.

On the status code: the API style guide counts a change to any status code other than 500 as a breaking change, so I want to say why I treat this one as a bug fix. The REST API documents a validation failure as a 400 whose message maps each attribute to its errors (Status code 400, whose example is a bio that is too long (maximum is 255 characters)), and documents 201 Created as the answer to a POST whose resource was created. Only a request that GitLab did not save gets the new answer, so a client that relied on the 200 or the 201 was relying on a write that never happened, and the update route already had a 400 branch written for this case that it could not reach. If you read the policy differently for this case, I am happy to follow your lead.

The GraphQL mutations already report both failures in their errors field: Mutations::Boards::Update returns the errors of the record it updated, so the name error itself (app/graphql/mutations/boards/update.rb:34), and Mutations::Boards::Create returns the service's message, There was an error when creating a board., with a null board (app/graphql/mutations/boards/create.rb:29-30). This MR makes the REST routes report the failure too, with the model errors on both.

Where the code is, on master at fa825b69
  • lib/api/boards_responses.rb:9-31: the board, create_board and update_board helpers, which lib/api/boards.rb, lib/api/group_boards.rb and ee/lib/ee/api/group_boards.rb include. The routes: lib/api/boards.rb:63 (project create), lib/api/boards.rb:79 (project update), lib/api/group_boards.rb:64 (group update), ee/lib/ee/api/group_boards.rb:39 (group create).
  • app/models/project.rb:242 and app/models/group.rb:111: has_many :boards, so each find in the helper loads a new record.
  • app/services/boards/update_service.rb:9: board.update(params), whose result the helper discarded.
  • app/services/boards/create_service.rb:23-28: parent_board_collection.create(params), and the ServiceResponse.error carrying the unsaved board.
  • app/models/board.rb:14: validates :name, presence: true, length: { maximum: 255, if: :name_changed? }.

References

I searched the issues and merge requests of this project for the routes, the helpers and the error message and found nothing about this.

Screenshots or screen recordings

A request with a 256-character name, as an API response:

Before After
PUT .../boards/:board_id: 200 OK with the board as stored, other attributes of the request dropped 400 Bad Request, {"message":{"name":["is too long (maximum is 255 characters)"]}}
POST .../boards: 201 Created with "id": null, no board created 400 Bad Request, {"message":{"name":["is too long (maximum is 255 characters)"]}}

An empty name gets the same answers as the 256-character one, before and after, with {"message":{"name":["can't be blank"]}} as the body after.

How to set up and validate locally

  1. As a user who can manage the boards of a project, with a personal access token that has the api scope, create a board:
    curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" \
      --data "name=Planning" "http://gdk.test:3000/api/v4/projects/<project_id>/boards"
  2. Rename it to a 256-character name, and hide its Open list in the same request:
    NAME=$(printf 'a%.0s' $(seq 256))
    curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" \
      --data "name=$NAME&hide_backlog_list=true" "http://gdk.test:3000/api/v4/projects/<project_id>/boards/<board_id>"
    On master this answers 200 OK with the board still named Planning and hide_backlog_list still false. On this branch it answers 400 Bad Request with the validation error, and the board is unchanged as before.
  3. Create a board with the same name:
    curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" \
      --data "name=$NAME" "http://gdk.test:3000/api/v4/projects/<project_id>/boards"
    On master this answers 201 Created with "id": null, and GET /projects/<project_id>/boards lists no new board. On this branch it answers 400 Bad Request.
  4. An empty name (--data "name=") gets the same answers as the two requests above, with can't be blank on this branch.
  5. The group routes behave the same: PUT /groups/<group_id>/boards/<board_id>, and POST /groups/<group_id>/boards with a license that includes multiple group issue boards.
  6. Run the specs:
    bin/rspec spec/requests/api/boards_spec.rb spec/requests/api/group_boards_spec.rb \
      ee/spec/requests/api/boards_spec.rb ee/spec/requests/api/group_boards_spec.rb
Specs and checks I ran
  • The new examples, each with :aggregate_failures: in the shared group and project boards examples for the update routes, which spec/requests/api/boards_spec.rb, spec/requests/api/group_boards_spec.rb and ee/spec/requests/api/group_boards_spec.rb each run, a table over a 256-character name and an empty one, plus an example that stubs Boards::UpdateService#execute to return false; and the same table and a stub of Boards::CreateService#execute for project create in spec/requests/api/boards_spec.rb and group create in ee/spec/requests/api/group_boards_spec.rb. Each table row expects 400, the validation error, and nothing saved (the update rows also send hide_backlog_list: true and check it was not saved). The stubbed examples expect a 400 with Failed to save board on update and with the service's message on create.
  • With master's lib/api/boards_responses.rb and the new specs, the four spec files above give 296 examples, 15 failures, 10 pending. The fifteen failures are the new examples: the nine update examples get 200 with the stored board, and the six create examples get 201 with "id": null.
  • With the first commit's lib/api/boards_responses.rb, which has no fallback, the 10 table rows pass and the 5 stubbed examples fail: the three update ones get 200 with null as the body, and the two create ones 201 with "id": null.
  • With the change, the same four files: 296 examples, 0 failures, 10 pending. The pending examples are the existing when namespace enforces granular tokens cases skipped with "namespace has no top-level group", the same 10 as with master's code.
  • RuboCop on the four changed Ruby files: no offenses. scripts/lint/commit_linter.rb on both commits: no problems. markdownlint-cli2, Vale and scripts/lint-doc.sh, in the image the docs-lint markdown job uses, report nothing on the two pages.
Where this comes from

I maintain gitlab-mcp-server, an MCP server that exposes the GitLab API to AI assistants. While reviewing a hint its group board update tool gave for a 400 naming the name length, I followed that 400 back through update_board and found GitLab never sends it, and that create_board beside it presents the unsaved board the same way. Its board tools report what GitLab answers, so until this lands a caller is told the board was created or renamed when nothing was saved. The project keeps a record of what it finds in its dependencies in upstream-bugs.md, and this MR is the change that entry proposes.

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