Skip to content

Add migration to update deployment_approvals index

Hunter Stewart requested to merge hustewart-dep-app-indexes into master

Issue: Database - replace deployment approvals user db... (#442601 - closed)

Related Bug: Multiple approvers for protected environments g... (#425759 - closed)

Why

This is a part of 3 backend MRs that will allow users to approve for multiple groups, instead of just once. Also they will be able to choose which group want to approve as. Currently representedAs somewhat supports this but after one approval, the field isn't respected and just returns the first approval.

The MRs should be merged in order of:

This will provide an initial work around for the problem in Multiple approvers for protected environments g... (#425759 - closed) as they users be able to use the GraphQL API for DeplomentApproval plus representedAs to approve more than once if they belong to multiple groups.

This MR

This is the database MR.

This MR adds a migration to update deployment_approvals index.

We want to include approval_rule_id in the unique index so that users are able to approve more than once if they are in more than one group.

For backend reviewer

I'm looking for a domain expert and to review this MR as well as the next 2 once they are ready.

For this MR:

We want to allow a user to approve a deployment more than once, in case they are part of more than one group required for approval. That's why we want to drop the unique constraint. That said, it still makes sense to have a uniqueness constraint that says "a user can only approve once per group"

I think this index change is the right approach.

How I tested locally:

  • Follow the steps in the original issue.

  • Verify that the model still validates uniqueness.

Test with existing model validation and new database index from this MR

[11] pry(main)> deployment.approvals.create(user: current_user, status: 'approved', comment: 'command line')

# it will check existence and roll the transaction back based on user and deployment (not approval rule), since the model validation still exists.

Test future change of model MR

To test the change in business logic this migration will provide, we need to temporarily update the model code using the diff in the link above so that our model code now validates uniqueness using approval_rule as well. There will be unit tests for this in the model update MR but I wanted to check here to make sure the migration was doing the business logic we want.

After temporarily changin the code, try creating the approval using the same approval rule:

(don't forget to restart rails console, or reload objects and make sure validators are updated on them)

[14] pry(main)> deployment.approvals.create(user: current_user, status: 'approved', comment: 'command line', approval_rule: approval_rule)

The model is checking the approval rule, and so it rolls it back, which is what we want.

If you use other rules, the user should be able to approve all 3 times:

[81] pry(main)> deployment.approvals.all
  Deployments::Approval Load (1.0ms)  SELECT "deployment_approvals".* FROM "deployment_approvals" WHERE "deployment_approvals"."deployment_id" = 1999 
=> [#<Deployments::Approval:0x0000000129610f10
  id: 7,
  deployment_id: 1999,
  user_id: 1,
  created_at: Fri, 23 Feb 2024 13:30:34.934207000 UTC +00:00,
  updated_at: Fri, 23 Feb 2024 13:30:34.934207000 UTC +00:00,
  status: "approved",
  comment: "Approval comment",
  approval_rule_id: 6>,
 #<Deployments::Approval:0x0000000129610dd0
  id: 13,
  deployment_id: 1999,
  user_id: 1,
  created_at: Fri, 23 Feb 2024 20:11:44.611331000 UTC +00:00,
  updated_at: Fri, 23 Feb 2024 20:11:44.611331000 UTC +00:00,
  status: "approved",
  comment: "command line",
  approval_rule_id: 7>,
 #<Deployments::Approval:0x0000000129610c90
  id: 14,
  deployment_id: 1999,
  user_id: 1,
  created_at: Fri, 23 Feb 2024 20:21:00.750300000 UTC +00:00,
  updated_at: Fri, 23 Feb 2024 20:21:00.750300000 UTC +00:00,
  status: "approved",
  comment: "command line",
  approval_rule_id: nil>]

And if the user tries to approve again with any of those approval rules or no approval rule, it won't work.

For database reviewer

I believe based on the database labs explain that the table involved is OK to do this way, but would like feedback from database reviewers.

Update: after running the db:gitlabcom-database-testing I see there is a warning about time.

Migration output

rake db:migrate output

main: == [advisory_lock_connection] object_id: 119440, pg_backend_pid: 46033
main: == 20240221200754 ChangeDeploymentApprovalsIndex: migrating ===================
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0099s
main: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id, :approval_rule_id], {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :unique=>true, :algorithm=>:concurrently})
main:    -> 0.0024s
main: -- execute("SET statement_timeout TO 0")
main:    -> 0.0001s
main: -- add_index(:deployment_approvals, [:deployment_id, :user_id, :approval_rule_id], {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :unique=>true, :algorithm=>:concurrently})
main:    -> 0.0026s
main: -- execute("RESET statement_timeout")
main:    -> 0.0001s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0004s
main: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id], {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :algorithm=>:concurrently})
main:    -> 0.0019s
main: -- remove_index(:deployment_approvals, {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :algorithm=>:concurrently, :column=>[:deployment_id, :user_id]})
main:    -> 0.0042s
main: == 20240221200754 ChangeDeploymentApprovalsIndex: migrated (0.0396s) ==========

main: == [advisory_lock_connection] object_id: 119440, pg_backend_pid: 46033
ci: == [advisory_lock_connection] object_id: 119680, pg_backend_pid: 46035
ci: == 20240221200754 ChangeDeploymentApprovalsIndex: migrating ===================
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0004s
ci: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id, :approval_rule_id], {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :unique=>true, :algorithm=>:concurrently})
ci:    -> 0.0021s
ci: -- execute("SET statement_timeout TO 0")
ci:    -> 0.0001s
ci: -- add_index(:deployment_approvals, [:deployment_id, :user_id, :approval_rule_id], {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :unique=>true, :algorithm=>:concurrently})
ci:    -> 0.0025s
ci: -- execute("RESET statement_timeout")
ci:    -> 0.0001s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0003s
ci: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id], {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :algorithm=>:concurrently})
ci:    -> 0.0019s
ci: -- remove_index(:deployment_approvals, {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :algorithm=>:concurrently, :column=>[:deployment_id, :user_id]})
ci:    -> 0.0039s
ci: == 20240221200754 ChangeDeploymentApprovalsIndex: migrated (0.0269s) ==========

ci: == [advisory_lock_connection] object_id: 119680, pg_backend_pid: 46035

rails db:migrate:down:main VERSION=20240221200754

main: == [advisory_lock_connection] object_id: 118920, pg_backend_pid: 48694
main: == 20240221200754 ChangeDeploymentApprovalsIndex: reverting ===================
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0136s
main: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id], {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :unique=>true, :algorithm=>:concurrently})
main:    -> 0.0034s
main: -- execute("SET statement_timeout TO 0")
main:    -> 0.0001s
main: -- add_index(:deployment_approvals, [:deployment_id, :user_id], {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :unique=>true, :algorithm=>:concurrently})
main:    -> 0.0023s
main: -- execute("RESET statement_timeout")
main:    -> 0.0002s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0005s
main: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id, :approval_rule_id], {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :algorithm=>:concurrently})
main:    -> 0.0022s
main: -- remove_index(:deployment_approvals, {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :algorithm=>:concurrently, :column=>[:deployment_id, :user_id, :approval_rule_id]})
main:    -> 0.0043s
main: == 20240221200754 ChangeDeploymentApprovalsIndex: reverted (0.0437s) ==========

main: == [advisory_lock_connection] object_id: 118920, pg_backend_pid: 48694

rails db:migrate:down:ci VERSION=20240221200754

ci: == [advisory_lock_connection] object_id: 118920, pg_backend_pid: 49220
ci: == 20240221200754 ChangeDeploymentApprovalsIndex: reverting ===================
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0145s
ci: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id], {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :unique=>true, :algorithm=>:concurrently})
ci:    -> 0.0035s
ci: -- execute("SET statement_timeout TO 0")
ci:    -> 0.0002s
ci: -- add_index(:deployment_approvals, [:deployment_id, :user_id], {:name=>"index_deployment_approvals_on_deployment_id_and_user_id", :unique=>true, :algorithm=>:concurrently})
ci:    -> 0.0025s
ci: -- execute("RESET statement_timeout")
ci:    -> 0.0004s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0006s
ci: -- index_exists?(:deployment_approvals, [:deployment_id, :user_id, :approval_rule_id], {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :algorithm=>:concurrently})
ci:    -> 0.0026s
ci: -- remove_index(:deployment_approvals, {:name=>"index_deployment_approvals_on_deployment_user_approval_rule", :algorithm=>:concurrently, :column=>[:deployment_id, :user_id, :approval_rule_id]})
ci:    -> 0.0049s
ci: == 20240221200754 ChangeDeploymentApprovalsIndex: reverted (0.0506s) ==========

ci: == [advisory_lock_connection] object_id: 118920, pg_backend_pid: 49220

MR acceptance checklist

Please 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 Hunter Stewart

Merge request reports