diff --git a/api_design/DDS-974-enforce_upload_chunk_size.md b/api_design/DDS-974-enforce_upload_chunk_size.md new file mode 100644 index 000000000..1b4b1d1c9 --- /dev/null +++ b/api_design/DDS-974-enforce_upload_chunk_size.md @@ -0,0 +1,174 @@ +# DDS-974 Enforce Upload Chunk Size + +## Deployment View + +**NOTE** The following must be done **before** the circle build + +In order for rake db:data:migrate to work, the following ENV must be set in all heroku applications: + - heroku config:set SWIFT_CHUNK_MAX_NUMBER=1000 + - heroku config:set SWIFT_CHUNK_MAX_SIZE_BYTES=5368709122 + +We must back up the Postgresql database! This is because we had to change the +size fields in uploads/chunks from int to bigint to allow for large files. + +## Logical View + +A critical issue was recently exposed with the chunked upload capability. The storage provider (Swift), is configured with a set maximum number of segments (i.e. Data Service chunks), beyond which it cannot coalesce a large file. To remedy this situation, the API will be extended to enforce a minumum chunk size, based on the overall file size and max segments setting. + +#### Summary of impacted APIs + +|Endpoint |Description | +|---|---| +| `GET /storage_providers` | List supported storage providers. | +| `GET /storage_providers/{id}` | Get a storage provider. | +| `POST /projects/{id}/uploads` | Inititate a chunked upload. | +| `PUT /uploads/{id}/chunks` | Generate and return a pre-signed upload URL for a chunk. | + +#### API Specification +This section defines the proposed API interface extensions. + +##### List supported storage providers / Get a storage provider +`GET /storage_providers` / `GET /storage_providers/{id}` + +###### Response Example (Extensions) +The following properties will be added to the storage providers resource: + ++ **chunk\_max\_size\_bytes** - The maximum size of a chunk that can be upload. ++ **chunk\_max\_number** - The maximum number of chunks that can be uploaded for a single file. ++ **file\_max\_size\_bytes** - Maximum supported file size that can be uploaded. (`chunk_max_size_bytes * chunk_max_number`) + +``` +{ + "id": "g5579f73-0558-4f96-afc7-9d251e65bv33", + "name": "duke_oit_swift", + "description": "Duke OIT Storage", + "chunk_hash_algorithm": "md5", + "chunk_max_size_bytes": 5368709120, + "chunk_max_number": 1000, + "file_max_size_bytes": 5497558138880, + "is_deprecated": false +} +``` + +##### Intitate a chunked upload +`POST /projects/{id}/uploads` + +###### Response Headers (Extensions) +The following custom response headers will be added to inform clients of the minimum chunk size that may be utlized to ensure chunks can be coalesced, as well as the maximum chunk size the storage provider can accommodate. + ++ **X-MIN-CHUNK-UPLOAD-SIZE** - The minimum chunk size in bytes. ++ **X-MAX-CHUNK-UPLOAD-SIZE** - The maximum chunk size in bytes. + +###### Response Messages (Extensions) ++ 400 - File size is currently not supported - maximum size is {max_segments * max_chunk_upload_size} + +###### Response Example +``` +{ + error: '400', + code: "not_provided", + reason: 'validation failed', + suggestion: 'Fix the following invalid fields and resubmit', + errors: + [ + { + "size": "File size is currently not supported - maximum size is {max_segments * max_chunk_upload_size}" + } + ] +} +``` + +##### Generate and return a pre-signed upload URL for a chunk +`PUT /uploads/{id}/chunks` + +###### Response Messages (Extensions) ++ 400 - Invalid chunk size specified - must be in range {min}-{max} ++ 400 - Upload chunks exceeded, must be less than {max} + +###### Response Example +``` +{ + error: '400', + code: "not_provided", + reason: 'validation failed', + suggestion: 'Fix the following invalid fields and resubmit', + errors: + [ + { + "size": "Invalid chunk size specified - must be in range {min}-{max}" + } + ] +} +``` +or +``` +{ + error: '400', + code: "not_provided", + reason: 'maximum upload chunks exceeded.', + suggestion: '' +} +``` + +## Implementation View + ++ The offcial GCB python client and DDS Web portal client will need to be modifed to interface with these chunked upload API extensions. + ++ The Swift `max_manifest_segements` will be set to 2000 and all uploads that are inconsistent due to exceeding the prior setting of 1000, will be re-queued for processing. + +## Process View + +Add notes about performance, scalability, throughput, etc. here. These can inform future proposals to change the implementation. + +This design introduces a change the the error response for validation errors. +Most validation_error responses will remain unchanged, reporting a list of field +errors that must be addressed: +``` +{ + error: '400', + code: "not_provided", + reason: 'validation failed', + suggestion: 'Fix the following invalid fields and resubmit', + errors: + [ + { + "field": "something is wrong with this" + } + ] +} +``` + +Some validation errors happen for the entire object, and not for any specific +field, such as when a user attempts to delete a property or template that is +associated with an object, or create a chunk that exceeds the storage_provider +maximum_chunk_number. + +In the past, these errors would have come in the list of 'errors', labeled +`base`: +``` +{ + error: '400', + code: "not_provided", + reason: 'validation failed', + suggestion: 'Fix the following invalid fields and resubmit', + errors: + [ + { + "base": "something is wrong with this" + } + ] +} +``` + +Going forward, these object errors will be placed into `reason`, and the response +payload may or may not have other fields that are invalid as well. If there are +no invalid fields, the suggestion will be a blank string, and there will not be +an errors entry in the payload. +``` +{ + error: '400', + code: "not_provided", + reason: 'something is wrong with this.', + suggestion: '' +} +``` diff --git a/app/api/dds/v1/base.rb b/app/api/dds/v1/base.rb index d4905f23c..3be65b5ef 100644 --- a/app/api/dds/v1/base.rb +++ b/app/api/dds/v1/base.rb @@ -104,17 +104,24 @@ def validation_error!(object) error: '400', code: "not_provided", reason: 'validation failed', - suggestion: 'Fix the following invalid fields and resubmit', - errors: [] + suggestion: '' } - object.errors.messages.each do |field, errors| + unless object.errors.messages[:base].empty? + error_payload[:reason] = object.errors.messages[:base].join(' ') + end + field_errors = [] + object.errors.messages.reject{|field| field == :base }.each do |field, errors| errors.each do |message| - error_payload[:errors] << { + field_errors << { field: field, message: message } end end + unless field_errors.empty? + error_payload[:errors] = field_errors + error_payload[:suggestion] = 'Fix the following invalid fields and resubmit' + end error!(error_payload, 400) end diff --git a/app/api/dds/v1/uploads_api.rb b/app/api/dds/v1/uploads_api.rb index 25a32cb43..59b34ce0a 100644 --- a/app/api/dds/v1/uploads_api.rb +++ b/app/api/dds/v1/uploads_api.rb @@ -37,6 +37,8 @@ class UploadsAPI < Grape::API }) authorize upload, :create? if upload.save + header 'X-MIN-CHUNK-UPLOAD-SIZE', upload.minimum_chunk_size + header 'X-MAX-CHUNK-UPLOAD-SIZE', upload.max_size_bytes upload else validation_error!(upload) diff --git a/app/models/chunk.rb b/app/models/chunk.rb index 7214ba365..f9430a937 100644 --- a/app/models/chunk.rb +++ b/app/models/chunk.rb @@ -11,13 +11,24 @@ class Chunk < ActiveRecord::Base has_many :project_permissions, through: :upload validates :upload_id, presence: true - validates :number, presence: true, + validates :number, presence: true, uniqueness: {scope: [:upload_id], case_sensitive: false} validates :size, presence: true + validates :size, numericality: { + less_than: :chunk_max_size_bytes, + greater_than_or_equal_to: :minimum_chunk_size, + message: ->(object, data) do + "Invalid chunk size specified - must be in range #{object.minimum_chunk_size}-#{object.chunk_max_size_bytes}" + end + }, if: :storage_provider + validates :fingerprint_value, presence: true validates :fingerprint_algorithm, presence: true - delegate :project_id, to: :upload + validate :upload_chunk_maximum, if: :storage_provider + + delegate :project_id, :minimum_chunk_size, to: :upload + delegate :chunk_max_size_bytes, to: :storage_provider def http_verb 'PUT' @@ -47,8 +58,18 @@ def url storage_provider.build_signed_url(http_verb, sub_path, expiry) end + def total_chunks + upload.chunks.count + end + private + def upload_chunk_maximum + unless total_chunks < storage_provider.chunk_max_number + errors[:base] << 'maximum upload chunks exceeded.' + end + end + def update_upload_etag last_audit = self.audits.last new_comment = last_audit.comment ? last_audit.comment.merge({raised_by_audit: last_audit.id}) : {raised_by_audit: last_audit.id} diff --git a/app/models/storage_provider.rb b/app/models/storage_provider.rb index f3e009db5..ae24df4e4 100644 --- a/app/models/storage_provider.rb +++ b/app/models/storage_provider.rb @@ -10,7 +10,9 @@ class StorageProvider < ActiveRecord::Base validates :service_pass, presence: true validates :primary_key, presence: true validates :secondary_key, presence: true - + validates :chunk_max_number, presence: true + validates :chunk_max_size_bytes, presence: true + def auth_token call_auth_uri['x-auth-token'] end diff --git a/app/models/upload.rb b/app/models/upload.rb index 44c8317d1..c3c8e79c1 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -14,8 +14,14 @@ class Upload < ActiveRecord::Base validates :project_id, presence: true validates :name, presence: true - validates :size, presence: true validates :storage_provider_id, presence: true + validates :size, presence: true + validates :size, numericality: { + less_than: :max_size_bytes, + message: ->(object, data) do + "File size is currently not supported - maximum size is #{object.max_size_bytes}" + end + }, if: :storage_provider validates :creator_id, presence: true validates :completed_at, immutable: true, if: :completed_at_was validates :completed_at, immutable: true, if: :error_at_was @@ -93,6 +99,14 @@ def create_and_validate_storage_manifest end end + def max_size_bytes + storage_provider.chunk_max_number * storage_provider.chunk_max_size_bytes + end + + def minimum_chunk_size + (size.to_f / storage_provider.chunk_max_number).ceil + end + private def integrity_exception(message) exactly_now = DateTime.now diff --git a/app/serializers/storage_provider_serializer.rb b/app/serializers/storage_provider_serializer.rb index 9dda01ddd..a4bf57256 100644 --- a/app/serializers/storage_provider_serializer.rb +++ b/app/serializers/storage_provider_serializer.rb @@ -1,5 +1,6 @@ class StorageProviderSerializer < ActiveModel::Serializer - attributes :id, :name, :description, :is_deprecated, :chunk_hash_algorithm + attributes :id, :name, :description, :is_deprecated, :chunk_hash_algorithm, + :chunk_max_number, :chunk_max_size_bytes def name object.display_name diff --git a/db/migrate/20170911204850_add_chunk_max_number_chunk_max_size_bytes_to_storage_providers.rb b/db/migrate/20170911204850_add_chunk_max_number_chunk_max_size_bytes_to_storage_providers.rb new file mode 100644 index 000000000..e24280c9e --- /dev/null +++ b/db/migrate/20170911204850_add_chunk_max_number_chunk_max_size_bytes_to_storage_providers.rb @@ -0,0 +1,6 @@ +class AddChunkMaxNumberChunkMaxSizeBytesToStorageProviders < ActiveRecord::Migration[5.0] + def change + add_column :storage_providers, :chunk_max_number, :integer + add_column :storage_providers, :chunk_max_size_bytes, :integer, limit: 8 + end +end diff --git a/db/migrate/20170912182841_change_chunk_size.rb b/db/migrate/20170912182841_change_chunk_size.rb new file mode 100644 index 000000000..236207f7c --- /dev/null +++ b/db/migrate/20170912182841_change_chunk_size.rb @@ -0,0 +1,5 @@ +class ChangeChunkSize < ActiveRecord::Migration[5.0] + def change + change_column :chunks, :size, :integer, limit: 8 + end +end diff --git a/db/schema.rb b/db/schema.rb index 5f49613c3..c169d202a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170622204323) do +ActiveRecord::Schema.define(version: 20170912182841) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -98,7 +98,7 @@ create_table "chunks", id: :uuid, default: -> { "uuid_generate_v4()" }, force: :cascade do |t| t.uuid "upload_id" t.integer "number" - t.integer "size" + t.bigint "size" t.string "fingerprint_value" t.string "fingerprint_algorithm" t.datetime "created_at", null: false @@ -264,6 +264,8 @@ t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "chunk_hash_algorithm", default: "md5" + t.integer "chunk_max_number" + t.bigint "chunk_max_size_bytes" end create_table "system_permissions", id: :uuid, default: -> { "uuid_generate_v4()" }, force: :cascade do |t| diff --git a/lib/tasks/db_data_migrate.rake b/lib/tasks/db_data_migrate.rake index 9336de0d0..91bca6fd9 100644 --- a/lib/tasks/db_data_migrate.rake +++ b/lib/tasks/db_data_migrate.rake @@ -108,6 +108,26 @@ def migrate_nil_consistency_status puts "#{updated_uploads} uploads updated." end +def migrate_storage_provider_chunk_environment + bad_storage_providers = StorageProvider.where( + chunk_max_size_bytes: nil, + chunk_max_number: nil + ) + + if bad_storage_providers.count > 0 + if (ENV['SWIFT_CHUNK_MAX_NUMBER'] && ENV['SWIFT_CHUNK_MAX_SIZE_BYTES']) + bad_storage_providers.each do |bad_storage_provider| + bad_storage_provider.update( + chunk_max_size_bytes: ENV['SWIFT_CHUNK_MAX_SIZE_BYTES'], + chunk_max_number: ENV['SWIFT_CHUNK_MAX_NUMBER'] + ) + end + else + $stderr.puts 'please set ENV[SWIFT_CHUNK_MAX_NUMBER] AND ENV[SWIFT_CHUNK_MAX_SIZE_BYTES]' + end + end +end + namespace :db do namespace :data do desc "Migrate existing data to fit current business rules" @@ -116,6 +136,7 @@ namespace :db do create_missing_fingerprints type_untyped_authentication_services migrate_nil_consistency_status + migrate_storage_provider_chunk_environment end end end diff --git a/spec/factories/chunks.rb b/spec/factories/chunks.rb index 0e80f17f0..7ae8b33ab 100644 --- a/spec/factories/chunks.rb +++ b/spec/factories/chunks.rb @@ -2,7 +2,7 @@ factory :chunk do upload sequence(:number, 1000) - size { Faker::Number.digit } + size { Faker::Number.between(100, 1000) } fingerprint_value { SecureRandom.hex(32) } fingerprint_algorithm "md5" diff --git a/spec/factories/storage_providers.rb b/spec/factories/storage_providers.rb index 78905c959..4016ac9df 100644 --- a/spec/factories/storage_providers.rb +++ b/spec/factories/storage_providers.rb @@ -11,6 +11,8 @@ primary_key { SecureRandom.hex } secondary_key { SecureRandom.hex } chunk_hash_algorithm { Faker::Hacker.abbreviation } + chunk_max_number { Faker::Number.between(100,1000) } + chunk_max_size_bytes { Faker::Number.between(4368709122, 6368709122) } trait :swift do name { ENV['SWIFT_ACCT'] || Faker::Name.name } @@ -22,6 +24,12 @@ service_user { ENV["SWIFT_USER"] || Faker::Internet.user_name } service_pass { ENV['SWIFT_PASS'] || Faker::Internet.password } chunk_hash_algorithm { ENV['SWIFT_CHUNK_HASH_ALGORITHM'] || Faker::Hacker.abbreviation } + chunk_max_number { ENV['SWIFT_CHUNK_MAX_NUMBER'] || Faker::Number.between(100,1000) } + chunk_max_size_bytes { ENV['SWIFT_CHUNK_MAX_SIZE_BYTES'] || Faker::Number.between(4368709122, 6368709122) } + end + + trait :skip_validation do + to_create {|instance| instance.save(validate: false) } end end end diff --git a/spec/lib/tasks/db_data_migrate_rake_spec.rb b/spec/lib/tasks/db_data_migrate_rake_spec.rb index a3b6ba19c..5cd163877 100644 --- a/spec/lib/tasks/db_data_migrate_rake_spec.rb +++ b/spec/lib/tasks/db_data_migrate_rake_spec.rb @@ -152,5 +152,92 @@ it_behaves_like 'a consistency migration', :init_upload end end + + describe 'migrate_storage_provider_chunk_environment' do + let(:bad_storage_providers) { + StorageProvider.where( + chunk_max_size_bytes: nil, + chunk_max_number: nil + ).count + } + + context 'a storage_provider has nil chunk_max_size_bytes, chunk_max_number' do + let(:storage_provider) { + FactoryGirl.create(:storage_provider, :skip_validation, + chunk_max_size_bytes: nil, + chunk_max_number: nil + ) + } + + context 'ENV includes SWIFT_CHUNK_MAX_NUMBER and SWIFT_CHUNK_MAX_SIZE_BYTES' do + include_context 'with env_override' + let(:env_override) { { + 'SWIFT_CHUNK_MAX_NUMBER' => 1, + 'SWIFT_CHUNK_MAX_SIZE_BYTES' => 5 + } } + + it { + expect(storage_provider).to be_persisted + expect(bad_storage_providers).to be > 0 + expect{invoke_task}.to change{ + StorageProvider.where( + chunk_max_size_bytes: nil, + chunk_max_number: nil + ).count + }.by(-bad_storage_providers) + } + end + + context 'ENV does not include SWIFT_CHUNK_MAX_NUMBER and SWIFT_CHUNK_MAX_SIZE_BYTES' do + before do + expect(ENV['SWIFT_CHUNK_MAX_NUMBER']).to be_nil + expect(ENV['SWIFT_CHUNK_MAX_SIZE_BYTES']).to be_nil + end + + it { + expect(storage_provider).to be_persisted + expect(bad_storage_providers).to be > 0 + invoke_task expected_stderr: /please set ENV\[SWIFT_CHUNK_MAX_NUMBER\] AND ENV\[SWIFT_CHUNK_MAX_SIZE_BYTES\]/ + } + end + end + + context 'no storage_providers have nil chunk_max_size_bytes, chunk_max_number' do + let(:storage_provider) { FactoryGirl.create(:storage_provider) } + + context 'ENV includes SWIFT_CHUNK_MAX_NUMBER and SWIFT_CHUNK_MAX_SIZE_BYTES' do + include_context 'with env_override' + let(:env_override) { { + 'SWIFT_CHUNK_MAX_NUMBER' => 1, + 'SWIFT_CHUNK_MAX_SIZE_BYTES' => 5 + } } + + it { + expect(storage_provider).to be_persisted + expect(bad_storage_providers).to eq 0 + expect {invoke_task}.not_to change{ StorageProvider.where( + chunk_max_size_bytes: nil, + chunk_max_number: nil + ).count } + } + end + + context 'ENV does not include SWIFT_CHUNK_MAX_NUMBER and SWIFT_CHUNK_MAX_SIZE_BYTES' do + before do + expect(ENV['SWIFT_CHUNK_MAX_NUMBER']).to be_nil + expect(ENV['SWIFT_CHUNK_MAX_SIZE_BYTES']).to be_nil + end + + it { + expect(storage_provider).to be_persisted + expect(bad_storage_providers).to eq 0 + expect {invoke_task}.not_to change{ StorageProvider.where( + chunk_max_size_bytes: nil, + chunk_max_number: nil + ).count } + } + end + end + end end end diff --git a/spec/models/chunk_spec.rb b/spec/models/chunk_spec.rb index 6e1fc789b..fcf57d3e3 100644 --- a/spec/models/chunk_spec.rb +++ b/spec/models/chunk_spec.rb @@ -22,40 +22,74 @@ it { is_expected.to validate_presence_of(:upload_id) } it { is_expected.to validate_presence_of(:number) } it { is_expected.to validate_presence_of(:size) } + it { + is_expected.to validate_numericality_of(:size) + .is_less_than(subject.chunk_max_size_bytes) + .is_greater_than_or_equal_to(subject.minimum_chunk_size) + .with_message("Invalid chunk size specified - must be in range #{subject.upload.minimum_chunk_size}-#{subject.chunk_max_size_bytes}") + } it { is_expected.to validate_presence_of(:fingerprint_value) } it { is_expected.to validate_presence_of(:fingerprint_algorithm) } it { is_expected.to validate_uniqueness_of(:number).scoped_to(:upload_id).case_insensitive } + + describe 'upload_chunk_maximum' do + let(:storage_provider) { FactoryGirl.create(:storage_provider, chunk_max_number: 1) } + context '< storage_provider.chunk_max_number' do + let(:upload) { FactoryGirl.create(:upload, storage_provider: storage_provider) } + subject { FactoryGirl.build(:chunk, upload: upload) } + it { is_expected.to be_valid } + end + + context '>= storage_provider.chunk_max_number' do + let(:upload) { FactoryGirl.create(:upload, :with_chunks, storage_provider: storage_provider) } + let(:expected_validation_message) { "maximum upload chunks exceeded." } + subject { FactoryGirl.build(:chunk, upload: upload, number: 2) } + + it { + is_expected.not_to be_valid + expect(subject.errors.messages[:base]).to include expected_validation_message + } + end + end end describe 'instance methods' do - it 'should delegate project_id to upload' do - should delegate_method(:project_id).to(:upload) + it 'is_expected.to delegate project_id to upload' do + is_expected.to delegate_method(:project_id).to(:upload) expect(subject.project_id).to eq(subject.upload.project_id) end - it 'should have a http_verb method' do - should respond_to :http_verb + it { is_expected.to delegate_method(:chunk_max_size_bytes).to(:storage_provider) } + it { is_expected.to delegate_method(:minimum_chunk_size).to(:upload) } + + it 'is_expected.to have a http_verb method' do + is_expected.to respond_to :http_verb expect(subject.http_verb).to eq 'PUT' end - it 'should have a host method' do - should respond_to :host + it 'is_expected.to have a host method' do + is_expected.to respond_to :host expect(subject.host).to eq storage_provider.url_root end - it 'should have a http_headers method' do - should respond_to :http_headers + it 'is_expected.to have a http_headers method' do + is_expected.to respond_to :http_headers expect(subject.http_headers).to eq [] end - it 'should have an object_path method' do - should respond_to :object_path + it 'is_expected.to have an object_path method' do + is_expected.to respond_to :object_path expect(subject.object_path).to eq(expected_object_path) end end - it 'should have a url method' do - should respond_to :url + it 'is_expected.to have a url method' do + is_expected.to respond_to :url expect(subject.url).to eq expected_url end + + describe '#total_chunks' do + it { is_expected.to respond_to :total_chunks } + it { expect(subject.total_chunks).to eq(subject.upload.chunks.count ) } + end end diff --git a/spec/models/storage_provider_spec.rb b/spec/models/storage_provider_spec.rb index 87f883af8..bb75b483a 100644 --- a/spec/models/storage_provider_spec.rb +++ b/spec/models/storage_provider_spec.rb @@ -299,17 +299,19 @@ describe 'validations' do it 'should require attributes' do - should validate_presence_of :name - should validate_presence_of :display_name - should validate_uniqueness_of :display_name - should validate_presence_of :description - should validate_presence_of :url_root - should validate_presence_of :provider_version - should validate_presence_of :auth_uri - should validate_presence_of :service_user - should validate_presence_of :service_pass - should validate_presence_of :primary_key - should validate_presence_of :secondary_key + is_expected.to validate_presence_of :name + is_expected.to validate_presence_of :display_name + is_expected.to validate_uniqueness_of :display_name + is_expected.to validate_presence_of :description + is_expected.to validate_presence_of :url_root + is_expected.to validate_presence_of :provider_version + is_expected.to validate_presence_of :auth_uri + is_expected.to validate_presence_of :service_user + is_expected.to validate_presence_of :service_pass + is_expected.to validate_presence_of :primary_key + is_expected.to validate_presence_of :secondary_key + is_expected.to validate_presence_of :chunk_max_number + is_expected.to validate_presence_of :chunk_max_size_bytes end end end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index d59dcd95a..8e6641d41 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -26,6 +26,11 @@ it { is_expected.to validate_presence_of :project_id } it { is_expected.to validate_presence_of :name } it { is_expected.to validate_presence_of :size } + it { + is_expected.to validate_numericality_of(:size) + .is_less_than(subject.max_size_bytes) + .with_message("File size is currently not supported - maximum size is #{subject.max_size_bytes}") + } it { is_expected.to validate_presence_of :storage_provider_id } it { is_expected.to validate_presence_of :creator_id } @@ -172,6 +177,38 @@ end end + describe '#max_size_bytes' do + let(:expected_max_size_bytes) { subject.storage_provider.chunk_max_number * subject.storage_provider.chunk_max_size_bytes } + it { is_expected.to respond_to :max_size_bytes } + it { expect(subject.max_size_bytes).to eq(expected_max_size_bytes) } + end + + describe '#minimum_chunk_size' do + let(:storage_provider) { FactoryGirl.create(:storage_provider) } + let(:size) { storage_provider.chunk_max_number } + subject { FactoryGirl.create(:upload, storage_provider: storage_provider, size: size) } + let(:expected_minimum_chunk_size) { + (subject.size.to_f / subject.storage_provider.chunk_max_number).ceil + } + + it { is_expected.to respond_to :minimum_chunk_size } + + context 'size = 0' do + let(:size) { 0 } + it { expect(subject.minimum_chunk_size).to eq(expected_minimum_chunk_size) } + end + + context 'size < storage_provider.chunk_max_number' do + let(:size) { storage_provider.chunk_max_number - 1 } + it { expect(subject.minimum_chunk_size).to eq(expected_minimum_chunk_size) } + end + + context 'size > storage_provider.chunk_max_number' do + let(:size) { storage_provider.chunk_max_number + 1 } + it { expect(subject.minimum_chunk_size).to eq(expected_minimum_chunk_size) } + end + end + describe 'swift methods', :vcr do subject { FactoryGirl.create(:upload, :swift, :with_chunks) } diff --git a/spec/requests/properties_api_spec.rb b/spec/requests/properties_api_spec.rb index 4e3268503..23085188e 100644 --- a/spec/requests/properties_api_spec.rb +++ b/spec/requests/properties_api_spec.rb @@ -193,7 +193,11 @@ context 'with associated meta_property' do include_context 'elasticsearch prep', [:resource], [] before { FactoryGirl.create(:meta_property, property: resource) } - it_behaves_like 'a validated resource' + it_behaves_like 'a validated resource' do + let(:expected_reason) { 'The property cannot be deleted if it has been associated to one or more DDS objects.' } + let(:expected_suggestion) { '' } + let(:expects_errors) { false } + end end end end diff --git a/spec/requests/templates_api_spec.rb b/spec/requests/templates_api_spec.rb index 598f4ee54..dc720adb3 100644 --- a/spec/requests/templates_api_spec.rb +++ b/spec/requests/templates_api_spec.rb @@ -180,7 +180,11 @@ end context 'with associated meta_template' do before { FactoryGirl.create(:meta_template, template: resource) } - it_behaves_like 'a validated resource' + it_behaves_like 'a validated resource' do + let(:expected_reason) { 'The template cannot be deleted if it has been associated to one or more DDS objects.' } + let(:expected_suggestion) { '' } + let(:expects_errors) { false } + end end it_behaves_like 'an identified resource' do let(:resource_id) { "doesNotExist" } diff --git a/spec/requests/uploads_api_spec.rb b/spec/requests/uploads_api_spec.rb index db732f716..235339804 100644 --- a/spec/requests/uploads_api_spec.rb +++ b/spec/requests/uploads_api_spec.rb @@ -62,11 +62,21 @@ size: upload_stub.size }} + let(:expected_response_headers) {{ + 'X-MIN-CHUNK-UPLOAD-SIZE' => upload_stub.minimum_chunk_size, + 'X-MAX-CHUNK-UPLOAD-SIZE' => upload_stub.max_size_bytes + }} + it_behaves_like 'a creatable resource' do it 'should set creator' do is_expected.to eq(expected_response_status) expect(new_object.creator_id).to eq(current_user.id) end + + it 'should return chunk-upload-size response headers' do + is_expected.to eq(expected_response_status) + expect(response.headers.to_h).to include(expected_response_headers) + end end it_behaves_like 'a validated resource' do @@ -136,7 +146,7 @@ describe 'PUT' do subject { put(url, params: payload.to_json, headers: headers) } let(:called_action) { "PUT" } - let!(:payload) {{ + let(:payload) {{ number: payload_chunk_number, size: chunk_stub.size, hash: { @@ -223,6 +233,40 @@ let(:resource_class) { Chunk } end end + + context 'chunk size too large' do + before do + chunk_stub.size = chunk_stub.chunk_max_size_bytes + 1 + end + it_behaves_like 'a validated resource' do + it 'should not persist changes' do + expect(resource).to be_persisted + expect { + is_expected.to eq(400) + }.not_to change{resource_class.count} + end + end + end + + context 'storage_provider.chunk_max_number exceeded' do + let(:other_chunk) { FactoryGirl.create(:chunk, upload_id: upload.id, number: 2) } + before do + storage_provider.update(chunk_max_number: 1) + end + + it_behaves_like 'a validated resource' do + let(:expected_reason) { 'maximum upload chunks exceeded.' } + let(:expected_suggestion) { '' } + let(:expects_errors) { false } + + it 'should not persist changes' do + expect(resource).to be_persisted + expect { + is_expected.to eq(400) + }.not_to change{resource_class.count} + end + end + end end end diff --git a/spec/serializers/storage_provider_serializer_spec.rb b/spec/serializers/storage_provider_serializer_spec.rb index d076454a8..7e4266780 100644 --- a/spec/serializers/storage_provider_serializer_spec.rb +++ b/spec/serializers/storage_provider_serializer_spec.rb @@ -7,7 +7,9 @@ 'name' => resource.display_name, 'description' => resource.description, 'is_deprecated' => resource.is_deprecated, - 'chunk_hash_algorithm' => resource.chunk_hash_algorithm + 'chunk_hash_algorithm' => resource.chunk_hash_algorithm, + 'chunk_max_number' => resource.chunk_max_number, + 'chunk_max_size_bytes' => resource.chunk_max_size_bytes }} it_behaves_like 'a json serializer' do diff --git a/spec/support/requests_shared_examples.rb b/spec/support/requests_shared_examples.rb index b2fc27f2a..b936d6cf9 100644 --- a/spec/support/requests_shared_examples.rb +++ b/spec/support/requests_shared_examples.rb @@ -342,6 +342,10 @@ end shared_examples 'a validated resource' do + let(:expected_reason) { 'validation failed' } + let(:expected_suggestion) { 'Fix the following invalid fields and resubmit' } + let(:expects_errors) { true } + it 'returns a failed response' do is_expected.to eq(400) expect(response.status).to eq(400) @@ -356,15 +360,17 @@ expect(response_json).to have_key('error') expect(response_json['error']).to eq('400') expect(response_json).to have_key('reason') - expect(response_json['reason']).to eq('validation failed') + expect(response_json['reason']).to eq(expected_reason) expect(response_json).to have_key('suggestion') - expect(response_json['suggestion']).to eq('Fix the following invalid fields and resubmit') - expect(response_json).to have_key('errors') - expect(response_json['errors']).to be_a(Array) - expect(response_json['errors']).not_to be_empty - response_json['errors'].each do |error| - expect(error).to have_key('field') - expect(error).to have_key('message') + expect(response_json['suggestion']).to eq(expected_suggestion) + if expects_errors + expect(response_json).to have_key('errors') + expect(response_json['errors']).to be_a(Array) + expect(response_json['errors']).not_to be_empty + response_json['errors'].each do |error| + expect(error).to have_key('field') + expect(error).to have_key('message') + end end end end diff --git a/swift.local.env b/swift.local.env index 55b3ddab5..d50601eb8 100644 --- a/swift.local.env +++ b/swift.local.env @@ -9,3 +9,5 @@ SWIFT_PASS=testing SWIFT_PRIMARY_KEY=5ea5d3ec4111586633e58b60ac1f542c96778ee51bce23602368ab5303df63db52239993cef8881fb78e0b39346d2ac11aac833b899aa4283dc3bb0659c2ef05 SWIFT_SECONDARY_KEY=1e8de2158d75b148f96d563e332b450fb7210b57f4bd76b8588d6dbc5cf445f47dc71bd4cf50d2693f144ba423ef4389a83757f4fdcecb35943ee67d2be81c0f SWIFT_CHUNK_HASH_ALGORITHM=md5 +SWIFT_CHUNK_MAX_NUMBER=1000 +SWIFT_CHUNK_MAX_SIZE_BYTES=5368709122