Skip to content

Project authorization is unique per user, project

Abdul Wadood requested to merge 218747-unique-project-auths into master

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

  1. 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]]
  1. git checkout this MR's branch
  2. 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.

Edited by Abdul Wadood

Merge request reports