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_idandPUT /groups/:id/boards/:board_idrunupdate_board, which handsboardtoBoards::UpdateService#execute(aboard.update, which returns false and saves nothing), then asksboard.valid?and presentsboard. But theboardhelper isboard_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 answers200 OKwith the board as it was, and every other attribute of the request is dropped with the name, since the update is one save. Thebad_request!("Failed to save board ...")branch is reached by nothing the route accepts.POST /projects/:id/boardsandPOST /groups/:id/boardsruncreate_board, which presentsresponse.payload[:board]whatever the response says.Boards::CreateService#create_board!returnsServiceResponse.errorwith the unsaved board in its payload when thecreatedid not persist it, so the route answers201 Createdwith a board whoseidisnull, and nothing is created.
The change, in lib/api/boards_responses.rb:
- The
boardhelper is memoized (@board ||=, aslib/api/freeze_periods.rb:147does for its record), soupdate_boardpresents and reports on the record the service updated.update_boardnow 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 interpolatingboard.errors.messagesinto a string, whose text depends on the Ruby version'sHash#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 withbad_request!:create_boardwith the service's message andupdate_boardwithFailed 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 theforbidden!guard, andBoardhas no callback that aborts a save), so this only keeps a later change from answering such a failure with a201or with a200whose body isnull. 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: theboard,create_boardandupdate_boardhelpers, whichlib/api/boards.rb,lib/api/group_boards.rbandee/lib/ee/api/group_boards.rbinclude. 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:242andapp/models/group.rb:111:has_many :boards, so eachfindin 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 theServiceResponse.errorcarrying 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
- As a user who can manage the boards of a project, with a personal access token that has the
apiscope, 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" - Rename it to a 256-character name, and hide its Open list in the same request:
On master this answers
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>"200 OKwith the board still namedPlanningandhide_backlog_liststillfalse. On this branch it answers400 Bad Requestwith the validation error, and the board is unchanged as before. - Create a board with the same name:
On master this answers
curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" \ --data "name=$NAME" "http://gdk.test:3000/api/v4/projects/<project_id>/boards"201 Createdwith"id": null, andGET /projects/<project_id>/boardslists no new board. On this branch it answers400 Bad Request. - An empty name (
--data "name=") gets the same answers as the two requests above, withcan't be blankon this branch. - The group routes behave the same:
PUT /groups/<group_id>/boards/<board_id>, andPOST /groups/<group_id>/boardswith a license that includes multiple group issue boards. - 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 sharedgroup and project boardsexamples for the update routes, whichspec/requests/api/boards_spec.rb,spec/requests/api/group_boards_spec.rbandee/spec/requests/api/group_boards_spec.rbeach run, a table over a 256-character name and an empty one, plus an example that stubsBoards::UpdateService#executeto returnfalse; and the same table and a stub ofBoards::CreateService#executefor project create inspec/requests/api/boards_spec.rband group create inee/spec/requests/api/group_boards_spec.rb. Each table row expects400, the validation error, and nothing saved (the update rows also sendhide_backlog_list: trueand check it was not saved). The stubbed examples expect a400withFailed to save boardon update and with the service's message on create. - With master's
lib/api/boards_responses.rband 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 get200with the stored board, and the six create examples get201with"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 get200withnullas the body, and the two create ones201with"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 tokenscases 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.rbon both commits: no problems.markdownlint-cli2, Vale andscripts/lint-doc.sh, in the image thedocs-lint markdownjob 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.