diff options
59 files changed, 359 insertions, 258 deletions
diff --git a/.travis.yml b/.travis.yml index d1b384720a..46b9c45133 100644 --- a/.travis.yml +++ b/.travis.yml @@ -45,7 +45,7 @@ env: matrix: - "GEM=railties" - "GEM=ap,ac" - - "GEM=am,amo,as,av,aj" + - "GEM=am,amo,as,av,aj,ast" - "GEM=as PRESERVE_TIMEZONES=1" - "GEM=ar:mysql2" - "GEM=ar:sqlite3" @@ -1,9 +1,6 @@ source "https://rubygems.org" -git_source(:github) do |repo_name| - repo_name = "#{repo_name}/#{repo_name}" unless repo_name.include?("/") - "https://github.com/#{repo_name}.git" -end +git_source(:github) { |repo| "https://github.com/#{repo}.git" } gemspec diff --git a/actioncable/test/connection/multiple_identifiers_test.rb b/actioncable/test/connection/multiple_identifiers_test.rb index 230433a110..7f90cb3876 100644 --- a/actioncable/test/connection/multiple_identifiers_test.rb +++ b/actioncable/test/connection/multiple_identifiers_test.rb @@ -36,8 +36,4 @@ class ActionCable::Connection::MultipleIdentifiersTest < ActionCable::TestCase @connection.process @connection.send :handle_open end - - def close_connection - @connection.send :handle_close - end end diff --git a/actioncable/test/connection/string_identifier_test.rb b/actioncable/test/connection/string_identifier_test.rb index 6387fb792c..4cb58e7fd0 100644 --- a/actioncable/test/connection/string_identifier_test.rb +++ b/actioncable/test/connection/string_identifier_test.rb @@ -38,8 +38,4 @@ class ActionCable::Connection::StringIdentifierTest < ActionCable::TestCase @connection.process @connection.send :on_open end - - def close_connection - @connection.send :on_close - end end diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 1bc26cd5e1..938b24a6b9 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,11 @@ +* Deprecate `ActionDispatch::TestResponse` response aliases + + `#success?`, `#missing?` & `#error?` are not supported by the actual + `ActionDispatch::Response` object and can produce false-positives. Instead, + use the response helpers provided by `Rack::Response`. + + *Trevor Wistaff* + * Protect from forgery by default Rather than protecting from forgery in the generated `ApplicationController`, diff --git a/actionpack/lib/action_dispatch/testing/test_response.rb b/actionpack/lib/action_dispatch/testing/test_response.rb index 0b7e9ca945..b23ea7479c 100644 --- a/actionpack/lib/action_dispatch/testing/test_response.rb +++ b/actionpack/lib/action_dispatch/testing/test_response.rb @@ -20,13 +20,31 @@ module ActionDispatch end # Was the response successful? - alias_method :success?, :successful? + def success? + ActiveSupport::Deprecation.warn(<<-MSG.squish) + The success? predicate is deprecated and will be removed in Rails 6.0. + Please use successful? as provided by Rack::Response::Helpers. + MSG + successful? + end # Was the URL not found? - alias_method :missing?, :not_found? + def missing? + ActiveSupport::Deprecation.warn(<<-MSG.squish) + The missing? predicate is deprecated and will be removed in Rails 6.0. + Please use not_found? as provided by Rack::Response::Helpers. + MSG + not_found? + end # Was there a server-side error? - alias_method :error?, :server_error? + def error? + ActiveSupport::Deprecation.warn(<<-MSG.squish) + The error? predicate is deprecated and will be removed in Rails 6.0. + Please use server_error? as provided by Rack::Response::Helpers. + MSG + server_error? + end def parsed_body @parsed_body ||= @response_parser.call(body) diff --git a/actionpack/test/dispatch/response_test.rb b/actionpack/test/dispatch/response_test.rb index 67d6dea86f..a14083c6a2 100644 --- a/actionpack/test/dispatch/response_test.rb +++ b/actionpack/test/dispatch/response_test.rb @@ -296,13 +296,8 @@ class ResponseTest < ActiveSupport::TestCase end test "read content type with default charset utf-8" do - original = ActionDispatch::Response.default_charset - begin - resp = ActionDispatch::Response.new(200, "Content-Type" => "text/xml") - assert_equal("utf-8", resp.charset) - ensure - ActionDispatch::Response.default_charset = original - end + resp = ActionDispatch::Response.new(200, "Content-Type" => "text/xml") + assert_equal("utf-8", resp.charset) end test "read content type with charset utf-16" do diff --git a/actionpack/test/dispatch/test_response_test.rb b/actionpack/test/dispatch/test_response_test.rb index 2629a61057..f0b8f7785d 100644 --- a/actionpack/test/dispatch/test_response_test.rb +++ b/actionpack/test/dispatch/test_response_test.rb @@ -27,4 +27,11 @@ class TestResponseTest < ActiveSupport::TestCase response = ActionDispatch::TestResponse.create(200, { "Content-Type" => "application/json" }, '{ "foo": "fighters" }') assert_equal({ "foo" => "fighters" }, response.parsed_body) end + + test "response status aliases deprecated" do + response = ActionDispatch::TestResponse.create + assert_deprecated { response.success? } + assert_deprecated { response.missing? } + assert_deprecated { response.error? } + end end diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index 8b6322f8ad..082ea85aba 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -1569,7 +1569,7 @@ module ActionView # In the above block, a +FormBuilder+ object is yielded as the # +person_form+ variable. This allows you to generate the +text_field+ # and +check_box+ fields by specifying their eponymous methods, which - # modify the underlying template and associates the +@person+ model object + # modify the underlying template and associates the <tt>@person</tt> model object # with the form. # # The +FormBuilder+ object can be thought of as serving as a proxy for the diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index f548fc24ed..f2edcb750c 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -355,7 +355,7 @@ module ActionView # finds the options and details and extracts them. The method also contains # logic that handles the type of object passed in as the partial. # - # If +options[:partial]+ is a string, then the +@path+ instance variable is + # If +options[:partial]+ is a string, then the <tt>@path</tt> instance variable is # set to that string. Otherwise, the +options[:partial]+ object must # respond to +to_partial_path+ in order to setup the path. def setup(context, options, block) diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index 9b0b50977d..3d79e540b8 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -68,11 +68,11 @@ module ActiveRecord foreign_key = join_keys.foreign_key value = transform_value(owner[foreign_key]) - scope = scope.where(table.name => { key => value }) + scope = apply_scope(scope, table, key, value) if reflection.type polymorphic_type = transform_value(owner.class.base_class.name) - scope = scope.where(table.name => { reflection.type => polymorphic_type }) + scope = apply_scope(scope, table, reflection.type, polymorphic_type) end scope @@ -91,10 +91,10 @@ module ActiveRecord if reflection.type value = transform_value(next_reflection.klass.base_class.name) - scope = scope.where(table.name => { reflection.type => value }) + scope = apply_scope(scope, table, reflection.type, value) end - scope = scope.joins(join(foreign_table, constraint)) + scope.joins!(join(foreign_table, constraint)) end class ReflectionProxy < SimpleDelegator # :nodoc: @@ -165,6 +165,14 @@ module ActiveRecord scope end + def apply_scope(scope, table, key, value) + if scope.table == table + scope.where!(key => value) + else + scope.where!(table.name => { key => value }) + end + end + def eval_scope(reflection, table, scope, owner) reflection.build_scope(table).instance_exec(owner, &scope) end diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 0001b804a8..1a424896fe 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -59,7 +59,9 @@ module ActiveRecord r.send(reflection.association_primary_key) end.values_at(*ids).compact if records.size != ids.size - klass.all.raise_record_not_found_exception!(ids, records.size, ids.size, reflection.association_primary_key) + found_ids = records.map { |record| record.send(reflection.association_primary_key) } + not_found_ids = ids - found_ids + klass.all.raise_record_not_found_exception!(ids, records.size, ids.size, reflection.association_primary_key, not_found_ids) else replace(records) end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index c11b7b012f..0759f4d2b3 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -270,7 +270,7 @@ module ActiveRecord # Connections must be leased while holding the main pool mutex. This is # an internal subclass that also +.leases+ returned connections while # still in queue's critical section (queue synchronizes with the same - # +@lock+ as the main pool) so that a returned connection is already + # <tt>@lock</tt> as the main pool) so that a returned connection is already # leased and there is no need to re-enter synchronized block. class ConnectionLeasingQueue < Queue # :nodoc: include BiasableQueue @@ -326,8 +326,6 @@ module ActiveRecord @spec = spec @checkout_timeout = (spec.config[:checkout_timeout] && spec.config[:checkout_timeout].to_f) || 5 - @reaper = Reaper.new(self, (spec.config[:reaping_frequency] && spec.config[:reaping_frequency].to_f)) - @reaper.run # default max pool size to 5 @size = (spec.config[:pool] && spec.config[:pool].to_i) || 5 @@ -340,7 +338,7 @@ module ActiveRecord # then that +thread+ does indeed own that +conn+. However, an absence of a such # mapping does not mean that the +thread+ doesn't own the said connection. In # that case +conn.owner+ attr should be consulted. - # Access and modification of +@thread_cached_conns+ does not require + # Access and modification of <tt>@thread_cached_conns</tt> does not require # synchronization. @thread_cached_conns = Concurrent::Map.new(initial_capacity: @size) @@ -357,6 +355,9 @@ module ActiveRecord @available = ConnectionLeasingQueue.new self @lock_thread = false + + @reaper = Reaper.new(self, spec.config[:reaping_frequency] && spec.config[:reaping_frequency].to_f) + @reaper.run end def lock_thread=(lock_thread) @@ -736,10 +737,10 @@ module ActiveRecord # Implementation detail: the connection returned by +acquire_connection+ # will already be "+connection.lease+ -ed" to the current thread. def acquire_connection(checkout_timeout) - # NOTE: we rely on +@available.poll+ and +try_to_checkout_new_connection+ to + # NOTE: we rely on <tt>@available.poll</tt> and +try_to_checkout_new_connection+ to # +conn.lease+ the returned connection (and to do this in a +synchronized+ # section). This is not the cleanest implementation, as ideally we would - # <tt>synchronize { conn.lease }</tt> in this method, but by leaving it to +@available.poll+ + # <tt>synchronize { conn.lease }</tt> in this method, but by leaving it to <tt>@available.poll</tt> # and +try_to_checkout_new_connection+ we can piggyback on +synchronize+ sections # of the said methods and avoid an additional +synchronize+ overhead. if conn = @available.poll || try_to_checkout_new_connection @@ -763,7 +764,7 @@ module ActiveRecord end end - # If the pool is not at a +@size+ limit, establish new connection. Connecting + # If the pool is not at a <tt>@size</tt> limit, establish new connection. Connecting # to the DB is done outside main synchronized section. #-- # Implementation constraint: a newly established connection returned by this diff --git a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb index ecf5201d12..41d93c4322 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "concurrent/map" + module ActiveRecord module ConnectionAdapters # :nodoc: module QueryCache diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 626c50470e..5da9573052 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -330,7 +330,7 @@ module ActiveRecord # of results obtained should be provided in the +result_size+ argument and # the expected number of results should be provided in the +expected_size+ # argument. - def raise_record_not_found_exception!(ids = nil, result_size = nil, expected_size = nil, key = primary_key) # :nodoc: + def raise_record_not_found_exception!(ids = nil, result_size = nil, expected_size = nil, key = primary_key, not_found_ids = nil) # :nodoc: conditions = arel.where_sql(@klass) conditions = " [#{conditions}]" if conditions name = @klass.name @@ -344,8 +344,8 @@ module ActiveRecord raise RecordNotFound.new(error, name, key, ids) else error = "Couldn't find all #{name.pluralize} with '#{key}': ".dup - error << "(#{ids.join(", ")})#{conditions} (found #{result_size} results, but was looking for #{expected_size})" - + error << "(#{ids.join(", ")})#{conditions} (found #{result_size} results, but was looking for #{expected_size})." + error << " Couldn't find #{name.pluralize(not_found_ids.size)} with #{key.to_s.pluralize(not_found_ids.size)} #{not_found_ids.join(', ')}." if not_found_ids raise RecordNotFound.new(error, name, primary_key, ids) end end diff --git a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb index a2e74efc2b..5681ccdd23 100644 --- a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb +++ b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb @@ -22,7 +22,7 @@ module ActiveRecord configuration.merge("encoding" => encoding) establish_connection configuration rescue ActiveRecord::StatementInvalid => error - if /database .* already exists/.match?(error.message) + if error.cause.is_a?(PG::DuplicateDatabase) raise DatabaseAlreadyExists else raise @@ -136,7 +136,7 @@ module ActiveRecord ensure tempfile.close end - FileUtils.mv(tempfile.path, filename) + FileUtils.cp(tempfile.path, filename) end end end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index f91f0cdf12..b2f5e39e09 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -472,7 +472,7 @@ module ActiveRecord # if it's associated with a transaction, then the state of the Active Record # object will be updated to reflect the current state of the transaction. # - # The +@transaction_state+ variable stores the states of the associated + # The <tt>@transaction_state</tt> variable stores the states of the associated # transaction. This relies on the fact that a transaction can only be in # one rollback or commit (otherwise a list of states would be required). # Each Active Record object inside of a transaction carries that transaction's diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index c5e35a146b..4c2723addc 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -891,7 +891,8 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase company = companies(:rails_core) ids = [Developer.first.id, -9999] e = assert_raises(ActiveRecord::RecordNotFound) { company.developer_ids = ids } - assert_match(/Couldn't find all Developers with 'id'/, e.message) + msg = "Couldn't find all Developers with 'id': (1, -9999) (found 1 results, but was looking for 2). Couldn't find Developer with id -9999." + assert_equal(msg, e.message) end def test_collection_singular_ids_setter_raises_exception_when_invalid_ids_set_with_changed_primary_key @@ -905,7 +906,8 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase author = authors(:david) ids = [categories(:general).name, "Unknown"] e = assert_raises(ActiveRecord::RecordNotFound) { author.essay_category_ids = ids } - assert_equal "Couldn't find all Categories with 'name': (General, Unknown) (found 1 results, but was looking for 2)", e.message + msg = "Couldn't find all Categories with 'name': (General, Unknown) (found 1 results, but was looking for 2). Couldn't find Category with name Unknown." + assert_equal msg, e.message end def test_build_a_model_from_hm_through_association_with_where_clause diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index 2f68bc5141..9d3d5353ff 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -404,7 +404,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase end def test_has_many_through_polymorphic_has_one - assert_equal Tagging.find(1, 2).sort_by(&:id), authors(:david).taggings_2 + assert_equal Tagging.find(1, 2).sort_by(&:id), authors(:david).taggings_2.sort_by(&:id) end def test_has_many_through_polymorphic_has_many diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb index fb3d691d51..a602f83d8c 100644 --- a/activerecord/test/cases/dirty_test.rb +++ b/activerecord/test/cases/dirty_test.rb @@ -466,11 +466,10 @@ class DirtyTest < ActiveRecord::TestCase def test_save_should_not_save_serialized_attribute_with_partial_writes_if_not_present with_partial_writes(Topic) do - Topic.create!(author_name: "Bill", content: { a: "a" }) - topic = Topic.select("id, author_name").first + topic = Topic.create!(author_name: "Bill", content: { a: "a" }) + topic = Topic.select("id, author_name").find(topic.id) topic.update_columns author_name: "John" - topic = Topic.first - assert_not_nil topic.content + assert_not_nil topic.reload.content end end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index a0d87dfb90..55496147c1 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -1153,7 +1153,7 @@ class FinderTest < ActiveRecord::TestCase e = assert_raises(ActiveRecord::RecordNotFound) do model.find "Hello", "World!" end - assert_equal "Couldn't find all MercedesCars with 'name': (Hello, World!) (found 0 results, but was looking for 2)", e.message + assert_equal "Couldn't find all MercedesCars with 'name': (Hello, World!) (found 0 results, but was looking for 2).", e.message end end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 05420a4240..47749c07d2 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -192,6 +192,7 @@ ActiveRecord::Schema.define do t.string :resource_type t.integer :developer_id t.datetime :deleted_at + t.integer :comments end create_table :companies, force: true do |t| diff --git a/activestorage/README.md b/activestorage/README.md index 957adc05c3..d4ffe0c484 100644 --- a/activestorage/README.md +++ b/activestorage/README.md @@ -83,14 +83,6 @@ Variation of image attachment: <%= image_tag user.avatar.variant(resize: "100x100") %> ``` -## Installation - -1. Run `rails activestorage:install` to create needed directories, migrations, and configuration. -2. Optional: Add `gem "aws-sdk", "~> 2"` to your Gemfile if you want to use AWS S3. -3. Optional: Add `gem "google-cloud-storage", "~> 1.3"` to your Gemfile if you want to use Google Cloud Storage. -4. Optional: Add `gem "azure-storage"` to your Gemfile if you want to use Microsoft Azure. -5. Optional: Add `gem "mini_magick"` to your Gemfile if you want to use variants. - ## Direct uploads Active Storage, with its included JavaScript library, supports uploading directly from the client to the cloud. diff --git a/activestorage/app/controllers/active_storage/blobs_controller.rb b/activestorage/app/controllers/active_storage/blobs_controller.rb index 05af29f8b2..cff88bd488 100644 --- a/activestorage/app/controllers/active_storage/blobs_controller.rb +++ b/activestorage/app/controllers/active_storage/blobs_controller.rb @@ -5,6 +5,7 @@ class ActiveStorage::BlobsController < ActionController::Base def show if blob = find_signed_blob + expires_in 5.minutes # service_url defaults to 5 minutes redirect_to blob.service_url(disposition: disposition_param) else head :not_found diff --git a/activestorage/app/controllers/active_storage/variants_controller.rb b/activestorage/app/controllers/active_storage/variants_controller.rb index 994c57aafd..b72b0ff7f5 100644 --- a/activestorage/app/controllers/active_storage/variants_controller.rb +++ b/activestorage/app/controllers/active_storage/variants_controller.rb @@ -5,6 +5,7 @@ class ActiveStorage::VariantsController < ActionController::Base def show if blob = find_signed_blob + expires_in 5.minutes # service_url defaults to 5 minutes redirect_to ActiveStorage::Variant.new(blob, decoded_variation).processed.service_url(disposition: disposition_param) else head :not_found diff --git a/activestorage/app/jobs/active_storage/purge_job.rb b/activestorage/app/jobs/active_storage/purge_job.rb index 815f908e6c..b504ee0df4 100644 --- a/activestorage/app/jobs/active_storage/purge_job.rb +++ b/activestorage/app/jobs/active_storage/purge_job.rb @@ -1,4 +1,4 @@ -# Provides delayed purging of attachments or blobs using their `#purge_later` method. +# Provides delayed purging of attachments or blobs using their +#purge_later+ method. class ActiveStorage::PurgeJob < ActiveJob::Base # FIXME: Limit this to a custom ActiveStorage error retry_on StandardError diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index e94fc69bba..07b5733ff8 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -21,7 +21,7 @@ class ActiveStorage::Attachment < ActiveRecord::Base # Purging an attachment means purging the blob, which means talking to the service, which means # talking over the internet. Whenever you're doing that, it's a good idea to put that work in a job, - # so it doesn't hold up other operations. That's what #purge_later provides. + # so it doesn't hold up other operations. That's what +#purge_later+ provides. def purge_later ActiveStorage::PurgeJob.perform_later(self) end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index c72073f9f6..113a7f774d 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -29,7 +29,7 @@ class ActiveStorage::Blob < ActiveRecord::Base find ActiveStorage.verifier.verify(id, purpose: :blob_id) end - # Returns a new, unsaved blob instance after the `io` has been uploaded to the service. + # Returns a new, unsaved blob instance after the +io+ has been uploaded to the service. def build_after_upload(io:, filename:, content_type: nil, metadata: nil) new.tap do |blob| blob.filename = filename @@ -40,8 +40,8 @@ class ActiveStorage::Blob < ActiveRecord::Base end end - # Returns a saved blob instance after the `io` has been uploaded to the service. Note, the blob is first built, - # then the `io` is uploaded, then the blob is saved. This is doing to avoid opening a transaction and talking to + # Returns a saved blob instance after the +io+ has been uploaded to the service. Note, the blob is first built, + # then the +io+ is uploaded, then the blob is saved. This is doing to avoid opening a transaction and talking to # the service during that (which is a bad idea and leads to deadlocks). def create_after_upload!(io:, filename:, content_type: nil, metadata: nil) build_after_upload(io: io, filename: filename, content_type: content_type, metadata: metadata).tap(&:save!) @@ -72,7 +72,7 @@ class ActiveStorage::Blob < ActiveRecord::Base self[:key] ||= self.class.generate_unique_secure_token end - # Returns a `ActiveStorage::Filename` instance of the filename that can be queried for basename, extension, and + # Returns a ActiveStorage::Filename instance of the filename that can be queried for basename, extension, and # a sanitized version of the filename that's safe to use in URLs. def filename ActiveStorage::Filename.new(self[:filename]) @@ -98,7 +98,7 @@ class ActiveStorage::Blob < ActiveRecord::Base content_type.start_with?("text") end - # Returns a `ActiveStorage::Variant` instance with the set of `transformations` passed in. This is only relevant + # Returns a ActiveStorage::Variant instance with the set of +transformations+ passed in. This is only relevant # for image files, and it allows any image to be transformed for size, colors, and the like. Example: # # avatar.variant(resize: "100x100").processed.service_url @@ -111,7 +111,7 @@ class ActiveStorage::Blob < ActiveRecord::Base # # <%= image_tag url_for(Current.user.avatar.variant(resize: "100x100")) %> # - # This will create a URL for that specific blob with that specific variant, which the `ActiveStorage::VariantsController` + # This will create a URL for that specific blob with that specific variant, which the ActiveStorage::VariantsController # can then produce on-demand. def variant(transformations) ActiveStorage::Variant.new(self, ActiveStorage::Variation.new(transformations)) @@ -119,9 +119,9 @@ class ActiveStorage::Blob < ActiveRecord::Base # Returns the URL of the blob on the service. This URL is intended to be short-lived for security and not used directly - # with users. Instead, the `service_url` should only be exposed as a redirect from a stable, possibly authenticated URL. - # Hiding the `service_url` behind a redirect also gives you the power to change services without updating all URLs. And - # it allows permanent URLs that redirect to the `service_url` to be cached in the view. + # with users. Instead, the +service_url+ should only be exposed as a redirect from a stable, possibly authenticated URL. + # Hiding the +service_url+ behind a redirect also gives you the power to change services without updating all URLs. And + # it allows permanent URLs that redirect to the +service_url+ to be cached in the view. def service_url(expires_in: 5.minutes, disposition: :inline) service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type end @@ -132,21 +132,21 @@ class ActiveStorage::Blob < ActiveRecord::Base service.url_for_direct_upload key, expires_in: expires_in, content_type: content_type, content_length: byte_size, checksum: checksum end - # Returns a Hash of headers for `service_url_for_direct_upload` requests. + # Returns a Hash of headers for +service_url_for_direct_upload+ requests. def service_headers_for_direct_upload service.headers_for_direct_upload key, filename: filename, content_type: content_type, content_length: byte_size, checksum: checksum end - # Uploads the `io` to the service on the `key` for this blob. Blobs are intended to be immutable, so you shouldn't be + # Uploads the +io+ to the service on the +key+ for this blob. Blobs are intended to be immutable, so you shouldn't be # using this method after a file has already been uploaded to fit with a blob. If you want to create a derivative blob, # you should instead simply create a new blob based on the old one. # # Prior to uploading, we compute the checksum, which is sent to the service for transit integrity validation. If the - # checksum does not match what the service receives, an exception will be raised. We also measure the size of the `io` - # and store that in `byte_size` on the blob record. + # checksum does not match what the service receives, an exception will be raised. We also measure the size of the +io+ + # and store that in +byte_size+ on the blob record. # - # Normally, you do not have to call this method directly at all. Use the factory class methods of `build_after_upload` - # and `create_after_upload!`. + # Normally, you do not have to call this method directly at all. Use the factory class methods of +build_after_upload+ + # and +create_after_upload!+. def upload(io) self.checksum = compute_checksum_in_chunks(io) self.byte_size = io.size @@ -162,7 +162,7 @@ class ActiveStorage::Blob < ActiveRecord::Base # Deletes the file on the service that's associated with this blob. This should only be done if the blob is going to be - # deleted as well or you will essentially have a dead reference. It's recommended to use the `#purge` and `#purge_later` + # deleted as well or you will essentially have a dead reference. It's recommended to use the +#purge+ and +#purge_later+ # methods in most circumstances. def delete service.delete key @@ -170,13 +170,13 @@ class ActiveStorage::Blob < ActiveRecord::Base # Deletes the file on the service and then destroys the blob record. This is the recommended way to dispose of unwanted # blobs. Note, though, that deleting the file off the service will initiate a HTTP connection to the service, which may - # be slow or prevented, so you should not use this method inside a transaction or in callbacks. Use `#purge_later` instead. + # be slow or prevented, so you should not use this method inside a transaction or in callbacks. Use +#purge_later+ instead. def purge delete destroy end - # Enqueues a `ActiveStorage::PurgeJob` job that'll call `#purge`. This is the recommended way to purge blobs when the call + # Enqueues a ActiveStorage::PurgeJob job that'll call +#purge+. This is the recommended way to purge blobs when the call # needs to be made from a transaction, a callback, or any other real-time scenario. def purge_later ActiveStorage::PurgeJob.perform_later(self) diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb index e3f22bcb25..b9b93b4c1b 100644 --- a/activestorage/app/models/active_storage/variant.rb +++ b/activestorage/app/models/active_storage/variant.rb @@ -9,17 +9,17 @@ # into memory. The larger the image, the more memory is used. Because of this process, you also want to be # considerate about when the variant is actually processed. You shouldn't be processing variants inline in a # template, for example. Delay the processing to an on-demand controller, like the one provided in -# `ActiveStorage::VariantsController`. +# ActiveStorage::VariantsController. # # To refer to such a delayed on-demand variant, simply link to the variant through the resolved route provided # by Active Storage like so: # # <%= image_tag url_for(Current.user.avatar.variant(resize: "100x100")) %> # -# This will create a URL for that specific blob with that specific variant, which the `ActiveStorage::VariantsController` +# This will create a URL for that specific blob with that specific variant, which the ActiveStorage::VariantsController # can then produce on-demand. # -# When you do want to actually produce the variant needed, call `#processed`. This will check that the variant +# When you do want to actually produce the variant needed, call +#processed+. This will check that the variant # has already been processed and uploaded to the service, and, if so, just return that. Otherwise it will perform # the transformations, upload the variant to the service, and return itself again. Example: # @@ -52,12 +52,12 @@ class ActiveStorage::Variant end # Returns the URL of the variant on the service. This URL is intended to be short-lived for security and not used directly - # with users. Instead, the `service_url` should only be exposed as a redirect from a stable, possibly authenticated URL. - # Hiding the `service_url` behind a redirect also gives you the power to change services without updating all URLs. And - # it allows permanent URLs that redirec to the `service_url` to be cached in the view. + # with users. Instead, the +service_url+ should only be exposed as a redirect from a stable, possibly authenticated URL. + # Hiding the +service_url+ behind a redirect also gives you the power to change services without updating all URLs. And + # it allows permanent URLs that redirect to the +service_url+ to be cached in the view. # # Use `url_for(variant)` (or the implied form, like `link_to variant` or `redirect_to variant`) to get the stable URL - # for a variant that points to the `ActiveStorage::VariantsController`, which in turn will use this `#service_call` method + # for a variant that points to the ActiveStorage::VariantsController, which in turn will use this +#service_call+ method # for its redirection. def service_url(expires_in: 5.minutes, disposition: :inline) service.url key, expires_in: expires_in, disposition: disposition, filename: blob.filename, content_type: blob.content_type diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb index e784506b4c..24168c064c 100644 --- a/activestorage/app/models/active_storage/variation.rb +++ b/activestorage/app/models/active_storage/variation.rb @@ -13,12 +13,12 @@ class ActiveStorage::Variation attr_reader :transformations class << self - # Returns a variation instance with the transformations that were encoded by `#encode`. + # Returns a variation instance with the transformations that were encoded by +#encode+. def decode(key) new ActiveStorage.verifier.verify(key, purpose: :variation) end - # Returns a signed key for the `transformations`, which can be used to refer to a specific + # Returns a signed key for the +transformations+, which can be used to refer to a specific # variation in a URL or combined key (like `ActiveStorage::Variant#key`). def encode(transformations) ActiveStorage.verifier.generate(transformations, purpose: :variation) @@ -30,7 +30,7 @@ class ActiveStorage::Variation end # Accepts an open MiniMagick image instance, like what's return by `MiniMagick::Image.read(io)`, - # and performs the `transformations` against it. The transformed image instance is then returned. + # and performs the +transformations+ against it. The transformed image instance is then returned. def transform(image) transformations.each do |(method, argument)| if eligible_argument?(argument) @@ -41,7 +41,7 @@ class ActiveStorage::Variation end end - # Returns a signed key for all the `transformations` that this variation was instantiated with. + # Returns a signed key for all the +transformations+ that this variation was instantiated with. def key self.class.encode(transformations) end diff --git a/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb b/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb index 6eab7e0fa0..2c7e3c5bc6 100644 --- a/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb +++ b/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb @@ -1,24 +1,24 @@ class CreateActiveStorageTables < ActiveRecord::Migration[5.1] def change create_table :active_storage_blobs do |t| - t.string :key - t.string :filename + t.string :key, null: false + t.string :filename, null: false t.string :content_type t.text :metadata - t.integer :byte_size - t.string :checksum - t.datetime :created_at + t.integer :byte_size, null: false + t.string :checksum, null: false + t.datetime :created_at, null: false t.index [ :key ], unique: true end create_table :active_storage_attachments do |t| - t.string :name - t.string :record_type - t.integer :record_id - t.integer :blob_id + t.string :name, null: false + t.string :record_type, null: false + t.integer :record_id, null: false + t.integer :blob_id, null: false - t.datetime :created_at + t.datetime :created_at, null: false t.index :blob_id t.index [ :record_type, :record_id, :name, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb index 5ac8ba5377..07e0d5c3ea 100644 --- a/activestorage/lib/active_storage/attached.rb +++ b/activestorage/lib/active_storage/attached.rb @@ -3,7 +3,7 @@ require "action_dispatch/http/upload" require "active_support/core_ext/module/delegation" module ActiveStorage -# Abstract baseclass for the concrete `ActiveStorage::Attached::One` and `ActiveStorage::Attached::Many` +# Abstract baseclass for the concrete ActiveStorage::Attached::One and ActiveStorage::Attached::Many # classes that both provide proxy access to the blob association for a record. class Attached attr_reader :name, :record diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb index 5779348148..eb877f10c0 100644 --- a/activestorage/lib/active_storage/attached/macros.rb +++ b/activestorage/lib/active_storage/attached/macros.rb @@ -10,25 +10,23 @@ module ActiveStorage # There is no column defined on the model side, Active Storage takes # care of the mapping between your records and the attachment. # - # Under the covers, this relationship is implemented as a `has_one` association to a - # `ActiveStorage::Attachment` record and a `has_one-through` association to a - # `ActiveStorage::Blob` record. These associations are available as `avatar_attachment` - # and `avatar_blob`. But you shouldn't need to work with these associations directly in + # Under the covers, this relationship is implemented as a +has_one+ association to a + # ActiveStorage::Attachment record and a +has_one-through+ association to a + # ActiveStorage::Blob record. These associations are available as +avatar_attachment+ + # and +avatar_blob+. But you shouldn't need to work with these associations directly in # most circumstances. # - # The system has been designed to having you go through the `ActiveStorage::Attached::One` - # proxy that provides the dynamic proxy to the associations and factory methods, like `#attach`. + # The system has been designed to having you go through the ActiveStorage::Attached::One + # proxy that provides the dynamic proxy to the associations and factory methods, like +#attach+. # # If the +:dependent+ option isn't set, the attachment will be purged # (i.e. destroyed) whenever the record is destroyed. def has_one_attached(name, dependent: :purge_later) - define_method(name) do - if instance_variable_defined?("@active_storage_attached_#{name}") - instance_variable_get("@active_storage_attached_#{name}") - else - instance_variable_set("@active_storage_attached_#{name}", ActiveStorage::Attached::One.new(name, self)) + class_eval <<-CODE, __FILE__, __LINE__ + 1 + def #{name} + @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self) end - end + CODE has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record has_one :"#{name}_blob", through: :"#{name}_attachment", class_name: "ActiveStorage::Blob", source: :blob @@ -51,25 +49,23 @@ module ActiveStorage # # Gallery.where(user: Current.user).with_attached_photos # - # Under the covers, this relationship is implemented as a `has_many` association to a - # `ActiveStorage::Attachment` record and a `has_many-through` association to a - # `ActiveStorage::Blob` record. These associations are available as `photos_attachments` - # and `photos_blobs`. But you shouldn't need to work with these associations directly in + # Under the covers, this relationship is implemented as a +has_many+ association to a + # ActiveStorage::Attachment record and a +has_many-through+ association to a + # ActiveStorage::Blob record. These associations are available as +photos_attachments+ + # and +photos_blobs+. But you shouldn't need to work with these associations directly in # most circumstances. # - # The system has been designed to having you go through the `ActiveStorage::Attached::Many` - # proxy that provides the dynamic proxy to the associations and factory methods, like `#attach`. + # The system has been designed to having you go through the ActiveStorage::Attached::Many + # proxy that provides the dynamic proxy to the associations and factory methods, like +#attach+. # # If the +:dependent+ option isn't set, all the attachments will be purged # (i.e. destroyed) whenever the record is destroyed. def has_many_attached(name, dependent: :purge_later) - define_method(name) do - if instance_variable_defined?("@active_storage_attached_#{name}") - instance_variable_get("@active_storage_attached_#{name}") - else - instance_variable_set("@active_storage_attached_#{name}", ActiveStorage::Attached::Many.new(name, self)) + class_eval <<-CODE, __FILE__, __LINE__ + 1 + def #{name} + @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self) end - end + CODE has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment" has_many :"#{name}_blobs", through: :"#{name}_attachments", class_name: "ActiveStorage::Blob", source: :blob @@ -81,4 +77,4 @@ module ActiveStorage end end end -end
\ No newline at end of file +end diff --git a/activestorage/lib/active_storage/attached/many.rb b/activestorage/lib/active_storage/attached/many.rb index 82989e4605..cc3e70ffe2 100644 --- a/activestorage/lib/active_storage/attached/many.rb +++ b/activestorage/lib/active_storage/attached/many.rb @@ -5,7 +5,7 @@ module ActiveStorage # Returns all the associated attachment records. # - # All methods called on this proxy object that aren't listed here will automatically be delegated to `attachments`. + # All methods called on this proxy object that aren't listed here will automatically be delegated to +attachments+. def attachments record.public_send("#{name}_attachments") end @@ -51,4 +51,3 @@ module ActiveStorage end end end - diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index d5bc70fc0c..a5562b32d3 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -5,6 +5,8 @@ require "active_storage" module ActiveStorage class Engine < Rails::Engine # :nodoc: + isolate_namespace ActiveStorage + config.active_storage = ActiveSupport::OrderedOptions.new config.eager_load_namespaces << ActiveStorage diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb index eb25e9f001..4dd6eb6045 100644 --- a/activestorage/lib/active_storage/service.rb +++ b/activestorage/lib/active_storage/service.rb @@ -59,43 +59,43 @@ module ActiveStorage end end - # Upload the `io` to the `key` specified. If a `checksum` is provided, the service will - # ensure a match when the upload has completed or raise an `ActiveStorage::IntegrityError`. + # Upload the +io+ to the +key+ specified. If a +checksum+ is provided, the service will + # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. def upload(key, io, checksum: nil) raise NotImplementedError end - # Return the content of the file at the `key`. + # Return the content of the file at the +key+. def download(key) raise NotImplementedError end - # Delete the file at the `key`. + # Delete the file at the +key+. def delete(key) raise NotImplementedError end - # Return true if a file exists at the `key`. + # Return true if a file exists at the +key+. def exist?(key) raise NotImplementedError end - # Returns a signed, temporary URL for the file at the `key`. The URL will be valid for the amount - # of seconds specified in `expires_in`. You most also provide the `disposition` (`:inline` or `:attachment`), - # `filename`, and `content_type` that you wish the file to be served with on request. + # Returns a signed, temporary URL for the file at the +key+. The URL will be valid for the amount + # of seconds specified in +expires_in+. You most also provide the +disposition+ (+:inline+ or +:attachment+), + # +filename+, and +content_type+ that you wish the file to be served with on request. def url(key, expires_in:, disposition:, filename:, content_type:) raise NotImplementedError end - # Returns a signed, temporary URL that a direct upload file can be PUT to on the `key`. - # The URL will be valid for the amount of seconds specified in `expires_in`. - # You most also provide the `content_type`, `content_length`, and `checksum` of the file + # Returns a signed, temporary URL that a direct upload file can be PUT to on the +key+. + # The URL will be valid for the amount of seconds specified in +expires_in+. + # You most also provide the +content_type+, +content_length+, and +checksum+ of the file # that will be uploaded. All these attributes will be validated by the service upon upload. def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:) raise NotImplementedError end - # Returns a Hash of headers for `url_for_direct_upload` requests. + # Returns a Hash of headers for +url_for_direct_upload+ requests. def headers_for_direct_upload(key, filename:, content_type:, content_length:, checksum:) {} end diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb index 2e0b20cce3..d1c62f7db1 100644 --- a/activestorage/lib/active_storage/service/azure_storage_service.rb +++ b/activestorage/lib/active_storage/service/azure_storage_service.rb @@ -4,7 +4,7 @@ require "azure/storage/core/auth/shared_access_signature" module ActiveStorage # Wraps the Microsoft Azure Storage Blob Service as a Active Storage service. - # See `ActiveStorage::Service` for the generic API documentation that applies to all services. + # See ActiveStorage::Service for the generic API documentation that applies to all services. class Service::AzureStorageService < Service attr_reader :client, :path, :blobs, :container, :signer diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb index 3d92102cf0..9498761cc5 100644 --- a/activestorage/lib/active_storage/service/disk_service.rb +++ b/activestorage/lib/active_storage/service/disk_service.rb @@ -4,7 +4,7 @@ require "digest/md5" require "active_support/core_ext/numeric/bytes" module ActiveStorage - # Wraps a local disk path as a Active Storage service. See `ActiveStorage::Service` for the generic API + # Wraps a local disk path as a Active Storage service. See ActiveStorage::Service for the generic API # documentation that applies to all services. class Service::DiskService < Service attr_reader :root @@ -124,4 +124,3 @@ module ActiveStorage end end end - diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb index ea4ec5a790..0707f26a74 100644 --- a/activestorage/lib/active_storage/service/gcs_service.rb +++ b/activestorage/lib/active_storage/service/gcs_service.rb @@ -2,7 +2,7 @@ require "google/cloud/storage" require "active_support/core_ext/object/to_query" module ActiveStorage - # Wraps the Google Cloud Storage as a Active Storage service. See `ActiveStorage::Service` for the generic API + # Wraps the Google Cloud Storage as a Active Storage service. See ActiveStorage::Service for the generic API # documentation that applies to all services. class Service::GCSService < Service attr_reader :client, :bucket diff --git a/activestorage/lib/active_storage/service/mirror_service.rb b/activestorage/lib/active_storage/service/mirror_service.rb index 2403eeb1e9..8491df4911 100644 --- a/activestorage/lib/active_storage/service/mirror_service.rb +++ b/activestorage/lib/active_storage/service/mirror_service.rb @@ -1,9 +1,9 @@ require "active_support/core_ext/module/delegation" module ActiveStorage - # Wraps a set of mirror services and provides a single `ActiveStorage::Service` object that will all - # have the files uploaded to them. A `primary` service is designated to answer calls to `download`, `exists?`, - # and `url`. + # Wraps a set of mirror services and provides a single ActiveStorage::Service object that will all + # have the files uploaded to them. A +primary+ service is designated to answer calls to +download+, +exists?+, + # and +url+. class Service::MirrorService < Service attr_reader :primary, :mirrors @@ -20,15 +20,15 @@ module ActiveStorage @primary, @mirrors = primary, mirrors end - # Upload the `io` to the `key` specified to all services. If a `checksum` is provided, all services will - # ensure a match when the upload has completed or raise an `ActiveStorage::IntegrityError`. + # Upload the +io+ to the +key+ specified to all services. If a +checksum+ is provided, all services will + # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. def upload(key, io, checksum: nil) each_service.collect do |service| service.upload key, io.tap(&:rewind), checksum: checksum end end - # Delete the file at the `key` on all services. + # Delete the file at the +key+ on all services. def delete(key) perform_across_services :delete, key end diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb index 5153f5db0d..b25eb409ef 100644 --- a/activestorage/lib/active_storage/service/s3_service.rb +++ b/activestorage/lib/active_storage/service/s3_service.rb @@ -3,7 +3,7 @@ require "active_support/core_ext/numeric/bytes" module ActiveStorage # Wraps the Amazon Simple Storage Service (S3) as a Active Storage service. - # See `ActiveStorage::Service` for the generic API documentation that applies to all services. + # See ActiveStorage::Service for the generic API documentation that applies to all services. class Service::S3Service < Service attr_reader :client, :bucket, :upload_options diff --git a/activestorage/lib/tasks/activestorage.rake b/activestorage/lib/tasks/activestorage.rake index d9e240b141..7a573be596 100644 --- a/activestorage/lib/tasks/activestorage.rake +++ b/activestorage/lib/tasks/activestorage.rake @@ -1,6 +1,6 @@ namespace :activestorage do desc "Copy over the migration needed to the application" task install: :environment do - Rake::Task["active_storage_engine:install:migrations"].invoke + Rake::Task["active_storage:install:migrations"].invoke end end diff --git a/activestorage/test/controllers/blobs_controller_test.rb b/activestorage/test/controllers/blobs_controller_test.rb new file mode 100644 index 0000000000..5ec353889c --- /dev/null +++ b/activestorage/test/controllers/blobs_controller_test.rb @@ -0,0 +1,15 @@ +require "test_helper" +require "database/setup" + +class ActiveStorage::BlobsControllerTest < ActionDispatch::IntegrationTest + setup do + @blob = create_image_blob filename: "racecar.jpg" + end + + test "showing blob utilizes browser caching" do + get rails_blob_url(@blob) + + assert_redirected_to(/racecar.jpg/) + assert_equal "max-age=300, private", @response.headers["Cache-Control"] + end +end diff --git a/activestorage/test/models/attachments_test.rb b/activestorage/test/models/attachments_test.rb index 82256e1f44..58af5dce6e 100644 --- a/activestorage/test/models/attachments_test.rb +++ b/activestorage/test/models/attachments_test.rb @@ -1,13 +1,6 @@ require "test_helper" require "database/setup" -# ActiveRecord::Base.logger = Logger.new(STDOUT) - -class User < ActiveRecord::Base - has_one_attached :avatar - has_many_attached :highlights -end - class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase include ActiveJob::TestHelper diff --git a/activestorage/test/service/configurations.yml b/activestorage/test/service/configurations.yml index d7aa672573..56ed37be5d 100644 --- a/activestorage/test/service/configurations.yml +++ b/activestorage/test/service/configurations.yml @@ -1,10 +1,10 @@ -s3: - service: S3 - access_key_id: <%= ENV["AWS_ACCESS_KEY_ID"] %> - secret_access_key: <%= ENV["AWS_SECRET_KEY"] %> - region: us-east-2 - bucket: rails-ci-activestorage - +# s3: +# service: S3 +# access_key_id: "" +# secret_access_key: "" +# region: "" +# bucket: "" +# # gcs: # service: GCS # keyfile: { diff --git a/activestorage/test/template/image_tag_test.rb b/activestorage/test/template/image_tag_test.rb index 83c95c01a1..b285cd2e14 100644 --- a/activestorage/test/template/image_tag_test.rb +++ b/activestorage/test/template/image_tag_test.rb @@ -1,10 +1,6 @@ require "test_helper" require "database/setup" -class User < ActiveRecord::Base - has_one_attached :avatar -end - class ActiveStorage::ImageTagTest < ActionView::TestCase tests ActionView::Helpers::AssetTagHelper diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 5b6d3b64c2..1190daa7b0 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -4,13 +4,19 @@ require "bundler/setup" require "active_support" require "active_support/test_case" require "active_support/testing/autorun" -require "byebug" + +begin + require "byebug" +rescue LoadError +end require "active_job" ActiveJob::Base.queue_adapter = :test -ActiveJob::Base.logger = nil +ActiveJob::Base.logger = ActiveSupport::Logger.new(nil) -require "active_storage" +# Filter out Minitest backtrace while allowing backtrace from other libraries +# to be shown. +Minitest.backtrace_filter = Minitest::BacktraceFilter.new require "yaml" SERVICE_CONFIGURATIONS = begin @@ -54,3 +60,8 @@ end require "global_id" GlobalID.app = "ActiveStorageExampleApp" ActiveRecord::Base.send :include, GlobalID::Identification + +class User < ActiveRecord::Base + has_one_attached :avatar + has_many_attached :highlights +end diff --git a/ci/travis.rb b/ci/travis.rb index 8339e0d08b..2313bece60 100755 --- a/ci/travis.rb +++ b/ci/travis.rb @@ -28,6 +28,7 @@ class Build "av" => "actionview", "aj" => "activejob", "ac" => "actioncable", + "ast" => "activestorage", "guides" => "guides" } @@ -163,6 +164,7 @@ ENV["GEM"].split(",").each do |gem| next if gem == "aj:integration" && isolated next if gem == "guides" && isolated next if gem == "av:ujs" && isolated + next if gem == "ast" && isolated build = Build.new(gem, isolated: isolated) results[build.key] = build.run! diff --git a/guides/source/active_job_basics.md b/guides/source/active_job_basics.md index 443be77934..2606bfe280 100644 --- a/guides/source/active_job_basics.md +++ b/guides/source/active_job_basics.md @@ -260,40 +260,48 @@ backends you need to specify the queues to listen to. Callbacks --------- -Active Job provides hooks during the life cycle of a job. Callbacks allow you to -trigger logic during the life cycle of a job. - -### Available callbacks - -* `before_enqueue` -* `around_enqueue` -* `after_enqueue` -* `before_perform` -* `around_perform` -* `after_perform` - -### Usage +Active Job provides hooks to trigger logic during the life cycle of a job. Like +other callbacks in Rails, you can implement the callbacks as ordinary methods +and use a macro-style class method to register them as callbacks: ```ruby class GuestsCleanupJob < ApplicationJob queue_as :default - before_enqueue do |job| - # Do something with the job instance - end - - around_perform do |job, block| - # Do something before perform - block.call - # Do something after perform - end + around_perform :around_cleanup def perform # Do something later end + + private + def around_cleanup(job) + # Do something before perform + yield + # Do something after perform + end end ``` +The macro-style class methods can also receive a block. Consider using this +style if the code inside your block is so short that it fits in a single line. +For example, you could send metrics for every job enqueued: + +```ruby +class ApplicationJob + before_enqueue { |job| $statsd.increment "#{job.name.underscore}.enqueue" } +end +``` + +### Available callbacks + +* `before_enqueue` +* `around_enqueue` +* `after_enqueue` +* `before_perform` +* `around_perform` +* `after_perform` + Action Mailer ------------ diff --git a/guides/source/generators.md b/guides/source/generators.md index be1be75e7a..389224d908 100644 --- a/guides/source/generators.md +++ b/guides/source/generators.md @@ -627,7 +627,7 @@ This method also takes a block: ```ruby lib "super_special.rb" do - puts "Super special!" + "puts 'Super special!'" end ``` @@ -636,7 +636,7 @@ end Creates a Rake file in the `lib/tasks` directory of the application. ```ruby -rakefile "test.rake", "hello there" +rakefile "test.rake", 'task(:hello) { puts "Hello, there" }' ``` This method also takes a block: diff --git a/guides/source/testing.md b/guides/source/testing.md index f71e963716..1c648ac47f 100644 --- a/guides/source/testing.md +++ b/guides/source/testing.md @@ -614,7 +614,7 @@ $ bin/rails generate system_test users create test/system/users_test.rb ``` -Here's what a freshly-generated system test looks like: +Here's what a freshly generated system test looks like: ```ruby require "application_system_test_case" @@ -788,7 +788,7 @@ $ bin/rails generate integration_test user_flows create test/integration/user_flows_test.rb ``` -Here's what a freshly-generated integration test looks like: +Here's what a freshly generated integration test looks like: ```ruby require 'test_helper' diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index 44449dac0e..509b8d20e3 100644 --- a/railties/CHANGELOG.md +++ b/railties/CHANGELOG.md @@ -1,7 +1,11 @@ -* Add git_source to `Gemfile` for plugin generator +* Add git_source to `Gemfile` for plugin generator. *Yoshiyuki Hirano* +* Add `--skip-action-cable` option to the plugin generator. + + *bogdanvlviv* + * Deprecate support of use `Rails::Application` subclass to start Rails server. *Yuji Yaginuma* diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/application_controller_renderer.rb b/railties/lib/rails/generators/rails/app/templates/config/initializers/application_controller_renderer.rb index 51639b67a0..89d2efab2b 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/application_controller_renderer.rb +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/application_controller_renderer.rb @@ -1,6 +1,8 @@ # Be sure to restart your server when you modify this file. -# ApplicationController.renderer.defaults.merge!( -# http_host: 'example.org', -# https: false -# ) +# ActiveSupport::Reloader.to_prepare do +# ApplicationController.renderer.defaults.merge!( +# http_host: 'example.org', +# https: false +# ) +# end diff --git a/railties/lib/rails/generators/rails/app/templates/config/storage.yml b/railties/lib/rails/generators/rails/app/templates/config/storage.yml index 1afe3eb4b3..089ed4567a 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/storage.yml +++ b/railties/lib/rails/generators/rails/app/templates/config/storage.yml @@ -26,7 +26,7 @@ local: # service: AzureStorage # path: your_azure_storage_path # storage_account_name: your_account_name -# storage_access_key: <%%= Rails.application.secrets.dig(:azure_storage, :secret_access_key) %> +# storage_access_key: <%%= Rails.application.secrets.dig(:azure_storage, :storage_access_key) %> # container: your_container_name # mirror: diff --git a/railties/lib/rails/generators/rails/plugin/plugin_generator.rb b/railties/lib/rails/generators/rails/plugin/plugin_generator.rb index 61c54b4222..a40fa78132 100644 --- a/railties/lib/rails/generators/rails/plugin/plugin_generator.rb +++ b/railties/lib/rails/generators/rails/plugin/plugin_generator.rb @@ -76,8 +76,8 @@ module Rails template "test/test_helper.rb" template "test/%namespaced_name%_test.rb" append_file "Rakefile", <<-EOF -#{rakefile_test_tasks} +#{rakefile_test_tasks} task default: :test EOF if engine? @@ -86,7 +86,7 @@ task default: :test end PASSTHROUGH_OPTIONS = [ - :skip_active_record, :skip_action_mailer, :skip_javascript, :skip_sprockets, :database, + :skip_active_record, :skip_action_mailer, :skip_javascript, :skip_action_cable, :skip_sprockets, :database, :javascript, :quiet, :pretend, :force, :skip ] diff --git a/railties/lib/rails/generators/rails/plugin/templates/Rakefile b/railties/lib/rails/generators/rails/plugin/templates/Rakefile index 3581dd401a..f3efe21cf1 100644 --- a/railties/lib/rails/generators/rails/plugin/templates/Rakefile +++ b/railties/lib/rails/generators/rails/plugin/templates/Rakefile @@ -13,17 +13,16 @@ RDoc::Task.new(:rdoc) do |rdoc| rdoc.rdoc_files.include('README.md') rdoc.rdoc_files.include('lib/**/*.rb') end - <% if engine? && !options[:skip_active_record] && with_dummy_app? -%> + APP_RAKEFILE = File.expand_path("<%= dummy_path -%>/Rakefile", __dir__) load 'rails/tasks/engine.rake' -<% end %> - +<% end -%> <% if engine? -%> -load 'rails/tasks/statistics.rake' -<% end %> +load 'rails/tasks/statistics.rake' +<% end -%> <% unless options[:skip_gemspec] -%> require 'bundler/gem_tasks' -<% end %> +<% end -%> diff --git a/railties/lib/rails/generators/rails/plugin/templates/bin/rails.tt b/railties/lib/rails/generators/rails/plugin/templates/bin/rails.tt index ffa277e334..aed5adf8ee 100644 --- a/railties/lib/rails/generators/rails/plugin/templates/bin/rails.tt +++ b/railties/lib/rails/generators/rails/plugin/templates/bin/rails.tt @@ -3,7 +3,9 @@ ENGINE_ROOT = File.expand_path('..', __dir__) ENGINE_PATH = File.expand_path('../lib/<%= namespaced_name -%>/engine', __dir__) +<% if with_dummy_app? -%> APP_PATH = File.expand_path('../<%= dummy_path -%>/config/application', __dir__) +<% end -%> # Set up gems listed in the Gemfile. ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index 44c4688aa4..ff73014046 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -394,6 +394,16 @@ class AppGeneratorTest < Rails::Generators::TestCase end end + def test_default_frameworks_are_required_when_others_are_removed + run_generator [destination_root, "--skip-active-record", "--skip-action-mailer", "--skip-action-cable", "--skip-sprockets", "--skip-test"] + assert_file "config/application.rb", /require\s+["']rails["']/ + assert_file "config/application.rb", /require\s+["']active_model\/railtie["']/ + assert_file "config/application.rb", /require\s+["']active_job\/railtie["']/ + assert_file "config/application.rb", /require\s+["']action_controller\/railtie["']/ + assert_file "config/application.rb", /require\s+["']action_view\/railtie["']/ + assert_file "config/application.rb", /require\s+["']active_storage\/engine["']/ + end + def test_generator_defaults_to_puma_version run_generator [destination_root] assert_gem "puma", "'~> 3.7'" @@ -449,22 +459,26 @@ class AppGeneratorTest < Rails::Generators::TestCase def test_generator_if_skip_sprockets_is_given run_generator [destination_root, "--skip-sprockets"] + assert_no_file "config/initializers/assets.rb" - assert_file "config/application.rb" do |content| - assert_match(/#\s+require\s+["']sprockets\/railtie["']/, content) - end + + assert_file "config/application.rb", /#\s+require\s+["']sprockets\/railtie["']/ + assert_file "Gemfile" do |content| assert_no_match(/sass-rails/, content) assert_no_match(/uglifier/, content) assert_no_match(/coffee-rails/, content) end + assert_file "config/environments/development.rb" do |content| - assert_no_match(/config\.assets\.debug = true/, content) + assert_no_match(/config\.assets\.debug/, content) end + assert_file "config/environments/production.rb" do |content| - assert_no_match(/config\.assets\.digest = true/, content) - assert_no_match(/config\.assets\.js_compressor = :uglifier/, content) - assert_no_match(/config\.assets\.css_compressor = :sass/, content) + assert_no_match(/config\.assets\.digest/, content) + assert_no_match(/config\.assets\.js_compressor/, content) + assert_no_match(/config\.assets\.css_compressor/, content) + assert_no_match(/config\.assets\.compile/, content) end end @@ -473,7 +487,8 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_file "config/application.rb", /#\s+require\s+["']action_cable\/engine["']/ assert_no_file "config/cable.yml" assert_no_file "app/assets/javascripts/cable.js" - assert_no_file "app/channels" + assert_no_directory "app/assets/javascripts/channels" + assert_no_directory "app/channels" assert_file "Gemfile" do |content| assert_no_match(/redis/, content) end @@ -486,10 +501,15 @@ class AppGeneratorTest < Rails::Generators::TestCase def test_generator_if_skip_test_is_given run_generator [destination_root, "--skip-test"] + + assert_file "config/application.rb", /#\s+require\s+["']rails\/test_unit\/railtie["']/ + assert_file "Gemfile" do |content| assert_no_match(/capybara/, content) assert_no_match(/selenium-webdriver/, content) end + + assert_no_directory("test") end def test_generator_if_skip_system_test_is_given @@ -498,6 +518,10 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_no_match(/capybara/, content) assert_no_match(/selenium-webdriver/, content) end + + assert_directory("test") + + assert_no_directory("test/system") end def test_does_not_generate_system_test_files_if_skip_system_test_is_given @@ -570,6 +594,11 @@ class AppGeneratorTest < Rails::Generators::TestCase run_generator([destination_root]) assert_file "package.json", /dependencies/ assert_file "config/initializers/assets.rb", /node_modules/ + + assert_file ".gitignore" do |content| + assert_match(/node_modules/, content) + assert_match(/yarn-error\.log/, content) + end end def test_generator_for_yarn_skipped @@ -654,18 +683,6 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_file "lib/test_file.rb", "heres test data" end - def test_tests_are_removed_from_frameworks_if_skip_test_is_given - run_generator [destination_root, "--skip-test"] - assert_file "config/application.rb", /#\s+require\s+["']rails\/test_unit\/railtie["']/ - end - - def test_no_active_record_or_tests_if_skips_given - run_generator [destination_root, "--skip-test", "--skip-active-record"] - assert_file "config/application.rb", /#\s+require\s+["']rails\/test_unit\/railtie["']/ - assert_file "config/application.rb", /#\s+require\s+["']active_record\/railtie["']/ - assert_file "config/application.rb", /\s+require\s+["']active_job\/railtie["']/ - end - def test_pretend_option output = run_generator [File.join(destination_root, "myapp"), "--pretend"] assert_no_match(/run bundle install/, output) @@ -896,18 +913,6 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_directory("test/system") end - def test_system_tests_are_not_generated_on_system_test_skip - run_generator [destination_root, "--skip-system-test"] - - assert_no_directory("test/system") - end - - def test_system_tests_are_not_generated_on_test_skip - run_generator [destination_root, "--skip-test"] - - assert_no_directory("test/system") - end - private def stub_rails_application(root) Rails.application.config.root = root diff --git a/railties/test/generators/plugin_generator_test.rb b/railties/test/generators/plugin_generator_test.rb index 0782e1ad1d..6637c329c8 100644 --- a/railties/test/generators/plugin_generator_test.rb +++ b/railties/test/generators/plugin_generator_test.rb @@ -102,23 +102,44 @@ class PluginGeneratorTest < Rails::Generators::TestCase run_generator [destination_root, "-T", "--full"] assert_no_directory "test/integration/" - assert_no_file "test" + assert_no_directory "test" assert_file "Rakefile" do |contents| assert_no_match(/APP_RAKEFILE/, contents) end + assert_file "bin/rails" do |contents| + assert_no_match(/APP_PATH/, contents) + end end - def test_generating_adds_dummy_app_in_full_mode_without_sprockets - run_generator [destination_root, "-S", "--full"] + def test_generating_adds_dummy_app_without_sprockets + run_generator [destination_root, "--skip-sprockets"] + + assert_no_file "test/dummy/config/initializers/assets.rb" + + assert_file "test/dummy/config/application.rb", /#\s+require\s+["']sprockets\/railtie["']/ + + assert_file "Gemfile" do |content| + assert_no_match(/sass-rails/, content) + assert_no_match(/uglifier/, content) + assert_no_match(/coffee-rails/, content) + end - assert_file "test/dummy/config/environments/production.rb" do |contents| - assert_no_match(/config\.assets/, contents) + assert_file "test/dummy/config/environments/development.rb" do |content| + assert_no_match(/config\.assets\.debug/, content) + end + + assert_file "test/dummy/config/environments/production.rb" do |content| + assert_no_match(/config\.assets\.digest/, content) + assert_no_match(/config\.assets\.js_compressor/, content) + assert_no_match(/config\.assets\.css_compressor/, content) + assert_no_match(/config\.assets\.compile/, content) end end def test_generating_adds_dummy_app_rake_tasks_without_unit_test_files run_generator [destination_root, "-T", "--mountable", "--dummy-path", "my_dummy_app"] assert_file "Rakefile", /APP_RAKEFILE/ + assert_file "bin/rails", /APP_PATH/ end def test_generating_adds_dummy_app_without_javascript_and_assets_deps @@ -139,8 +160,8 @@ class PluginGeneratorTest < Rails::Generators::TestCase def test_ensure_that_test_dummy_can_be_generated_from_a_template FileUtils.cd(Rails.root) run_generator([destination_root, "-m", "lib/create_test_dummy_template.rb", "--skip-test"]) - assert_file "spec/dummy" - assert_no_file "test" + assert_directory "spec/dummy" + assert_no_directory "test" end def test_database_entry_is_generated_for_sqlite3_by_default_in_full_mode @@ -174,9 +195,19 @@ class PluginGeneratorTest < Rails::Generators::TestCase end assert_file "test/dummy/config/environments/production.rb" do |content| assert_match(/# config\.action_mailer\.raise_delivery_errors = false/, content) + assert_match(/^ config\.read_encrypted_secrets = true/, content) end end + def test_default_frameworks_are_required_when_others_are_removed + run_generator [destination_root, "--skip-active-record", "--skip-action-mailer", "--skip-action-cable", "--skip-sprockets"] + assert_file "test/dummy/config/application.rb", /require\s+["']rails["']/ + assert_file "test/dummy/config/application.rb", /require\s+["']active_model\/railtie["']/ + assert_file "test/dummy/config/application.rb", /require\s+["']active_job\/railtie["']/ + assert_file "test/dummy/config/application.rb", /require\s+["']action_controller\/railtie["']/ + assert_file "test/dummy/config/application.rb", /require\s+["']action_view\/railtie["']/ + end + def test_active_record_is_removed_from_frameworks_if_skip_active_record_is_given run_generator [destination_root, "--skip-active-record"] assert_file "test/dummy/config/application.rb", /#\s+require\s+["']active_record\/railtie["']/ @@ -202,6 +233,18 @@ class PluginGeneratorTest < Rails::Generators::TestCase assert_file "test/dummy/config/environments/production.rb" do |content| assert_no_match(/config\.action_mailer/, content) end + assert_no_directory "test/dummy/app/mailers" + end + + def test_action_cable_is_removed_from_frameworks_if_skip_action_cable_is_given + run_generator [destination_root, "--skip-action-cable"] + assert_file "test/dummy/config/application.rb", /#\s+require\s+["']action_cable\/engine["']/ + assert_no_file "test/dummy/config/cable.yml" + assert_no_file "test/dummy/app/assets/javascripts/cable.js" + assert_no_directory "test/dummy/app/channels" + assert_file "Gemfile" do |content| + assert_no_match(/redis/, content) + end end def test_ensure_that_database_option_is_passed_to_app_generator @@ -464,10 +507,9 @@ class PluginGeneratorTest < Rails::Generators::TestCase def test_creating_dummy_without_tests_but_with_dummy_path run_generator [destination_root, "--dummy_path", "spec/dummy", "--skip-test"] - assert_file "spec/dummy" - assert_file "spec/dummy/config/application.rb" - assert_no_file "test" - assert_no_file "test/test_helper.rb" + assert_directory "spec/dummy" + assert_file "spec/dummy/config/application.rb", /#\s+require\s+["']rails\/test_unit\/railtie["']/ + assert_no_directory "test" assert_file ".gitignore" do |contents| assert_match(/spec\/dummy/, contents) end @@ -504,9 +546,9 @@ class PluginGeneratorTest < Rails::Generators::TestCase def test_skipping_test_files run_generator [destination_root, "--skip-test"] - assert_no_file "test" + assert_no_directory "test" assert_file ".gitignore" do |contents| - assert_no_match(/test\dummy/, contents) + assert_no_match(/test\/dummy/, contents) end end |