Backend changes for duplicate token feature:
project field to GranularScopeType to resolve the actual Project
from a Namespaces::ProjectNamespace, enabling correct prefill of project scopesid field to PersonalAccessTokensResolver
by_id filter to PersonalAccessTokensFinder
Issue: #591665
| Before | After |
|---|---|
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Would there be a performance problem if the scope was used in the future with an array of ID's?
@alexbuijs Only if it's very large. Since this seems like it's internal-only, correct me if I am wrong here, but the graphql API will ensure token_id is an integer (not an array).
@alexbuijs Small non-blocking suggestion. Let me know what you think.
suggestion: The id argument is defined as a singular GlobalIDType[::PersonalAccessToken] (not an array), so only a single ID can ever be passed through the GraphQL API. The generated query will be a simple primary key equality lookup, so there's no risk of an unbounded IN (...) clause here.
That said, the by_token_id method in the finder itself doesn't enforce this constraint at the Ruby level. If a future caller were to pass an array via PersonalAccessTokensFinder.new(token_id: [1, 2, 3, ...]), id_in would accept it. This is a minor hardening consideration rather than a blocking concern.
token_id = Array(params[:token_id]).first
return tokens unless token_id
tokens.id_in(token_id)Table "public.pool_repositories"
Column | Type | Collation | Nullable | Default
-------------------+-------------------+-----------+----------+-----------------------------------------------
id | bigint | | not null | nextval('pool_repositories_id_seq'::regclass)
shard_id | integer | | not null |
disk_path | character varying | | |
state | character varying | | |
source_project_id | integer | | |
organization_id | bigint | | |
Indexes:
"pool_repositories_pkey" PRIMARY KEY, btree (id)
"index_pool_repositories_on_organization_id" btree (organization_id)
"index_pool_repositories_on_shard_id" btree (shard_id)
"index_pool_repositories_on_source_project_id_and_shard_id" UNIQUE, btree (source_project_id, shard_id)
"unique_pool_repositories_on_disk_path_and_shard_id" UNIQUE, btree (disk_path, shard_id)
Check constraints:
"check_96233d37c0" CHECK (organization_id IS NOT NULL)
Foreign-key constraints:
"fk_775c554d89" FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE NOT VALID
"fk_rails_af3f8c5d62" FOREIGN KEY (shard_id) REFERENCES shards(id) ON DELETE RESTRICT
Triggers:
pool_repositories_loose_fk_trigger AFTER DELETE ON pool_repositories REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records()Mark pool_repositories as sharded
Follow up to #490484
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Tomasz Skorupa (3dcb62cc) at 17 Mar 15:45
Mark pool_repositories as sharded
@alexbuijs I might be mistaken, but this MR does not seem as if it has been looked at by a database reviewer. Let's have @knejad review it first (chosen through the review roulette).
@skundapur I made some changes based on your feedback. Could you have another look?
Tomasz Skorupa (0d033924) at 16 Mar 22:48
Fix merge order so explicit routing keys override defaults
Tomasz Skorupa (a27938cd) at 16 Mar 19:37
Roll ROUTING_PAYLOAD_REGEX tests into functional tests
Tomasz Skorupa (275a14dd) at 16 Mar 18:50
Update milestones from 18.10 to 18.11
... and 5 more commits
I won't be able to get to this MR in time. Using the reviewer-roulette to find another reviewer, @missy-gitlab could you do the backend review?
Tomasz Skorupa (67dd5bcb) at 16 Mar 17:57
Update milestones from 18.10 to 18.11
... and 690 more commits
Tomasz Skorupa (d4644fd0) at 16 Mar 17:54
Document when FK constraints on sharding key columns can be omitted