From 509c7737ea23aade2f988b8a3602c0bd47434ff3 Mon Sep 17 00:00:00 2001 From: Zach Wolfenbarger Date: Thu, 13 Jun 2024 14:32:54 -0500 Subject: [PATCH] Batch aggregation (#4303) * Aggregation migration, model, spec, factory * Client & spec * Policy and spec * Schemas * Update policy & schemas * Update status enum * Add default to status migration * Update serializer * Update factory * Controller and spec * Fix migration * Hound * Hound again * Third round hound * Feed hound * Consistency * Add project_id to Aggregations to allow Doorkeeper to scope by project * Clarify admin specs, add collab spec * Remove ignored_columns * Add spec for failed service connections * Add serializer spec * Implement #destroy, add error specs for collisions * Aggregation documentation * reorder spec * Fix hash alignment * Remove aggregation association from subject * Remove user uniqueness constraint * MIgrate database, update structure.sql * Resolve migrations --- .../api/v1/aggregations_controller.rb | 29 +++- app/models/aggregation.rb | 24 ++- app/models/subject.rb | 1 - app/models/workflow.rb | 2 +- app/policies/aggregation_policy.rb | 27 ++-- app/schemas/aggregation_create_schema.rb | 35 +++-- app/schemas/aggregation_update_schema.rb | 26 ++-- app/serializers/aggregation_serializer.rb | 6 +- app/services/aggregation_client.rb | 58 ++++++++ ...240304201959_refactor_aggregation_model.rb | 31 ++++ db/structure.sql | 41 +++--- docs/source/includes/_aggregations.md | 139 ++++++++++++++++++ .../api/v1/aggregations_controller_spec.rb | 116 +++++++++++++++ spec/factories/aggregations.rb | 5 +- spec/models/aggregation_spec.rb | 52 +++---- spec/policies/aggregation_policy_spec.rb | 65 ++++---- .../aggregation_serializer_spec.rb | 13 ++ spec/services/aggregation_client_spec.rb | 87 +++++++++++ 18 files changed, 604 insertions(+), 153 deletions(-) create mode 100644 app/services/aggregation_client.rb create mode 100644 db/migrate/20240304201959_refactor_aggregation_model.rb create mode 100644 docs/source/includes/_aggregations.md create mode 100644 spec/controllers/api/v1/aggregations_controller_spec.rb create mode 100644 spec/serializers/aggregation_serializer_spec.rb create mode 100644 spec/services/aggregation_client_spec.rb diff --git a/app/controllers/api/v1/aggregations_controller.rb b/app/controllers/api/v1/aggregations_controller.rb index cc45e0b02..9a52a4331 100644 --- a/app/controllers/api/v1/aggregations_controller.rb +++ b/app/controllers/api/v1/aggregations_controller.rb @@ -1,12 +1,25 @@ -class Api::V1::AggregationsController < Api::ApiController - # include JsonApiController::PunditPolicy +# frozen_string_literal: true - # THIS FUNCTIONALITY IS BEING DEPRECATED EFFECTIVE IMMEDIATELY - # A REPLACEMENT IS FORTHCOMING. +class Api::V1::AggregationsController < Api::ApiController + include JsonApiController::PunditPolicy - # require_authentication :create, :update, scopes: [:project] - # resource_actions :create, :update, :show, :index - # schema_type :json_schema - # before_action :filter_by_subject_set, only: :index + require_authentication :index, :show, :update, :destroy, :create, scopes: [:project] + resource_actions :index, :show, :create, :update, :destroy + schema_type :json_schema + def create + workflow = Workflow.find(create_params['links']['workflow']) + project_id = workflow.project.id + create_params['links']['project'] = project_id + response = AggregationClient.new.send_aggregation_request( + project_id, + workflow.id, + create_params['links']['user'] + ) + super do |agg| + agg.update({ task_id: response.body[:task_id], status: 'pending' }) + end + rescue AggregationClient::ConnectionError + json_api_render(:service_unavailable, 'The aggregation service is unavailable or not responding') + end end diff --git a/app/models/aggregation.rb b/app/models/aggregation.rb index d7471888f..39cec2728 100644 --- a/app/models/aggregation.rb +++ b/app/models/aggregation.rb @@ -1,20 +1,16 @@ # frozen_string_literal: true class Aggregation < ApplicationRecord - belongs_to :workflow - belongs_to :subject - - validates_presence_of :workflow, :subject, :aggregation - validates_uniqueness_of :subject_id, scope: :workflow_id - validate :aggregation, :workflow_version_present - - private + belongs_to :project + belongs_to :user + validates :project, :workflow, :user, presence: true + validates :workflow, uniqueness: true - def workflow_version_present - wv_key = :workflow_version - if aggregation && !aggregation.symbolize_keys.has_key?(wv_key) - errors.add(:aggregation, "must have #{wv_key} metadata") - end - end + enum status: { + created: 0, + pending: 1, + completed: 2, + failed: 3 + } end diff --git a/app/models/subject.rb b/app/models/subject.rb index 2203c45bc..581baef30 100644 --- a/app/models/subject.rb +++ b/app/models/subject.rb @@ -17,7 +17,6 @@ class Subject < ApplicationRecord class_name: "Medium", as: :linked has_many :recents, dependent: :destroy - has_many :aggregations, dependent: :destroy has_many :tutorial_workflows, class_name: 'Workflow', foreign_key: 'tutorial_subject_id', diff --git a/app/models/workflow.rb b/app/models/workflow.rb index 64830ceea..dec249dd6 100644 --- a/app/models/workflow.rb +++ b/app/models/workflow.rb @@ -24,7 +24,7 @@ class Workflow < ApplicationRecord has_many :user_seen_subjects, dependent: :destroy has_many :workflow_tutorials, dependent: :destroy has_many :tutorials, through: :workflow_tutorials - has_many :aggregations, dependent: :destroy + has_one :aggregation, dependent: :destroy has_many :attached_images, -> { where(type: "workflow_attached_image") }, class_name: "Medium", as: :linked has_one :classifications_export, -> { where(type: "workflow_classifications_export").order(created_at: :desc) }, diff --git a/app/policies/aggregation_policy.rb b/app/policies/aggregation_policy.rb index 64f213f1e..f2d314290 100644 --- a/app/policies/aggregation_policy.rb +++ b/app/policies/aggregation_policy.rb @@ -1,27 +1,24 @@ +# frozen_string_literal: true + class AggregationPolicy < ApplicationPolicy - class ReadScope < Scope - # Short circuiting scopes for private aggrevations before they get removed next PR + class Scope < Scope def resolve(action) - parent_scope = policy_for(Workflow).scope_for(action) + parent_scope = policy_for(Workflow).scope_for(:update) scope.where(workflow_id: parent_scope.select(:id)) end end - class WriteScope < Scope - def resolve(action) - parent_scope = policy_for(Workflow).scope_for(action) - scope.where(workflow_id: parent_scope.select(:id)) - end - end + scope :index, :show, :create, :update, :destroy, with: Scope - scope :index, :show, with: ReadScope - scope :update, :destroy, :versions, :version, with: WriteScope + def linkable_workflows + policy_for(Workflow).scope_for(:update) + end - def linkable_subjects - policy_for(Subject).scope_for(:show) + def linkable_projects + policy_for(Project).scope_for(:update) end - def linkable_workflows - policy_for(Workflow).scope_for(:update) + def linkable_users + policy_for(User).scope_for(:update) end end diff --git a/app/schemas/aggregation_create_schema.rb b/app/schemas/aggregation_create_schema.rb index a649ae3a7..fee920d54 100644 --- a/app/schemas/aggregation_create_schema.rb +++ b/app/schemas/aggregation_create_schema.rb @@ -1,26 +1,35 @@ class AggregationCreateSchema < JsonSchema schema do - type "object" - description "An Aggregation for a subject" - required "links" + type 'object' + description 'An Aggregation for a workflow' + required 'links' additional_properties false - property "aggregation" do - type "object" + property 'uuid' do + type 'string' end - property "links" do - type "object" - additional_properties false + property 'task_id' do + type 'string' + end - required "subject", "workflow" + property 'status' do + type 'string' + end + + property 'links' do + type 'object' + required 'workflow' + additional_properties false - property "subject" do - type "integer", "string" + property 'workflow' do + type 'integer', 'string' + pattern '^[0-9]*$' end - property "workflow" do - type "integer", "string" + property 'user' do + type 'integer', 'string' + pattern '^[0-9]*$' end end end diff --git a/app/schemas/aggregation_update_schema.rb b/app/schemas/aggregation_update_schema.rb index f3c41837f..2830821ac 100644 --- a/app/schemas/aggregation_update_schema.rb +++ b/app/schemas/aggregation_update_schema.rb @@ -1,27 +1,19 @@ class AggregationUpdateSchema < JsonSchema schema do - type "object" - description "An Aggregation for a subject" - required "aggregation", "links" + type 'object' + description 'An Aggregation for a workflow' additional_properties false - property "aggregation" do - type "object" + property 'uuid' do + type 'string' end - property "links" do - type "object" - additional_properties false - - required "subject", "workflow" - - property "subject" do - type "integer", "string" - end + property 'task_id' do + type 'string' + end - property "workflow" do - type "integer", "string" - end + property 'status' do + type 'string' end end end diff --git a/app/serializers/aggregation_serializer.rb b/app/serializers/aggregation_serializer.rb index 498fc78c2..73db29b54 100644 --- a/app/serializers/aggregation_serializer.rb +++ b/app/serializers/aggregation_serializer.rb @@ -2,8 +2,8 @@ class AggregationSerializer include Serialization::PanoptesRestpack include CachedSerializer - attributes :id, :created_at, :updated_at, :aggregation, :href - can_include :workflow, :subject + attributes :id, :href, :created_at, :updated_at, :uuid, :task_id, :status + can_include :project, :workflow, :user - can_filter_by :workflow, :subject + can_filter_by :project, :workflow end diff --git a/app/services/aggregation_client.rb b/app/services/aggregation_client.rb new file mode 100644 index 000000000..afe19be3e --- /dev/null +++ b/app/services/aggregation_client.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +class AggregationClient + class ConnectionError < StandardError; end + class ResourceNotFound < ConnectionError; end + class ServerError < ConnectionError; end + + attr_reader :connection + + def initialize(adapter=Faraday.default_adapter) + @connection = connect!(adapter) + @host ||= ENV.fetch('AGGREGATION_HOST', 'http://test.example.com') + end + + def connect!(adapter) + Faraday.new(@host, ssl: { verify: false }) do |faraday| + faraday.request :json + faraday.response :json, content_type: /\bjson$/ + faraday.adapter(*adapter) + end + end + + def send_aggregation_request(project_id, workflow_id, user_id) + params = { project_id: project_id, workflow_id: workflow_id, user_id: user_id } + + request(:post, '/run_aggregation') do |req| + req.body = params.to_json + end + end + + private + + def request(http_method, params) + response = connection.send(http_method, *params) do |req| + req.headers['Accept'] = 'application/json' + req.headers['Content-Type'] = 'application/json' + req.options.timeout = 5 # open/read timeout in seconds + req.options.open_timeout = 2 # connection open timeout in seconds + yield req if block_given? + end + + handle_response(response) + rescue Faraday::TimeoutError, + Faraday::ConnectionFailed => e + raise ConnectionError, e.message + end + + def handle_response(response) + case response.status + when 404 + raise ResourceNotFound, status: response.status, body: response.body + when 400..600 + raise ServerError, response.body + else + response.body + end + end +end diff --git a/db/migrate/20240304201959_refactor_aggregation_model.rb b/db/migrate/20240304201959_refactor_aggregation_model.rb new file mode 100644 index 000000000..68a86aa34 --- /dev/null +++ b/db/migrate/20240304201959_refactor_aggregation_model.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class RefactorAggregationModel < ActiveRecord::Migration[6.1] + def up + # delete existing unused columns + safety_assured { remove_column :aggregations, :subject_id } + safety_assured { remove_column :aggregations, :aggregation } + + # and the new aggregations columns + add_column :aggregations, :project_id, :integer + add_foreign_key :aggregations, :projects, column: :project_id, validate: false + + add_column :aggregations, :user_id, :integer + add_foreign_key :aggregations, :users, column: :user_id, validate: false + + add_column :aggregations, :uuid, :string + add_column :aggregations, :task_id, :string + add_column :aggregations, :status, :integer, default: 0 + end + + def down + add_column :aggregations, :subject_id, :integer + add_column :aggregations, :aggregation, :jsonb + + remove_column :aggregations, :user_id + remove_column :aggregations, :project_id + remove_column :aggregations, :uuid + remove_column :aggregations, :task_id + remove_column :aggregations, :status + end +end diff --git a/db/structure.sql b/db/structure.sql index 6140bdc73..87c94a278 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -96,10 +96,13 @@ ALTER SEQUENCE public.access_control_lists_id_seq OWNED BY public.access_control CREATE TABLE public.aggregations ( id integer NOT NULL, workflow_id integer, - subject_id integer, - aggregation jsonb DEFAULT '{}'::jsonb NOT NULL, created_at timestamp without time zone, - updated_at timestamp without time zone + updated_at timestamp without time zone, + project_id integer, + user_id integer, + uuid character varying, + task_id character varying, + status integer DEFAULT 0 ); @@ -2801,13 +2804,6 @@ CREATE INDEX index_access_control_lists_on_resource_id_and_resource_type ON publ CREATE INDEX index_access_control_lists_on_user_group_id ON public.access_control_lists USING btree (user_group_id); --- --- Name: index_aggregations_on_subject_id_and_workflow_id; Type: INDEX; Schema: public; Owner: - --- - -CREATE UNIQUE INDEX index_aggregations_on_subject_id_and_workflow_id ON public.aggregations USING btree (subject_id, workflow_id); - - -- -- Name: index_aggregations_on_workflow_id; Type: INDEX; Schema: public; Owner: - -- @@ -3909,14 +3905,6 @@ ALTER TABLE ONLY public.subject_groups ADD CONSTRAINT fk_rails_283ede5252 FOREIGN KEY (project_id) REFERENCES public.projects(id); --- --- Name: aggregations fk_rails_28a7ada458; Type: FK CONSTRAINT; Schema: public; Owner: - --- - -ALTER TABLE ONLY public.aggregations - ADD CONSTRAINT fk_rails_28a7ada458 FOREIGN KEY (subject_id) REFERENCES public.subjects(id) ON UPDATE CASCADE ON DELETE CASCADE; - - -- -- Name: project_contents fk_rails_305e6d8bf1; Type: FK CONSTRAINT; Schema: public; Owner: - -- @@ -4069,6 +4057,14 @@ ALTER TABLE ONLY public.collections_projects ADD CONSTRAINT fk_rails_895b025564 FOREIGN KEY (project_id) REFERENCES public.projects(id); +-- +-- Name: aggregations fk_rails_8eb620b6f6; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.aggregations + ADD CONSTRAINT fk_rails_8eb620b6f6 FOREIGN KEY (user_id) REFERENCES public.users(id) NOT VALID; + + -- -- Name: set_member_subjects fk_rails_93073bf3b1; Type: FK CONSTRAINT; Schema: public; Owner: - -- @@ -4221,6 +4217,14 @@ ALTER TABLE ONLY public.organization_versions ADD CONSTRAINT fk_rails_be858ed31d FOREIGN KEY (organization_id) REFERENCES public.organizations(id); +-- +-- Name: aggregations fk_rails_c7d229ada4; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.aggregations + ADD CONSTRAINT fk_rails_c7d229ada4 FOREIGN KEY (project_id) REFERENCES public.projects(id) NOT VALID; + + -- -- Name: subject_set_imports fk_rails_d596712569; Type: FK CONSTRAINT; Schema: public; Owner: - -- @@ -4598,6 +4602,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20240216142515'), ('20240216171653'), ('20240216171937'), +('20240304201959'), ('20240531184258'); diff --git a/docs/source/includes/_aggregations.md b/docs/source/includes/_aggregations.md new file mode 100644 index 000000000..c8d22c0eb --- /dev/null +++ b/docs/source/includes/_aggregations.md @@ -0,0 +1,139 @@ +# Aggregations + +```json +{ + "aggregations": [ + { + "id": "1", + "href": "/aggregations/1", + "created_at": "2024-05-22T22:52:59.173Z", + "updated_at": "2024-05-22T22:52:59.181Z", + "uuid": "557ebcfa3c29496787336bfbd7c4d856", + "task_id": "9c963646-e7cd-4c5a-8749-c97086d416bd", + "status": "completed", + "links": { + "workflow": "2", + "user": "2" + } + } + ], + "links": { + "aggregations.workflow": { + "href": "/workflows/{aggregations.workflow}", + "type": "workflows" + }, + "aggregations.user": { + "href": "/users/{aggregations.user}", + "type": "users" + } + } +} +``` + +A single Aggregation resource object. This represents the automated aggregation by the external service by a _user_ of a single _workflow_ via the online Aggregation service. + +An Aggregation has the following attributes: + +Attribute | Type | Description +--------- | ---- | ----------- +id | integer | read-only +workflow_id | integer | read-only +project_id | integer | read-only +created_at | string | read-only +updated_at | string | read-only +user_id | integer | read-only +uuid | string | The unique identifier of the aggregation run, used in the URLs of output files +task_id | string | The Celery task ID, used to query the status of the backgrounded task on the aggregation service +status | integer | created, pending, completed, failed + + +## List aggregations + +```http +GET /api/aggregations HTTP/1.1 +Accept: application/vnd.api+json; version=1 +Content-Type: application/json +``` + +Only lists aggregations where the active user has has edit permissions on the related project. + +Any of the scopes may be further filtered using the *project_id*, *workflow_id* +and *user_id* parameters. + +### Parameters + ++ page (optional, integer) ... index of the page to retrieve, 1 is default ++ page_size (optional, integer) ... number of items on a page. 20 is default ++ sort (optional, string) ... fields to sort by. updated_at is default ++ project_id (optional, integer) ... only retrieve aggregations for a specific project ++ workflow_id (optional, integer) ... only retrieve aggregations for a specific workflow ++ user_id (optional, integer) ... only retrieve aggregations for a specific user ++ include (optional, string) ... comma separated list of linked resources to return in the response + + +## Retrieve a single Aggregation + +```http +GET /api/aggregations/123 HTTP/1.1 +Accept: application/vnd.api+json; version=1 +Content-Type: application/json +``` + +A User may only retrieve Aggregations they have permission to access. + +### Parameters + ++ `id` (required, integer) ... integer id of the resource to retrieve + + +## Create an Aggregation [POST] + +```http +POST /api/aggregations HTTP/1.1 +Accept: application/vnd.api+json; version=1 +Content-Type: application/json + +{ + "aggregations": { + "links": { + "user": "3", + "workflow": "2" + } + } +} +``` + +Creating a new Aggregation will initiate a new batch aggregation run. +A call out to the Aggregation Service is made and the newly created Aggregation resource +is updated with the Aggregation Service's Celery background task ID so that its status can be checked. + + +## Edit a single Aggregation [PUT] + +```http +PUT /api/aggregations/123 HTTP/1.1 +Accept: application/vnd.api+json; version=1 +Content-Type: application/json + +{ + "aggregations": { + "uuid": "557ebcfa3c29496787336bfbd7c4d856", + "status": "completed" + } +} +``` + +Aggregations are updated by the Aggregation Service when a run has succeeded or failed. A successful run +will send a request to this endpoint and update the UUID generated by that run. This UUID is a 32-character +lowercase hexadecimal string and can be used to construct the URL to download that run's output data. + +The status can also be updated via this route (see the _aggregations_ model) and can be the only attribute included. + +## Destroy a single Aggregation [DELETE] +```http +DELETE /api/Aggregations/123 HTTP/1.1 +Accept: application/vnd.api+json; version=1 +Content-Type: application/json +``` + +A user may destroy an Aggregation by ID if they have destroy permissions for the parent project. \ No newline at end of file diff --git a/spec/controllers/api/v1/aggregations_controller_spec.rb b/spec/controllers/api/v1/aggregations_controller_spec.rb new file mode 100644 index 000000000..71a494dfe --- /dev/null +++ b/spec/controllers/api/v1/aggregations_controller_spec.rb @@ -0,0 +1,116 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Api::V1::AggregationsController, type: :controller do + let(:api_resource_name) { 'aggregations' } + let(:api_resource_attributes) { %w[id created_at updated_at uuid task_id status] } + let(:api_resource_links) { %w[aggregations.workflow] } + + let(:scopes) { %w[project] } + let(:resource_class) { Aggregation } + + let(:workflow) { create(:workflow) } + let(:authorized_user) { workflow.project.owner } + let(:resource) { create(:aggregation, workflow: workflow) } + + describe '#index' do + let(:other_workflow) { create(:workflow) } + let!(:aggregations) { create(:aggregation, workflow: workflow) } + let!(:private_resource) { create(:aggregation, workflow: other_workflow) } + let(:authorized_user) { workflow.project.owner } + let(:n_visible) { 1 } + + it_behaves_like 'is indexable' + end + + describe '#show' do + it_behaves_like 'is showable' + end + + describe 'create' do + let(:test_attr) { :workflow_id } + let(:test_attr_value) { workflow.id } + let(:fake_response) { double } + let(:mock_agg) { instance_double(AggregationClient) } + + let(:create_params) do + { + aggregations: + { + links: { + user: authorized_user.id.to_s, + workflow: workflow.id.to_s + } + } + } + end + + before do + allow(AggregationClient).to receive(:new).and_return(mock_agg) + allow(fake_response).to receive(:body).and_return({ 'task_id': 'asdf-1234-asdf' }) + allow(mock_agg).to receive(:send_aggregation_request).and_return(fake_response) + default_request scopes: scopes, user_id: authorized_user.id + end + + it_behaves_like 'is creatable' + + it 'saves the project id' do + post :create, params: create_params + expect(Aggregation.first.project_id).to eq(workflow.project.id) + end + + it 'makes a request to the aggregation service' do + post :create, params: create_params + expect(mock_agg).to have_received(:send_aggregation_request) + end + + it 'stores the task_id from the client response' do + post :create, params: create_params + expect(Aggregation.first.task_id).to eq('asdf-1234-asdf') + end + + context 'when there is an existing aggregation for that workflow' do + let!(:existing_agg) { create(:aggregation, workflow: workflow) } + + before { post :create, params: create_params } + + it 'returns an error' do + expect(response).to have_http_status(:bad_request) + end + + it 'includes a validation error' do + expect(response.body).to include('Validation failed: Workflow has already been taken') + end + end + + context 'when the aggregation service is unavailable' do + before { allow(mock_agg).to receive(:send_aggregation_request).and_raise(AggregationClient::ConnectionError) } + + it 'sends back an error response' do + post :create, params: create_params + expect(response.status).to eq(503) + end + end + end + + describe '#update' do + let(:test_attr) { :uuid } + let(:test_attr_value) { '557ebcfa3c29496787336bfbd7c4d856' } + + let(:update_params) do + { aggregations: + { + uuid: '557ebcfa3c29496787336bfbd7c4d856', + status: 'completed' + } + } + end + + it_behaves_like 'is updatable' + end + + describe '#destroy' do + it_behaves_like 'is destructable' + end +end diff --git a/spec/factories/aggregations.rb b/spec/factories/aggregations.rb index d1a0c1b53..df01cb93f 100644 --- a/spec/factories/aggregations.rb +++ b/spec/factories/aggregations.rb @@ -1,7 +1,8 @@ FactoryBot.define do factory :aggregation do workflow - subject - aggregation { { data: "goes here", workflow_version: "1.1" } } + project + user + status { Aggregation.statuses[:pending] } end end diff --git a/spec/models/aggregation_spec.rb b/spec/models/aggregation_spec.rb index 608aa7137..f657cf2e2 100644 --- a/spec/models/aggregation_spec.rb +++ b/spec/models/aggregation_spec.rb @@ -1,58 +1,44 @@ +# frozen_string_literal: true + require 'spec_helper' -RSpec.describe Aggregation, :type => :model do +RSpec.describe Aggregation, type: :model do let(:aggregation) { build(:aggregation) } - it 'should have a valid factory' do + it 'has a valid factory' do expect(aggregation).to be_valid end - it 'should not be valid without an aggregation hash' do - expect(build(:aggregation, aggregation: nil)).to_not be_valid + it 'is not be valid without a workflow' do + expect(build(:aggregation, workflow: nil)).not_to be_valid end - it 'should not be valid without a workflow' do - expect(build(:aggregation, workflow: nil)).to_not be_valid + it 'is not be valid without a project' do + expect(build(:aggregation, project: nil)).not_to be_valid end - it 'should not be valid without a subject' do - expect(build(:aggregation, subject: nil)).to_not be_valid + it 'is not be valid without a user' do + expect(build(:aggregation, user: nil)).not_to be_valid end - context "when missing the workflow_version aggregation metadata" do - let(:no_workflow_version) { build(:aggregation, aggregation: { test: 1}) } - - it 'should not be valid' do - expect(no_workflow_version).to_not be_valid - end + context 'when there is a duplicate workflow_id' do + before { aggregation.save } - it 'should not have the correct error message' do - no_workflow_version.valid? - error_message = "must have workflow_version metadata" - expect(no_workflow_version.errors[:aggregation]).to include(error_message) - end - end - - context "when there is a duplicate subject_id workflow_id entry" do - before(:each) do - aggregation.save - end let(:duplicate) do - build(:aggregation, workflow: aggregation.workflow, - subject: aggregation.subject) + build(:aggregation, workflow: aggregation.workflow, user: aggregation.user) end - it "should not be valid" do - expect(duplicate).to_not be_valid + it 'is not be valid' do + expect(duplicate).not_to be_valid end - it "should have the correct error message on subject_id" do + it 'has the correct error message' do duplicate.valid? - expect(duplicate.errors[:subject_id]).to include("has already been taken") + expect(duplicate.errors.full_messages.first).to include('has already been taken') end - it "should raise a uniq index db error" do - expect{duplicate.save(validate: false)}.to raise_error(ActiveRecord::RecordNotUnique) + it 'raises a uniq index db error' do + expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid) end end end diff --git a/spec/policies/aggregation_policy_spec.rb b/spec/policies/aggregation_policy_spec.rb index 73f53ef49..5ebd50629 100644 --- a/spec/policies/aggregation_policy_spec.rb +++ b/spec/policies/aggregation_policy_spec.rb @@ -5,11 +5,14 @@ let(:anonymous_user) { nil } let(:logged_in_user) { create(:user) } let(:resource_owner) { create(:user) } - let(:project) { build(:project, owner: resource_owner) } - let(:public_aggregation) { build(:aggregation, workflow: build(:workflow, project: project)) } + let(:collaborator) { create(:user) } + + let(:project) { build(:project, owner: resource_owner) } + + let(:aggregation) { build(:aggregation, workflow: build(:workflow, project: project)) } before do - public_aggregation.save! + aggregation.save! end describe 'index' do @@ -17,40 +20,46 @@ Pundit.policy!(api_user, Aggregation).scope_for(:index) end - context 'for an anonymous user' do + context 'when the user is an anonymous user' do let(:api_user) { ApiUser.new(anonymous_user) } - it "includes aggregations from public projects" do - expect(resolved_scope).to match_array(public_aggregation) + it 'returns nothing' do + expect(resolved_scope).to be_empty end end - context 'for a normal user' do + context 'when the user is a normal user' do let(:api_user) { ApiUser.new(logged_in_user) } - it "includes aggregations from public projects" do - expect(resolved_scope).to match_array(public_aggregation) + it 'returns nothing' do + expect(resolved_scope).to be_empty end end - context 'for the resource owner' do - let(:api_user) { ApiUser.new(resource_owner) } + context 'when the user is a resource owner' do + let(:api_user) { ApiUser.new(collaborator) } - it "includes aggregations from public projects" do - expect(resolved_scope).to include(public_aggregation) + before { create :access_control_list, user_group: collaborator.identity_group, resource: project, roles: ['collaborator'] } + + it 'includes aggregation' do + expect(resolved_scope).to include(aggregation) end + end + + context 'when the user is a resource collaborators' do + let(:api_user) { ApiUser.new(resource_owner) } - xit 'includes aggregations from owned private projects' do - expect(resolved_scope).to include(private_aggregation) + it 'includes the aggregation' do + expect(resolved_scope).to include(aggregation) end end - context 'for an admin' do + context 'when the user is an admin' do let(:admin_user) { create :user, admin: true } let(:api_user) { ApiUser.new(admin_user, admin: true) } - it 'includes everything' do - expect(resolved_scope).to include(public_aggregation) + it 'includes the aggregation' do + expect(resolved_scope).to include(aggregation) end end end @@ -60,36 +69,36 @@ Pundit.policy!(api_user, Aggregation).scope_for(:update) end - context 'for an anonymous user' do + context 'when the user is an anonymous user' do let(:api_user) { ApiUser.new(anonymous_user) } - it "returns nothing" do + it 'returns nothing' do expect(resolved_scope).to be_empty end end - context 'for a normal user' do + context 'when the user is a normal user' do let(:api_user) { ApiUser.new(logged_in_user) } - it "returns nothing" do + it 'returns nothing' do expect(resolved_scope).to be_empty end end - context 'for the resource owner' do + context 'when the user is the resource owner' do let(:api_user) { ApiUser.new(resource_owner) } - it "includes aggregations from public projects" do - expect(resolved_scope).to include(public_aggregation) + it 'includes the aggregation' do + expect(resolved_scope).to include(aggregation) end end - context 'for an admin' do + context 'when the user is an admin' do let(:admin_user) { create :user, admin: true } let(:api_user) { ApiUser.new(admin_user, admin: true) } - it 'includes everything' do - expect(resolved_scope).to include(public_aggregation) + it 'includes the aggregation' do + expect(resolved_scope).to include(aggregation) end end end diff --git a/spec/serializers/aggregation_serializer_spec.rb b/spec/serializers/aggregation_serializer_spec.rb new file mode 100644 index 000000000..7b5a49cbd --- /dev/null +++ b/spec/serializers/aggregation_serializer_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AggregationSerializer do + let(:aggregation) { create(:aggregation) } + + it_behaves_like 'a panoptes restpack serializer' do + let(:resource) { aggregation } + let(:includes) { %i[project user workflow] } + let(:preloads) { [] } + end +end diff --git a/spec/services/aggregation_client_spec.rb b/spec/services/aggregation_client_spec.rb new file mode 100644 index 000000000..b5a03fb33 --- /dev/null +++ b/spec/services/aggregation_client_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AggregationClient do + describe '#send_aggregation_request' do + before do + allow(described_class).to receive(:host).and_return('http://test.example.com') + end + + let(:headers) { { 'Content-Type': 'application/json', 'Accept': 'application/json' } } + let(:params) { { project_id: 1, workflow_id: 10, user_id: 100 } } + let(:body) { { task_id: '1234-asdf-1234' } } + let(:path) { '/run_aggregation' } + + it 'was successful' do + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post(path, params.to_json, headers) do + [200, { 'Content-Type': 'application/json' }, body.to_json] + end + end + + response = described_class.new([:test, stubs]).send_aggregation_request(1, 10, 100) + expect(response).to eq(body.with_indifferent_access) + end + + context 'when there is a problem' do + describe 'handles server errors' do + it 'raises if it cannot connect' do + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post(path, params.to_json, headers) do + raise Faraday::ConnectionFailed, 'execution expired' + end + end + + expect do + described_class.new([:test, stubs]).send_aggregation_request(1, 10, 100) + end.to raise_error(AggregationClient::ConnectionError) + end + + it 'raises if it receives a 500' do + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post(path, params.to_json, headers) do + [ + 500, + { 'Content-Type': 'application/json' }, + { 'errors' => { 'detail' => 'Server internal error' } }.to_json + ] + end + end + + expect do + described_class.new([:test, stubs]).send_aggregation_request(1, 10, 100) + end.to raise_error(AggregationClient::ServerError) + end + + it 'raises if it times out' do + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post(path, params.to_json, headers) do + raise Faraday::TimeoutError + end + end + + expect do + described_class.new([:test, stubs]).send_aggregation_request(1, 10, 100) + end.to raise_error(AggregationClient::ConnectionError) + end + + it 'raises if response is an HTTP 404' do + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post(path, params.to_json, headers) do + [ + 404, + { 'Content-Type' => 'application/json' }, + { 'errors' => 'Not Found' }.to_json + ] + end + end + + expect do + described_class.new([:test, stubs]).send_aggregation_request(1, 10, 100) + end.to raise_error(AggregationClient::ResourceNotFound) + end + end + end + end +end