Project authorization is unique per user, project
What does this MR do and why?
Closes https://gitlab.com/gitlab-org/gitlab/-/issues/218747
This MR is a duplicate of !67936 (closed) which was quite outdated so I created this one with this review suggestion.
Description from !67936 (closed):
Currently the code allows duplicate <user,project,access_level> triples in ProjectAuthorization
records. This is invalid, and too loose which results in more complex querying than necessary.
This MR tightens the uniqueness validation to only allow <user,project> pairs in ProjectAuthorization
.
Duplicate records in production were removed a year ago.
I believe this query plan (Old query plan) says we don't have any duplicates today.
Migration
Dump
rails db:migrate:up VERSION=20210812013042
== 20210812013042 RemoveDuplicateProjectAuthorizations: migrating =============
WARNING: Active Record does not support composite primary key.
project_authorizations has composite primary key. Composite primary key is ignored.
-- transaction_open?()
-> 0.0000s
-- index_exists?(:project_authorizations, [:project_id, :user_id], {:unique=>true, :name=>"index_unique_project_authorizations_on_project_id_user_id", :algorithm=>:concurrently})
-> 0.0009s
-- execute("SET statement_timeout TO 0")
-> 0.0002s
-- add_index(:project_authorizations, [:project_id, :user_id], {:unique=>true, :name=>"index_unique_project_authorizations_on_project_id_user_id", :algorithm=>:concurrently})
-> 0.0014s
-- execute("RESET statement_timeout")
-> 0.0002s
-- transaction_open?()
-> 0.0000s
-- indexes(:project_authorizations)
-> 0.0007s
-- remove_index(:project_authorizations, {:algorithm=>:concurrently, :name=>"index_project_authorizations_on_project_id_user_id"})
-> 0.0010s
== 20210812013042 RemoveDuplicateProjectAuthorizations: migrated (0.0128s) ====
== 20210812013042 RemoveDuplicateProjectAuthorizations: reverting =============
-- transaction_open?()
-> 0.0000s
-- index_exists?(:project_authorizations, [:project_id, :user_id], {:name=>"index_project_authorizations_on_project_id_user_id", :algorithm=>:concurrently})
-> 0.0012s
-- execute("SET statement_timeout TO 0")
-> 0.0002s
-- add_index(:project_authorizations, [:project_id, :user_id], {:name=>"index_project_authorizations_on_project_id_user_id", :algorithm=>:concurrently})
-> 0.0026s
-- execute("RESET statement_timeout")
-> 0.0003s
-- transaction_open?()
-> 0.0000s
-- indexes(:project_authorizations)
-> 0.0009s
-- remove_index(:project_authorizations, {:algorithm=>:concurrently, :name=>"index_unique_project_authorizations_on_project_id_user_id"})
-> 0.0019s
== 20210812013042 RemoveDuplicateProjectAuthorizations: reverted (0.0113s) ====
How to set up and validate locally
- Add a duplicate entry to
project_authorizations
table:
pa = ProjectAuthorization.new(user_id: 1, project_id: 1, access_level: 40)
pa.save(validate: false)
q = %Q{SELECT user_id, project_id, COUNT(*)
FROM project_authorizations
GROUP BY user_id, project_id
HAVING COUNT(*) > 1;}
ActiveRecord::Base.connection.execute(q).values
>[[1, 1, 2]]
-
git checkout
this MR's branch - Run the migration and the duplicate pairs of
user_id
,project_id
will be removed and the row with the max access level will be kept.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.