Validate the topics a project save would create

What does this MR do and why?

A project save creates the topics it names that do not exist yet, in an after_save callback. A topic that fails its own validation there aborts the save with no error on the project. On master:

  • PUT /projects/:id with topics=bx::code::c++, in an organization that has bx::code::c (both give the slug bxcodec), answers 422 {"message":"Project could not be updated!"} and changes nothing. A name over 255 characters or not in ASCII gets the same answer.
  • POST /projects with that topic answers 201 and creates the project without it.
  • Two names that differ only in case, such as ruby, Ruby, also get the 422, because the one topic is assigned twice.

This MR checks the topics a save would create before saving. Each one that would fail adds an error on topics that names it, so nothing is written and both endpoints answer 400 with {"message":{"topics":["'bx::code::c++' is invalid: Slug has already been taken"]}}. Names that differ only in case count as one topic, keeping the first spelling.

The check runs only when a topic list is assigned and only for topics that do not exist yet. Topics are looked up with lower(name) = lower(?), so PostgreSQL lowercases both sides as the uniqueness validator does (Ruby lowercases U+0130 differently), and a name the validator finds taken is reused. The code is in a new Projects::TopicList, as Gitlab/OmniscientClassMethods keeps new methods out of Project.

A regular migration adds an index on topics (organization_id, lower(slug)) WHERE slug IS NOT NULL for the slug uniqueness check each new topic runs. Two status codes a client sees also change; details below.

References

Closes #548609

Related to #632634: the same 422 for tier:1, whose slug is that of the topic tier1.

Follow-ups from the database review: #638931 and #638932.

I maintain https://github.com/jmrplens/gitlab-mcp-server, whose project tools send topics to both endpoints.

Screenshots or screen recordings

None: the alert on the project settings page now names the topic.

How to set up and validate locally

  1. Give a project the topic bx::code::c.

  2. Update another project of the same organization:

    curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" \
      --url "https://gitlab.example.com/api/v4/projects/<project_id>" --data-urlencode "topics=bx::code::c++"
  3. On master: 422 with Project could not be updated!. With this MR: 400 naming the topic, and the project is unchanged.

Queries and plans

Plans from a local PostgreSQL 17.11 database, not Database Lab: seeded with 505,000 topics in organization 1 (mixed-case names, 1% without a slug) and 5,000 in another, after VACUUM ANALYZE, each query run once before its plan. The SQL is what Rails sent for a new project of organization 1. A database reviewer is welcome to rerun them in Database Lab. The Database Lab plans on gitlab-production-main from !266664 (comment 4194537109) are linked under each query; those of the two lookups are for their earlier version, which lowercased the names in Ruby.

A save that assigns topics looks the existing ones up in one query, lowercasing each name in PostgreSQL, here with two names that exist:

SELECT DISTINCT lower(name) FROM "topics" WHERE "topics"."organization_id" = 1 AND LOWER("topics"."name") IN (LOWER('C4ca42Lib1'), LOWER('E4DA-5')) LIMIT 2
Limit  (cost=0.42..14.65 rows=2 width=32) (actual time=0.051..0.067 rows=2 loops=1)
  Buffers: shared hit=7
  ->  Unique  (cost=0.42..14.65 rows=2 width=32) (actual time=0.050..0.065 rows=2 loops=1)
        Buffers: shared hit=7
        ->  Index Scan using index_topics_on_org_id_and_lower_name_and_projects_count on topics  (cost=0.42..14.64 rows=2 width=32) (actual time=0.049..0.062 rows=2 loops=1)
              Index Cond: ((organization_id = 1) AND (lower(name) = ANY ('{c4ca42lib1,e4da-5}'::text[])))
              Buffers: shared hit=7
Planning Time: 0.215 ms
Execution Time: 0.093 ms

The same with ten names, five of them new (LIMIT 10): the same index scan, rows=5, Buffers: shared hit=19, Execution Time: 0.205 ms.

Database Lab, earlier version of this SQL: 2 names, 10 names.

Each topic that does not exist yet is then validated, which runs the two uniqueness checks of Projects::Topic, unchanged by this MR (plans of 2026-11-06, same database). Name:

SELECT 1 AS one FROM "topics" WHERE LOWER("topics"."name") = LOWER('newtopic') AND "topics"."organization_id" = 1 LIMIT 1
Limit  (cost=0.42..8.44 rows=1 width=4) (actual time=0.028..0.028 rows=0 loops=1)
  Buffers: shared hit=3
  ->  Index Scan using index_topics_on_org_id_and_lower_name_and_projects_count on topics  (cost=0.42..8.44 rows=1 width=4) (actual time=0.026..0.027 rows=0 loops=1)
        Index Cond: ((organization_id = 1) AND (lower(name) = 'newtopic'::text))
        Buffers: shared hit=3
Planning Time: 0.111 ms
Execution Time: 0.042 ms

Database Lab: plan.

Slug, with the new index:

SELECT 1 AS one FROM "topics" WHERE LOWER("topics"."slug") = LOWER('newtopic') AND "topics"."organization_id" = 1 LIMIT 1
Limit  (cost=0.42..2.01 rows=1 width=4) (actual time=0.018..0.018 rows=0 loops=1)
  Buffers: shared hit=3
  ->  Index Scan using index_topics_on_organization_id_and_lower_slug on topics  (cost=0.42..4006.67 rows=2525 width=4) (actual time=0.017..0.017 rows=0 loops=1)
        Index Cond: ((organization_id = 1) AND (lower(slug) = 'newtopic'::text))
        Buffers: shared hit=3
Planning Time: 0.120 ms
Execution Time: 0.033 ms

Database Lab: plan.

The same without the new index, dropped in a transaction that was rolled back:

Limit  (cost=0.00..7.08 rows=1 width=4) (actual time=398.776..398.777 rows=0 loops=1)
  Buffers: shared hit=129 read=8813
  I/O Timings: shared read=16.882
  ->  Seq Scan on topics  (cost=0.00..17867.00 rows=2525 width=4) (actual time=398.774..398.774 rows=0 loops=1)
        Filter: ((organization_id = 1) AND (lower(slug) = 'newtopic'::text))
        Rows Removed by Filter: 510000
        Buffers: shared hit=129 read=8813
        I/O Timings: shared read=16.882
Planning Time: 0.180 ms
Execution Time: 398.800 ms

Database Lab: plan.

The two uniqueness checks return no rows because they test a name that does not exist yet.

After the save, Projects::TopicList#topics looks each name up again, as master does but lowercasing the value in PostgreSQL, and creates the ones that do not exist with first_or_create, which runs the two checks above again before the insert. A name that exists:

SELECT "topics".* FROM "topics" WHERE "topics"."organization_id" = 1 AND (lower(name) = lower('C4ca42Lib1')) ORDER BY "topics"."total_projects_count" DESC LIMIT 1
Limit  (cost=0.42..8.44 rows=1 width=193) (actual time=0.027..0.028 rows=1 loops=1)
  Buffers: shared hit=4
  ->  Index Scan using index_topics_on_org_id_and_lower_name_and_projects_count on topics  (cost=0.42..8.44 rows=1 width=193) (actual time=0.026..0.026 rows=1 loops=1)
        Index Cond: ((organization_id = 1) AND (lower(name) = 'c4ca42lib1'::text))
        Buffers: shared hit=4
Planning:
  Buffers: shared hit=26
Planning Time: 0.325 ms
Execution Time: 0.056 ms

With newtopic, which does not exist: the same index scan, rows=0, Buffers: shared hit=3, Execution Time: 0.056 ms.

Database Lab, earlier version of this SQL: plan.

Migration

A regular migration, as topics is a small table (db/docs/topics.yml) and the index serves a check that runs on every save that creates a topic. It ran up, down and up again on a local development database; db:migrate:down and db:migrate at 613adaf7, on the seeded data (the database maintainer review removed only two comments from the migration since):

main: == 20261106141912 AddIndexTopicsOnOrganizationIdAndLowerSlug: reverting =======
main: -- transaction_open?(nil)
main:    -> 0.0001s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0205s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0025s
main: -- index_name_exists?(:topics, "index_topics_on_organization_id_and_lower_slug")
main:    -> 0.0081s
main: -- remove_index(:topics, {:algorithm=>:concurrently, :name=>"index_topics_on_organization_id_and_lower_slug"})
main:    -> 0.0168s
main: == 20261106141912 AddIndexTopicsOnOrganizationIdAndLowerSlug: reverted (0.1926s)
main: == 20261106141912 AddIndexTopicsOnOrganizationIdAndLowerSlug: migrating =======
main: -- transaction_open?(nil)
main:    -> 0.0001s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0224s
main: -- index_exists?(:topics, "organization_id, lower(slug)", {:where=>"slug IS NOT NULL", :name=>"index_topics_on_organization_id_and_lower_slug", :algorithm=>:concurrently})
main:    -> 0.0183s
main: -- add_index(:topics, "organization_id, lower(slug)", {:where=>"slug IS NOT NULL", :name=>"index_topics_on_organization_id_and_lower_slug", :algorithm=>:concurrently})
main:    -> 2.0840s
main: == 20261106141912 AddIndexTopicsOnOrganizationIdAndLowerSlug: migrated (2.2118s)

Down removes the index line from db/structure.sql and deletes db/schema_migrations/20261106141912; up writes both back exactly as committed. The milestone is 19.6, as VERSION reads.

Status codes and spec runs

Status codes: PUT /projects/:id already answers 400 with the validation errors whenever the project carries any, and keeps its 422 for a save that failed without one, which is what happened here. POST /projects answered 201 while dropping a topic it was given, and now refuses it like any other invalid attribute.

Spec runs, locally on Ruby 3.3.11 and PostgreSQL 17, on master at 46d26016:

  • With app/models/project.rb as on master and the new specs in place, the topic examples of the four changed spec files: 81 examples, 8 failures, the eight examples this MR adds or changes. Requests on that code: a slug collision, a collision inside one list, a name over 255 characters, a name that is not ASCII and two names that differ only in case all got 422 Project could not be updated!, and POST /projects got 201 without the topic.
  • With the change, the same plus spec/models/projects/topic_list_spec.rb, spec/models/projects/topic_spec.rb and the topic examples of the create service spec: 136 examples, 0 failures. The same requests got 400 naming the topic, and the two case variants were saved as one topic.
  • RuboCop on the changed Ruby files and the commit linter: no offenses.

After the database review, on 7dfb1696, same setup:

  • spec/models/projects/topic_list_spec.rb with the earlier topic_list.rb and project.rb: 16 examples, 2 failures, the two new ones: the U+0130 topic fails the name and ASCII checks as described in the review, and a save of a list that keeps it fails.
  • With the change, the topic examples of topic_list_spec.rb, topic_spec.rb, project_spec.rb and the update and create service specs: 107 examples, 0 failures.
  • RuboCop on the changed Ruby files and the commit linter on the three commits: no offenses.

After the database maintainer review, on 7dfb1696, same setup:

  • spec/models/projects/topic_list_spec.rb with the new examples and the earlier code: 17 examples, 2 failures, the two new ones: with a legacy U+0130 topic elsewhere in the organization, the check reported it as Name has already been taken, and assigning it to the project failed.
  • With the change: 0 failures there. Without the skip on Name has already been taken, 3 failures; with Ruby's downcase back in #topics, 2 failures.
  • With the change, the topic examples of topic_list_spec.rb, topic_spec.rb, project_spec.rb, the update and create service specs and spec/requests/api/projects_spec.rb: 132 examples, 0 failures.
  • RuboCop on the changed Ruby files and the commit linter on the four commits: no offenses.

Left to the pipeline: the whole spec files, the controller specs (the application CSS is not built in my setup), Ruby 3.4, the FOSS run and the gettext check.

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