From 72b829083f5456ab51f70ad0f049d76e304b954e Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Tue, 9 Feb 2021 10:59:45 -0700 Subject: [PATCH 1/5] Active Storage - Direct Upload Validation Co-authored-by: Abhishek Chandrasekhar --- .../lib/action_view/helpers/form_helper.rb | 2 +- .../action_view/helpers/form_tag_helper.rb | 9 +- actionview/test/template/form_helper_test.rb | 26 ++ .../test/template/form_tag_helper_test.rb | 10 + activestorage/CHANGELOG.md | 80 ++++ activestorage/README.md | 23 + .../assets/javascripts/activestorage.esm.js | 15 +- .../app/assets/javascripts/activestorage.js | 15 +- .../direct_uploads_controller.rb | 9 +- .../javascript/activestorage/blob_record.js | 5 +- .../javascript/activestorage/direct_upload.js | 5 +- .../activestorage/direct_upload_controller.js | 6 +- .../app/models/active_storage/blob.rb | 27 +- activestorage/lib/active_storage.rb | 4 + activestorage/lib/active_storage/engine.rb | 3 + .../lib/active_storage/locale/en.yml | 8 + .../lib/active_storage/validations.rb | 68 +++ .../attachment_byte_size_validator.rb | 98 ++++ .../attachment_content_type_validator.rb | 72 +++ .../validations/base_validator.rb | 98 ++++ .../direct_uploads_controller_test.rb | 100 ++++ .../attachment_byte_size_validator_test.rb | 398 ++++++++++++++++ .../attachment_content_type_validator_test.rb | 439 ++++++++++++++++++ guides/source/active_storage_overview.md | 50 ++ 24 files changed, 1549 insertions(+), 21 deletions(-) create mode 100644 activestorage/lib/active_storage/locale/en.yml create mode 100644 activestorage/lib/active_storage/validations.rb create mode 100644 activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb create mode 100644 activestorage/lib/active_storage/validations/attachment_content_type_validator.rb create mode 100644 activestorage/lib/active_storage/validations/base_validator.rb create mode 100644 activestorage/test/models/validations/attachment_byte_size_validator_test.rb create mode 100644 activestorage/test/models/validations/attachment_content_type_validator_test.rb diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index 9ca576759bd28..060426bb3e3da 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -1242,7 +1242,7 @@ def hidden_field(object_name, method, options = {}) def file_field(object_name, method, options = {}) options = { include_hidden: multiple_file_field_include_hidden }.merge!(options) - Tags::FileField.new(object_name, method, self, convert_direct_upload_option_to_url(options.dup)).render + Tags::FileField.new(object_name, method, self, add_direct_upload_attributes(options.dup)).render end # Returns a textarea opening and closing tag set tailored for accessing a specified attribute (identified by +method+) diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index 7d08cf8d0174d..302f959409676 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -342,7 +342,7 @@ def hidden_field_tag(name, value = nil, options = {}) # file_field_tag 'file', accept: 'text/html', class: 'upload', value: 'index.html' # # => def file_field_tag(name, options = {}) - text_field_tag(name, nil, convert_direct_upload_option_to_url(options.merge(type: :file))) + text_field_tag(name, nil, add_direct_upload_attributes(options.merge(type: :file))) end # Creates a password field, a masked text field that will hide the users input behind a mask character. @@ -984,9 +984,14 @@ def set_default_disable_with(value, tag_options) tag_options.delete("data-disable-with") end - def convert_direct_upload_option_to_url(options) + def add_direct_upload_attributes(options) if options.delete(:direct_upload) && respond_to?(:rails_direct_uploads_url) options["data-direct-upload-url"] = rails_direct_uploads_url + if options[:direct_upload_model] + options["data-direct-upload-model"] = options.delete(:direct_upload_model) + elsif options[:object] + options["data-direct-upload-model"] = options[:object].class.name + end end options end diff --git a/actionview/test/template/form_helper_test.rb b/actionview/test/template/form_helper_test.rb index d08f00cd36c35..6d13202831598 100644 --- a/actionview/test/template/form_helper_test.rb +++ b/actionview/test/template/form_helper_test.rb @@ -597,6 +597,13 @@ def test_file_field_with_direct_upload_when_rails_direct_uploads_url_is_defined assert_dom_equal expected, file_field("import", "file", direct_upload: true) end + def test_file_field_with_direct_upload_when_rails_direct_uploads_url_is_defined_and_model_is_provided + @controller = WithActiveStorageRoutesControllers.new + + expected = '' + assert_dom_equal expected, file_field("import", "file", direct_upload: true, object: Post.new) + end + def test_file_field_with_direct_upload_dont_mutate_arguments original_options = { class: "pix", direct_upload: true } @@ -3764,6 +3771,25 @@ def test_default_form_builder ActionView::Base.default_form_builder = old_default_form_builder end + def test_form_builder_includes_direct_upload_attributes + old_default_form_builder, ActionView::Base.default_form_builder = + ActionView::Base.default_form_builder, LabelledFormBuilder + + @controller = WithActiveStorageRoutesControllers.new + + form_for(@post) do |f| + concat f.file_field(:title, direct_upload: true) + end + + expected = whole_form("/posts/123", "edit_post_123", "edit_post", method: "patch", multipart: true) do + "
" \ + end + + assert_dom_equal expected, output_buffer + ensure + ActionView::Base.default_form_builder = old_default_form_builder + end + def test_lazy_loading_default_form_builder old_default_form_builder, ActionView::Base.default_form_builder = ActionView::Base.default_form_builder, "FormHelperTest::LabelledFormBuilder" diff --git a/actionview/test/template/form_tag_helper_test.rb b/actionview/test/template/form_tag_helper_test.rb index cf86dbdb88e51..1b492b6dafdd4 100644 --- a/actionview/test/template/form_tag_helper_test.rb +++ b/actionview/test/template/form_tag_helper_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "abstract_unit" +require "controller/fake_models" class FormTagHelperTest < ActionView::TestCase include RenderERBUtils @@ -320,6 +321,15 @@ def test_file_field_tag_with_direct_upload_when_rails_direct_uploads_url_is_defi ) end + def test_file_field_tag_with_direct_upload_when_rails_direct_uploads_url_is_defined_and_model_is_provided + @controller = WithActiveStorageRoutesControllers.new + + assert_dom_equal( + "", + file_field_tag("picsplz", class: "pix", direct_upload: true, direct_upload_model: "Post") + ) + end + def test_file_field_tag_with_direct_upload_dont_mutate_arguments original_options = { class: "pix", direct_upload: true } diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 0f6a55f436575..cc3b5ff1c8d47 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,5 +1,85 @@ +* Introduce Active Storage validators. Subclasses of `ActiveStorage::Validations::BaseValidator` run before creating a + `Blob` on direct upload, and before saving an `Attachment` via direct or indirect uploads. Includes built in validators + for content type and byte size. + + See https://github.com/rails/rails/pull/41178 or the Active Storage guide for examples. + + *Abhishek Chandrasekhar*, *Alex Ghiculescu* + * Fixes multiple `attach` calls within transaction not uploading files correctly. +* Attachments can be deleted after their association is no longer defined. + + Fixes #42514 + + *Don Sisco* + +* Make `vips` the default variant processor for new apps. + + See the upgrade guide for instructions on converting from `mini_magick` to `vips`. `mini_magick` is + not deprecated, existing apps can keep using it. + + *Breno Gazzola* + +* Deprecate `ActiveStorage::Current.host` in favor of `ActiveStorage::Current.url_options` which accepts + a host, protocol and port. + + *Santiago Bartesaghi* + +* Allow using [IAM](https://cloud.google.com/storage/docs/access-control/signed-urls) when signing URLs with GCS. + + ```yaml + gcs: + service: GCS + ... + iam: true + ``` + + *RRethy* + +* OpenSSL constants are now used for Digest computations. + + *Dirkjan Bussink* + +* Deprecate `config.active_storage.replace_on_assign_to_many`. Future versions of Rails + will behave the same way as when the config is set to `true`. + + *Santiago Bartesaghi* + +* Remove deprecated methods: `build_after_upload`, `create_after_upload!` in favor of `create_and_upload!`, + and `service_url` in favor of `url`. + + *Santiago Bartesaghi* + +* Add support of `strict_loading_by_default` to `ActiveStorage::Representations` controllers. + + *Anton Topchii*, *Andrew White* + +* Allow to detach an attachment when record is not persisted. + + *Jacopo Beschi* + +* Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips`. + + *Breno Gazzola* + +* Add metadata value for presence of video channel in video blobs. + + The `metadata` attribute of video blobs has a new boolean key named `video` that is set to + `true` if the file has an video channel and `false` if it doesn't. + + *Breno Gazzola* + +* Deprecate usage of `purge` and `purge_later` from the association extension. + + *Jacopo Beschi* + +* Passing extra parameters in `ActiveStorage::Blob#url` to S3 Client. + + This allows calls of `ActiveStorage::Blob#url` to have more interaction with + the S3 Presigner, enabling, amongst other options, custom S3 domain URL + Generation. + In the following example, the code failed to upload all but the last file to the configured service. ```ruby ActiveRecord::Base.transaction do diff --git a/activestorage/README.md b/activestorage/README.md index 67709c8c2d220..f710248063221 100644 --- a/activestorage/README.md +++ b/activestorage/README.md @@ -105,6 +105,29 @@ Variation of image attachment: <%= image_tag user.avatar.variant(resize_to_limit: [100, 100]) %> ``` +## Validations + +Active Storage includes attachment validators for the following properties: + +* Byte Size +* Content Type + +```ruby +class User < ActiveRecord::Base + has_one_attached :avatar + # Validating Size + # Accepts options for: `:in`, `:minimum`, `:maximum` + validates :avatar, attachment_byte_size: { in: 0..1.megabyte } + validates :avatar, attachment_byte_size: 0..1.megabyte + # Validating Content Type + # Accepts options for: `:in`, `:not` + validates :avatar, attachment_content_type: { in: %w[image/jpeg image/png] } + validates :avatar, attachment_content_type: "image/jpeg" +end +``` + +See the [rails guides](https://edgeguides.rubyonrails.org/active_storage_overview.html#validations) for more information. + ## File serving strategies Active Storage supports two ways to serve files: redirecting and proxying. diff --git a/activestorage/app/assets/javascripts/activestorage.esm.js b/activestorage/app/assets/javascripts/activestorage.esm.js index caa3a14860bdf..7740659906a42 100644 --- a/activestorage/app/assets/javascripts/activestorage.esm.js +++ b/activestorage/app/assets/javascripts/activestorage.esm.js @@ -508,13 +508,14 @@ function toArray(value) { } class BlobRecord { - constructor(file, checksum, url) { + constructor(file, checksum, url, model) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, - checksum: checksum + checksum: checksum, + model: model }; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); @@ -604,11 +605,12 @@ class BlobUpload { let id = 0; class DirectUpload { - constructor(file, url, delegate) { + constructor(file, url, delegate, model) { this.id = ++id; this.file = file; this.url = url; this.delegate = delegate; + this.model = model; } create(callback) { FileChecksum.create(this.file, ((error, checksum) => { @@ -616,7 +618,7 @@ class DirectUpload { callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url); + const blob = new BlobRecord(this.file, checksum, this.url, this.model); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -647,7 +649,7 @@ class DirectUploadController { constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this); + this.directUpload = new DirectUpload(this.file, this.url, this, this.model); this.dispatch("initialize"); } start(callback) { @@ -678,6 +680,9 @@ class DirectUploadController { get url() { return this.input.getAttribute("data-direct-upload-url"); } + get model() { + return this.input.getAttribute("data-direct-upload-model"); + } dispatch(name, detail = {}) { detail.file = this.file; detail.id = this.directUpload.id; diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js index 9e6a5bf79799b..ad6219b706ea5 100644 --- a/activestorage/app/assets/javascripts/activestorage.js +++ b/activestorage/app/assets/javascripts/activestorage.js @@ -503,13 +503,14 @@ } } class BlobRecord { - constructor(file, checksum, url) { + constructor(file, checksum, url, model) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, - checksum: checksum + checksum: checksum, + model: model }; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); @@ -596,11 +597,12 @@ } let id = 0; class DirectUpload { - constructor(file, url, delegate) { + constructor(file, url, delegate, model) { this.id = ++id; this.file = file; this.url = url; this.delegate = delegate; + this.model = model; } create(callback) { FileChecksum.create(this.file, ((error, checksum) => { @@ -608,7 +610,7 @@ callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url); + const blob = new BlobRecord(this.file, checksum, this.url, this.model); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -637,7 +639,7 @@ constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this); + this.directUpload = new DirectUpload(this.file, this.url, this, this.model); this.dispatch("initialize"); } start(callback) { @@ -668,6 +670,9 @@ get url() { return this.input.getAttribute("data-direct-upload-url"); } + get model() { + return this.input.getAttribute("data-direct-upload-model"); + } dispatch(name, detail = {}) { detail.file = this.file; detail.id = this.directUpload.id; diff --git a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb index 99634597f3f90..bff2698b68fa3 100644 --- a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb +++ b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb @@ -5,8 +5,13 @@ # the blob that was created up front. class ActiveStorage::DirectUploadsController < ActiveStorage::BaseController def create - blob = ActiveStorage::Blob.create_before_direct_upload!(**blob_args) - render json: direct_upload_json(blob) + blob = ActiveStorage::Blob.build_for_direct_upload(**blob_args) + if blob.valid_with?(params.dig(:blob, :model)) + blob.save! + render json: direct_upload_json(blob) + else + head :unprocessable_entity + end end private diff --git a/activestorage/app/javascript/activestorage/blob_record.js b/activestorage/app/javascript/activestorage/blob_record.js index 997c123870aee..24327f8251867 100644 --- a/activestorage/app/javascript/activestorage/blob_record.js +++ b/activestorage/app/javascript/activestorage/blob_record.js @@ -1,14 +1,15 @@ import { getMetaValue } from "./helpers" export class BlobRecord { - constructor(file, checksum, url) { + constructor(file, checksum, url, model) { this.file = file this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, - checksum: checksum + checksum: checksum, + model: model } this.xhr = new XMLHttpRequest diff --git a/activestorage/app/javascript/activestorage/direct_upload.js b/activestorage/app/javascript/activestorage/direct_upload.js index c2eedf289b807..ef5bbd3b99d5a 100644 --- a/activestorage/app/javascript/activestorage/direct_upload.js +++ b/activestorage/app/javascript/activestorage/direct_upload.js @@ -5,11 +5,12 @@ import { BlobUpload } from "./blob_upload" let id = 0 export class DirectUpload { - constructor(file, url, delegate) { + constructor(file, url, delegate, model) { this.id = ++id this.file = file this.url = url this.delegate = delegate + this.model = model } create(callback) { @@ -19,7 +20,7 @@ export class DirectUpload { return } - const blob = new BlobRecord(this.file, checksum, this.url) + const blob = new BlobRecord(this.file, checksum, this.url, this.model) notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr) blob.create(error => { diff --git a/activestorage/app/javascript/activestorage/direct_upload_controller.js b/activestorage/app/javascript/activestorage/direct_upload_controller.js index 987050889a750..e114fde2e313a 100644 --- a/activestorage/app/javascript/activestorage/direct_upload_controller.js +++ b/activestorage/app/javascript/activestorage/direct_upload_controller.js @@ -5,7 +5,7 @@ export class DirectUploadController { constructor(input, file) { this.input = input this.file = file - this.directUpload = new DirectUpload(this.file, this.url, this) + this.directUpload = new DirectUpload(this.file, this.url, this, this.model) this.dispatch("initialize") } @@ -41,6 +41,10 @@ export class DirectUploadController { return this.input.getAttribute("data-direct-upload-url") } + get model() { + return this.input.getAttribute("data-direct-upload-model") + } + dispatch(name, detail = {}) { detail.file = this.file detail.id = this.directUpload.id diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 5f64e28257eac..28a7292de6368 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -114,7 +114,16 @@ def create_and_upload!(key: nil, io:, filename:, content_type: nil, metadata: ni # Once the form using the direct upload is submitted, the blob can be associated with the right record using # the signed ID. def create_before_direct_upload!(key: nil, filename:, byte_size:, checksum:, content_type: nil, metadata: nil, service_name: nil, record: nil) - create! key: key, filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, metadata: metadata, service_name: service_name + build_for_direct_upload(key: key, filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, metadata: metadata, service_name: service_name).tap(&:save!) + end + + # Returns a blob _without_ uploading a file to the service. This blob will point to a key where there is + # no file yet. It's intended to be used together with a client-side upload, which will first create the blob + # in order to produce the signed URL for uploading. This signed URL points to the key generated by the blob. + # Once the form using the direct upload is submitted, the blob can be associated with the right record using + # the signed ID. + def build_for_direct_upload(key: nil, filename:, byte_size:, checksum:, content_type: nil, metadata: nil, service_name: nil, record: nil) + new key: key, filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, metadata: metadata, service_name: service_name end # To prevent problems with case-insensitive filesystems, especially in combination @@ -365,6 +374,22 @@ def content_type=(value) INVALID_VARIABLE_CONTENT_TYPES_DEPRECATED_IN_RAILS_7 = ["image/jpg", "image/pjpeg"] INVALID_VARIABLE_CONTENT_TYPES_TO_SERVE_AS_BINARY_DEPRECATED_IN_RAILS_7 = ["text/javascript"] + # Given a model that descends from ActiveRecord::Base, returns true if the blob would make a valid attachment to that model. + # An attachment would be valid if Active Storage validators (subclassing ActiveStorage::Validations::BaseValidator) would pass. + # + # Returns false if the model cannot be found. + # Returns true if no validators are configured on the model. + def valid_with?(klass_name = nil) + return true if klass_name.nil? + + model = ActiveRecord::Base.const_get(klass_name) rescue nil + return false if model.nil? + + model.validators.select { |v| v.is_a?(ActiveStorage::Validations::BaseValidator) }.all? do |validator| + validator.valid_with?(self) + end + end + private def compute_checksum_in_chunks(io) OpenSSL::Digest::MD5.new.tap do |checksum| diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb index ab06a37081a23..4a97c6bbefdef 100644 --- a/activestorage/lib/active_storage.rb +++ b/activestorage/lib/active_storage.rb @@ -371,3 +371,7 @@ module Transformers autoload :ImageProcessingTransformer end end + +ActiveSupport.on_load(:i18n) do + I18n.load_path << File.expand_path("active_storage/locale/en.yml", __dir__) +end diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index e6d3c0a68f2ed..94c194ff4cb1c 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -21,6 +21,8 @@ require "active_storage/reflection" +require "active_storage/validations" + module ActiveStorage class Engine < Rails::Engine # :nodoc: isolate_namespace ActiveStorage @@ -129,6 +131,7 @@ class Engine < Rails::Engine # :nodoc: ActiveSupport.on_load(:active_record) do include ActiveStorage::Attached::Model + include ActiveStorage::Validations end end diff --git a/activestorage/lib/active_storage/locale/en.yml b/activestorage/lib/active_storage/locale/en.yml new file mode 100644 index 0000000000000..cd3b2eeb4d275 --- /dev/null +++ b/activestorage/lib/active_storage/locale/en.yml @@ -0,0 +1,8 @@ +en: + errors: + # The values :model, :attribute ,and :value are always available for interpolation + # The value :count is available when applicable. Can be used for pluralization. + messages: + in_between: "must be between %{minimum} and %{maximum}" + minimum: "must be greater than or equal to %{minimum}" + maximum: "must be less than or equal to %{maximum}" diff --git a/activestorage/lib/active_storage/validations.rb b/activestorage/lib/active_storage/validations.rb new file mode 100644 index 0000000000000..58c0c6b75f4cc --- /dev/null +++ b/activestorage/lib/active_storage/validations.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require "active_model" +require "active_support/concern" +require "active_support/core_ext/array/wrap" +require "active_storage/validations/base_validator" +require "active_storage/validations/attachment_byte_size_validator" +require "active_storage/validations/attachment_content_type_validator" + +module ActiveStorage + # Provides the class-level DSL for declaring ActiveStorage validations + module Validations + extend ActiveSupport::Concern + + included do + extend HelperMethods + include HelperMethods + end + + module ClassMethods + # A helper method to run various Active Storage attachment validators. + # + # Effectively the same as the ActiveModel::Validations#validates + # method but more readable since it does not require the +attachment_+ + # prefix for its keys. + # + # validates_attachment :avatar, size: { in: 2..4.megabytes } + # validates_attachment :avatar, content_type: { in: "image/jpeg" } + # + # Like the ActiveModel::Validations#validates, it also + # supports shortcut options which can handle ranges, arrays, and strings. + # + # validates_attachment :avatar, size: 2..4.megabytes + # validates_attachment :avatar, content_type: "image/jpeg" + # + # When using shortcut form, ranges and arrays are passed to the + # validator as if they were specified with the +:in+ option, while other + # types including regular expressions and strings are passed as if they + # were specified using +:with+. + def validates_attachment(*attributes) + options = attributes.extract_options!.dup + + ActiveStorage::Validations.constants.each do |constant| + if constant.to_s =~ /\AAttachment(.+)Validator\z/ + validator_kind = $1.underscore.to_sym + + if options.has_key?(validator_kind) + validator_options = options.delete(validator_kind) + validator_options = parse_shortcut_options(validator_options) + + conditional_options = options.slice(:if, :unless) + + Array.wrap(validator_options).each do |local_options| + method_name = ActiveStorage::Validations.const_get(constant.to_s).helper_method_name + send(method_name, attributes, local_options.merge(conditional_options)) + end + end + end + end + end + + private + def parse_shortcut_options(options) + _parse_validates_options(options) + end + end + end +end diff --git a/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb b/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb new file mode 100644 index 0000000000000..0ed8c119a6232 --- /dev/null +++ b/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +module ActiveStorage + module Validations + class AttachmentByteSizeValidator < BaseValidator + AVAILABLE_CHECKS = %i[minimum maximum in] + + def self.helper_method_name + :validates_attachment_byte_size + end + + def initialize(options = {}) + super + error_options.merge!( + minimum: to_human_size(minimum), + maximum: to_human_size(maximum) + ) + end + + def check_validity! + if options_blank? + raise( + ArgumentError, + "You must pass either :minimum, :maximum, or :in to the validator" + ) + end + + if options_redundant? + raise( + ArgumentError, + "Cannot pass :minimum or :maximum if already passing :in" + ) + end + end + + private + def error_key_for(check_name) + check_name == :in ? :in_between : check_name + end + + def options_redundant? + options.has_key?(:in) && + (options.has_key?(:minimum) || options.has_key?(:minimum)) + end + + def minimum + @minimum ||= options[:minimum] || options[:in].try(:min) || 0 + end + + def maximum + @maximum ||= options[:maximum] || options[:in].try(:max) || Float::INFINITY + end + + def to_human_size(size) + return "∞" if size == Float::INFINITY + ActiveSupport::NumberHelper.number_to_human_size(size) + end + + def passes_check?(blob, check_name, check_value) + case check_name.to_sym + when :in + check_value.include?(blob.byte_size) + when :minimum + blob.byte_size >= check_value + when :maximum + blob.byte_size <= check_value + end + end + end + + module HelperMethods + # Validates the size (in bytes) of the ActiveStorage attachments. Happens + # by default on save. + # + # class Employee < ActiveRecord::Base + # has_one_attached :avatar + # + # validates_attachment_byte_size :avatar, in: 0..2.megabytes + # end + # + # Configuration options: + # * in - a +Range+ of bytes (e.g. +0..1.megabyte+), + # * maximum - equivalent to +in: 0..options[:maximum]+ + # * minimum - equivalent to +in: options[:minimum]..Infinity+ + # * :message - A custom error message which overrides the + # default error message. The following keys are available for + # interpolation within the message: +model+, +attribute+, +value+, + # +minimum+, and +maximum+. + # + # There is also a list of default options supported by every validator: + # +:if+, +:unless+, +:on+, +:allow_nil+, +:allow_blank+, and +:strict+. + # See ActiveModel::Validations#validates for more information + def validates_attachment_byte_size(*attributes) + validates_with AttachmentByteSizeValidator, _merge_attributes(attributes) + end + end + end +end diff --git a/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb b/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb new file mode 100644 index 0000000000000..3b1b87cce744a --- /dev/null +++ b/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module ActiveStorage + module Validations + class AttachmentContentTypeValidator < BaseValidator + AVAILABLE_CHECKS = %i[in not with] + + def self.helper_method_name + :validates_attachment_content_type + end + + def check_validity! + if options_blank? + raise( + ArgumentError, + "You must pass at least one of #{AVAILABLE_CHECKS.join(", ")} to the validator" + ) + end + + if options_redundant? + raise(ArgumentError, "Cannot pass both :in and :not") + end + end + + private + def error_key_for(check_name) + { in: :inclusion, not: :exclusion, with: :inclusion }[check_name.to_sym] + end + + def options_redundant? + options.has_key?(:in) && options.has_key?(:not) + end + + def passes_check?(blob, check_name, check_value) + case check_name.to_sym + when :in + check_value.include?(blob.content_type) + when :not + !check_value.include?(blob.content_type) + when :with + # TODO: implement check_options_validity from AM::Validators::FormatValidator + check_value.match?(blob.content_type) + end + end + end + + module HelperMethods + # Validates the content type of the ActiveStorage attachments. Happens by + # default on save. + # + # class Employee < ActiveRecord::Base + # has_one_attached :avatar + # + # validates_attachment_content_type :avatar, in: %w[image/jpeg audio/ogg] + # validates_attachment_content_type :avatar, in: "image/jpeg" + # end + # + # Configuration options: + # * in - a +Array+ or +String+ of allowable content types + # * not - a +Array+ or +String+ of content types to exclude + # * :message - A custom error message which overrides the + # default error message. + # + # There is also a list of default options supported by every validator: + # +:if+, +:unless+, +:on+, +:allow_nil+, +:allow_blank+, and +:strict+. + # See ActiveModel::Validations#validates for more information + def validates_attachment_content_type(*attributes) + validates_with AttachmentContentTypeValidator, _merge_attributes(attributes) + end + end + end +end diff --git a/activestorage/lib/active_storage/validations/base_validator.rb b/activestorage/lib/active_storage/validations/base_validator.rb new file mode 100644 index 0000000000000..c72f45506c456 --- /dev/null +++ b/activestorage/lib/active_storage/validations/base_validator.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +module ActiveStorage + module Validations + class BaseValidator < ActiveModel::EachValidator + def initialize(options = {}) + super + @error_options = { message: options[:message] } + end + + def valid_with?(blob) + valid = true + each_check do |check_name, check_value| + next if passes_check?(blob, check_name, check_value) + + @record.errors.add(@name, error_key_for(check_name), **error_options) if @record + valid = false + end + valid + end + + def validate_each(record, attribute, _value) + @record = record + @name = attribute + + each_blob do |blob| + valid_with?(blob) + end + end + + private + attr_reader :error_options + + def available_checks + self.class::AVAILABLE_CHECKS + end + + def options_blank? + available_checks.none? { |arg| options.has_key?(arg) } + end + + def options_redundant? + raise NotImplementedError, "Subclasses must implement an options_redundant? method" + end + + def error_key_for(check_name) + raise NotImplementedError, "Subclasses must implement error_key_for(check_name)" + end + + def each_blob(&block) + changes = attachment_changes + + blobs = + case + when marked_for_creation? then changes.try(:blob) || changes.blobs + when marked_for_deletion? then [] + else + @record.send(blob_association) + end + + blobs = [blobs].flatten.compact + blobs.each { |blob| yield(blob) } + end + + def each_check(&block) + options.slice(*available_checks).each do |name, value| + yield(name, value) + end + end + + def passes_check?(blob, check_name, check_value) + raise NotImplementedError, "Subclasses must implement a passes_check?(blob, check_name, check_value) method" + end + + def attachment_changes + @attachment_changes ||= @record.attachment_changes[@name.to_s] + end + + def marked_for_creation? + [ + ActiveStorage::Attached::Changes::CreateOne, + ActiveStorage::Attached::Changes::CreateMany + ].include?(attachment_changes.class) + end + + def marked_for_deletion? + [ + ActiveStorage::Attached::Changes::DeleteOne, + ActiveStorage::Attached::Changes::DeleteMany + ].include?(attachment_changes.class) + end + + def blob_association + @record.respond_to?("#{@name}_blob") ? "#{@name}_blob" : "#{@name}_blobs" + end + end + end +end diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index 38d2bd542b6bf..e0460dcc2baac 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -135,6 +135,22 @@ class ActiveStorage::AzureStorageDirectUploadsControllerTest < ActionDispatch::I end class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::IntegrationTest + setup do + @old_validators = User._validators.deep_dup + @old_callbacks = User._validate_callbacks.deep_dup + end + + teardown do + User.destroy_all + ActiveStorage::Blob.all.each(&:purge) + + User.clear_validators! + # NOTE: `clear_validators!` clears both registered validators and any + # callbacks registered by `validate()`, so ensure that both are restored + User._validators = @old_validators if @old_validators + User._validate_callbacks = @old_callbacks if @old_callbacks + end + test "creating new direct upload" do checksum = OpenSSL::Digest::MD5.base64digest("Hello") metadata = { @@ -148,6 +164,8 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati post rails_direct_uploads_url, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } + assert_response :success + @response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) assert_equal "hello.txt", details["filename"] @@ -181,6 +199,88 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati end end + test "creating new direct upload with model with no active storage validations" do + # validations that aren't active storage validations are ignored + User.validates :name, length: { minimum: 2 } + + file = file_fixture("racecar.jpg").open + checksum = Digest::MD5.base64digest(file.read) + metadata = { + "foo": "bar", + "my_key_1": "my_value_1", + "my_key_2": "my_value_2", + "platform": "my_platform", + "library_ID": "12345" + } + + post rails_direct_uploads_url, params: { blob: { + filename: "racecar.jpg", byte_size: file.size, checksum: checksum, content_type: "image/jpg", metadata: metadata, model: "User" } } + + assert_response :success + + @response.parsed_body.tap do |details| + assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) + assert_equal "racecar.jpg", details["filename"] + assert_equal file.size, details["byte_size"] + assert_equal checksum, details["checksum"] + assert_equal metadata, details["metadata"].transform_keys(&:to_sym) + assert_equal "image/jpg", details["content_type"] + assert_match(/rails\/active_storage\/disk/, details["direct_upload"]["url"]) + assert_equal({ "Content-Type" => "image/jpg" }, details["direct_upload"]["headers"]) + end + end + + test "creating new direct upload with model where validations pass" do + User.validates :avatar, attachment_content_type: { with: /\Aimage\//, message: "must be an image" } + User.validates :avatar, attachment_byte_size: { maximum: 50.megabytes, message: "can't be larger than 50 MB" } + + file = file_fixture("racecar.jpg").open + checksum = Digest::MD5.base64digest(file.read) + metadata = { + "foo": "bar", + "my_key_1": "my_value_1", + "my_key_2": "my_value_2", + "platform": "my_platform", + "library_ID": "12345" + } + + post rails_direct_uploads_url, params: { blob: { + filename: "racecar.jpg", byte_size: file.size, checksum: checksum, content_type: "image/jpg", metadata: metadata, model: "User" } } + + assert_response :success + + @response.parsed_body.tap do |details| + assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) + assert_equal "racecar.jpg", details["filename"] + assert_equal file.size, details["byte_size"] + assert_equal checksum, details["checksum"] + assert_equal metadata, details["metadata"].transform_keys(&:to_sym) + assert_equal "image/jpg", details["content_type"] + assert_match(/rails\/active_storage\/disk/, details["direct_upload"]["url"]) + assert_equal({ "Content-Type" => "image/jpg" }, details["direct_upload"]["headers"]) + end + end + + test "creating new direct upload with model where validations fail" do + User.validates :avatar, attachment_content_type: { with: /\Atext\//, message: "must be a text file" } + User.validates :avatar, attachment_byte_size: { minimum: 50.megabytes, message: "can't be smaller than 50 MB" } + + file = file_fixture("racecar.jpg").open + checksum = Digest::MD5.base64digest(file.read) + metadata = { + "foo": "bar", + "my_key_1": "my_value_1", + "my_key_2": "my_value_2", + "platform": "my_platform", + "library_ID": "12345" + } + + post rails_direct_uploads_url, params: { blob: { + filename: "racecar.jpg", byte_size: file.size, checksum: checksum, content_type: "image/jpg", metadata: metadata, model: "User" } } + + assert_response :unprocessable_entity + end + private def set_include_root_in_json(value) original = ActiveRecord::Base.include_root_in_json diff --git a/activestorage/test/models/validations/attachment_byte_size_validator_test.rb b/activestorage/test/models/validations/attachment_byte_size_validator_test.rb new file mode 100644 index 0000000000000..87a543c896e57 --- /dev/null +++ b/activestorage/test/models/validations/attachment_byte_size_validator_test.rb @@ -0,0 +1,398 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" + +class ActiveStorage::AttachmentByteSizeValidatorTest < ActiveSupport::TestCase + VALIDATOR = ActiveStorage::Validations::AttachmentByteSizeValidator + + setup do + @old_validators = User._validators.deep_dup + @old_callbacks = User._validate_callbacks.deep_dup + + @blob = create_blob(filename: "funky.jpg") + @user = User.create(name: "Anjali") + + @byte_size = @blob.byte_size + + @minimum = @byte_size - 1 + @maximum = @byte_size + 1 + @range = @minimum..@maximum + + @bad_minimum = 50.gigabytes + @bad_maximum = 1.byte + @bad_range = 50.gigabytes..51.gigabytes + end + + teardown do + User.destroy_all + ActiveStorage::Blob.all.each(&:purge) + + User.clear_validators! + # NOTE: `clear_validators!` clears both registered validators and any + # callbacks registered by `validate()`, so ensure that both are restored + User._validators = @old_validators if @old_validators + User._validate_callbacks = @old_callbacks if @old_callbacks + end + + test "record has no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + assert @user.save + end + + test "new record, creating attachments" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + @user = User.new(name: "Rohini") + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @range) + + assert @user.save + end + + test "persisted record, creating attachments" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @range) + + assert @user.save + end + + test "persisted record, updating attachments" do + other_blob = create_blob(filename: "town.jpg") + @user.avatar.attach(other_blob) + @user.highlights.attach(other_blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @range) + + assert @user.save + end + + test "persisted record, updating some other field" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @range) + + @user.name = "Rohini" + + assert @user.save + end + + test "persisted record, destroying attachments" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @range) + + @user.avatar.detach + @user.highlights.detach + + assert @user.save + end + + test "destroying record with attachments" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @range) + + @user.avatar.detach + @user.highlights.detach + + assert @user.destroy + assert_not @user.persisted? + end + + test "new record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + @user = User.new(name: "Rohini") + + assert @user.save + end + + test "persisted record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + assert @user.save + end + + test "destroying record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range) + + assert @user.destroy + assert_not @user.persisted? + end + + test "specifying :minimum option" do + User.validates_with(VALIDATOR, attributes: :avatar, minimum: @bad_minimum) + User.validates_with(VALIDATOR, attributes: :highlights, minimum: @bad_minimum) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be greater than or equal to 50 GB"], @user.errors.messages[:avatar] + assert_equal ["must be greater than or equal to 50 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, minimum: @minimum) + User.validates_with(VALIDATOR, attributes: :highlights, minimum: @minimum) + + assert @user.save + end + + test "specifying :maximum option" do + User.validates_with(VALIDATOR, attributes: :avatar, maximum: @bad_maximum) + User.validates_with(VALIDATOR, attributes: :highlights, maximum: @bad_maximum) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be less than or equal to 1 Byte"], @user.errors.messages[:avatar] + assert_equal ["must be less than or equal to 1 Byte"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, maximum: @maximum) + User.validates_with(VALIDATOR, attributes: :highlights, maximum: @maximum) + + assert @user.save + end + + test "specifying both :minimum and :maximum options" do + User.validates_with(VALIDATOR, attributes: :avatar, minimum: @bad_minimum, maximum: @bad_maximum) + User.validates_with(VALIDATOR, attributes: :highlights, minimum: @bad_minimum, maximum: @bad_maximum) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + errors = ["must be greater than or equal to 50 GB", + "must be less than or equal to 1 Byte"] + assert_equal errors, @user.errors.messages[:avatar] + assert_equal errors, @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, minimum: @minimum, maximum: @maximum) + User.validates_with(VALIDATOR, attributes: :highlights, minimum: @minimum, maximum: @maximum) + + assert @user.save + end + + test "specifying no options" do + exception = assert_raise(ArgumentError) do + User.validates_with(VALIDATOR, attributes: :avatar) + end + + assert_equal( + "You must pass either :minimum, :maximum, or :in to the validator", + exception.message + ) + end + + test "specifying redundant options" do + exception = assert_raise(ArgumentError) do + User.validates_with(VALIDATOR, attributes: :avatar, in: @range, minimum: @minimum) + end + + assert_equal( + "Cannot pass :minimum or :maximum if already passing :in", + exception.message + ) + end + + test "validating with `validates()`" do + User.validates(:avatar, attachment_byte_size: { in: @bad_range }) + User.validates(:highlights, attachment_byte_size: { in: @bad_range }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates(:avatar, attachment_byte_size: { in: @range }) + User.validates(:highlights, attachment_byte_size: { in: @range }) + + assert @user.save + end + + test "validating with `validates()`, Range shortcut option" do + User.validates(:avatar, attachment_byte_size: @bad_range) + User.validates(:highlights, attachment_byte_size: @bad_range) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates(:avatar, attachment_byte_size: @range) + User.validates(:highlights, attachment_byte_size: @range) + + assert @user.save + end + + test "validating with `validates()`, invalid shortcut option" do + exception = assert_raise(ArgumentError) do + User.validates(:avatar, attachment_byte_size: "foo") + end + + assert_equal( + "You must pass either :minimum, :maximum, or :in to the validator", + exception.message + ) + end + + test "validating with `validates_attachment()`" do + User.validates_attachment(:avatar, byte_size: { in: @bad_range }) + User.validates_attachment(:highlights, byte_size: { in: @bad_range }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment(:avatar, byte_size: { in: @range }) + User.validates_attachment(:highlights, byte_size: { in: @range }) + + assert @user.save + end + + test "validating with `validates_attachment()`, Range shortcut option" do + User.validates_attachment(:avatar, byte_size: @bad_range) + User.validates_attachment(:highlights, byte_size: @bad_range) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment(:avatar, byte_size: @range) + User.validates_attachment(:highlights, byte_size: @range) + + assert @user.save + end + + test "validating with `validates_attachment()`, invalid shortcut option" do + exception = assert_raise(ArgumentError) do + User.validates_attachment(:avatar, byte_size: "foo") + end + + assert_equal( + "You must pass either :minimum, :maximum, or :in to the validator", + exception.message + ) + end + + test "validating with `validates_attachment_byte_size()`" do + User.validates_attachment_byte_size(:avatar, in: @bad_range) + User.validates_attachment_byte_size(:highlights, in: @bad_range) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:avatar] + assert_equal ["must be between 50 GB and 51 GB"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment_byte_size(:avatar, in: @range) + User.validates_attachment_byte_size(:highlights, in: @range) + + assert @user.save + end + + test "specifying a :message option" do + message = "Validating %{model}#%{attribute}. The min is %{minimum} and "\ + "the max is %{maximum}" + + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range, message: message) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range, message: message) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal( + ["Validating User#Avatar. The min is 50 GB and the max is 51 GB"], + @user.errors.messages[:avatar] + ) + assert_equal( + ["Validating User#Highlights. The min is 50 GB and the max is 51 GB"], + @user.errors.messages[:highlights] + ) + end + + test "inheritance of default ActiveModel options" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_range, if: Proc.new { false }) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_range, if: Proc.new { false }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.save + end +end diff --git a/activestorage/test/models/validations/attachment_content_type_validator_test.rb b/activestorage/test/models/validations/attachment_content_type_validator_test.rb new file mode 100644 index 0000000000000..90a231e53d03a --- /dev/null +++ b/activestorage/test/models/validations/attachment_content_type_validator_test.rb @@ -0,0 +1,439 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" + +class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCase + VALIDATOR = ActiveStorage::Validations::AttachmentContentTypeValidator + + setup do + @old_validators = User._validators.deep_dup + @old_callbacks = User._validate_callbacks.deep_dup + + @blob = create_blob(filename: "funky.jpg") + @user = User.create(name: "Anjali") + + @content_types = %w[text/plain image/jpeg] + @bad_content_types = %w[audio/ogg application/pdf] + end + + teardown do + User.destroy_all + ActiveStorage::Blob.all.each(&:purge) + + User.clear_validators! + # NOTE: `clear_validators!` clears both registered validators and any + # callbacks registered by `validate()`, so ensure that both are restored + User._validators = @old_validators if @old_validators + User._validate_callbacks = @old_callbacks if @old_callbacks + end + + test "record has no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + assert @user.save + end + + test "new record, creating attachments" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + @user = User.new(name: "Rohini") + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types) + + assert @user.save + end + + test "persisted record, creating attachments" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types) + + assert @user.save + end + + test "persisted record, updating attachments" do + old_blob = create_blob(filename: "town.jpg") + @user.avatar.attach(old_blob) + @user.highlights.attach(old_blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types) + + assert @user.save + end + + test "persisted record, updating some other field" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types) + + @user.name = "Rohini" + + assert @user.save + end + + test "persisted record, destroying attachments" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types) + + @user.avatar.detach + @user.highlights.detach + + assert @user.save + end + + test "destroying record with attachments" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types) + + @user.avatar.detach + @user.highlights.detach + + assert @user.destroy + assert_not @user.persisted? + end + + test "new record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + @user = User.new(name: "Rohini") + + assert @user.save + end + + test "persisted record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + assert @user.save + end + + test "destroying record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types) + + assert @user.destroy + assert_not @user.persisted? + end + + test "specifying :in option as String" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types.first) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types.first) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types.first) + User.validates_with(VALIDATOR, attributes: :highlights, in: @content_types.first) + + assert @user.save + end + + test "specifying :not option" do + User.validates_with(VALIDATOR, attributes: :avatar, not: @content_types) + User.validates_with(VALIDATOR, attributes: :highlights, not: @content_types) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is reserved"], @user.errors.messages[:avatar] + assert_equal ["is reserved"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, not: @bad_content_types) + User.validates_with(VALIDATOR, attributes: :highlights, not: @bad_content_types) + + assert @user.save + end + + test "specifying :not option as a String" do + User.validates_with(VALIDATOR, attributes: :avatar, not: @content_types.first) + User.validates_with(VALIDATOR, attributes: :highlights, not: @content_types.first) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is reserved"], @user.errors.messages[:avatar] + assert_equal ["is reserved"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar, not: @bad_content_types.first) + User.validates_with(VALIDATOR, attributes: :highlights, not: @bad_content_types.first) + + assert @user.save + end + + test "specifying no options" do + exception = assert_raise(ArgumentError) do + User.validates_with(VALIDATOR, attributes: :avatar) + end + + assert_equal( + "You must pass at least one of in, not, with to the validator", + exception.message + ) + end + + test "specifying redundant options" do + exception = assert_raise(ArgumentError) do + User.validates_with(VALIDATOR, attributes: :avatar, in: @content_types, not: @bad_content_types) + end + + assert_equal("Cannot pass both :in and :not", exception.message) + end + + test "validating with `validates()`" do + User.validates(:avatar, attachment_content_type: { in: @bad_content_types }) + User.validates(:highlights, attachment_content_type: { in: @bad_content_types }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates(:avatar, attachment_content_type: { in: @content_types }) + User.validates(:highlights, attachment_content_type: { in: @content_types }) + + assert @user.save + end + + test "validating with `validates()`, String shortcut option" do + User.validates(:avatar, attachment_content_type: @bad_content_types.first) + User.validates(:highlights, attachment_content_type: @bad_content_types.first) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates(:avatar, attachment_content_type: @content_types.first) + User.validates(:highlights, attachment_content_type: @content_types.first) + + assert @user.save + end + + test "validating with `validates()`, Array shortcut option" do + User.validates(:avatar, attachment_content_type: @bad_content_types) + User.validates(:highlights, attachment_content_type: @bad_content_types) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates(:avatar, attachment_content_type: @content_types) + User.validates(:highlights, attachment_content_type: @content_types) + + assert @user.save + end + + test "validating with `validates()`, invalid shortcut option" do + User.validates(:avatar, attachment_content_type: @bad_content_types.first.to_sym) + User.validates(:highlights, attachment_content_type: @bad_content_types.first.to_sym) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates(:avatar, attachment_content_type: @content_types.first.to_sym) + User.validates(:highlights, attachment_content_type: @content_types.first.to_sym) + + assert @user.save + end + + test "validating with `validates_attachment()`" do + User.validates_attachment(:avatar, content_type: { in: @bad_content_types }) + User.validates_attachment(:highlights, content_type: { in: @bad_content_types }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment(:avatar, content_type: { in: @content_types }) + User.validates_attachment(:highlights, content_type: { in: @content_types }) + + assert @user.save + end + + test "validating with `validates_attachment()`, String shortcut option" do + User.validates_attachment(:avatar, content_type: @bad_content_types.first) + User.validates_attachment(:highlights, content_type: @bad_content_types.first) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment(:avatar, content_type: @content_types.first) + User.validates_attachment(:highlights, content_type: @content_types.first) + + assert @user.save + end + + test "validating with `validates_attachment()`, Array shortcut option" do + User.validates_attachment(:avatar, content_type: @bad_content_types) + User.validates_attachment(:highlights, content_type: @bad_content_types) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment(:avatar, content_type: @content_types) + User.validates_attachment(:highlights, content_type: @content_types) + + assert @user.save + end + + test "validating with `validates_attachment()`, Symbol shortcut option" do + User.validates_attachment(:avatar, content_type: @bad_content_types.first.to_sym) + User.validates_attachment(:highlights, content_type: @bad_content_types.first.to_sym) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment(:avatar, content_type: @content_types.first.to_sym) + User.validates_attachment(:highlights, content_type: @content_types.first.to_sym) + + assert @user.save + end + + test "validating with `validates_attachment_content_type()`" do + User.validates_attachment_content_type(:avatar, in: @bad_content_types) + User.validates_attachment_content_type(:highlights, in: @bad_content_types) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal ["is not included in the list"], @user.errors.messages[:avatar] + assert_equal ["is not included in the list"], @user.errors.messages[:highlights] + + User.clear_validators! + + User.validates_attachment_content_type(:avatar, in: @content_types) + User.validates_attachment_content_type(:highlights, in: @content_types) + + assert @user.save + end + + test "specifying a :message option" do + message = "Content Type not valid for %{model}#%{attribute}" + + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types, message: message) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types, message: message) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert_not @user.valid? + assert_equal( + ["Content Type not valid for User#Avatar"], + @user.errors.messages[:avatar] + ) + assert_equal( + ["Content Type not valid for User#Highlights"], + @user.errors.messages[:highlights] + ) + end + + test "inheritance of default ActiveModel options" do + User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types, if: Proc.new { false }) + User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types, if: Proc.new { false }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.save + end +end diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 590fba91069c7..9bd5457e4a9ce 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -571,6 +571,56 @@ user.avatar.purge_later [Attached::One#purge]: https://api.rubyonrails.org/classes/ActiveStorage/Attached/One.html#method-i-purge [Attached::One#purge_later]: https://api.rubyonrails.org/classes/ActiveStorage/Attached/One.html#method-i-purge_later +Validating Files +---------------- + +Active Storage includes attachment validators for the following properties: + +* Byte Size +* Content Type + +### Size + +Validates the size (in bytes) of the attached `Blob` object: + + ```ruby + validates :avatar, attachment_byte_size: { in: 0..1.megabyte } + validates :avatar, attachment_byte_size: { minimum: 17.kilobytes } + validates :avatar, attachment_byte_size: { maximum: 38.megabytes } + ``` + +Also accepts a `Range` as a shortcut option for `:in`: + + ```ruby + validates :avatar, attachment_size: 0..1.megabyte + ``` + +### Content Type + +Validates the content type of the attached `Blob` object: + + ```ruby + validates :avatar, attachment_byte_content_type: { in: %w[image/jpeg image/png] } + validates :avatar, attachment_byte_content_type: { not: %w[application/pdf] } + ``` + +Also accepts a `Array` or `String` as a shortcut option for `:in`: + + ```ruby + validates :avatar, attachment_byte_content_type: %w[image/jpeg image/png] + validates :avatar, attachment_byte_content_type: "image/jpeg" + ``` + +### Validation Helper + +Active Storage also provides a more readable validation helper named +`validates_attachment()` which provides the same functionality as `validates()` +but does not require the `attachment_` prefix on keys: + + ```ruby + validates_attachment :avatar, byte_size: { in: 0..1.megabyte }, content_type: "image/jpeg" + ``` + Serving Files ------------- From 1f7ab09156ec8a73d3344770f1752bff2a6019a4 Mon Sep 17 00:00:00 2001 From: Recker Swartz Date: Wed, 29 Sep 2021 05:22:55 +0530 Subject: [PATCH 2/5] attachment presence validator --- .../lib/active_storage/validations.rb | 1 + .../attachment_presence_validator.rb | 55 +++++ .../attachment_precense_validator_test.rb | 222 ++++++++++++++++++ 3 files changed, 278 insertions(+) create mode 100644 activestorage/lib/active_storage/validations/attachment_presence_validator.rb create mode 100644 activestorage/test/models/validations/attachment_precense_validator_test.rb diff --git a/activestorage/lib/active_storage/validations.rb b/activestorage/lib/active_storage/validations.rb index 58c0c6b75f4cc..0680e382e31e4 100644 --- a/activestorage/lib/active_storage/validations.rb +++ b/activestorage/lib/active_storage/validations.rb @@ -6,6 +6,7 @@ require "active_storage/validations/base_validator" require "active_storage/validations/attachment_byte_size_validator" require "active_storage/validations/attachment_content_type_validator" +require "active_storage/validations/attachment_presence_validator" module ActiveStorage # Provides the class-level DSL for declaring ActiveStorage validations diff --git a/activestorage/lib/active_storage/validations/attachment_presence_validator.rb b/activestorage/lib/active_storage/validations/attachment_presence_validator.rb new file mode 100644 index 0000000000000..89ecd5c81494b --- /dev/null +++ b/activestorage/lib/active_storage/validations/attachment_presence_validator.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module ActiveStorage + module Validations + class AttachmentPresenceValidator < BaseValidator + AVAILABLE_CHECKS = [] + + def self.helper_method_name + :validates_attachment_presence + end + + def validate_each(record, attribute, _value) + return if record.send(attribute).attached? + + record.errors.add(attribute, :blank, **options) + end + + private + def error_key_for(check_name) + ## Not Required + end + + def options_redundant? + ## Not Required + end + + def passes_check?(blob, check_name, check_value) + ## Not Required + end + end + + module HelperMethods + # Validates the content type of the ActiveStorage attachments. Happens by + # default on save. + # + # class Employee < ActiveRecord::Base + # has_one_attached :avatar + # + # validates_attachment_presence :avatar + # end + # + # Configuration options: + # * :message - A custom error message which overrides the + # default error message. + # + # There is also a list of default options supported by every validator: + # +:if+, +:unless+, +:on+, +:allow_nil+, +:allow_blank+, and +:strict+. + # See ActiveModel::Validations#validates for more information + def validates_attachment_presence(*attributes) + validates_with AttachmentPresenceValidator, _merge_attributes(attributes) + end + end + end +end + diff --git a/activestorage/test/models/validations/attachment_precense_validator_test.rb b/activestorage/test/models/validations/attachment_precense_validator_test.rb new file mode 100644 index 0000000000000..306f94e9a6cf1 --- /dev/null +++ b/activestorage/test/models/validations/attachment_precense_validator_test.rb @@ -0,0 +1,222 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" + +class ActiveStorage::AttachmentPresenceValidatorTest < ActiveSupport::TestCase + VALIDATOR = ActiveStorage::Validations::AttachmentPresenceValidator + + setup do + @old_validators = User._validators.deep_dup + @old_callbacks = User._validate_callbacks.deep_dup + + @blob = create_blob(filename: "funky.jpg") + @user = User.create(name: "Anjali") + end + + teardown do + User.destroy_all + ActiveStorage::Blob.all.each(&:purge) + + User.clear_validators! + # NOTE: `clear_validators!` clears both registered validators and any + # callbacks registered by `validate()`, so ensure that both are restored + User._validators = @old_validators if @old_validators + User._validate_callbacks = @old_callbacks if @old_callbacks + end + + test "record has no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + assert_not @user.save + end + + test "new record, creating attachments" do + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user = User.new(name: "Rohini") + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.save + end + + test "persisted record, creating attachments" do + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.save + end + + test "persisted record, updating attachments" do + other_blob = create_blob(filename: "town.jpg") + @user.avatar.attach(other_blob) + @user.highlights.attach(other_blob) + + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.valid? + + User.clear_validators! + + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + assert @user.save + end + + test "persisted record, updating some other field" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user.name = "Rohini" + + assert @user.save + end + + test "persisted record, destroying attachments" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user.avatar.detach + @user.highlights.detach + + assert_not @user.save + end + + test "destroying record with attachments" do + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user.avatar.detach + @user.highlights.detach + + assert @user.destroy + assert_not @user.persisted? + end + + test "new record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + @user = User.new(name: "Rohini") + + assert_not @user.save + end + + test "persisted record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + assert_not @user.save + end + + test "destroying record, with no attachment" do + User.validates_with(VALIDATOR, attributes: :avatar) + User.validates_with(VALIDATOR, attributes: :highlights) + + assert @user.destroy + assert_not @user.persisted? + end + + test "validating with `validates()`" do + User.validates(:avatar, attachment_presence: true) + User.validates(:highlights, attachment_presence: true) + + assert_not @user.valid? + assert_equal ["can't be blank"], @user.errors.messages[:avatar] + assert_equal ["can't be blank"], @user.errors.messages[:highlights] + + User.clear_validators! + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates(:avatar, attachment_presence: true) + User.validates(:highlights, attachment_presence: true) + + assert @user.save + end + + test "validating with `validates_attachment()`" do + User.validates_attachment(:avatar, presence: true) + User.validates_attachment(:highlights, presence: true) + + assert_not @user.valid? + + User.clear_validators! + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_attachment(:avatar, presence: true) + User.validates_attachment(:highlights, presence: true) + + assert @user.save + end + + test "validating with `validates_attachment_presence()`" do + User.validates_attachment_presence(:avatar) + User.validates_attachment_presence(:highlights) + + assert_not @user.valid? + assert_equal ["can't be blank"], @user.errors.messages[:avatar] + assert_equal ["can't be blank"], @user.errors.messages[:highlights] + + User.clear_validators! + + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + User.validates_attachment_presence(:avatar) + User.validates_attachment_presence(:highlights) + + assert @user.save + end + + test "specifying a :message option" do + message = "Validating %{model}#%{attribute}. The %{attribute} can't be blank" + + User.validates_with(VALIDATOR, attributes: :avatar, message: message) + User.validates_with(VALIDATOR, attributes: :highlights, message: message) + + assert_not @user.valid? + assert_equal( + ["Validating User#Avatar. The Avatar can't be blank"], + @user.errors.messages[:avatar] + ) + assert_equal( + ["Validating User#Highlights. The Highlights can't be blank"], + @user.errors.messages[:highlights] + ) + end + + test "inheritance of default ActiveModel options" do + User.validates_with(VALIDATOR, attributes: :avatar, presence: true, if: Proc.new { false }) + User.validates_with(VALIDATOR, attributes: :highlights, presence: true, if: Proc.new { false }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.save + end +end From 6962f168c944d1958159853e0a1afb3b471b9f36 Mon Sep 17 00:00:00 2001 From: Sean Abrahams Date: Thu, 9 Jun 2022 15:10:53 -0700 Subject: [PATCH 3/5] Use Model.attribute to specify which validations to use. --- .../action_view/helpers/form_tag_helper.rb | 10 +-- actionview/test/template/form_helper_test.rb | 26 ------ .../test/template/form_tag_helper_test.rb | 10 --- activestorage/CHANGELOG.md | 76 +--------------- activestorage/README.md | 20 +++++ .../assets/javascripts/activestorage.esm.js | 16 ++-- .../app/assets/javascripts/activestorage.js | 16 ++-- .../direct_uploads_controller.rb | 4 +- .../active_storage/direct_uploads_helper.rb | 19 ++++ .../javascript/activestorage/blob_record.js | 4 +- .../javascript/activestorage/direct_upload.js | 6 +- .../activestorage/direct_upload_controller.js | 6 +- .../app/models/active_storage/blob.rb | 12 ++- activestorage/lib/active_storage/engine.rb | 2 + .../attachment_byte_size_validator.rb | 4 + .../attachment_content_type_validator.rb | 23 ++++- .../attachment_presence_validator.rb | 21 +++-- .../validations/base_validator.rb | 14 ++- .../direct_uploads_controller_test.rb | 86 +++++++++++++++++-- .../attachment_byte_size_validator_test.rb | 4 +- .../attachment_content_type_validator_test.rb | 31 ++----- .../attachment_precense_validator_test.rb | 4 +- .../test/template/file_field_tag_test.rb | 41 +++++++++ activestorage/test/test_helper.rb | 2 + guides/source/active_storage_overview.md | 32 ++++++- 25 files changed, 291 insertions(+), 198 deletions(-) create mode 100644 activestorage/app/helpers/active_storage/direct_uploads_helper.rb create mode 100644 activestorage/test/template/file_field_tag_test.rb diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index 302f959409676..31e72fd1f9073 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -987,12 +987,12 @@ def set_default_disable_with(value, tag_options) def add_direct_upload_attributes(options) if options.delete(:direct_upload) && respond_to?(:rails_direct_uploads_url) options["data-direct-upload-url"] = rails_direct_uploads_url - if options[:direct_upload_model] - options["data-direct-upload-model"] = options.delete(:direct_upload_model) - elsif options[:object] - options["data-direct-upload-model"] = options[:object].class.name - end end + + if options[:direct_upload_signed_model_and_attribute].present? && respond_to?(:rails_direct_uploads_signed_model_and_attribute) + options["data-direct-upload-signed-model-and-attribute"] = rails_direct_uploads_signed_model_and_attribute(options.delete(:direct_upload_signed_model_and_attribute)) + end + options end end diff --git a/actionview/test/template/form_helper_test.rb b/actionview/test/template/form_helper_test.rb index 6d13202831598..d08f00cd36c35 100644 --- a/actionview/test/template/form_helper_test.rb +++ b/actionview/test/template/form_helper_test.rb @@ -597,13 +597,6 @@ def test_file_field_with_direct_upload_when_rails_direct_uploads_url_is_defined assert_dom_equal expected, file_field("import", "file", direct_upload: true) end - def test_file_field_with_direct_upload_when_rails_direct_uploads_url_is_defined_and_model_is_provided - @controller = WithActiveStorageRoutesControllers.new - - expected = '' - assert_dom_equal expected, file_field("import", "file", direct_upload: true, object: Post.new) - end - def test_file_field_with_direct_upload_dont_mutate_arguments original_options = { class: "pix", direct_upload: true } @@ -3771,25 +3764,6 @@ def test_default_form_builder ActionView::Base.default_form_builder = old_default_form_builder end - def test_form_builder_includes_direct_upload_attributes - old_default_form_builder, ActionView::Base.default_form_builder = - ActionView::Base.default_form_builder, LabelledFormBuilder - - @controller = WithActiveStorageRoutesControllers.new - - form_for(@post) do |f| - concat f.file_field(:title, direct_upload: true) - end - - expected = whole_form("/posts/123", "edit_post_123", "edit_post", method: "patch", multipart: true) do - "
" \ - end - - assert_dom_equal expected, output_buffer - ensure - ActionView::Base.default_form_builder = old_default_form_builder - end - def test_lazy_loading_default_form_builder old_default_form_builder, ActionView::Base.default_form_builder = ActionView::Base.default_form_builder, "FormHelperTest::LabelledFormBuilder" diff --git a/actionview/test/template/form_tag_helper_test.rb b/actionview/test/template/form_tag_helper_test.rb index 1b492b6dafdd4..cf86dbdb88e51 100644 --- a/actionview/test/template/form_tag_helper_test.rb +++ b/actionview/test/template/form_tag_helper_test.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "abstract_unit" -require "controller/fake_models" class FormTagHelperTest < ActionView::TestCase include RenderERBUtils @@ -321,15 +320,6 @@ def test_file_field_tag_with_direct_upload_when_rails_direct_uploads_url_is_defi ) end - def test_file_field_tag_with_direct_upload_when_rails_direct_uploads_url_is_defined_and_model_is_provided - @controller = WithActiveStorageRoutesControllers.new - - assert_dom_equal( - "", - file_field_tag("picsplz", class: "pix", direct_upload: true, direct_upload_model: "Post") - ) - end - def test_file_field_tag_with_direct_upload_dont_mutate_arguments original_options = { class: "pix", direct_upload: true } diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index cc3b5ff1c8d47..628647d89f363 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -2,84 +2,12 @@ `Blob` on direct upload, and before saving an `Attachment` via direct or indirect uploads. Includes built in validators for content type and byte size. - See https://github.com/rails/rails/pull/41178 or the Active Storage guide for examples. + See Active Storage guide for examples. - *Abhishek Chandrasekhar*, *Alex Ghiculescu* + *Abhishek Chandrasekhar*, *Alex Ghiculescu*, *Sean Abrahams* * Fixes multiple `attach` calls within transaction not uploading files correctly. -* Attachments can be deleted after their association is no longer defined. - - Fixes #42514 - - *Don Sisco* - -* Make `vips` the default variant processor for new apps. - - See the upgrade guide for instructions on converting from `mini_magick` to `vips`. `mini_magick` is - not deprecated, existing apps can keep using it. - - *Breno Gazzola* - -* Deprecate `ActiveStorage::Current.host` in favor of `ActiveStorage::Current.url_options` which accepts - a host, protocol and port. - - *Santiago Bartesaghi* - -* Allow using [IAM](https://cloud.google.com/storage/docs/access-control/signed-urls) when signing URLs with GCS. - - ```yaml - gcs: - service: GCS - ... - iam: true - ``` - - *RRethy* - -* OpenSSL constants are now used for Digest computations. - - *Dirkjan Bussink* - -* Deprecate `config.active_storage.replace_on_assign_to_many`. Future versions of Rails - will behave the same way as when the config is set to `true`. - - *Santiago Bartesaghi* - -* Remove deprecated methods: `build_after_upload`, `create_after_upload!` in favor of `create_and_upload!`, - and `service_url` in favor of `url`. - - *Santiago Bartesaghi* - -* Add support of `strict_loading_by_default` to `ActiveStorage::Representations` controllers. - - *Anton Topchii*, *Andrew White* - -* Allow to detach an attachment when record is not persisted. - - *Jacopo Beschi* - -* Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips`. - - *Breno Gazzola* - -* Add metadata value for presence of video channel in video blobs. - - The `metadata` attribute of video blobs has a new boolean key named `video` that is set to - `true` if the file has an video channel and `false` if it doesn't. - - *Breno Gazzola* - -* Deprecate usage of `purge` and `purge_later` from the association extension. - - *Jacopo Beschi* - -* Passing extra parameters in `ActiveStorage::Blob#url` to S3 Client. - - This allows calls of `ActiveStorage::Blob#url` to have more interaction with - the S3 Presigner, enabling, amongst other options, custom S3 domain URL - Generation. - In the following example, the code failed to upload all but the last file to the configured service. ```ruby ActiveRecord::Base.transaction do diff --git a/activestorage/README.md b/activestorage/README.md index f710248063221..97ce4c72e335d 100644 --- a/activestorage/README.md +++ b/activestorage/README.md @@ -115,14 +115,34 @@ Active Storage includes attachment validators for the following properties: ```ruby class User < ActiveRecord::Base has_one_attached :avatar + # Validating Size # Accepts options for: `:in`, `:minimum`, `:maximum` validates :avatar, attachment_byte_size: { in: 0..1.megabyte } + validates :avatar, attachment_byte_size: { minimum: 17.kilobytes } + validates :avatar, attachment_byte_size: { maximum: 37.megabytes } + + # If you pass in a range `:in` is assumed and used validates :avatar, attachment_byte_size: 0..1.megabyte + + # Alternative syntax: + # validates_attachment :avatar, byte_size: { in: 0..1.megabyte } + # validates_attachment :avatar, byte_size: 0..1.megabyte + # validates_attachment_byte_size :avatar, in: 0..1.megabyte + # Validating Content Type # Accepts options for: `:in`, `:not` validates :avatar, attachment_content_type: { in: %w[image/jpeg image/png] } + validates :avatar, attachment_content_type: { not: %[image/gif] } + + # If you pass in a string or array `:in` is assumed and used validates :avatar, attachment_content_type: "image/jpeg" + validates :avatar, attachment_content_type: %w[image/jpeg image/png] + + # Alternative syntax: + # validates_attachment :avatar, content_type: { in: %w[image/jpeg image/png] } + # validates_attachment :avatar, content_type: "image/jpeg" + # validates_attachment_content_type :avatar, in: %w[image/jpeg image/png] end ``` diff --git a/activestorage/app/assets/javascripts/activestorage.esm.js b/activestorage/app/assets/javascripts/activestorage.esm.js index 7740659906a42..5a45c94fb35b3 100644 --- a/activestorage/app/assets/javascripts/activestorage.esm.js +++ b/activestorage/app/assets/javascripts/activestorage.esm.js @@ -508,14 +508,14 @@ function toArray(value) { } class BlobRecord { - constructor(file, checksum, url, model) { + constructor(file, checksum, url, signedModelAndAttribute) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, checksum: checksum, - model: model + signedModelAndAttribute: signedModelAndAttribute }; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); @@ -605,12 +605,12 @@ class BlobUpload { let id = 0; class DirectUpload { - constructor(file, url, delegate, model) { + constructor(file, url, delegate, signedModelAndAttribute) { this.id = ++id; this.file = file; this.url = url; this.delegate = delegate; - this.model = model; + this.signedModelAndAttribute = signedModelAndAttribute; } create(callback) { FileChecksum.create(this.file, ((error, checksum) => { @@ -618,7 +618,7 @@ class DirectUpload { callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.model); + const blob = new BlobRecord(this.file, checksum, this.url, this.signedModelAndAttribute); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -649,7 +649,7 @@ class DirectUploadController { constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this, this.model); + this.directUpload = new DirectUpload(this.file, this.url, this, this.signedModelAndAttribute); this.dispatch("initialize"); } start(callback) { @@ -680,8 +680,8 @@ class DirectUploadController { get url() { return this.input.getAttribute("data-direct-upload-url"); } - get model() { - return this.input.getAttribute("data-direct-upload-model"); + get signedModelAndAttribute() { + return this.input.getAttribute("data-direct-upload-signed-model-and-attribute"); } dispatch(name, detail = {}) { detail.file = this.file; diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js index ad6219b706ea5..aaed24261a47b 100644 --- a/activestorage/app/assets/javascripts/activestorage.js +++ b/activestorage/app/assets/javascripts/activestorage.js @@ -503,14 +503,14 @@ } } class BlobRecord { - constructor(file, checksum, url, model) { + constructor(file, checksum, url, signedModelAndAttribute) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, checksum: checksum, - model: model + signedModelAndAttribute: signedModelAndAttribute }; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); @@ -597,12 +597,12 @@ } let id = 0; class DirectUpload { - constructor(file, url, delegate, model) { + constructor(file, url, delegate, signedModelAndAttribute) { this.id = ++id; this.file = file; this.url = url; this.delegate = delegate; - this.model = model; + this.signedModelAndAttribute = signedModelAndAttribute; } create(callback) { FileChecksum.create(this.file, ((error, checksum) => { @@ -610,7 +610,7 @@ callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.model); + const blob = new BlobRecord(this.file, checksum, this.url, this.signedModelAndAttribute); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -639,7 +639,7 @@ constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this, this.model); + this.directUpload = new DirectUpload(this.file, this.url, this, this.signedModelAndAttribute); this.dispatch("initialize"); } start(callback) { @@ -670,8 +670,8 @@ get url() { return this.input.getAttribute("data-direct-upload-url"); } - get model() { - return this.input.getAttribute("data-direct-upload-model"); + get signedModelAndAttribute() { + return this.input.getAttribute("data-direct-upload-signed-model-and-attribute"); } dispatch(name, detail = {}) { detail.file = this.file; diff --git a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb index bff2698b68fa3..fce06ad06e0db 100644 --- a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb +++ b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb @@ -6,11 +6,11 @@ class ActiveStorage::DirectUploadsController < ActiveStorage::BaseController def create blob = ActiveStorage::Blob.build_for_direct_upload(**blob_args) - if blob.valid_with?(params.dig(:blob, :model)) + if blob.valid_with?(params.dig(:blob, :signed_model_and_attribute)) blob.save! render json: direct_upload_json(blob) else - head :unprocessable_entity + render json: { errors: blob.errors.as_json }, status: :unprocessable_entity end end diff --git a/activestorage/app/helpers/active_storage/direct_uploads_helper.rb b/activestorage/app/helpers/active_storage/direct_uploads_helper.rb new file mode 100644 index 0000000000000..819515c993bb0 --- /dev/null +++ b/activestorage/app/helpers/active_storage/direct_uploads_helper.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module ActiveStorage + module DirectUploadsHelper + # Requires a `Model.attribute` formatted string that represents the + # validations to be run. For example, if passed "User.avatar" ActiveStorage + # will run the ActiveStorage related validations defined for the User + # model's avatar attribute. + # + # Returns a signed string + def rails_direct_uploads_signed_model_and_attribute(model_and_attribute) + klass_name, attribute = model_and_attribute.split(".") + model = ActiveRecord::Base.const_get(klass_name) rescue nil + raise ArgumentError, "#{model_and_attribute} does not match a defined Model.attribute" if model.nil? || !model.method_defined?(attribute) + + ActiveStorage.verifier.generate(model_and_attribute) + end + end +end diff --git a/activestorage/app/javascript/activestorage/blob_record.js b/activestorage/app/javascript/activestorage/blob_record.js index 24327f8251867..b2219e748d1d7 100644 --- a/activestorage/app/javascript/activestorage/blob_record.js +++ b/activestorage/app/javascript/activestorage/blob_record.js @@ -1,7 +1,7 @@ import { getMetaValue } from "./helpers" export class BlobRecord { - constructor(file, checksum, url, model) { + constructor(file, checksum, url, signedModelAndAttribute) { this.file = file this.attributes = { @@ -9,7 +9,7 @@ export class BlobRecord { content_type: file.type || "application/octet-stream", byte_size: file.size, checksum: checksum, - model: model + signedModelAndAttribute: signedModelAndAttribute } this.xhr = new XMLHttpRequest diff --git a/activestorage/app/javascript/activestorage/direct_upload.js b/activestorage/app/javascript/activestorage/direct_upload.js index ef5bbd3b99d5a..26b735ccfb2a4 100644 --- a/activestorage/app/javascript/activestorage/direct_upload.js +++ b/activestorage/app/javascript/activestorage/direct_upload.js @@ -5,12 +5,12 @@ import { BlobUpload } from "./blob_upload" let id = 0 export class DirectUpload { - constructor(file, url, delegate, model) { + constructor(file, url, delegate, signedModelAndAttribute) { this.id = ++id this.file = file this.url = url this.delegate = delegate - this.model = model + this.signedModelAndAttribute = signedModelAndAttribute } create(callback) { @@ -20,7 +20,7 @@ export class DirectUpload { return } - const blob = new BlobRecord(this.file, checksum, this.url, this.model) + const blob = new BlobRecord(this.file, checksum, this.url, this.signedModelAndAttribute) notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr) blob.create(error => { diff --git a/activestorage/app/javascript/activestorage/direct_upload_controller.js b/activestorage/app/javascript/activestorage/direct_upload_controller.js index e114fde2e313a..709ef8a3035f6 100644 --- a/activestorage/app/javascript/activestorage/direct_upload_controller.js +++ b/activestorage/app/javascript/activestorage/direct_upload_controller.js @@ -5,7 +5,7 @@ export class DirectUploadController { constructor(input, file) { this.input = input this.file = file - this.directUpload = new DirectUpload(this.file, this.url, this, this.model) + this.directUpload = new DirectUpload(this.file, this.url, this, this.signedModelAndAttribute) this.dispatch("initialize") } @@ -41,8 +41,8 @@ export class DirectUploadController { return this.input.getAttribute("data-direct-upload-url") } - get model() { - return this.input.getAttribute("data-direct-upload-model") + get signedModelAndAttribute() { + return this.input.getAttribute("data-direct-upload-signed-model-and-attribute") } dispatch(name, detail = {}) { diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 28a7292de6368..aa2105e962b24 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -379,15 +379,19 @@ def content_type=(value) # # Returns false if the model cannot be found. # Returns true if no validators are configured on the model. - def valid_with?(klass_name = nil) - return true if klass_name.nil? + def valid_with?(signed_model_and_attribute = nil) + return true if signed_model_and_attribute.nil? + + klass_name, attribute = ActiveStorage.verifier.verified(signed_model_and_attribute).split(".") model = ActiveRecord::Base.const_get(klass_name) rescue nil return false if model.nil? - model.validators.select { |v| v.is_a?(ActiveStorage::Validations::BaseValidator) }.all? do |validator| - validator.valid_with?(self) + model.validators.select { |v| v.is_a?(ActiveStorage::Validations::BaseValidator) && v.attributes.include?(attribute.to_sym) }.each do |validator| + validator.valid_with?(self, attribute) end + + self.errors.blank? end private diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index 94c194ff4cb1c..bb7decd45611e 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -176,6 +176,8 @@ class Engine < Rails::Engine # :nodoc: initializer "action_view.configuration" do config.after_initialize do |app| ActiveSupport.on_load(:action_view) do + ActionView::Helpers.include(ActiveStorage::DirectUploadsHelper) + multiple_file_field_include_hidden = app.config.active_storage.delete(:multiple_file_field_include_hidden) unless multiple_file_field_include_hidden.nil? diff --git a/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb b/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb index 0ed8c119a6232..dfc8b56844430 100644 --- a/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb +++ b/activestorage/lib/active_storage/validations/attachment_byte_size_validator.rb @@ -34,6 +34,10 @@ def check_validity! end private + def blob_field_name + :byte_size + end + def error_key_for(check_name) check_name == :in ? :in_between : check_name end diff --git a/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb b/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb index 3b1b87cce744a..add9b16b60834 100644 --- a/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb +++ b/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb @@ -23,6 +23,10 @@ def check_validity! end private + def blob_field_name + :content_type + end + def error_key_for(check_name) { in: :inclusion, not: :exclusion, with: :inclusion }[check_name.to_sym] end @@ -39,7 +43,24 @@ def passes_check?(blob, check_name, check_value) !check_value.include?(blob.content_type) when :with # TODO: implement check_options_validity from AM::Validators::FormatValidator - check_value.match?(blob.content_type) + # QUESTION: How best to implement check_options_validity? Going with copy+paste for now. + check_value = Regexp.new(check_value) if check_value.is_a?(String) + if check_value.is_a?(Regexp) + if check_value.source.start_with?("^") || (check_value.source.end_with?("$") && !check_value.source.end_with?("\\$")) + raise ArgumentError, "The provided regular expression is using multiline anchors (^ or $), " \ + "which may present a security risk. Did you mean to use \\A and \\z, or forgot to add the " \ + ":multiline => true option?" + end + check_value.match?(blob.content_type) + elsif check_value.respond_to?(:call) + # QUESTION: One would expect the proc or lambda to be called with + # the Attachable, not the Blob. However, for direct uploads there + # is no Attachable available. Should we pass the blob instead? Or + # nil? + check_value.call(@record || blob).match?(blob.content_type) + else + raise ArgumentError, "A regular expression, proc, or lambda must be supplied to :with" + end end end end diff --git a/activestorage/lib/active_storage/validations/attachment_presence_validator.rb b/activestorage/lib/active_storage/validations/attachment_presence_validator.rb index 89ecd5c81494b..8e89ff770ede1 100644 --- a/activestorage/lib/active_storage/validations/attachment_presence_validator.rb +++ b/activestorage/lib/active_storage/validations/attachment_presence_validator.rb @@ -16,21 +16,21 @@ def validate_each(record, attribute, _value) end private - def error_key_for(check_name) - ## Not Required - end + def error_key_for(check_name) + ## Not Required + end - def options_redundant? - ## Not Required - end + def options_redundant? + ## Not Required + end - def passes_check?(blob, check_name, check_value) - ## Not Required - end + def passes_check?(blob, check_name, check_value) + ## Not Required + end end module HelperMethods - # Validates the content type of the ActiveStorage attachments. Happens by + # Validates the presence of the ActiveStorage attachments. Happens by # default on save. # # class Employee < ActiveRecord::Base @@ -52,4 +52,3 @@ def validates_attachment_presence(*attributes) end end end - diff --git a/activestorage/lib/active_storage/validations/base_validator.rb b/activestorage/lib/active_storage/validations/base_validator.rb index c72f45506c456..496f67e1aa741 100644 --- a/activestorage/lib/active_storage/validations/base_validator.rb +++ b/activestorage/lib/active_storage/validations/base_validator.rb @@ -8,12 +8,18 @@ def initialize(options = {}) @error_options = { message: options[:message] } end - def valid_with?(blob) + def valid_with?(blob, attribute=nil) valid = true each_check do |check_name, check_value| next if passes_check?(blob, check_name, check_value) - @record.errors.add(@name, error_key_for(check_name), **error_options) if @record + # If we're dealing with a Blob that has an Attachment to an Attachable + if @record + @record.errors.add(@name, error_key_for(check_name), **error_options) + # If we're dealing with a direct upload Blob that has no Attachment to an Attachable + elsif attribute.present? + blob.errors.add(blob_field_name, error_key_for(check_name), **error_options) + end valid = false end valid @@ -35,6 +41,10 @@ def available_checks self.class::AVAILABLE_CHECKS end + def blob_field_name + :base + end + def options_blank? available_checks.none? { |arg| options.has_key?(arg) } end diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index e0460dcc2baac..e34712ea0d6c0 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -213,8 +213,16 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati "library_ID": "12345" } - post rails_direct_uploads_url, params: { blob: { - filename: "racecar.jpg", byte_size: file.size, checksum: checksum, content_type: "image/jpg", metadata: metadata, model: "User" } } + post rails_direct_uploads_url, params: { + blob: { + filename: "racecar.jpg", + byte_size: file.size, + checksum: checksum, + content_type: "image/jpg", + metadata: metadata, + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + } + } assert_response :success @@ -244,8 +252,16 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati "library_ID": "12345" } - post rails_direct_uploads_url, params: { blob: { - filename: "racecar.jpg", byte_size: file.size, checksum: checksum, content_type: "image/jpg", metadata: metadata, model: "User" } } + post rails_direct_uploads_url, params: { + blob: { + filename: "racecar.jpg", + byte_size: file.size, + checksum: checksum, + content_type: "image/jpg", + metadata: metadata, + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + } + } assert_response :success @@ -262,6 +278,45 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati end test "creating new direct upload with model where validations fail" do + User.validates :avatar, attachment_content_type: { with: /\Atext\// } + User.validates :avatar, attachment_byte_size: { minimum: 50.megabytes } + + file = file_fixture("racecar.jpg").open + checksum = Digest::MD5.base64digest(file.read) + metadata = { + "foo": "bar", + "my_key_1": "my_value_1", + "my_key_2": "my_value_2", + "platform": "my_platform", + "library_ID": "12345" + } + + post rails_direct_uploads_url, params: { + blob: { + filename: "racecar.jpg", + byte_size: file.size, + checksum: checksum, + content_type: "image/jpg", + metadata: metadata, + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + } + } + + assert_response :unprocessable_entity + + @response.parsed_body.tap do |details| + assert_equal Hash[ + "content_type" => [ + "is not included in the list", + ], + "byte_size" => [ + "must be greater than or equal to 50 MB", + ], + ], details["errors"] + end + end + + test "creating new direct upload with model where validations fail with custom messages" do User.validates :avatar, attachment_content_type: { with: /\Atext\//, message: "must be a text file" } User.validates :avatar, attachment_byte_size: { minimum: 50.megabytes, message: "can't be smaller than 50 MB" } @@ -275,10 +330,29 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati "library_ID": "12345" } - post rails_direct_uploads_url, params: { blob: { - filename: "racecar.jpg", byte_size: file.size, checksum: checksum, content_type: "image/jpg", metadata: metadata, model: "User" } } + post rails_direct_uploads_url, params: { + blob: { + filename: "racecar.jpg", + byte_size: file.size, + checksum: checksum, + content_type: "image/jpg", + metadata: metadata, + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + } + } assert_response :unprocessable_entity + + @response.parsed_body.tap do |details| + assert_equal Hash[ + "content_type" => [ + "must be a text file", + ], + "byte_size" => [ + "can't be smaller than 50 MB", + ], + ], details["errors"] + end end private diff --git a/activestorage/test/models/validations/attachment_byte_size_validator_test.rb b/activestorage/test/models/validations/attachment_byte_size_validator_test.rb index 87a543c896e57..a3b0ace57249d 100644 --- a/activestorage/test/models/validations/attachment_byte_size_validator_test.rb +++ b/activestorage/test/models/validations/attachment_byte_size_validator_test.rb @@ -10,7 +10,7 @@ class ActiveStorage::AttachmentByteSizeValidatorTest < ActiveSupport::TestCase @old_validators = User._validators.deep_dup @old_callbacks = User._validate_callbacks.deep_dup - @blob = create_blob(filename: "funky.jpg") + @blob = create_blob @user = User.create(name: "Anjali") @byte_size = @blob.byte_size @@ -82,7 +82,7 @@ class ActiveStorage::AttachmentByteSizeValidatorTest < ActiveSupport::TestCase end test "persisted record, updating attachments" do - other_blob = create_blob(filename: "town.jpg") + other_blob = create_blob @user.avatar.attach(other_blob) @user.highlights.attach(other_blob) diff --git a/activestorage/test/models/validations/attachment_content_type_validator_test.rb b/activestorage/test/models/validations/attachment_content_type_validator_test.rb index 90a231e53d03a..3a02347f581f2 100644 --- a/activestorage/test/models/validations/attachment_content_type_validator_test.rb +++ b/activestorage/test/models/validations/attachment_content_type_validator_test.rb @@ -10,7 +10,7 @@ class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCas @old_validators = User._validators.deep_dup @old_callbacks = User._validate_callbacks.deep_dup - @blob = create_blob(filename: "funky.jpg") + @blob = create_blob @user = User.create(name: "Anjali") @content_types = %w[text/plain image/jpeg] @@ -75,7 +75,7 @@ class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCas end test "persisted record, updating attachments" do - old_blob = create_blob(filename: "town.jpg") + old_blob = create_blob @user.avatar.attach(old_blob) @user.highlights.attach(old_blob) @@ -294,8 +294,8 @@ class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCas end test "validating with `validates()`, invalid shortcut option" do - User.validates(:avatar, attachment_content_type: @bad_content_types.first.to_sym) - User.validates(:highlights, attachment_content_type: @bad_content_types.first.to_sym) + User.validates(:avatar, attachment_content_type: @bad_content_types.first) + User.validates(:highlights, attachment_content_type: @bad_content_types.first) @user.avatar.attach(@blob) @user.highlights.attach(@blob) @@ -306,8 +306,8 @@ class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCas User.clear_validators! - User.validates(:avatar, attachment_content_type: @content_types.first.to_sym) - User.validates(:highlights, attachment_content_type: @content_types.first.to_sym) + User.validates(:avatar, attachment_content_type: @content_types.first) + User.validates(:highlights, attachment_content_type: @content_types.first) assert @user.save end @@ -369,25 +369,6 @@ class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCas assert @user.save end - test "validating with `validates_attachment()`, Symbol shortcut option" do - User.validates_attachment(:avatar, content_type: @bad_content_types.first.to_sym) - User.validates_attachment(:highlights, content_type: @bad_content_types.first.to_sym) - - @user.avatar.attach(@blob) - @user.highlights.attach(@blob) - - assert_not @user.valid? - assert_equal ["is not included in the list"], @user.errors.messages[:avatar] - assert_equal ["is not included in the list"], @user.errors.messages[:highlights] - - User.clear_validators! - - User.validates_attachment(:avatar, content_type: @content_types.first.to_sym) - User.validates_attachment(:highlights, content_type: @content_types.first.to_sym) - - assert @user.save - end - test "validating with `validates_attachment_content_type()`" do User.validates_attachment_content_type(:avatar, in: @bad_content_types) User.validates_attachment_content_type(:highlights, in: @bad_content_types) diff --git a/activestorage/test/models/validations/attachment_precense_validator_test.rb b/activestorage/test/models/validations/attachment_precense_validator_test.rb index 306f94e9a6cf1..d6b8634174fb5 100644 --- a/activestorage/test/models/validations/attachment_precense_validator_test.rb +++ b/activestorage/test/models/validations/attachment_precense_validator_test.rb @@ -10,7 +10,7 @@ class ActiveStorage::AttachmentPresenceValidatorTest < ActiveSupport::TestCase @old_validators = User._validators.deep_dup @old_callbacks = User._validate_callbacks.deep_dup - @blob = create_blob(filename: "funky.jpg") + @blob = create_blob @user = User.create(name: "Anjali") end @@ -54,7 +54,7 @@ class ActiveStorage::AttachmentPresenceValidatorTest < ActiveSupport::TestCase end test "persisted record, updating attachments" do - other_blob = create_blob(filename: "town.jpg") + other_blob = create_blob @user.avatar.attach(other_blob) @user.highlights.attach(other_blob) diff --git a/activestorage/test/template/file_field_tag_test.rb b/activestorage/test/template/file_field_tag_test.rb new file mode 100644 index 0000000000000..dc9ef4f049b28 --- /dev/null +++ b/activestorage/test/template/file_field_tag_test.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" +class ActiveStorage::FileFieldTagTest < ActionView::TestCase + tests ActionView::Helpers::FormTagHelper + + test "file_field_tag has access to rails_direct_uploads_signed_model_and_attribute" do + expected = "" + assert_dom_equal expected, file_field_tag("avatar", direct_upload: true) + + expected = "" + assert_dom_equal expected, file_field_tag("avatar", direct_upload: true, direct_upload_signed_model_and_attribute: "User.avatar") + end + + test "file_field has access to rails_direct_uploads_signed_model_and_attribute" do + expected = "" + assert_dom_equal expected, file_field("user", "avatar", direct_upload: true) + + expected = "" + assert_dom_equal expected, file_field("user", "avatar", direct_upload: true, direct_upload_signed_model_and_attribute: "User.avatar") + end + + Routes = ActionDispatch::Routing::RouteSet.new + Routes.draw do + resources :users + end + include Routes.url_helpers + + test "form builder includes direct upload attributes" do + @user = User.create!(name: "DHH") + + form_for(@user) do |f| + concat f.file_field(:avatar, direct_upload: true) + end + + expected = "
" + + assert_includes expected, output_buffer + end +end diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 6f92e787c33fd..a80b531af0a46 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -45,6 +45,8 @@ ActiveStorage.logger = ActiveSupport::Logger.new(nil) ActiveStorage.verifier = ActiveSupport::MessageVerifier.new("Testing") ActiveStorage::FixtureSet.file_fixture_path = File.expand_path("fixtures/files", __dir__) +ActionDispatch::IntegrationTest.include(ActiveStorage::DirectUploadsHelper) +ActionView::TestCase.include(ActiveStorage::DirectUploadsHelper) class ActiveSupport::TestCase self.file_fixture_path = ActiveStorage::FixtureSet.file_fixture_path diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 9bd5457e4a9ce..2cb900aea89a3 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -600,15 +600,15 @@ Also accepts a `Range` as a shortcut option for `:in`: Validates the content type of the attached `Blob` object: ```ruby - validates :avatar, attachment_byte_content_type: { in: %w[image/jpeg image/png] } - validates :avatar, attachment_byte_content_type: { not: %w[application/pdf] } + validates :avatar, attachment_content_type: { in: %w[image/jpeg image/png] } + validates :avatar, attachment_content_type: { not: %w[application/pdf] } ``` Also accepts a `Array` or `String` as a shortcut option for `:in`: ```ruby - validates :avatar, attachment_byte_content_type: %w[image/jpeg image/png] - validates :avatar, attachment_byte_content_type: "image/jpeg" + validates :avatar, attachment_content_type: %w[image/jpeg image/png] + validates :avatar, attachment_content_type: "image/jpeg" ``` ### Validation Helper @@ -985,6 +985,30 @@ directly from the client to the cloud. ``` + `FormBuilder` will ensure any defined ActiveStorage validations get run for + direct uploads, however if you're not using `FormBuilder` and you want to + apply validations to direct uploads you will need to pass in the + "Model.attribute" you wish to use for validating the upload: + + ```erb + + ``` + + Will apply any defined ActiveStorage validations on the `User` model's + `avatar` ActiveStorage attribute such as byte_size and content_type + validations. For example: + ```ruby + validates :avatar, attachment_byte_size: { in: 0..1.megabyte } + validates :avatar, attachment_content_type: { in: %w[image/jpeg image/png] } + ``` + + The direct upload will not proceed if the file's *reported* byte_size or + content_type do not pass validation. Note that ActiveStorage validations for + direct uploads are checking the byte_size and content_type information + provided by the client. They are not verifying the *actual* byte_size and + content_type of the file. That is beyond the scope of ActiveStorage + validations for direct uploads. + 3. Configure CORS on third-party storage services to allow direct upload requests. 4. That's it! Uploads begin upon form submission. From 2ac8b97159b20d3a2b1347d36586e0f1309c4632 Mon Sep 17 00:00:00 2001 From: Sean Abrahams Date: Tue, 21 Jun 2022 09:58:12 -0700 Subject: [PATCH 4/5] Update rails_direct_uploads_signed_model_and_attribute to take in a model instance and attribute name. --- .../action_view/helpers/form_tag_helper.rb | 4 - activestorage/CHANGELOG.md | 4 +- .../active_storage/direct_uploads_helper.rb | 28 ++++-- .../app/models/active_storage/blob.rb | 12 ++- .../attachment_content_type_validator.rb | 6 +- .../validations/base_validator.rb | 12 +-- .../direct_uploads_controller_test.rb | 97 ++++++++++++++++--- .../attachment_content_type_validator_test.rb | 24 +++++ .../test/template/file_field_tag_test.rb | 10 +- 9 files changed, 148 insertions(+), 49 deletions(-) diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index 31e72fd1f9073..0a003c05c3d11 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -989,10 +989,6 @@ def add_direct_upload_attributes(options) options["data-direct-upload-url"] = rails_direct_uploads_url end - if options[:direct_upload_signed_model_and_attribute].present? && respond_to?(:rails_direct_uploads_signed_model_and_attribute) - options["data-direct-upload-signed-model-and-attribute"] = rails_direct_uploads_signed_model_and_attribute(options.delete(:direct_upload_signed_model_and_attribute)) - end - options end end diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 628647d89f363..95bec98aa3652 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,6 +1,4 @@ -* Introduce Active Storage validators. Subclasses of `ActiveStorage::Validations::BaseValidator` run before creating a - `Blob` on direct upload, and before saving an `Attachment` via direct or indirect uploads. Includes built in validators - for content type and byte size. +* Introduce Active Storage validators. Subclasses of `ActiveStorage::Validations::BaseValidator` run before creating a `Blob` on direct upload, and before saving an `Attachment` via direct or indirect uploads. Includes built in validators for content type and byte size. See Active Storage guide for examples. diff --git a/activestorage/app/helpers/active_storage/direct_uploads_helper.rb b/activestorage/app/helpers/active_storage/direct_uploads_helper.rb index 819515c993bb0..1936ddb716ace 100644 --- a/activestorage/app/helpers/active_storage/direct_uploads_helper.rb +++ b/activestorage/app/helpers/active_storage/direct_uploads_helper.rb @@ -2,18 +2,28 @@ module ActiveStorage module DirectUploadsHelper - # Requires a `Model.attribute` formatted string that represents the - # validations to be run. For example, if passed "User.avatar" ActiveStorage - # will run the ActiveStorage related validations defined for the User - # model's avatar attribute. + # Requires an instance of a Model and an attribute which allows us to know + # what validations to run. Supports persisted and non-persisted model + # instances so that direct uploads can be made for new records before + # they've been persisted. + # + # For example, regardless of whether the passed in model is persisted, + # `@user, :avatar` or `User.new, :avatar`, ActiveStorage will run the + # ActiveStorage related validations defined for the User model's avatar + # attribute. The passed in model instance comes into play when it's passed + # to any procs defined for those validations. + # + # class User < ApplicationRecord + # validates :avatar, attachment_content_type: { with: Proc.new { |user| user.persisted? ? %w[image/jpeg image/png image/gif] : %w[image/jpeg] } } + # end # # Returns a signed string - def rails_direct_uploads_signed_model_and_attribute(model_and_attribute) - klass_name, attribute = model_and_attribute.split(".") - model = ActiveRecord::Base.const_get(klass_name) rescue nil - raise ArgumentError, "#{model_and_attribute} does not match a defined Model.attribute" if model.nil? || !model.method_defined?(attribute) + def rails_direct_uploads_signed_model_and_attribute(model, attribute) + raise ArgumentError, "#{model.model_name} does not have an attribute named #{attribute}" if !model.respond_to?(attribute) - ActiveStorage.verifier.generate(model_and_attribute) + ActiveStorage.verifier.generate("#{model.to_global_id}--#{attribute}") + rescue URI::GID::MissingModelIdError + ActiveStorage.verifier.generate("#{model.model_name}--#{attribute}") end end end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index aa2105e962b24..39317cc10132b 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -382,13 +382,15 @@ def content_type=(value) def valid_with?(signed_model_and_attribute = nil) return true if signed_model_and_attribute.nil? - klass_name, attribute = ActiveStorage.verifier.verified(signed_model_and_attribute).split(".") + model_gid, attribute = ActiveStorage.verifier.verified(signed_model_and_attribute).split("--") + model = GlobalID::Locator.locate(model_gid) - model = ActiveRecord::Base.const_get(klass_name) rescue nil - return false if model.nil? + # When model_gid is a class name pointing to an unpersisted model + # model_gid => "User" + model ||= ActiveRecord::Base.const_get(model_gid).new - model.validators.select { |v| v.is_a?(ActiveStorage::Validations::BaseValidator) && v.attributes.include?(attribute.to_sym) }.each do |validator| - validator.valid_with?(self, attribute) + model.class.validators.select { |v| v.is_a?(ActiveStorage::Validations::BaseValidator) && v.attributes.include?(attribute.to_sym) }.each do |validator| + validator.valid_with?(self, model, attribute) end self.errors.blank? diff --git a/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb b/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb index add9b16b60834..bb1fb11edd29d 100644 --- a/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb +++ b/activestorage/lib/active_storage/validations/attachment_content_type_validator.rb @@ -53,11 +53,7 @@ def passes_check?(blob, check_name, check_value) end check_value.match?(blob.content_type) elsif check_value.respond_to?(:call) - # QUESTION: One would expect the proc or lambda to be called with - # the Attachable, not the Blob. However, for direct uploads there - # is no Attachable available. Should we pass the blob instead? Or - # nil? - check_value.call(@record || blob).match?(blob.content_type) + passes_check?(blob, :in, check_value.call(@record)) else raise ArgumentError, "A regular expression, proc, or lambda must be supplied to :with" end diff --git a/activestorage/lib/active_storage/validations/base_validator.rb b/activestorage/lib/active_storage/validations/base_validator.rb index 496f67e1aa741..574e3559e627f 100644 --- a/activestorage/lib/active_storage/validations/base_validator.rb +++ b/activestorage/lib/active_storage/validations/base_validator.rb @@ -8,17 +8,17 @@ def initialize(options = {}) @error_options = { message: options[:message] } end - def valid_with?(blob, attribute=nil) + def valid_with?(blob, record=nil, attribute=nil) + @record ||= record valid = true each_check do |check_name, check_value| next if passes_check?(blob, check_name, check_value) - # If we're dealing with a Blob that has an Attachment to an Attachable - if @record - @record.errors.add(@name, error_key_for(check_name), **error_options) - # If we're dealing with a direct upload Blob that has no Attachment to an Attachable - elsif attribute.present? + # If we're validating a blob directly, not through their Attachable + if attribute.present? blob.errors.add(blob_field_name, error_key_for(check_name), **error_options) + else + @record.errors.add(@name, error_key_for(check_name), **error_options) end valid = false end diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index e34712ea0d6c0..2651744458ffe 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -218,9 +218,9 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati filename: "racecar.jpg", byte_size: file.size, checksum: checksum, - content_type: "image/jpg", + content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), } } @@ -232,9 +232,9 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati assert_equal file.size, details["byte_size"] assert_equal checksum, details["checksum"] assert_equal metadata, details["metadata"].transform_keys(&:to_sym) - assert_equal "image/jpg", details["content_type"] + assert_equal "image/jpeg", details["content_type"] assert_match(/rails\/active_storage\/disk/, details["direct_upload"]["url"]) - assert_equal({ "Content-Type" => "image/jpg" }, details["direct_upload"]["headers"]) + assert_equal({ "Content-Type" => "image/jpeg" }, details["direct_upload"]["headers"]) end end @@ -257,9 +257,9 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati filename: "racecar.jpg", byte_size: file.size, checksum: checksum, - content_type: "image/jpg", + content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), } } @@ -271,9 +271,9 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati assert_equal file.size, details["byte_size"] assert_equal checksum, details["checksum"] assert_equal metadata, details["metadata"].transform_keys(&:to_sym) - assert_equal "image/jpg", details["content_type"] + assert_equal "image/jpeg", details["content_type"] assert_match(/rails\/active_storage\/disk/, details["direct_upload"]["url"]) - assert_equal({ "Content-Type" => "image/jpg" }, details["direct_upload"]["headers"]) + assert_equal({ "Content-Type" => "image/jpeg" }, details["direct_upload"]["headers"]) end end @@ -296,9 +296,9 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati filename: "racecar.jpg", byte_size: file.size, checksum: checksum, - content_type: "image/jpg", + content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), } } @@ -335,9 +335,9 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati filename: "racecar.jpg", byte_size: file.size, checksum: checksum, - content_type: "image/jpg", + content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute("User.avatar"), + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), } } @@ -355,6 +355,79 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati end end + test "creating new direct upload with model where validation uses proc and succeeds" do + User.validates :avatar, attachment_content_type: { with: Proc.new { |user| user.persisted? ? %w[image/png] : %w[image/jpeg] } } + + file = file_fixture("racecar.jpg").open + checksum = Digest::MD5.base64digest(file.read) + metadata = { + "foo": "bar", + "my_key_1": "my_value_1", + "my_key_2": "my_value_2", + "platform": "my_platform", + "library_ID": "12345" + } + + post rails_direct_uploads_url, params: { + blob: { + filename: "racecar.jpg", + byte_size: file.size, + checksum: checksum, + content_type: "image/jpeg", + metadata: metadata, + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + } + } + + assert_response :success + + @response.parsed_body.tap do |details| + assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) + assert_equal "racecar.jpg", details["filename"] + assert_equal file.size, details["byte_size"] + assert_equal checksum, details["checksum"] + assert_equal metadata, details["metadata"].transform_keys(&:to_sym) + assert_equal "image/jpeg", details["content_type"] + assert_match(/rails\/active_storage\/disk/, details["direct_upload"]["url"]) + assert_equal({ "Content-Type" => "image/jpeg" }, details["direct_upload"]["headers"]) + end + end + + test "creating new direct upload with model where validation uses proc and fails" do + User.validates :avatar, attachment_content_type: { with: Proc.new { |user| user.persisted? ? %w[image/jpeg] : %w[image/png] } } + + file = file_fixture("racecar.jpg").open + checksum = Digest::MD5.base64digest(file.read) + metadata = { + "foo": "bar", + "my_key_1": "my_value_1", + "my_key_2": "my_value_2", + "platform": "my_platform", + "library_ID": "12345" + } + + post rails_direct_uploads_url, params: { + blob: { + filename: "racecar.jpg", + byte_size: file.size, + checksum: checksum, + content_type: "image/jpeg", + metadata: metadata, + signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + } + } + + assert_response :unprocessable_entity + + @response.parsed_body.tap do |details| + assert_equal Hash[ + "content_type" => [ + "is not included in the list", + ], + ], details["errors"] + end + end + private def set_include_root_in_json(value) original = ActiveRecord::Base.include_root_in_json diff --git a/activestorage/test/models/validations/attachment_content_type_validator_test.rb b/activestorage/test/models/validations/attachment_content_type_validator_test.rb index 3a02347f581f2..84e2b572b392a 100644 --- a/activestorage/test/models/validations/attachment_content_type_validator_test.rb +++ b/activestorage/test/models/validations/attachment_content_type_validator_test.rb @@ -408,6 +408,30 @@ class ActiveStorage::AttachmentContentTypeValidatorTest < ActiveSupport::TestCas ) end + test "passing a proc to :with option" do + User.validates_with(VALIDATOR, attributes: :avatar, with: Proc.new { |user| user.persisted? ? @content_types : @bad_content_types }) + User.validates_with(VALIDATOR, attributes: :highlights, with: Proc.new { |user| user.persisted? ? @content_types : @bad_content_types }) + + @user.avatar.attach(@blob) + @user.highlights.attach(@blob) + + assert @user.save + + new_user = User.new + new_user.avatar.attach(@blob) + new_user.highlights.attach(@blob) + + assert_not new_user.valid? + assert_equal( + ["is not included in the list"], + new_user.errors.messages[:avatar] + ) + assert_equal( + ["is not included in the list"], + new_user.errors.messages[:highlights] + ) + end + test "inheritance of default ActiveModel options" do User.validates_with(VALIDATOR, attributes: :avatar, in: @bad_content_types, if: Proc.new { false }) User.validates_with(VALIDATOR, attributes: :highlights, in: @bad_content_types, if: Proc.new { false }) diff --git a/activestorage/test/template/file_field_tag_test.rb b/activestorage/test/template/file_field_tag_test.rb index dc9ef4f049b28..979b1ca331a73 100644 --- a/activestorage/test/template/file_field_tag_test.rb +++ b/activestorage/test/template/file_field_tag_test.rb @@ -9,16 +9,16 @@ class ActiveStorage::FileFieldTagTest < ActionView::TestCase expected = "" assert_dom_equal expected, file_field_tag("avatar", direct_upload: true) - expected = "" - assert_dom_equal expected, file_field_tag("avatar", direct_upload: true, direct_upload_signed_model_and_attribute: "User.avatar") + expected = "" + assert_dom_equal expected, file_field_tag("avatar", direct_upload: true, data: { direct_upload_signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar) }) end test "file_field has access to rails_direct_uploads_signed_model_and_attribute" do expected = "" assert_dom_equal expected, file_field("user", "avatar", direct_upload: true) - expected = "" - assert_dom_equal expected, file_field("user", "avatar", direct_upload: true, direct_upload_signed_model_and_attribute: "User.avatar") + expected = "" + assert_dom_equal expected, file_field("user", "avatar", direct_upload: true, data: { direct_upload_signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar) }) end Routes = ActionDispatch::Routing::RouteSet.new @@ -34,7 +34,7 @@ class ActiveStorage::FileFieldTagTest < ActionView::TestCase concat f.file_field(:avatar, direct_upload: true) end - expected = "
" + expected = "
" assert_includes expected, output_buffer end From 103bae8c6f45e38fe9756734f9a8c2eb4af51705 Mon Sep 17 00:00:00 2001 From: Sean Abrahams Date: Tue, 21 Jun 2022 15:40:00 -0700 Subject: [PATCH 5/5] signed_validation_id --- .../lib/action_view/helpers/form_helper.rb | 2 +- .../action_view/helpers/form_tag_helper.rb | 9 +++++- .../assets/javascripts/activestorage.esm.js | 16 +++++----- .../app/assets/javascripts/activestorage.js | 16 +++++----- .../direct_uploads_controller.rb | 2 +- .../active_storage/direct_uploads_helper.rb | 29 ------------------- .../javascript/activestorage/blob_record.js | 4 +-- .../javascript/activestorage/direct_upload.js | 6 ++-- .../activestorage/direct_upload_controller.js | 6 ++-- .../app/models/active_storage/blob.rb | 6 ++-- activestorage/lib/active_storage/attached.rb | 8 +++++ activestorage/lib/active_storage/engine.rb | 2 -- .../direct_uploads_controller_test.rb | 12 ++++---- .../test/template/file_field_tag_test.rb | 20 ++++++++----- activestorage/test/test_helper.rb | 2 -- guides/source/active_storage_overview.md | 7 +++-- 16 files changed, 67 insertions(+), 80 deletions(-) delete mode 100644 activestorage/app/helpers/active_storage/direct_uploads_helper.rb diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index 060426bb3e3da..c0effe2e821f6 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -1242,7 +1242,7 @@ def hidden_field(object_name, method, options = {}) def file_field(object_name, method, options = {}) options = { include_hidden: multiple_file_field_include_hidden }.merge!(options) - Tags::FileField.new(object_name, method, self, add_direct_upload_attributes(options.dup)).render + Tags::FileField.new(object_name, method, self, add_direct_upload_attributes(options.dup.merge({ attribute: method }))).render end # Returns a textarea opening and closing tag set tailored for accessing a specified attribute (identified by +method+) diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index 0a003c05c3d11..e6c772186f073 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -342,7 +342,7 @@ def hidden_field_tag(name, value = nil, options = {}) # file_field_tag 'file', accept: 'text/html', class: 'upload', value: 'index.html' # # => def file_field_tag(name, options = {}) - text_field_tag(name, nil, add_direct_upload_attributes(options.merge(type: :file))) + text_field_tag(name, nil, add_direct_upload_attributes(options.merge(type: :file, attribute: name))) end # Creates a password field, a masked text field that will hide the users input behind a mask character. @@ -987,8 +987,15 @@ def set_default_disable_with(value, tag_options) def add_direct_upload_attributes(options) if options.delete(:direct_upload) && respond_to?(:rails_direct_uploads_url) options["data-direct-upload-url"] = rails_direct_uploads_url + + if options[:object].present? && + options[:attribute].present? && + options[:object].send(options[:attribute]).respond_to?(:to_signed_validation_id) + options["data-direct-upload-signed-validation-id"] = options[:object].send(options[:attribute]).to_signed_validation_id + end end + options.delete(:attribute) options end end diff --git a/activestorage/app/assets/javascripts/activestorage.esm.js b/activestorage/app/assets/javascripts/activestorage.esm.js index 5a45c94fb35b3..c8151257dabf7 100644 --- a/activestorage/app/assets/javascripts/activestorage.esm.js +++ b/activestorage/app/assets/javascripts/activestorage.esm.js @@ -508,14 +508,14 @@ function toArray(value) { } class BlobRecord { - constructor(file, checksum, url, signedModelAndAttribute) { + constructor(file, checksum, url, signedValidationId) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, checksum: checksum, - signedModelAndAttribute: signedModelAndAttribute + signedValidationId: signedValidationId }; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); @@ -605,12 +605,12 @@ class BlobUpload { let id = 0; class DirectUpload { - constructor(file, url, delegate, signedModelAndAttribute) { + constructor(file, url, delegate, signedValidationId) { this.id = ++id; this.file = file; this.url = url; this.delegate = delegate; - this.signedModelAndAttribute = signedModelAndAttribute; + this.signedValidationId = signedValidationId; } create(callback) { FileChecksum.create(this.file, ((error, checksum) => { @@ -618,7 +618,7 @@ class DirectUpload { callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.signedModelAndAttribute); + const blob = new BlobRecord(this.file, checksum, this.url, this.signedValidationId); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -649,7 +649,7 @@ class DirectUploadController { constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this, this.signedModelAndAttribute); + this.directUpload = new DirectUpload(this.file, this.url, this, this.signedValidationId); this.dispatch("initialize"); } start(callback) { @@ -680,8 +680,8 @@ class DirectUploadController { get url() { return this.input.getAttribute("data-direct-upload-url"); } - get signedModelAndAttribute() { - return this.input.getAttribute("data-direct-upload-signed-model-and-attribute"); + get signedValidationId() { + return this.input.getAttribute("data-direct-upload-signed-validation-id"); } dispatch(name, detail = {}) { detail.file = this.file; diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js index aaed24261a47b..55e8f9e56a0a8 100644 --- a/activestorage/app/assets/javascripts/activestorage.js +++ b/activestorage/app/assets/javascripts/activestorage.js @@ -503,14 +503,14 @@ } } class BlobRecord { - constructor(file, checksum, url, signedModelAndAttribute) { + constructor(file, checksum, url, signedValidationId) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, checksum: checksum, - signedModelAndAttribute: signedModelAndAttribute + signedValidationId: signedValidationId }; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); @@ -597,12 +597,12 @@ } let id = 0; class DirectUpload { - constructor(file, url, delegate, signedModelAndAttribute) { + constructor(file, url, delegate, signedValidationId) { this.id = ++id; this.file = file; this.url = url; this.delegate = delegate; - this.signedModelAndAttribute = signedModelAndAttribute; + this.signedValidationId = signedValidationId; } create(callback) { FileChecksum.create(this.file, ((error, checksum) => { @@ -610,7 +610,7 @@ callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.signedModelAndAttribute); + const blob = new BlobRecord(this.file, checksum, this.url, this.signedValidationId); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -639,7 +639,7 @@ constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this, this.signedModelAndAttribute); + this.directUpload = new DirectUpload(this.file, this.url, this, this.signedValidationId); this.dispatch("initialize"); } start(callback) { @@ -670,8 +670,8 @@ get url() { return this.input.getAttribute("data-direct-upload-url"); } - get signedModelAndAttribute() { - return this.input.getAttribute("data-direct-upload-signed-model-and-attribute"); + get signedValidationId() { + return this.input.getAttribute("data-direct-upload-signed-validation-id"); } dispatch(name, detail = {}) { detail.file = this.file; diff --git a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb index fce06ad06e0db..74b1fec2ea83b 100644 --- a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb +++ b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb @@ -6,7 +6,7 @@ class ActiveStorage::DirectUploadsController < ActiveStorage::BaseController def create blob = ActiveStorage::Blob.build_for_direct_upload(**blob_args) - if blob.valid_with?(params.dig(:blob, :signed_model_and_attribute)) + if blob.valid_with?(params.dig(:blob, :signed_validation_id)) blob.save! render json: direct_upload_json(blob) else diff --git a/activestorage/app/helpers/active_storage/direct_uploads_helper.rb b/activestorage/app/helpers/active_storage/direct_uploads_helper.rb deleted file mode 100644 index 1936ddb716ace..0000000000000 --- a/activestorage/app/helpers/active_storage/direct_uploads_helper.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module ActiveStorage - module DirectUploadsHelper - # Requires an instance of a Model and an attribute which allows us to know - # what validations to run. Supports persisted and non-persisted model - # instances so that direct uploads can be made for new records before - # they've been persisted. - # - # For example, regardless of whether the passed in model is persisted, - # `@user, :avatar` or `User.new, :avatar`, ActiveStorage will run the - # ActiveStorage related validations defined for the User model's avatar - # attribute. The passed in model instance comes into play when it's passed - # to any procs defined for those validations. - # - # class User < ApplicationRecord - # validates :avatar, attachment_content_type: { with: Proc.new { |user| user.persisted? ? %w[image/jpeg image/png image/gif] : %w[image/jpeg] } } - # end - # - # Returns a signed string - def rails_direct_uploads_signed_model_and_attribute(model, attribute) - raise ArgumentError, "#{model.model_name} does not have an attribute named #{attribute}" if !model.respond_to?(attribute) - - ActiveStorage.verifier.generate("#{model.to_global_id}--#{attribute}") - rescue URI::GID::MissingModelIdError - ActiveStorage.verifier.generate("#{model.model_name}--#{attribute}") - end - end -end diff --git a/activestorage/app/javascript/activestorage/blob_record.js b/activestorage/app/javascript/activestorage/blob_record.js index b2219e748d1d7..92919bbf112a5 100644 --- a/activestorage/app/javascript/activestorage/blob_record.js +++ b/activestorage/app/javascript/activestorage/blob_record.js @@ -1,7 +1,7 @@ import { getMetaValue } from "./helpers" export class BlobRecord { - constructor(file, checksum, url, signedModelAndAttribute) { + constructor(file, checksum, url, signedValidationId) { this.file = file this.attributes = { @@ -9,7 +9,7 @@ export class BlobRecord { content_type: file.type || "application/octet-stream", byte_size: file.size, checksum: checksum, - signedModelAndAttribute: signedModelAndAttribute + signedValidationId: signedValidationId } this.xhr = new XMLHttpRequest diff --git a/activestorage/app/javascript/activestorage/direct_upload.js b/activestorage/app/javascript/activestorage/direct_upload.js index 26b735ccfb2a4..e6ff09e95b2ca 100644 --- a/activestorage/app/javascript/activestorage/direct_upload.js +++ b/activestorage/app/javascript/activestorage/direct_upload.js @@ -5,12 +5,12 @@ import { BlobUpload } from "./blob_upload" let id = 0 export class DirectUpload { - constructor(file, url, delegate, signedModelAndAttribute) { + constructor(file, url, delegate, signedValidationId) { this.id = ++id this.file = file this.url = url this.delegate = delegate - this.signedModelAndAttribute = signedModelAndAttribute + this.signedValidationId = signedValidationId } create(callback) { @@ -20,7 +20,7 @@ export class DirectUpload { return } - const blob = new BlobRecord(this.file, checksum, this.url, this.signedModelAndAttribute) + const blob = new BlobRecord(this.file, checksum, this.url, this.signedValidationId) notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr) blob.create(error => { diff --git a/activestorage/app/javascript/activestorage/direct_upload_controller.js b/activestorage/app/javascript/activestorage/direct_upload_controller.js index 709ef8a3035f6..a89abc1ed210a 100644 --- a/activestorage/app/javascript/activestorage/direct_upload_controller.js +++ b/activestorage/app/javascript/activestorage/direct_upload_controller.js @@ -5,7 +5,7 @@ export class DirectUploadController { constructor(input, file) { this.input = input this.file = file - this.directUpload = new DirectUpload(this.file, this.url, this, this.signedModelAndAttribute) + this.directUpload = new DirectUpload(this.file, this.url, this, this.signedValidationId) this.dispatch("initialize") } @@ -41,8 +41,8 @@ export class DirectUploadController { return this.input.getAttribute("data-direct-upload-url") } - get signedModelAndAttribute() { - return this.input.getAttribute("data-direct-upload-signed-model-and-attribute") + get signedValidationId() { + return this.input.getAttribute("data-direct-upload-signed-validation-id") } dispatch(name, detail = {}) { diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 39317cc10132b..795bfaae6bf1d 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -379,10 +379,10 @@ def content_type=(value) # # Returns false if the model cannot be found. # Returns true if no validators are configured on the model. - def valid_with?(signed_model_and_attribute = nil) - return true if signed_model_and_attribute.nil? + def valid_with?(signed_validation_id = nil) + return true if signed_validation_id.nil? - model_gid, attribute = ActiveStorage.verifier.verified(signed_model_and_attribute).split("--") + model_gid, attribute = ActiveStorage.verifier.verified(signed_validation_id).split("--") model = GlobalID::Locator.locate(model_gid) # When model_gid is a class name pointing to an unpersisted model diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb index b540f85fbefe6..8f782b704d9e5 100644 --- a/activestorage/lib/active_storage/attached.rb +++ b/activestorage/lib/active_storage/attached.rb @@ -12,6 +12,14 @@ def initialize(name, record) @name, @record = name, record end + def to_signed_validation_id + if record.persisted? + ActiveStorage.verifier.generate("#{record.to_global_id}--#{name}") + else + ActiveStorage.verifier.generate("#{record.model_name}--#{name}") + end + end + private def change record.attachment_changes[name] diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index bb7decd45611e..94c194ff4cb1c 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -176,8 +176,6 @@ class Engine < Rails::Engine # :nodoc: initializer "action_view.configuration" do config.after_initialize do |app| ActiveSupport.on_load(:action_view) do - ActionView::Helpers.include(ActiveStorage::DirectUploadsHelper) - multiple_file_field_include_hidden = app.config.active_storage.delete(:multiple_file_field_include_hidden) unless multiple_file_field_include_hidden.nil? diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index 2651744458ffe..a0b83ad799d64 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -220,7 +220,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati checksum: checksum, content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + signed_validation_id: User.new.avatar.to_signed_validation_id, } } @@ -259,7 +259,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati checksum: checksum, content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + signed_validation_id: User.new.avatar.to_signed_validation_id, } } @@ -298,7 +298,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati checksum: checksum, content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + signed_validation_id: User.new.avatar.to_signed_validation_id, } } @@ -337,7 +337,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati checksum: checksum, content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + signed_validation_id: User.new.avatar.to_signed_validation_id, } } @@ -375,7 +375,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati checksum: checksum, content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + signed_validation_id: User.new.avatar.to_signed_validation_id, } } @@ -413,7 +413,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati checksum: checksum, content_type: "image/jpeg", metadata: metadata, - signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar), + signed_validation_id: User.new.avatar.to_signed_validation_id, } } diff --git a/activestorage/test/template/file_field_tag_test.rb b/activestorage/test/template/file_field_tag_test.rb index 979b1ca331a73..e7e786ab912b9 100644 --- a/activestorage/test/template/file_field_tag_test.rb +++ b/activestorage/test/template/file_field_tag_test.rb @@ -5,20 +5,20 @@ class ActiveStorage::FileFieldTagTest < ActionView::TestCase tests ActionView::Helpers::FormTagHelper - test "file_field_tag has access to rails_direct_uploads_signed_model_and_attribute" do + test "file_field_tag has access to rails_direct_uploads_signed_validation_id" do expected = "" assert_dom_equal expected, file_field_tag("avatar", direct_upload: true) - expected = "" - assert_dom_equal expected, file_field_tag("avatar", direct_upload: true, data: { direct_upload_signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar) }) + expected = "" + assert_dom_equal expected, file_field_tag("avatar", direct_upload: true, data: { direct_upload_signed_validation_id: User.new.avatar.to_signed_validation_id }) end - test "file_field has access to rails_direct_uploads_signed_model_and_attribute" do + test "file_field has access to rails_direct_uploads_signed_validation_id" do expected = "" assert_dom_equal expected, file_field("user", "avatar", direct_upload: true) - expected = "" - assert_dom_equal expected, file_field("user", "avatar", direct_upload: true, data: { direct_upload_signed_model_and_attribute: rails_direct_uploads_signed_model_and_attribute(User.new, :avatar) }) + expected = "" + assert_dom_equal expected, file_field("user", "avatar", direct_upload: true, data: { direct_upload_signed_validation_id: User.new.avatar.to_signed_validation_id }) end Routes = ActionDispatch::Routing::RouteSet.new @@ -27,6 +27,10 @@ class ActiveStorage::FileFieldTagTest < ActionView::TestCase end include Routes.url_helpers + def form_for(*) + @output_buffer = super + end + test "form builder includes direct upload attributes" do @user = User.create!(name: "DHH") @@ -34,8 +38,8 @@ class ActiveStorage::FileFieldTagTest < ActionView::TestCase concat f.file_field(:avatar, direct_upload: true) end - expected = "
" + expected = %Q() - assert_includes expected, output_buffer + assert_includes output_buffer, expected end end diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index a80b531af0a46..6f92e787c33fd 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -45,8 +45,6 @@ ActiveStorage.logger = ActiveSupport::Logger.new(nil) ActiveStorage.verifier = ActiveSupport::MessageVerifier.new("Testing") ActiveStorage::FixtureSet.file_fixture_path = File.expand_path("fixtures/files", __dir__) -ActionDispatch::IntegrationTest.include(ActiveStorage::DirectUploadsHelper) -ActionView::TestCase.include(ActiveStorage::DirectUploadsHelper) class ActiveSupport::TestCase self.file_fixture_path = ActiveStorage::FixtureSet.file_fixture_path diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 2cb900aea89a3..e83e535a750c1 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -987,11 +987,12 @@ directly from the client to the cloud. `FormBuilder` will ensure any defined ActiveStorage validations get run for direct uploads, however if you're not using `FormBuilder` and you want to - apply validations to direct uploads you will need to pass in the - "Model.attribute" you wish to use for validating the upload: + apply validations to direct uploads you will need to pass in the signed + validation id for the ActiveStorage enabled attribute. This ensures that + the correct validations are run for the direct upload. ```erb - + ``` Will apply any defined ActiveStorage validations on the `User` model's