From 495813969928d28a49219560b9b8f7a26a65c70e Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Fri, 24 Jul 2020 14:56:42 +0200 Subject: [PATCH 1/2] Extend asset API upload to support base64 --- app/controllers/api/v1/assets_controller.rb | 22 ++- app/controllers/api/v1/results_controller.rb | 24 ++- .../requests/api/v1/assets_controller_spec.rb | 163 +++++++++++------- .../api/v1/results_controller_spec.rb | 83 +++++---- 4 files changed, 192 insertions(+), 100 deletions(-) diff --git a/app/controllers/api/v1/assets_controller.rb b/app/controllers/api/v1/assets_controller.rb index 37aabe49d..6d1798f7b 100644 --- a/app/controllers/api/v1/assets_controller.rb +++ b/app/controllers/api/v1/assets_controller.rb @@ -5,6 +5,7 @@ module Api class AssetsController < BaseController before_action :load_team, :load_project, :load_experiment, :load_task, :load_protocol, :load_step before_action :load_asset, only: :show + before_action :check_upload_type, only: :create def index attachments = @step.assets @@ -21,9 +22,18 @@ module Api def create raise PermissionError.new(Asset, :create) unless can_manage_protocol_in_module?(@protocol) - asset = @step.assets.new(asset_params) - asset.save!(context: :on_api_upload) + if @form_multipart_upload + asset = @step.assets.new(asset_params) + else + blob = ActiveStorage::Blob.create_after_upload!( + io: StringIO.new(Base64.decode64(asset_params[:file_data])), + filename: asset_params[:file_name], + content_type: asset_params[:content_type] + ) + asset = @step.assets.new(file: blob) + end + asset.save!(context: :on_api_upload) asset.post_process_file render jsonapi: asset, @@ -36,7 +46,9 @@ module Api def asset_params raise TypeError unless params.require(:data).require(:type) == 'attachments' - attr_list = %i(file) + return params.require(:data).require(:attributes).permit(:file) if @form_multipart_upload + + attr_list = %i(file_data content_type file_name) params.require(:data).require(:attributes).require(attr_list) params.require(:data).require(:attributes).permit(attr_list) end @@ -45,6 +57,10 @@ module Api @asset = @step.assets.find(params.require(:id)) raise PermissionError.new(Asset, :read) unless can_read_protocol_in_module?(@asset.step.protocol) end + + def check_upload_type + @form_multipart_upload = true if params.dig(:data, :attributes)[:file] + end end end end diff --git a/app/controllers/api/v1/results_controller.rb b/app/controllers/api/v1/results_controller.rb index 27d4d56e5..ae71175e9 100644 --- a/app/controllers/api/v1/results_controller.rb +++ b/app/controllers/api/v1/results_controller.rb @@ -103,7 +103,22 @@ module Api def create_file_result Result.transaction do @result = @task.results.create!(result_params.merge(user_id: current_user.id)) - asset = Asset.create!(result_file_params) + + if !result_file_params[:file] + filedata = Base64.decode64(result_file_params[:file_data]) + content_type = result_file_params[:content_type] + filename = result_file_params[:file_name] + + blob = ActiveStorage::Blob.create_after_upload!( + io: StringIO.new(filedata), + filename: filename, + content_type: content_type + ) + asset = Asset.create!(file: blob) + else # multipart form + asset = Asset.create!(result_file_params) + end + ResultAsset.create!(asset: asset, result: @result) end end @@ -139,8 +154,11 @@ module Api prms = params[:included]&.select { |el| el[:type] == 'result_files' }&.first return nil unless prms - prms.require(:attributes).require(:file) - prms.dig(:attributes).permit(:file) + return prms.dig(:attributes).permit(:file) if prms.require(:attributes)[:file] + + prms.require(:attributes).require(:file_data) + prms.require(:attributes).require(:file_name) + prms.dig(:attributes).permit(:file_data, :file_name) end def tiny_mce_asset_params diff --git a/spec/requests/api/v1/assets_controller_spec.rb b/spec/requests/api/v1/assets_controller_spec.rb index 8369648ae..4929ea721 100644 --- a/spec/requests/api/v1/assets_controller_spec.rb +++ b/spec/requests/api/v1/assets_controller_spec.rb @@ -27,13 +27,13 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do create(:step_asset, step: @step, asset: asset) get(api_v1_team_project_experiment_task_protocol_step_assets_path( - team_id: @team.id, - project_id: @project.id, - experiment_id: @experiment.id, - task_id: @task.id, - protocol_id: @protocol.id, - step_id: @step.id - ), headers: @valid_headers) + team_id: @team.id, + project_id: @project.id, + experiment_id: @experiment.id, + task_id: @task.id, + protocol_id: @protocol.id, + step_id: @step.id + ), headers: @valid_headers) expect(response).to have_http_status(200) end @@ -42,13 +42,13 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do context 'when protocol is not found' do it 'renders 404' do get(api_v1_team_project_experiment_task_protocol_step_assets_path( - team_id: @team.id, - project_id: @project.id, - experiment_id: @experiment.id, - task_id: @task.id, - protocol_id: -1, - step_id: @step.id - ), headers: @valid_headers) + team_id: @team.id, + project_id: @project.id, + experiment_id: @experiment.id, + task_id: @task.id, + protocol_id: -1, + step_id: @step.id + ), headers: @valid_headers) expect(response).to have_http_status(404) end @@ -61,14 +61,14 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do create(:step_asset, step: @step, asset: asset) get(api_v1_team_project_experiment_task_protocol_step_asset_path( - team_id: @team.id, - project_id: @project.id, - experiment_id: @experiment.id, - task_id: @task.id, - protocol_id: @protocol.id, - step_id: @step.id, - id: asset.id - ), headers: @valid_headers) + team_id: @team.id, + project_id: @project.id, + experiment_id: @experiment.id, + task_id: @task.id, + protocol_id: @protocol.id, + step_id: @step.id, + id: asset.id + ), headers: @valid_headers) expect(response).to have_http_status(200) end @@ -77,14 +77,14 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do context 'when experiment is not found' do it 'renders 404' do get(api_v1_team_project_experiment_task_protocol_step_asset_path( - team_id: @team.id, - project_id: @project.id, - experiment_id: -1, - task_id: @task.id, - protocol_id: @protocol.id, - step_id: @step.id, - id: asset.id - ), headers: @valid_headers) + team_id: @team.id, + project_id: @project.id, + experiment_id: -1, + task_id: @task.id, + protocol_id: @protocol.id, + step_id: @step.id, + id: asset.id + ), headers: @valid_headers) expect(response).to have_http_status(404) end @@ -93,14 +93,14 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do context 'when asset is not found' do it 'renders 404' do get(api_v1_team_project_experiment_task_protocol_step_asset_path( - team_id: @team.id, - project_id: @project.id, - experiment_id: @experiment.id, - task_id: @task.id, - protocol_id: @protocol.id, - step_id: @step.id, - id: -1 - ), headers: @valid_headers) + team_id: @team.id, + project_id: @project.id, + experiment_id: @experiment.id, + task_id: @task.id, + protocol_id: @protocol.id, + step_id: @step.id, + id: -1 + ), headers: @valid_headers) expect(response).to have_http_status(404) end @@ -113,49 +113,82 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do end before :each do - @file = fixture_file_upload('files/test.jpg', 'image/jpg') allow_any_instance_of(Asset).to receive(:post_process_file) end let(:action) do post(api_v1_team_project_experiment_task_protocol_step_assets_path( - team_id: @team.id, - project_id: @project.id, - experiment_id: @experiment.id, - task_id: @task.id, - protocol_id: @protocol.id, - step_id: @step.id - ), + team_id: @team.id, + project_id: @project.id, + experiment_id: @experiment.id, + task_id: @task.id, + protocol_id: @protocol.id, + step_id: @step.id + ), params: request_body, headers: @valid_headers) end + let(:request_body) do + { + data: { + type: 'attachments', + attributes: attributes + } + } + end context 'when has valid params' do - let(:request_body) do - { - data: { - type: 'attachments', - attributes: { - file: @file - } + context 'when multipart form' do + let(:file) { fixture_file_upload('files/test.jpg', 'image/jpg') } + let(:attributes) { { file: file } } + + it 'creates new asset' do + expect { action }.to change { Asset.count }.by(1) + end + + it 'returns status 201' do + action + + expect(response).to have_http_status 201 + end + + it 'calls post_process_file function for text extraction' do + expect_any_instance_of(Asset).to receive(:post_process_file) + + action + end + end + + context 'when base64 with json' do + let(:filedata_base64) do + 'iVBORw0KGgoAAAANSUhEUgAAAAIAA'\ + 'AACCAIAAAD91JpzAAAACXBIWXMAAC4jAAAuIwF4pT92AAAAE0lE'\ + 'QVQIHWP8//8/AwMDExADAQAkBgMBOOSShwAAAABJRU5ErkJggg==' + end + let(:attributes) do + { + file_data: filedata_base64, + file_name: 'file.png', + content_type: 'image/png' } - } - end + end + let(:request_body) { super().to_json } - it 'creates new asset' do - expect { action }.to change { Asset.count }.by(1) - end + it 'creates new asset' do + expect { action }.to change { Asset.count }.by(1) + end - it 'returns status 201' do - action + it 'returns status 201' do + action - expect(response).to have_http_status 201 - end + expect(response).to have_http_status 201 + end - it 'calls post_process_file function for text extraction' do - expect_any_instance_of(Asset).to receive(:post_process_file) + it 'calls post_process_file function for text extraction' do + expect_any_instance_of(Asset).to receive(:post_process_file) - action + action + end end end diff --git a/spec/requests/api/v1/results_controller_spec.rb b/spec/requests/api/v1/results_controller_spec.rb index 2e36e4079..962db0ecd 100644 --- a/spec/requests/api/v1/results_controller_spec.rb +++ b/spec/requests/api/v1/results_controller_spec.rb @@ -268,7 +268,6 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do end context 'when resultType is File' do - let(:file) { fixture_file_upload('files/test.jpg', 'image/jpg') } let(:request_body) do { data: { @@ -277,31 +276,57 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do name: 'my result' } }, - included: [ - { type: 'result_files', - attributes: { - file: file - } } - ] + included: [{ type: 'result_files', attributes: attributes }] } end + let(:action) do post(api_v1_team_project_experiment_task_results_path( - team_id: @teams.first.id, - project_id: @valid_project, - experiment_id: @valid_experiment, - task_id: @valid_task - ), params: request_body, headers: @valid_headers) + team_id: @teams.first.id, + project_id: @valid_project, + experiment_id: @valid_experiment, + task_id: @valid_task + ), params: request_body, headers: @valid_headers) end - it 'creates new asset' do - expect { action }.to change { ResultAsset.count }.by(1) + context 'when file is base64' do + let(:filedata_base64) do + 'iVBORw0KGgoAAAANSUhEUgAAAAIAA'\ + 'AACCAIAAAD91JpzAAAACXBIWXMAAC4jAAAuIwF4pT92AAAAE0lE'\ + 'QVQIHWP8//8/AwMDExADAQAkBgMBOOSShwAAAABJRU5ErkJggg==' + end + let(:attributes) do + { + file_data: filedata_base64, + file_name: 'file.png', + content_type: 'image/png' + } + end + let(:request_body) { super().to_json } + + it 'creates new asset' do + expect { action }.to change { ResultAsset.count }.by(1) + end + + it 'returns status 201' do + action + + expect(response).to have_http_status 201 + end end - it 'returns status 201' do - action + context 'when file is multipart form' do + let(:attributes) { { file: fixture_file_upload('files/test.jpg', 'image/jpg') } } - expect(response).to have_http_status 201 + it 'creates new asset' do + expect { action }.to change { ResultAsset.count }.by(1) + end + + it 'returns status 201' do + action + + expect(response).to have_http_status 201 + end end end end @@ -390,12 +415,12 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do end let(:action) do put(api_v1_team_project_experiment_task_result_path( - team_id: @teams.first.id, - project_id: @valid_project, - experiment_id: @valid_experiment, - task_id: @valid_task, - id: result_file.id - ), params: request_body, headers: @valid_headers) + team_id: @teams.first.id, + project_id: @valid_project, + experiment_id: @valid_experiment, + task_id: @valid_task, + id: result_file.id + ), params: request_body, headers: @valid_headers) end context 'when has attributes for update' do @@ -459,12 +484,12 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do it 'returns 204' do put(api_v1_team_project_experiment_task_result_path( - team_id: @teams.first.id, - project_id: @valid_project, - experiment_id: @valid_experiment, - task_id: @valid_task, - id: result_file.id - ), params: request_body_with_same_name.to_json, headers: @valid_headers) + team_id: @teams.first.id, + project_id: @valid_project, + experiment_id: @valid_experiment, + task_id: @valid_task, + id: result_file.id + ), params: request_body_with_same_name.to_json, headers: @valid_headers) expect(response).to have_http_status 204 end From 9b06510dcc1ca802a9c1632ad6d405b0f046d9d9 Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Sat, 25 Jul 2020 11:41:26 +0200 Subject: [PATCH 2/2] Update PUT result file endpoint, refactor --- app/controllers/api/v1/assets_controller.rb | 6 +- app/controllers/api/v1/results_controller.rb | 47 +++++----- .../requests/api/v1/assets_controller_spec.rb | 18 +--- .../api/v1/results_controller_spec.rb | 89 +++++++++++-------- 4 files changed, 85 insertions(+), 75 deletions(-) diff --git a/app/controllers/api/v1/assets_controller.rb b/app/controllers/api/v1/assets_controller.rb index 6d1798f7b..358e49708 100644 --- a/app/controllers/api/v1/assets_controller.rb +++ b/app/controllers/api/v1/assets_controller.rb @@ -28,7 +28,7 @@ module Api blob = ActiveStorage::Blob.create_after_upload!( io: StringIO.new(Base64.decode64(asset_params[:file_data])), filename: asset_params[:file_name], - content_type: asset_params[:content_type] + content_type: asset_params[:file_type] ) asset = @step.assets.new(file: blob) end @@ -48,7 +48,7 @@ module Api return params.require(:data).require(:attributes).permit(:file) if @form_multipart_upload - attr_list = %i(file_data content_type file_name) + attr_list = %i(file_data file_type file_name) params.require(:data).require(:attributes).require(attr_list) params.require(:data).require(:attributes).permit(attr_list) end @@ -59,7 +59,7 @@ module Api end def check_upload_type - @form_multipart_upload = true if params.dig(:data, :attributes)[:file] + @form_multipart_upload = true if params.dig(:data, :attributes, :file) end end end diff --git a/app/controllers/api/v1/results_controller.rb b/app/controllers/api/v1/results_controller.rb index ae71175e9..6d6bcde2d 100644 --- a/app/controllers/api/v1/results_controller.rb +++ b/app/controllers/api/v1/results_controller.rb @@ -17,7 +17,6 @@ module Api def create create_text_result if result_text_params.present? - create_file_result if !@result && result_file_params.present? render jsonapi: @result, @@ -31,6 +30,7 @@ module Api update_file_result if result_file_params.present? && @result.is_asset update_text_result if result_text_params.present? && @result.is_text + if (@result.changed? && @result.save!) || @asset_result_updated render jsonapi: @result, serializer: ResultSerializer, @@ -103,22 +103,12 @@ module Api def create_file_result Result.transaction do @result = @task.results.create!(result_params.merge(user_id: current_user.id)) - - if !result_file_params[:file] - filedata = Base64.decode64(result_file_params[:file_data]) - content_type = result_file_params[:content_type] - filename = result_file_params[:file_name] - - blob = ActiveStorage::Blob.create_after_upload!( - io: StringIO.new(filedata), - filename: filename, - content_type: content_type - ) - asset = Asset.create!(file: blob) - else # multipart form + if @form_multipart_upload asset = Asset.create!(result_file_params) + else + blob = create_blob_from_params + asset = Asset.create!(file: blob) end - ResultAsset.create!(asset: asset, result: @result) end end @@ -127,12 +117,26 @@ module Api old_checksum, new_checksum = nil Result.transaction do old_checksum = @result.asset.file.blob.checksum - @result.asset.file.attach(result_file_params[:file]) + if @form_multipart_upload + @result.asset.file.attach(result_file_params[:file]) + else + blob = create_blob_from_params + @result.asset.update!(file: blob) + end new_checksum = @result.asset.file.blob.checksum end @asset_result_updated = old_checksum != new_checksum end + def create_blob_from_params + blob = ActiveStorage::Blob.create_after_upload!( + io: StringIO.new(Base64.decode64(result_file_params[:file_data])), + filename: result_file_params[:file_name], + content_type: result_file_params[:file_type] + ) + blob + end + def result_params raise TypeError unless params.require(:data).require(:type) == 'results' @@ -154,11 +158,14 @@ module Api prms = params[:included]&.select { |el| el[:type] == 'result_files' }&.first return nil unless prms - return prms.dig(:attributes).permit(:file) if prms.require(:attributes)[:file] + if prms.require(:attributes)[:file] + @form_multipart_upload = true + return prms.dig(:attributes).permit(:file) + end + attr_list = %i(file_data file_type file_name) - prms.require(:attributes).require(:file_data) - prms.require(:attributes).require(:file_name) - prms.dig(:attributes).permit(:file_data, :file_name) + prms.require(:attributes).require(attr_list) + prms.dig(:attributes).permit(attr_list) end def tiny_mce_asset_params diff --git a/spec/requests/api/v1/assets_controller_spec.rb b/spec/requests/api/v1/assets_controller_spec.rb index 4929ea721..725c04d37 100644 --- a/spec/requests/api/v1/assets_controller_spec.rb +++ b/spec/requests/api/v1/assets_controller_spec.rb @@ -165,13 +165,7 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do 'AACCAIAAAD91JpzAAAACXBIWXMAAC4jAAAuIwF4pT92AAAAE0lE'\ 'QVQIHWP8//8/AwMDExADAQAkBgMBOOSShwAAAABJRU5ErkJggg==' end - let(:attributes) do - { - file_data: filedata_base64, - file_name: 'file.png', - content_type: 'image/png' - } - end + let(:attributes) { { file_data: filedata_base64, file_name: 'file.png', file_type: 'image/png' } } let(:request_body) { super().to_json } it 'creates new asset' do @@ -193,15 +187,7 @@ RSpec.describe 'Api::V1::AssetsController', type: :request do end context 'when has missing param' do - let(:request_body) do - { - data: { - type: 'attachments', - attributes: { - } - } - } - end + let(:request_body) { { data: { type: 'attachments', attributes: {} } } } it 'renders 400' do action diff --git a/spec/requests/api/v1/results_controller_spec.rb b/spec/requests/api/v1/results_controller_spec.rb index 962db0ecd..8e0745f0a 100644 --- a/spec/requests/api/v1/results_controller_spec.rb +++ b/spec/requests/api/v1/results_controller_spec.rb @@ -289,7 +289,7 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do ), params: request_body, headers: @valid_headers) end - context 'when file is base64' do + context 'when sending base64' do let(:filedata_base64) do 'iVBORw0KGgoAAAANSUhEUgAAAAIAA'\ 'AACCAIAAAD91JpzAAAACXBIWXMAAC4jAAAuIwF4pT92AAAAE0lE'\ @@ -299,7 +299,7 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do { file_data: filedata_base64, file_name: 'file.png', - content_type: 'image/png' + file_type: 'image/png' } end let(:request_body) { super().to_json } @@ -315,7 +315,7 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do end end - context 'when file is multipart form' do + context 'when sending multipart form' do let(:attributes) { { file: fixture_file_upload('files/test.jpg', 'image/jpg') } } it 'creates new asset' do @@ -396,7 +396,6 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do context 'when resultType is file' do let(:result_file) { @valid_task.results.last } let(:file) { fixture_file_upload('files/test.jpg', 'image/jpg') } - let(:second_file) { fixture_file_upload('files/apple.jpg', 'image/jpg') } let(:request_body) do { data: { @@ -437,38 +436,56 @@ RSpec.describe 'Api::V1::ResultsController', type: :request do end end - # ### Refactor without instance variables - # - # context 'when has new image for update' do - # let(:request_body_with_same_name_new_file) do - # { - # data: { - # type: 'results', - # attributes: { - # name: result_file.reload.name - # } - # }, - # included: [ - # { type: 'result_files', - # attributes: { - # file: second_file - # } } - # ] - # } - # end - # - # it 'returns status 200' do - # put(api_v1_team_project_experiment_task_result_path( - # team_id: @teams.first.id, - # project_id: @valid_project, - # experiment_id: @valid_experiment, - # task_id: @valid_task, - # id: result_file.id - # ), params: request_body_with_same_name_new_file, headers: @valid_headers) - # - # expect(response).to have_http_status 200 - # end - # end + context 'when has new image for update' do + let(:action) do + put(api_v1_team_project_experiment_task_result_path( + team_id: @teams.first.id, + project_id: @valid_project, + experiment_id: @valid_experiment, + task_id: @valid_task, + id: result_file.id + ), params: request_body, headers: @valid_headers) + end + + let(:request_body) do + { + data: { type: 'results', attributes: { name: result_file.reload.name } }, + included: [{ type: 'result_files', attributes: attributes }] + } + end + + context 'when sending base64' do + let(:filedata_base64) do + 'iVBORw0KGgoAAAANSUhEUgAAAAIAA'\ + 'AACCAIAAAD91JpzAAAACXBIWXMAAC4jAAAuIwF4pT92AAAAE0lE'\ + 'QVQIHWP8//8/AwMDExADAQAkBgMBOOSShwAAAABJRU5ErkJggg==' + end + let(:attributes) do + { + file_data: filedata_base64, + file_name: 'file.png', + file_type: 'image/png' + } + end + let(:request_body) { super().to_json } + + it 'returns status 200' do + action + + expect(response).to have_http_status 200 + end + end + + context 'when sending multipart form' do + let(:attributes) { { file: fixture_file_upload('files/apple.jpg', 'image/jpg') } } + + it 'returns status 200' do + action + + expect(response).to have_http_status 200 + end + end + end context 'when there is nothing to update' do let(:request_body_with_same_name) do