From e3dbf07a926eff21567b5e29e4b0c12467e8837b Mon Sep 17 00:00:00 2001 From: Alishan Ladhani Date: Thu, 24 Feb 2022 15:51:45 -0500 Subject: [PATCH 1/2] Expose Deployment Approvals to internal Environments API - Fix N+1 issue with Deployment#last? - Optimize Deployment#pending_approval_count so it doesn't run a COUNT(*) query for each deployment - Update EnvironmentSerializer N+1 query threshold --- .../environments/deployment_preloader.rb | 10 +++++++++- ee/app/models/ee/deployment.rb | 2 +- ee/app/serializers/ee/deployment_entity.rb | 1 + .../serializers/ee/environment_serializer.rb | 7 +++++++ ee/spec/models/deployment_spec.rb | 11 +++++++++++ .../serializers/ee/deployment_entity_spec.rb | 6 ++++++ .../ee/environment_serializer_spec.rb | 6 ++++-- .../environments/deployment_preloader_spec.rb | 18 ++++++++++++++++++ .../environment_serializer_shared_examples.rb | 2 +- 9 files changed, 58 insertions(+), 5 deletions(-) diff --git a/app/models/preloaders/environments/deployment_preloader.rb b/app/models/preloaders/environments/deployment_preloader.rb index fcf892698bbdab..592d3e54a30c71 100644 --- a/app/models/preloaders/environments/deployment_preloader.rb +++ b/app/models/preloaders/environments/deployment_preloader.rb @@ -34,8 +34,16 @@ def load_deployment_association(association_name, association_attributes) deployments_by_environment_id = deployments.index_by(&:environment_id) environments.each do |environment| - environment.association(association_name).target = deployments_by_environment_id[environment.id] + associated_deployment = deployments_by_environment_id[environment.id] + + environment.association(association_name).target = associated_deployment environment.association(association_name).loaded! + + if associated_deployment + # `last?` in DeploymentEntity requires this environment to be loaded + associated_deployment.association(:environment).target = environment + associated_deployment.association(:environment).loaded! + end end end end diff --git a/ee/app/models/ee/deployment.rb b/ee/app/models/ee/deployment.rb index e6ed0c411b5cdc..2024d66f7929ae 100644 --- a/ee/app/models/ee/deployment.rb +++ b/ee/app/models/ee/deployment.rb @@ -34,7 +34,7 @@ module Deployment def pending_approval_count return 0 unless blocked? - environment.required_approval_count - approvals.count + environment.required_approval_count - approvals.length end end end diff --git a/ee/app/serializers/ee/deployment_entity.rb b/ee/app/serializers/ee/deployment_entity.rb index 54453d34d4eb0f..84f8e4a3cd4b6c 100644 --- a/ee/app/serializers/ee/deployment_entity.rb +++ b/ee/app/serializers/ee/deployment_entity.rb @@ -6,6 +6,7 @@ module DeploymentEntity prepended do expose :pending_approval_count + expose :approvals, using: ::API::Entities::Deployments::Approval end end end diff --git a/ee/app/serializers/ee/environment_serializer.rb b/ee/app/serializers/ee/environment_serializer.rb index 93247cf07a3d23..f5a31df6a34eb8 100644 --- a/ee/app/serializers/ee/environment_serializer.rb +++ b/ee/app/serializers/ee/environment_serializer.rb @@ -10,6 +10,13 @@ def environment_associations super.deep_merge(latest_opened_most_severe_alert: []) end + override :deployment_associations + def deployment_associations + super.deep_merge(approvals: { + user: [] + }) + end + override :project_associations def project_associations super.deep_merge(protected_environments: []) diff --git a/ee/spec/models/deployment_spec.rb b/ee/spec/models/deployment_spec.rb index 9cfdc56d6a8922..1d37a9a339e9cf 100644 --- a/ee/spec/models/deployment_spec.rb +++ b/ee/spec/models/deployment_spec.rb @@ -69,6 +69,17 @@ expect(deployment.pending_approval_count).to eq(0) end end + + context 'loading approval count' do + before do + deployment.environment.required_approval_count + deployment.approvals.to_a + end + + it 'does not perform an extra query when approvals are loaded', :request_store do + expect { deployment.pending_approval_count }.not_to exceed_query_limit(0) + end + end end context 'when Protected Environments feature is not available' do diff --git a/ee/spec/serializers/ee/deployment_entity_spec.rb b/ee/spec/serializers/ee/deployment_entity_spec.rb index a4026210390710..e2b91ea983a82a 100644 --- a/ee/spec/serializers/ee/deployment_entity_spec.rb +++ b/ee/spec/serializers/ee/deployment_entity_spec.rb @@ -21,4 +21,10 @@ expect(subject[:pending_approval_count]).to eq(2) end end + + describe '#approvals' do + it 'exposes approvals' do + expect(subject[:approvals].length).to eq(1) + end + end end diff --git a/ee/spec/serializers/ee/environment_serializer_spec.rb b/ee/spec/serializers/ee/environment_serializer_spec.rb index aee337363e5ee5..0a01582adf6b47 100644 --- a/ee/spec/serializers/ee/environment_serializer_spec.rb +++ b/ee/spec/serializers/ee/environment_serializer_spec.rb @@ -19,8 +19,10 @@ def create_environment_with_associations(project) create(:environment, project: project).tap do |environment| create(:deployment, :success, environment: environment, project: project) - create(:deployment, :running, environment: environment, project: project) - create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project) + create(:deployment, :blocked, environment: environment, project: project) do |deployment| + create(:deployment_approval, deployment: deployment) + end + create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project, required_approval_count: 2) prometheus_alert = create(:prometheus_alert, project: project, environment: environment) create(:alert_management_alert, :triggered, :prometheus, project: project, environment: environment, prometheus_alert: prometheus_alert) end diff --git a/spec/models/preloaders/environments/deployment_preloader_spec.rb b/spec/models/preloaders/environments/deployment_preloader_spec.rb index c1812d45628771..3f2f28a069e6c3 100644 --- a/spec/models/preloaders/environments/deployment_preloader_spec.rb +++ b/spec/models/preloaders/environments/deployment_preloader_spec.rb @@ -62,4 +62,22 @@ def deployment_associations expect(default_preload_query).to be(false) end + + it 'sets environment on the associated deployment', :aggregate_failures do + preload_association(:last_deployment) + + expect do + project.environments.each { |environment| environment.last_deployment.environment } + end.not_to exceed_query_limit(0) + + project.environments.each do |environment| + expect(environment.last_deployment.environment).to eq(environment) + end + end + + it 'does not attempt to set environment on a nil deployment' do + create(:environment, project: project, state: :available) + + expect { preload_association(:last_deployment) }.not_to raise_error + end end diff --git a/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb b/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb index ad4057a972a9b2..87a330604353e9 100644 --- a/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb +++ b/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.shared_examples 'avoid N+1 on environments serialization' do |ee: false| # Investigating in https://gitlab.com/gitlab-org/gitlab/-/issues/353209 - let(:query_threshold) { 9 + (ee ? 4 : 0) } + let(:query_threshold) { 1 + (ee ? 4 : 0) } it 'avoids N+1 database queries with grouping', :request_store do create_environment_with_associations(project) -- GitLab From 428038a04d30bf048b10de1a2acae4e913a1069a Mon Sep 17 00:00:00 2001 From: Alishan Ladhani Date: Thu, 24 Feb 2022 15:52:45 -0500 Subject: [PATCH 2/2] Expose created_at when serializing Deployment Approvals Changelog: changed EE: true --- doc/api/deployments.md | 12 ++++++++---- ee/lib/api/entities/deployments/approval.rb | 1 + .../schemas/public_api/v4/deployment_approval.json | 4 ++++ .../lib/api/entities/deployments/approval_spec.rb | 2 +- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/doc/api/deployments.md b/doc/api/deployments.md index 29f9bd3e2ee300..87ff65495403f0 100644 --- a/doc/api/deployments.md +++ b/doc/api/deployments.md @@ -281,7 +281,8 @@ Deployments created by users on GitLab Premium or higher include the `approvals` "avatar_url": "https://www.gravatar.com/avatar/e83ac685f68ea07553ad3054c738c709?s=80&d=identicon", "web_url": "http://localhost:3000/project_6_bot" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ], ... @@ -351,7 +352,8 @@ Deployments created by users on GitLab Premium or higher include the `approvals` "avatar_url": "https://www.gravatar.com/avatar/e83ac685f68ea07553ad3054c738c709?s=80&d=identicon", "web_url": "http://localhost:3000/project_6_bot" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ], ... @@ -417,7 +419,8 @@ Deployments created by users on GitLab Premium or higher include the `approvals` "avatar_url": "https://www.gravatar.com/avatar/e83ac685f68ea07553ad3054c738c709?s=80&d=identicon", "web_url": "http://localhost:3000/project_6_bot" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ], ... @@ -480,6 +483,7 @@ Example response: "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "web_url": "http://localhost:3000/root" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ``` diff --git a/ee/lib/api/entities/deployments/approval.rb b/ee/lib/api/entities/deployments/approval.rb index 102e237f568fcd..a24c4910a477cf 100644 --- a/ee/lib/api/entities/deployments/approval.rb +++ b/ee/lib/api/entities/deployments/approval.rb @@ -6,6 +6,7 @@ module Deployments class Approval < Grape::Entity expose :user, using: Entities::UserBasic expose :status + expose :created_at end end end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json b/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json index 517454d1709dc8..469b1c9142c6d1 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json @@ -13,6 +13,10 @@ "items": { "$ref": "../../../../../../../spec/fixtures/api/schemas/public_api/v4/user/basic.json" } + }, + "created_at": { + "type": "string", + "format": "date-time" } }, "additionalProperties": false diff --git a/ee/spec/lib/api/entities/deployments/approval_spec.rb b/ee/spec/lib/api/entities/deployments/approval_spec.rb index 3c8dd1f07bc590..ec7d138f3e69cd 100644 --- a/ee/spec/lib/api/entities/deployments/approval_spec.rb +++ b/ee/spec/lib/api/entities/deployments/approval_spec.rb @@ -8,6 +8,6 @@ let(:approval) { build(:deployment_approval) } it 'exposes correct attributes' do - expect(subject.keys).to contain_exactly(:user, :status) + expect(subject.keys).to contain_exactly(:user, :status, :created_at) end end -- GitLab