From acfe394018df50bfc08aa9e41265231747675646 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 28 Oct 2016 14:32:07 +0200 Subject: [PATCH 01/15] Add PoC for resource serializers --- app/serializers/base_serializer.rb | 24 +++++++++++++ app/serializers/entity_request.rb | 16 +++++++++ app/serializers/environment_entity.rb | 17 +++++++++ app/serializers/environment_serializer.rb | 3 ++ app/serializers/project_entity.rb | 8 +++++ app/serializers/request_aware_entity.rb | 11 ++++++ spec/serializers/entity_request_spec.rb | 26 ++++++++++++++ .../environment_serializer_spec.rb | 35 +++++++++++++++++++ 8 files changed, 140 insertions(+) create mode 100644 app/serializers/base_serializer.rb create mode 100644 app/serializers/entity_request.rb create mode 100644 app/serializers/environment_entity.rb create mode 100644 app/serializers/environment_serializer.rb create mode 100644 app/serializers/project_entity.rb create mode 100644 app/serializers/request_aware_entity.rb create mode 100644 spec/serializers/entity_request_spec.rb create mode 100644 spec/serializers/environment_serializer_spec.rb diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb new file mode 100644 index 00000000000..f9f7135551b --- /dev/null +++ b/app/serializers/base_serializer.rb @@ -0,0 +1,24 @@ +class BaseSerializer + def initialize(request = {}) + @request = EntityRequest.new(request) + @opts = { request: @request } + end + + def set(opts) + @request.merge!(opts) + self + end + + def represent(resource, opts = {}) + self.class.entity_class + .represent(resource, @opts.reverse_merge(opts)) + end + + def self.entity(entity_class) + @entity_class ||= entity_class + end + + def self.entity_class + @entity_class + end +end diff --git a/app/serializers/entity_request.rb b/app/serializers/entity_request.rb new file mode 100644 index 00000000000..12ceb38b284 --- /dev/null +++ b/app/serializers/entity_request.rb @@ -0,0 +1,16 @@ +class EntityRequest + # We use EntityRequest object to collect parameters and variables + # from the controller. Because options that are being passed to the entity + # do appear in each entity object in the chain, we need a way to pass data + # that is present in the controller (see #20045). + # + def initialize(parameters) + merge!(parameters) + end + + def merge!(parameters) + parameters.each do |key, value| + define_singleton_method(key) { value } + end + end +end diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb new file mode 100644 index 00000000000..9415f1dd450 --- /dev/null +++ b/app/serializers/environment_entity.rb @@ -0,0 +1,17 @@ +class EnvironmentEntity < Grape::Entity + include RequestAwareEntity + include Gitlab::Routing.url_helpers + + expose :id + expose :name + expose :project, with: ProjectEntity + expose :last_deployment, + as: :deployment, + using: API::Entities::Deployment + + expose :environment_path + + def environment_path + request.path + end +end diff --git a/app/serializers/environment_serializer.rb b/app/serializers/environment_serializer.rb new file mode 100644 index 00000000000..91955542f25 --- /dev/null +++ b/app/serializers/environment_serializer.rb @@ -0,0 +1,3 @@ +class EnvironmentSerializer < BaseSerializer + entity EnvironmentEntity +end diff --git a/app/serializers/project_entity.rb b/app/serializers/project_entity.rb new file mode 100644 index 00000000000..b8e23db470b --- /dev/null +++ b/app/serializers/project_entity.rb @@ -0,0 +1,8 @@ +class ProjectEntity < Grape::Entity + expose :id + expose :name + + expose :test do |project| + 'something' + end +end diff --git a/app/serializers/request_aware_entity.rb b/app/serializers/request_aware_entity.rb new file mode 100644 index 00000000000..f6b6f64d0f8 --- /dev/null +++ b/app/serializers/request_aware_entity.rb @@ -0,0 +1,11 @@ +module RequestAwareEntity + # We use SerializableRequest class to collect parameters and variables + # from the controller. Because options that are being passed to the entity + # are appear in each entity in the chain, we need a way to access data + # that is present in the controller (see #20045). + # + def request + options[:request] || + raise(StandardError, 'Request not set!!') + end +end diff --git a/spec/serializers/entity_request_spec.rb b/spec/serializers/entity_request_spec.rb new file mode 100644 index 00000000000..1c220a7b95d --- /dev/null +++ b/spec/serializers/entity_request_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe EntityRequest do + subject do + described_class.new(user: 'user', project: 'some project') + end + + describe 'methods created' do + it 'defines accessible attributes' do + expect(subject.user).to eq 'user' + expect(subject.project).to eq 'some project' + end + + it 'raises error when attribute is not defined' do + expect { subject.some_method }.to raise_error NoMethodError + end + end + + describe '#merge!' do + before { subject.merge!(build: 'some build') } + + it 'appends parameters' do + expect(subject.build).to eq 'some build' + end + end +end diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb new file mode 100644 index 00000000000..3470863681c --- /dev/null +++ b/spec/serializers/environment_serializer_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' + +describe EnvironmentSerializer do + let(:serializer) do + described_class.new(path: 'some path').represent(resource) + end + + context 'when there is a single object provided' do + let(:resource) { create(:environment) } + + it 'shows json' do + puts serializer.to_json + end + + it 'it generates payload for single object' do + expect(parsed_json).to be_an_instance_of Hash + end + end + + context 'when there is a collection of objects provided' do + let(:resource) { create_list(:environment, 2) } + + it 'shows json' do + puts serializer.to_json + end + + it 'generates payload for collection' do + expect(parsed_json).to be_an_instance_of Array + end + end + + def parsed_json + JSON.parse(serializer.to_json) + end +end From f1e9c97d64b96bdd398616743ad048f8d147e26b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 2 Nov 2016 14:36:21 +0100 Subject: [PATCH 02/15] Use entity request object in environment entity --- app/serializers/environment_entity.rb | 14 +++++++++++--- app/serializers/project_entity.rb | 4 +++- app/serializers/request_aware_entity.rb | 5 ----- spec/serializers/environment_serializer_spec.rb | 17 ++++++++--------- 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index 9415f1dd450..006b2841e8f 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -9,9 +9,17 @@ class EnvironmentEntity < Grape::Entity as: :deployment, using: API::Entities::Deployment - expose :environment_path + expose :gitlab_path do |environment| + namespace_project_environment_path( + environment.project.namespace, + environment.project, + environment + ) + end - def environment_path - request.path + expose :can_read? + + def can_read? + Ability.allowed?(request.user, :read_environment, @object) end end diff --git a/app/serializers/project_entity.rb b/app/serializers/project_entity.rb index b8e23db470b..047366eb687 100644 --- a/app/serializers/project_entity.rb +++ b/app/serializers/project_entity.rb @@ -1,8 +1,10 @@ class ProjectEntity < Grape::Entity + include RequestAwareEntity + expose :id expose :name expose :test do |project| - 'something' + request.user.email end end diff --git a/app/serializers/request_aware_entity.rb b/app/serializers/request_aware_entity.rb index f6b6f64d0f8..fc7d1698b1a 100644 --- a/app/serializers/request_aware_entity.rb +++ b/app/serializers/request_aware_entity.rb @@ -1,9 +1,4 @@ module RequestAwareEntity - # We use SerializableRequest class to collect parameters and variables - # from the controller. Because options that are being passed to the entity - # are appear in each entity in the chain, we need a way to access data - # that is present in the controller (see #20045). - # def request options[:request] || raise(StandardError, 'Request not set!!') diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index 3470863681c..cd9486111f1 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -2,18 +2,21 @@ require 'spec_helper' describe EnvironmentSerializer do let(:serializer) do - described_class.new(path: 'some path').represent(resource) + described_class.new(path: 'some path', user: user) + .represent(resource) end + let(:user) { create(:user) } + context 'when there is a single object provided' do let(:resource) { create(:environment) } it 'shows json' do - puts serializer.to_json + puts serializer.as_json end it 'it generates payload for single object' do - expect(parsed_json).to be_an_instance_of Hash + expect(serializer.as_json).to be_an_instance_of Hash end end @@ -21,15 +24,11 @@ describe EnvironmentSerializer do let(:resource) { create_list(:environment, 2) } it 'shows json' do - puts serializer.to_json + puts serializer.as_json end it 'generates payload for collection' do - expect(parsed_json).to be_an_instance_of Array + expect(serializer.as_json).to be_an_instance_of Array end end - - def parsed_json - JSON.parse(serializer.to_json) - end end From 573921cb9d0b139dec324fb1368feb1347b51624 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 12:54:10 +0100 Subject: [PATCH 03/15] Implement entities needed for environments folders --- app/serializers/base_serializer.rb | 12 ++++----- app/serializers/build_entity.rb | 20 ++++++++++++++ app/serializers/commit_entity.rb | 10 +++++++ app/serializers/deployment_entity.rb | 27 +++++++++++++++++++ app/serializers/environment_entity.rb | 22 +++++++-------- app/serializers/request_aware_entity.rb | 10 ++++--- spec/factories/deployments.rb | 4 +-- .../environment_serializer_spec.rb | 15 ++++++++--- 8 files changed, 93 insertions(+), 27 deletions(-) create mode 100644 app/serializers/build_entity.rb create mode 100644 app/serializers/commit_entity.rb create mode 100644 app/serializers/deployment_entity.rb diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb index f9f7135551b..aeb01dc2ad5 100644 --- a/app/serializers/base_serializer.rb +++ b/app/serializers/base_serializer.rb @@ -1,17 +1,17 @@ class BaseSerializer - def initialize(request = {}) - @request = EntityRequest.new(request) + def initialize(parameters = {}) + @entity = self.class.entity_class + @request = EntityRequest.new(parameters) @opts = { request: @request } end - def set(opts) - @request.merge!(opts) + def set(parameters) + @request.merge!(parameters) self end def represent(resource, opts = {}) - self.class.entity_class - .represent(resource, @opts.reverse_merge(opts)) + @entity.represent(resource, @opts.reverse_merge(opts)) end def self.entity(entity_class) diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb new file mode 100644 index 00000000000..38169ab9864 --- /dev/null +++ b/app/serializers/build_entity.rb @@ -0,0 +1,20 @@ +class BuildEntity < Grape::Entity + include RequestAwareEntity + + expose :id + expose :name + + expose :build_url do |build| + @urls.namespace_project_build_url( + build.project.namespace, + build.project, + build) + end + + expose :retry_url do |build| + @urls.retry_namespace_project_build_url( + build.project.namespace, + build.project, + build) + end +end diff --git a/app/serializers/commit_entity.rb b/app/serializers/commit_entity.rb new file mode 100644 index 00000000000..ae53de0c750 --- /dev/null +++ b/app/serializers/commit_entity.rb @@ -0,0 +1,10 @@ +class CommitEntity < API::Entities::RepoCommit + include RequestAwareEntity + + expose :commit_url do |commit| + @urls.namespace_project_tree_url( + @request.project.namespace, + @request.project, + id: commit.id) + end +end diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb new file mode 100644 index 00000000000..3462b188337 --- /dev/null +++ b/app/serializers/deployment_entity.rb @@ -0,0 +1,27 @@ +class DeploymentEntity < Grape::Entity + include RequestAwareEntity + + expose :id + expose :iid + expose :sha + + expose :ref do + expose :name do |deployment| + deployment.ref + end + + expose :ref_url do |deployment| + @urls.namespace_project_tree_url( + deployment.project.namespace, + deployment.project, + id: deployment.ref) + end + end + + expose :tag + expose :last? + expose :user, using: API::Entities::UserBasic + expose :commit, using: CommitEntity + expose :deployable, using: BuildEntity + expose :manual_actions +end diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index 006b2841e8f..6b037e65e55 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -1,25 +1,21 @@ class EnvironmentEntity < Grape::Entity include RequestAwareEntity - include Gitlab::Routing.url_helpers expose :id expose :name + expose :state + expose :external_url + expose :environment_type expose :project, with: ProjectEntity - expose :last_deployment, - as: :deployment, - using: API::Entities::Deployment + expose :last_deployment, using: DeploymentEntity + expose :stoppable? - expose :gitlab_path do |environment| - namespace_project_environment_path( + expose :environmenturl do |environment| + @urls.namespace_project_environment_url( environment.project.namespace, environment.project, - environment - ) + environment) end - expose :can_read? - - def can_read? - Ability.allowed?(request.user, :read_environment, @object) - end + expose :created_at, :updated_at end diff --git a/app/serializers/request_aware_entity.rb b/app/serializers/request_aware_entity.rb index fc7d1698b1a..1586507492d 100644 --- a/app/serializers/request_aware_entity.rb +++ b/app/serializers/request_aware_entity.rb @@ -1,6 +1,10 @@ module RequestAwareEntity - def request - options[:request] || - raise(StandardError, 'Request not set!!') + attr_reader :request + + def initialize(object, options = {}) + super(object, options) + + @request = options.fetch(:request) + @urls = Gitlab::Routing.url_helpers end end diff --git a/spec/factories/deployments.rb b/spec/factories/deployments.rb index 6f24bf58d14..8c8e5dc54c2 100644 --- a/spec/factories/deployments.rb +++ b/spec/factories/deployments.rb @@ -1,9 +1,9 @@ FactoryGirl.define do factory :deployment, class: Deployment do - sha '97de212e80737a608d939f648d959671fb0a0142' + sha 'b83d6e391c22777fca1ed3012fce84f633d7fed0' ref 'master' tag false - project nil + project environment factory: :environment diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index cd9486111f1..f7de6c450ab 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -2,17 +2,25 @@ require 'spec_helper' describe EnvironmentSerializer do let(:serializer) do - described_class.new(path: 'some path', user: user) + described_class + .new(user: user, project: project) .represent(resource) end let(:user) { create(:user) } context 'when there is a single object provided' do - let(:resource) { create(:environment) } + let(:deployment) do + create(:deployment, deployable: deployable, + user: user) + end + + let(:deployable) { create(:ci_build) } + let(:project) { deployment.project } + let(:resource) { deployment.environment } it 'shows json' do - puts serializer.as_json + pp serializer.as_json end it 'it generates payload for single object' do @@ -21,6 +29,7 @@ describe EnvironmentSerializer do end context 'when there is a collection of objects provided' do + let(:project) { create(:empty_project) } let(:resource) { create_list(:environment, 2) } it 'shows json' do From 52d333b9f4793ca677b938317f4c8e480b835284 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 13:35:57 +0100 Subject: [PATCH 04/15] Remove duplication from build serializer entity --- app/serializers/build_entity.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index 38169ab9864..b37bff0b500 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -5,16 +5,20 @@ class BuildEntity < Grape::Entity expose :name expose :build_url do |build| - @urls.namespace_project_build_url( - build.project.namespace, - build.project, - build) + url_to(:namespace_project_build, build) end expose :retry_url do |build| - @urls.retry_namespace_project_build_url( - build.project.namespace, - build.project, - build) + url_to(:retry_namespace_project_build, build) + end + + expose :play_url, if: ->(build, _) { build.manual? } do |build| + url_to(:retry_namespace_project_build, build) + end + + private + + def url_to(route, build) + @urls.send("#{route}_url", build.project.namespace, build.project, build) end end From 7dd6485a0fd7fc264f86f389da3ab846c62e8724 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 13:42:22 +0100 Subject: [PATCH 05/15] Add manual actions for deployment serialization --- app/serializers/deployment_entity.rb | 2 +- spec/serializers/environment_serializer_spec.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb index 3462b188337..d3ed6a5eef4 100644 --- a/app/serializers/deployment_entity.rb +++ b/app/serializers/deployment_entity.rb @@ -23,5 +23,5 @@ class DeploymentEntity < Grape::Entity expose :user, using: API::Entities::UserBasic expose :commit, using: CommitEntity expose :deployable, using: BuildEntity - expose :manual_actions + expose :manual_actions, using: BuildEntity end diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index f7de6c450ab..2c94a076aba 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -10,6 +10,11 @@ describe EnvironmentSerializer do let(:user) { create(:user) } context 'when there is a single object provided' do + before do + create(:ci_build, :manual, name: 'manual1', + pipeline: deployable.pipeline) + end + let(:deployment) do create(:deployment, deployable: deployable, user: user) From c3dae798e16dee0b64bc8c9fd0e0e21b2502318e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 14:01:35 +0100 Subject: [PATCH 06/15] Extend tests for environments serializer --- app/serializers/environment_entity.rb | 2 +- .../environment_serializer_spec.rb | 21 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index 6b037e65e55..0a6c5e7f87f 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -10,7 +10,7 @@ class EnvironmentEntity < Grape::Entity expose :last_deployment, using: DeploymentEntity expose :stoppable? - expose :environmenturl do |environment| + expose :environment_url do |environment| @urls.namespace_project_environment_url( environment.project.namespace, environment.project, diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index 2c94a076aba..81393beb416 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -24,21 +24,30 @@ describe EnvironmentSerializer do let(:project) { deployment.project } let(:resource) { deployment.environment } - it 'shows json' do - pp serializer.as_json - end - it 'it generates payload for single object' do expect(serializer.as_json).to be_an_instance_of Hash end + + it 'contains important elements of environment' do + expect(serializer.as_json) + .to include(:name, :external_url, :environment_url, :last_deployment) + end + + it 'contains relevant information about last deployment' do + last_deployment = serializer.as_json.fetch(:last_deployment) + + expect(last_deployment) + .to include(:ref, :user, :commit, :deployable, :manual_actions) + end end context 'when there is a collection of objects provided' do let(:project) { create(:empty_project) } let(:resource) { create_list(:environment, 2) } - it 'shows json' do - puts serializer.as_json + it 'contains important elements of environment' do + expect(serializer.as_json.first) + .to include(:last_deployment, :name, :external_url) end it 'generates payload for collection' do From 6c94adea3541914e7c36fb2e36a0f3ee44d2bcb4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 14:08:13 +0100 Subject: [PATCH 07/15] Remove project entity that was part of the PoC --- app/serializers/environment_entity.rb | 2 +- app/serializers/project_entity.rb | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) delete mode 100644 app/serializers/project_entity.rb diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index 0a6c5e7f87f..0035d369e61 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -6,7 +6,7 @@ class EnvironmentEntity < Grape::Entity expose :state expose :external_url expose :environment_type - expose :project, with: ProjectEntity + expose :project, with: API::Entities::BasicProjectDetails expose :last_deployment, using: DeploymentEntity expose :stoppable? diff --git a/app/serializers/project_entity.rb b/app/serializers/project_entity.rb deleted file mode 100644 index 047366eb687..00000000000 --- a/app/serializers/project_entity.rb +++ /dev/null @@ -1,10 +0,0 @@ -class ProjectEntity < Grape::Entity - include RequestAwareEntity - - expose :id - expose :name - - expose :test do |project| - request.user.email - end -end From a68e7e605aa684e88523f6e28f995c3b497945c6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 14:22:19 +0100 Subject: [PATCH 08/15] Add basic tests for build entity used by serialzier --- spec/serializers/build_entity_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 spec/serializers/build_entity_spec.rb diff --git a/spec/serializers/build_entity_spec.rb b/spec/serializers/build_entity_spec.rb new file mode 100644 index 00000000000..b7acc2066ba --- /dev/null +++ b/spec/serializers/build_entity_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe BuildEntity do + subject do + described_class.new(build, request: double).as_json + end + + context 'when build is a regular job' do + let(:build) { create(:ci_build) } + + it 'contains url to build page and retry action' do + expect(subject).to include(:build_url, :retry_url) + expect(subject).not_to include(:play_url) + end + end + + context 'when build is a manual action' do + let(:build) { create(:ci_build, :manual) } + + it 'contains url to play action' do + expect(subject).to include(:play_url) + end + end +end From 3c3e61aaca34d1113d3cfb6076462beaf353e739 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 14:24:10 +0100 Subject: [PATCH 09/15] Exclude project information from environment entity --- app/serializers/environment_entity.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index 0035d369e61..e80a4335f53 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -6,7 +6,6 @@ class EnvironmentEntity < Grape::Entity expose :state expose :external_url expose :environment_type - expose :project, with: API::Entities::BasicProjectDetails expose :last_deployment, using: DeploymentEntity expose :stoppable? From f203ca5c932cc24787e1c44c5801c15923cd04ab Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Nov 2016 15:13:57 +0100 Subject: [PATCH 10/15] Fix specs related to deployments and environments --- app/serializers/build_entity.rb | 2 +- spec/factories/deployments.rb | 4 ++-- spec/serializers/environment_serializer_spec.rb | 6 ++++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index b37bff0b500..203bd747ad9 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -13,7 +13,7 @@ class BuildEntity < Grape::Entity end expose :play_url, if: ->(build, _) { build.manual? } do |build| - url_to(:retry_namespace_project_build, build) + url_to(:play_namespace_project_build, build) end private diff --git a/spec/factories/deployments.rb b/spec/factories/deployments.rb index 8c8e5dc54c2..6f24bf58d14 100644 --- a/spec/factories/deployments.rb +++ b/spec/factories/deployments.rb @@ -1,9 +1,9 @@ FactoryGirl.define do factory :deployment, class: Deployment do - sha 'b83d6e391c22777fca1ed3012fce84f633d7fed0' + sha '97de212e80737a608d939f648d959671fb0a0142' ref 'master' tag false - project + project nil environment factory: :environment diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index 81393beb416..32f144faf54 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -8,6 +8,7 @@ describe EnvironmentSerializer do end let(:user) { create(:user) } + let(:project) { create(:project) } context 'when there is a single object provided' do before do @@ -17,11 +18,12 @@ describe EnvironmentSerializer do let(:deployment) do create(:deployment, deployable: deployable, - user: user) + user: user, + project: project, + sha: project.commit.id) end let(:deployable) { create(:ci_build) } - let(:project) { deployment.project } let(:resource) { deployment.environment } it 'it generates payload for single object' do From a2c1178c21796933916bb44b3d6b8e4d11d89d7f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 4 Nov 2016 10:16:30 +0100 Subject: [PATCH 11/15] Expose commit author if author exists --- app/serializers/commit_entity.rb | 2 ++ spec/serializers/environment_serializer_spec.rb | 11 ++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/serializers/commit_entity.rb b/app/serializers/commit_entity.rb index ae53de0c750..3b6c2323e3e 100644 --- a/app/serializers/commit_entity.rb +++ b/app/serializers/commit_entity.rb @@ -1,6 +1,8 @@ class CommitEntity < API::Entities::RepoCommit include RequestAwareEntity + expose :author, using: API::Entities::UserBasic + expose :commit_url do |commit| @urls.namespace_project_tree_url( @request.project.namespace, diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index 32f144faf54..37bc086826c 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -7,6 +7,7 @@ describe EnvironmentSerializer do .represent(resource) end + let(:json) { serializer.as_json } let(:user) { create(:user) } let(:project) { create(:project) } @@ -27,16 +28,16 @@ describe EnvironmentSerializer do let(:resource) { deployment.environment } it 'it generates payload for single object' do - expect(serializer.as_json).to be_an_instance_of Hash + expect(json).to be_an_instance_of Hash end it 'contains important elements of environment' do - expect(serializer.as_json) + expect(json) .to include(:name, :external_url, :environment_url, :last_deployment) end it 'contains relevant information about last deployment' do - last_deployment = serializer.as_json.fetch(:last_deployment) + last_deployment = json.fetch(:last_deployment) expect(last_deployment) .to include(:ref, :user, :commit, :deployable, :manual_actions) @@ -48,12 +49,12 @@ describe EnvironmentSerializer do let(:resource) { create_list(:environment, 2) } it 'contains important elements of environment' do - expect(serializer.as_json.first) + expect(json.first) .to include(:last_deployment, :name, :external_url) end it 'generates payload for collection' do - expect(serializer.as_json).to be_an_instance_of Array + expect(json).to be_an_instance_of Array end end end From c315332b8a90d26197ad93a6e982888aa575e2d4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 4 Nov 2016 10:19:29 +0100 Subject: [PATCH 12/15] Refine build entity tests a little --- spec/serializers/build_entity_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/serializers/build_entity_spec.rb b/spec/serializers/build_entity_spec.rb index b7acc2066ba..2282b560d7f 100644 --- a/spec/serializers/build_entity_spec.rb +++ b/spec/serializers/build_entity_spec.rb @@ -1,10 +1,12 @@ require 'spec_helper' describe BuildEntity do - subject do - described_class.new(build, request: double).as_json + let(:entity) do + described_class.new(build, request: double) end + subject { entity.as_json } + context 'when build is a regular job' do let(:build) { create(:ci_build) } From 3968b07d7ea1fd2ca07e427a487ea94ca0c6081d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 4 Nov 2016 13:08:58 +0100 Subject: [PATCH 13/15] Add tests for serialization entities, add user entity --- app/serializers/commit_entity.rb | 2 +- app/serializers/deployment_entity.rb | 2 +- app/serializers/user_entity.rb | 2 ++ spec/serializers/build_entity_spec.rb | 5 +++ spec/serializers/commit_entity_spec.rb | 44 ++++++++++++++++++++++++++ spec/serializers/user_entity_spec.rb | 23 ++++++++++++++ 6 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 app/serializers/user_entity.rb create mode 100644 spec/serializers/commit_entity_spec.rb create mode 100644 spec/serializers/user_entity_spec.rb diff --git a/app/serializers/commit_entity.rb b/app/serializers/commit_entity.rb index 3b6c2323e3e..827782e85bb 100644 --- a/app/serializers/commit_entity.rb +++ b/app/serializers/commit_entity.rb @@ -1,7 +1,7 @@ class CommitEntity < API::Entities::RepoCommit include RequestAwareEntity - expose :author, using: API::Entities::UserBasic + expose :author, using: UserEntity expose :commit_url do |commit| @urls.namespace_project_tree_url( diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb index d3ed6a5eef4..d743b44c4a0 100644 --- a/app/serializers/deployment_entity.rb +++ b/app/serializers/deployment_entity.rb @@ -20,7 +20,7 @@ class DeploymentEntity < Grape::Entity expose :tag expose :last? - expose :user, using: API::Entities::UserBasic + expose :user, using: UserEntity expose :commit, using: CommitEntity expose :deployable, using: BuildEntity expose :manual_actions, using: BuildEntity diff --git a/app/serializers/user_entity.rb b/app/serializers/user_entity.rb new file mode 100644 index 00000000000..43754ea94f7 --- /dev/null +++ b/app/serializers/user_entity.rb @@ -0,0 +1,2 @@ +class UserEntity < API::Entities::UserBasic +end diff --git a/spec/serializers/build_entity_spec.rb b/spec/serializers/build_entity_spec.rb index 2282b560d7f..2734f5bedca 100644 --- a/spec/serializers/build_entity_spec.rb +++ b/spec/serializers/build_entity_spec.rb @@ -14,6 +14,11 @@ describe BuildEntity do expect(subject).to include(:build_url, :retry_url) expect(subject).not_to include(:play_url) end + + it 'does not contain sensitive information' do + expect(subject).not_to include(/token/) + expect(subject).not_to include(/variables/) + end end context 'when build is a manual action' do diff --git a/spec/serializers/commit_entity_spec.rb b/spec/serializers/commit_entity_spec.rb new file mode 100644 index 00000000000..628e35c9a28 --- /dev/null +++ b/spec/serializers/commit_entity_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' + +describe CommitEntity do + let(:entity) do + described_class.new(commit, request: request) + end + + let(:request) { double('request') } + let(:project) { create(:project) } + let(:commit) { project.commit } + + subject { entity.as_json } + + before do + allow(request).to receive(:project).and_return(project) + end + + context 'when commit author is a user' do + before do + create(:user, email: commit.author_email) + end + + it 'contains information about user' do + expect(subject.fetch(:author)).not_to be_nil + end + end + + context 'when commit author is not a user' do + it 'does not contain author details' do + expect(subject.fetch(:author)).to be_nil + end + end + + it 'contains commit URL' do + expect(subject).to include(:commit_url) + end + + it 'needs to receive project in the request' do + expect(request).to receive(:project) + .and_return(project) + + subject + end +end diff --git a/spec/serializers/user_entity_spec.rb b/spec/serializers/user_entity_spec.rb new file mode 100644 index 00000000000..c5d11cbcf5e --- /dev/null +++ b/spec/serializers/user_entity_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe UserEntity do + let(:entity) { described_class.new(user) } + let(:user) { create(:user) } + subject { entity.as_json } + + it 'exposes user name and login' do + expect(subject).to include(:username, :name) + end + + it 'does not expose passwords' do + expect(subject).not_to include(/password/) + end + + it 'does not expose tokens' do + expect(subject).not_to include(/token/) + end + + it 'does not expose 2FA OTPs' do + expect(subject).not_to include(/otp/) + end +end From e49fb264e6cb5eff44330d69d34b5f74ef262659 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 4 Nov 2016 13:23:06 +0100 Subject: [PATCH 14/15] Add tests for deployment and environment entitites --- spec/serializers/deployment_entity_spec.rb | 20 ++++++++++++++++++++ spec/serializers/environment_entity_spec.rb | 18 ++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 spec/serializers/deployment_entity_spec.rb create mode 100644 spec/serializers/environment_entity_spec.rb diff --git a/spec/serializers/deployment_entity_spec.rb b/spec/serializers/deployment_entity_spec.rb new file mode 100644 index 00000000000..51b6de91571 --- /dev/null +++ b/spec/serializers/deployment_entity_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe DeploymentEntity do + let(:entity) do + described_class.new(deployment, request: double) + end + + let(:deployment) { create(:deployment) } + + subject { entity.as_json } + + it 'exposes internal deployment id' do + expect(subject).to include(:iid) + end + + it 'exposes nested information about branch' do + expect(subject[:ref][:name]).to eq 'master' + expect(subject[:ref][:ref_url]).not_to be_empty + end +end diff --git a/spec/serializers/environment_entity_spec.rb b/spec/serializers/environment_entity_spec.rb new file mode 100644 index 00000000000..4ca8c299147 --- /dev/null +++ b/spec/serializers/environment_entity_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe EnvironmentEntity do + let(:entity) do + described_class.new(environment, request: double) + end + + let(:environment) { create(:environment) } + subject { entity.as_json } + + it 'exposes latest deployment' do + expect(subject).to include(:last_deployment) + end + + it 'exposes core elements of environment' do + expect(subject).to include(:id, :name, :state, :environment_url) + end +end From b0a4635be395f0ce14e15e1671d7acfc2360d1ba Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 4 Nov 2016 15:00:39 +0100 Subject: [PATCH 15/15] Simplify implementation of entity serializers --- app/serializers/base_serializer.rb | 10 ++-------- app/serializers/build_entity.rb | 2 +- app/serializers/commit_entity.rb | 6 +++--- app/serializers/deployment_entity.rb | 2 +- app/serializers/entity_request.rb | 4 ---- app/serializers/environment_entity.rb | 2 +- app/serializers/request_aware_entity.rb | 11 ++++++----- spec/serializers/entity_request_spec.rb | 8 -------- 8 files changed, 14 insertions(+), 31 deletions(-) diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb index aeb01dc2ad5..de9a181db90 100644 --- a/app/serializers/base_serializer.rb +++ b/app/serializers/base_serializer.rb @@ -1,17 +1,11 @@ class BaseSerializer def initialize(parameters = {}) - @entity = self.class.entity_class @request = EntityRequest.new(parameters) - @opts = { request: @request } - end - - def set(parameters) - @request.merge!(parameters) - self end def represent(resource, opts = {}) - @entity.represent(resource, @opts.reverse_merge(opts)) + self.class.entity_class + .represent(resource, opts.merge(request: @request)) end def self.entity(entity_class) diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index 203bd747ad9..3d9ac66de0e 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -19,6 +19,6 @@ class BuildEntity < Grape::Entity private def url_to(route, build) - @urls.send("#{route}_url", build.project.namespace, build.project, build) + send("#{route}_url", build.project.namespace, build.project, build) end end diff --git a/app/serializers/commit_entity.rb b/app/serializers/commit_entity.rb index 827782e85bb..f7eba6fc1e3 100644 --- a/app/serializers/commit_entity.rb +++ b/app/serializers/commit_entity.rb @@ -4,9 +4,9 @@ class CommitEntity < API::Entities::RepoCommit expose :author, using: UserEntity expose :commit_url do |commit| - @urls.namespace_project_tree_url( - @request.project.namespace, - @request.project, + namespace_project_tree_url( + request.project.namespace, + request.project, id: commit.id) end end diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb index d743b44c4a0..ad6fc8d665b 100644 --- a/app/serializers/deployment_entity.rb +++ b/app/serializers/deployment_entity.rb @@ -11,7 +11,7 @@ class DeploymentEntity < Grape::Entity end expose :ref_url do |deployment| - @urls.namespace_project_tree_url( + namespace_project_tree_url( deployment.project.namespace, deployment.project, id: deployment.ref) diff --git a/app/serializers/entity_request.rb b/app/serializers/entity_request.rb index 12ceb38b284..456ba1174c0 100644 --- a/app/serializers/entity_request.rb +++ b/app/serializers/entity_request.rb @@ -5,10 +5,6 @@ class EntityRequest # that is present in the controller (see #20045). # def initialize(parameters) - merge!(parameters) - end - - def merge!(parameters) parameters.each do |key, value| define_singleton_method(key) { value } end diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index e80a4335f53..ee4392cc46d 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -10,7 +10,7 @@ class EnvironmentEntity < Grape::Entity expose :stoppable? expose :environment_url do |environment| - @urls.namespace_project_environment_url( + namespace_project_environment_url( environment.project.namespace, environment.project, environment) diff --git a/app/serializers/request_aware_entity.rb b/app/serializers/request_aware_entity.rb index 1586507492d..ff8c1142abc 100644 --- a/app/serializers/request_aware_entity.rb +++ b/app/serializers/request_aware_entity.rb @@ -1,10 +1,11 @@ module RequestAwareEntity - attr_reader :request + extend ActiveSupport::Concern - def initialize(object, options = {}) - super(object, options) + included do + include Gitlab::Routing.url_helpers + end - @request = options.fetch(:request) - @urls = Gitlab::Routing.url_helpers + def request + @options.fetch(:request) end end diff --git a/spec/serializers/entity_request_spec.rb b/spec/serializers/entity_request_spec.rb index 1c220a7b95d..86654adfd54 100644 --- a/spec/serializers/entity_request_spec.rb +++ b/spec/serializers/entity_request_spec.rb @@ -15,12 +15,4 @@ describe EntityRequest do expect { subject.some_method }.to raise_error NoMethodError end end - - describe '#merge!' do - before { subject.merge!(build: 'some build') } - - it 'appends parameters' do - expect(subject.build).to eq 'some build' - end - end end