Add a unique constraint to `software_licenses.name` column (!19840) · Merge requests · GitLab.org / GitLab

What does this MR do?

When we create a new software license policy, we attempt to do a safe_find_or_create_by here. This method depends on a unique constraint in the database. Today, the software_licenses.name column has an index but it is not unique.

This change adds a unique constraint to the software_licenses.name column. To accomplish this, I had to identify the oldest software license by id because the table lacks timestamp columns. Then I had to update any software_license_policies that refer to any duplicate software_licenses to refer to the oldest record.

Database

Data migration part isn't reversible

On gitlab.com we have

~800 software licenses
with 12 duplicates per name
2315 software_license_policies

Details

I don't have access to read data from production so I pulled the data that I could from chatops.

select name, count(name) from software_licenses group by name having count(name) > 1;
GroupAggregate  (cost=0.28..37.99 rows=783 width=34) (actual time=5.058..6.108 rows=12 loops=1)
  Group Key: name
  Filter: (count(name) > 1)
  Rows Removed by Filter: 774
  Buffers: shared hit=15 read=10
  I/O Timings: read=5.332
  ->  Index Only Scan using index_software_licenses_on_name on software_licenses  (cost=0.28..24.20 rows=795 width=26) (actual time=4.718..5.862 rows=798 loops=1)
        Heap Fetches: 29
        Buffers: shared hit=15 read=10
        I/O Timings: read=5.332
Planning time: 0.872 ms
Execution time: 6.167 ms

I would love to find out what the names of the 12 duplicates are. I would also want the creation time to see when these duplicates where produced but we currently don't have timestamp columns on this table.

Example run:

モ RAILS_ENV=test bin/rails db:migrate
== 20191108202722 RemoveNameIndexFromSoftwareLicenses: migrating ==============
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:software_licenses, :name, {:algorithm=>:concurrently})
   -> 0.0041s
-- execute("SET statement_timeout TO 0")
   -> 0.0004s
-- remove_index(:software_licenses, {:algorithm=>:concurrently, :column=>:name})
   -> 0.0094s
-- execute("RESET ALL")
   -> 0.0005s
== 20191108202722 RemoveNameIndexFromSoftwareLicenses: migrated (0.0146s) =====

== 20191108202723 AddUniqueConstraintToSoftwareLicenses: migrating ============
-- Detected 1 duplicates.
-- * MIT
-- Reassigning policies that reference software license MIT.
   -> 0.0675s
   -> 1 rows
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:software_licenses, :name, {:unique=>true, :algorithm=>:concurrently})
   -> 0.0020s
-- execute("SET statement_timeout TO 0")
   -> 0.0004s
-- add_index(:software_licenses, :name, {:unique=>true, :algorithm=>:concurrently})
   -> 0.0301s
-- execute("RESET ALL")
   -> 0.0004s
== 20191108202723 AddUniqueConstraintToSoftwareLicenses: migrated (0.0440s) ===

Rollback procedure

モ RAILS_ENV=test bin/rails db:migrate:down VERSION=20191108202723
== 20191108202723 AddUniqueConstraintToSoftwareLicenses: reverting ============
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:software_licenses, :name, {:unique=>true, :algorithm=>:concurrently})
   -> 0.0044s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- remove_index(:software_licenses, {:unique=>true, :algorithm=>:concurrently, :column=>:name})
   -> 0.0086s
-- execute("RESET ALL")
   -> 0.0005s
== 20191108202723 AddUniqueConstraintToSoftwareLicenses: reverted (0.0143s) ===

モ RAILS_ENV=test bin/rails db:migrate:down VERSION=20191108202722
== 20191108202722 RemoveNameIndexFromSoftwareLicenses: reverting ==============
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:software_licenses, :name, {:algorithm=>:concurrently})
   -> 0.0035s
-- execute("SET statement_timeout TO 0")
   -> 0.0003s
-- add_index(:software_licenses, :name, {:algorithm=>:concurrently})
   -> 0.0119s
-- execute("RESET ALL")
   -> 0.0004s
== 20191108202722 RemoveNameIndexFromSoftwareLicenses: reverted (0.0162s) =====

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Tested in all supported browsers

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

Label as security and @ mention @gitlab-com/gl-security/appsec
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
Security reports checked/validated by a reviewer from the AppSec team

Related to #35938 (closed)

Add a unique constraint to `software_licenses.name` column