diff options
43 files changed, 763 insertions, 155 deletions
diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb index cdfc523bd4..e680432127 100644 --- a/actionpack/lib/action_controller/metal/params_wrapper.rb +++ b/actionpack/lib/action_controller/metal/params_wrapper.rb @@ -4,8 +4,8 @@ require 'active_support/core_ext/module/anonymous' require 'action_dispatch/http/mime_type' module ActionController - # Wraps the parameters hash into a nested hash. This will allow clients to submit - # POST requests without having to specify any root elements. + # Wraps the parameters hash into a nested hash. This will allow clients to + # submit requests without having to specify any root elements. # # This functionality is enabled in +config/initializers/wrap_parameters.rb+ # and can be customized. @@ -14,7 +14,7 @@ module ActionController # a non-empty array: # # class UsersController < ApplicationController - # wrap_parameters format: [:json, :xml] + # wrap_parameters format: [:json, :xml, :url_encoded_form, :multipart_form] # end # # If you enable +ParamsWrapper+ for +:json+ format, instead of having to diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index 4cb634477e..9e09242872 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -20,7 +20,7 @@ module ActionController #:nodoc: # Since HTML and JavaScript requests are typically made from the browser, we # need to ensure to verify request authenticity for the web browser. We can # use session-oriented authentication for these types of requests, by using - # the `protect_form_forgery` method in our controllers. + # the `protect_from_forgery` method in our controllers. # # GET requests are not protected since they don't have side effects like writing # to the database and don't leak sensitive information. JavaScript requests are diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index cf6a64009f..e78f1f0d7e 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -378,16 +378,14 @@ module ActionController # params.fetch(:none, 'Francesco') # => "Francesco" # params.fetch(:none) { 'Francesco' } # => "Francesco" def fetch(key, *args, &block) - convert_hashes_to_parameters( - key, + convert_value_to_parameters( @parameters.fetch(key) { if block_given? yield else args.fetch(0) { raise ActionController::ParameterMissing.new(key) } end - }, - false + } ) end @@ -475,7 +473,7 @@ module ActionController # optional code block is given and the key is not found, pass in the key # and return the result of block. def delete(key, &block) - convert_hashes_to_parameters(key, @parameters.delete(key), false) + convert_value_to_parameters(@parameters.delete(key)) end # Returns a new instance of <tt>ActionController::Parameters</tt> with only @@ -555,21 +553,23 @@ module ActionController end end - def convert_hashes_to_parameters(key, value, assign_if_converted=true) + def convert_hashes_to_parameters(key, value) converted = convert_value_to_parameters(value) - @parameters[key] = converted if assign_if_converted && !converted.equal?(value) + @parameters[key] = converted unless converted.equal?(value) converted end def convert_value_to_parameters(value) - if value.is_a?(Array) && !converted_arrays.member?(value) + case value + when Array + return value if converted_arrays.member?(value) converted = value.map { |_| convert_value_to_parameters(_) } converted_arrays << converted converted - elsif value.is_a?(Parameters) || !value.is_a?(Hash) - value - else + when Hash self.class.new(value) + else + value end end diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index c2f05ecc86..4defb7f858 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -37,22 +37,7 @@ module ActionDispatch # Convert nested Hash to HashWithIndifferentAccess. # def normalize_encode_params(params) - case params - when Hash - if params.has_key?(:tempfile) - UploadedFile.new(params) - else - params.each_with_object({}) do |(key, val), new_hash| - new_hash[key] = if val.is_a?(Array) - val.map! { |el| normalize_encode_params(el) } - else - normalize_encode_params(val) - end - end.with_indifferent_access - end - else - params - end + ActionDispatch::Request::Utils.normalize_encode_params params end end end diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 3c62c055e5..6985cec5f5 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -290,7 +290,7 @@ module ActionDispatch # Override Rack's GET method to support indifferent access def GET - @env["action_dispatch.request.query_parameters"] ||= Utils.deep_munge(normalize_encode_params(super || {})) + @env["action_dispatch.request.query_parameters"] ||= normalize_encode_params(super || {}) rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e raise ActionController::BadRequest.new(:query, e) end @@ -298,7 +298,7 @@ module ActionDispatch # Override Rack's POST method to support indifferent access def POST - @env["action_dispatch.request.request_parameters"] ||= Utils.deep_munge(normalize_encode_params(super || {})) + @env["action_dispatch.request.request_parameters"] ||= normalize_encode_params(super || {}) rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e raise ActionController::BadRequest.new(:request, e) end @@ -318,11 +318,6 @@ module ActionDispatch LOCALHOST =~ remote_addr && LOCALHOST =~ remote_ip end - protected - def parse_query(*) - Utils.deep_munge(super) - end - private def check_method(name) HTTP_METHOD_LOOKUP[name] || raise(ActionController::UnknownHttpMethod, "#{name}, accepted HTTP methods are #{HTTP_METHODS[0...-1].join(', ')}, and #{HTTP_METHODS[-1]}") diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb index e2b3b06fd8..2617956c74 100644 --- a/actionpack/lib/action_dispatch/middleware/params_parser.rb +++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb @@ -17,7 +17,7 @@ module ActionDispatch Mime::JSON => lambda { |raw_post| data = ActiveSupport::JSON.decode(raw_post) data = {:_json => data} unless data.is_a?(Hash) - Request::Utils.deep_munge(data).with_indifferent_access + Request::Utils.normalize_encode_params(data) } } diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb index 1c9371d89c..3973ea6346 100644 --- a/actionpack/lib/action_dispatch/request/utils.rb +++ b/actionpack/lib/action_dispatch/request/utils.rb @@ -5,24 +5,45 @@ module ActionDispatch mattr_accessor :perform_deep_munge self.perform_deep_munge = true - class << self - # Remove nils from the params hash - def deep_munge(hash, keys = []) - return hash unless perform_deep_munge + def self.normalize_encode_params(params) + if perform_deep_munge + NoNilParamEncoder.normalize_encode_params params + else + ParamEncoder.normalize_encode_params params + end + end - hash.each do |k, v| - keys << k - case v - when Array - v.grep(Hash) { |x| deep_munge(x, keys) } - v.compact! - when Hash - deep_munge(v, keys) + class ParamEncoder # :nodoc: + # Convert nested Hash to HashWithIndifferentAccess. + # + def self.normalize_encode_params(params) + case params + when Array + handle_array params + when Hash + if params.has_key?(:tempfile) + ActionDispatch::Http::UploadedFile.new(params) + else + params.each_with_object({}) do |(key, val), new_hash| + new_hash[key] = normalize_encode_params(val) + end.with_indifferent_access end - keys.pop + else + params end + end + + def self.handle_array(params) + params.map! { |el| normalize_encode_params(el) } + end + end - hash + # Remove nils from the params hash + class NoNilParamEncoder < ParamEncoder # :nodoc: + def self.handle_array(params) + list = super + list.compact! + list end end end diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 1f6bb31cd4..55a2e155d1 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,21 @@ +* Add wildcard matching to explicit dependencies. + + Turns: + + ```erb + <% # Template Dependency: recordings/threads/events/subscribers_changed %> + <% # Template Dependency: recordings/threads/events/completed %> + <% # Template Dependency: recordings/threads/events/uncompleted %> + ``` + + Into: + + ```erb + <% # Template Dependency: recordings/threads/events/* %> + ``` + + *Kasper Timm Hansen* + * Allow defining explicit collection caching using a `# Template Collection: ...` directive inside templates. diff --git a/actionview/lib/action_view/dependency_tracker.rb b/actionview/lib/action_view/dependency_tracker.rb index 7a7e116dbb..6e8c7f8203 100644 --- a/actionview/lib/action_view/dependency_tracker.rb +++ b/actionview/lib/action_view/dependency_tracker.rb @@ -1,16 +1,18 @@ require 'thread_safe' +require 'action_view/path_set' module ActionView class DependencyTracker # :nodoc: @trackers = ThreadSafe::Cache.new - def self.find_dependencies(name, template) + def self.find_dependencies(name, template, view_paths = nil) tracker = @trackers[template.handler] + return [] unless tracker.present? - if tracker.present? - tracker.call(name, template) + if tracker.respond_to?(:supports_view_paths?) && tracker.supports_view_paths? + tracker.call(name, template, view_paths) else - [] + tracker.call(name, template) end end @@ -82,12 +84,16 @@ module ActionView (?:#{STRING}|#{VARIABLE_OR_METHOD_CHAIN}) # finally, the dependency name of interest /xm - def self.call(name, template) - new(name, template).dependencies + def self.supports_view_paths? # :nodoc: + true + end + + def self.call(name, template, view_paths = nil) + new(name, template, view_paths).dependencies end - def initialize(name, template) - @name, @template = name, template + def initialize(name, template, view_paths = nil) + @name, @template, @view_paths = name, template, view_paths end def dependencies @@ -142,8 +148,22 @@ module ActionView end end + def resolve_directories(wildcard_dependencies) + return [] unless @view_paths + + wildcard_dependencies.each_with_object([]) do |query, templates| + @view_paths.find_all_with_query(query).each do |template| + templates << "#{File.dirname(query)}/#{File.basename(template).split('.').first}" + end + end + end + def explicit_dependencies - source.scan(EXPLICIT_DEPENDENCY).flatten.uniq + dependencies = source.scan(EXPLICIT_DEPENDENCY).flatten.uniq + + wildcards, explicits = dependencies.partition { |dependency| dependency[-1] == '*' } + + (explicits + resolve_directories(wildcards)).uniq end end diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index 4224346a2f..9a8a4feb2e 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -72,7 +72,7 @@ module ActionView end def dependencies - DependencyTracker.find_dependencies(name, template) + DependencyTracker.find_dependencies(name, template, finder.view_paths) rescue ActionView::MissingTemplate logger.try :error, " '#{name}' file doesn't exist, so no dependencies" [] diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb index 797d029317..636d7d4cc3 100644 --- a/actionview/lib/action_view/helpers/cache_helper.rb +++ b/actionview/lib/action_view/helpers/cache_helper.rb @@ -98,7 +98,19 @@ module ActionView # <%# Template Dependency: todolists/todolist %> # <%= render_sortable_todolists @project.todolists %> # - # The pattern used to match these is <tt>/# Template Dependency: (\S+)/</tt>, + # In some cases, like a single table inheritance setup, you might have + # a bunch of explicit dependencies. Instead of writing every template out, + # you can use a wildcard to match any template in a directory: + # + # <%# Template Dependency: events/* %> + # <%= render_categorizable_events @person.events %> + # + # This marks every template in the directory as a dependency. To find those + # templates, the wildcard path must be absolutely defined from app/views or paths + # otherwise added with +prepend_view_path+ or +append_view_path+. + # This way the wildcard for `app/views/recordings/events` would be `recordings/events/*` etc. + # + # The pattern used to match explicit dependencies is <tt>/# Template Dependency: (\S+)/</tt>, # so it's important that you type it out just so. # You can only declare one template dependency per line. # diff --git a/actionview/lib/action_view/helpers/url_helper.rb b/actionview/lib/action_view/helpers/url_helper.rb index afb1265ad9..7d92651183 100644 --- a/actionview/lib/action_view/helpers/url_helper.rb +++ b/actionview/lib/action_view/helpers/url_helper.rb @@ -468,7 +468,8 @@ module ActionView }.compact extras = extras.empty? ? '' : '?' + extras.join('&') - html_options["href"] = "mailto:#{email_address}#{extras}" + encoded_email_address = ERB::Util.url_encode(email_address).gsub("%40", "@") + html_options["href"] = "mailto:#{encoded_email_address}#{extras}" content_tag(:a, name || email_address, html_options, &block) end diff --git a/actionview/lib/action_view/path_set.rb b/actionview/lib/action_view/path_set.rb index 91ee2ea8f5..7a88f6bc50 100644 --- a/actionview/lib/action_view/path_set.rb +++ b/actionview/lib/action_view/path_set.rb @@ -61,6 +61,15 @@ module ActionView #:nodoc: find_all(path, prefixes, *args).any? end + def find_all_with_query(query) # :nodoc: + paths.each do |resolver| + templates = resolver.find_all_with_query(query) + return templates unless templates.empty? + end + + [] + end + private def typecast(paths) diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index e7208cce49..28967f40a6 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -52,6 +52,7 @@ module ActionView def initialize @data = SmallCache.new(&KEY_BLOCK) + @query_cache = SmallCache.new end # Cache the templates returned by the block @@ -70,8 +71,17 @@ module ActionView end end + def cache_query(query) # :nodoc: + if Resolver.caching? + @query_cache[query] ||= canonical_no_templates(yield) + else + yield + end + end + def clear @data.clear + @query_cache.clear end private @@ -116,6 +126,10 @@ module ActionView end end + def find_all_with_query(query) # :nodoc: + @cache.cache_query(query) { find_template_paths(File.join(@path, query)) } + end + private delegate :caching?, to: :class diff --git a/actionview/test/fixtures/digestor/events/_completed.html.erb b/actionview/test/fixtures/digestor/events/_completed.html.erb new file mode 100644 index 0000000000..e69de29bb2 --- /dev/null +++ b/actionview/test/fixtures/digestor/events/_completed.html.erb diff --git a/actionview/test/fixtures/digestor/events/index.html.erb b/actionview/test/fixtures/digestor/events/index.html.erb new file mode 100644 index 0000000000..bc45e41bcb --- /dev/null +++ b/actionview/test/fixtures/digestor/events/index.html.erb @@ -0,0 +1 @@ +<% # Template Dependency: events/* %>
\ No newline at end of file diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index 24bc76cbbb..dde757b5a2 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -1,5 +1,6 @@ require 'abstract_unit' require 'fileutils' +require 'action_view/dependency_tracker' class FixtureTemplate attr_reader :source, :handler @@ -15,12 +16,13 @@ end class FixtureFinder FIXTURES_DIR = "#{File.dirname(__FILE__)}/../fixtures/digestor" - attr_reader :details + attr_reader :details, :view_paths attr_accessor :formats attr_accessor :variants def initialize @details = {} + @view_paths = ActionView::PathSet.new(['digestor']) @formats = [] @variants = [] end @@ -75,6 +77,34 @@ class TemplateDigestorTest < ActionView::TestCase end end + def test_explicit_dependency_wildcard + assert_digest_difference("events/index") do + change_template("events/_completed") + end + end + + def test_explicit_dependency_wildcard_picks_up_added_file + old_caching, ActionView::Resolver.caching = ActionView::Resolver.caching, false + + assert_digest_difference("events/index") do + add_template("events/_uncompleted") + end + ensure + remove_template("events/_uncompleted") + ActionView::Resolver.caching = old_caching + end + + def test_explicit_dependency_wildcard_picks_up_removed_file + old_caching, ActionView::Resolver.caching = ActionView::Resolver.caching, false + add_template("events/_subscribers_changed") + + assert_digest_difference("events/index") do + remove_template("events/_subscribers_changed") + end + ensure + ActionView::Resolver.caching = old_caching + end + def test_second_level_dependency assert_digest_difference("messages/show") do change_template("comments/_comments") @@ -319,4 +349,9 @@ class TemplateDigestorTest < ActionView::TestCase f.write "\nTHIS WAS CHANGED!" end end + alias_method :add_template, :change_template + + def remove_template(template_name) + File.delete("digestor/#{template_name}.html.erb") + end end diff --git a/actionview/test/template/url_helper_test.rb b/actionview/test/template/url_helper_test.rb index 0e35c67516..416d30938a 100644 --- a/actionview/test/template/url_helper_test.rb +++ b/actionview/test/template/url_helper_test.rb @@ -500,6 +500,13 @@ class UrlHelperTest < ActiveSupport::TestCase mail_to("david@loudthinking.com", "David Heinemeier Hansson", class: "admin") end + def test_mail_to_with_special_characters + assert_dom_equal( + %{<a href="mailto:%23%21%24%25%26%27%2A%2B-%2F%3D%3F%5E_%60%7B%7D%7C%7E@example.org">#!$%&'*+-/=?^_`{}|~@example.org</a>}, + mail_to("#!$%&'*+-/=?^_`{}|~@example.org") + ) + end + def test_mail_with_options assert_dom_equal( %{<a href="mailto:me@example.com?cc=ccaddress%40example.com&bcc=bccaddress%40example.com&body=This%20is%20the%20body%20of%20the%20message.&subject=This%20is%20an%20example%20email&reply-to=foo%40bar.com">My email</a>}, diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index fb64156b78..4cf54ffeb1 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,16 @@ +* Properly allow uniqueness validations on primary keys. + + Fixes #20966. + + *Sean Griffin & presskey* + +* Don't raise an error if an association failed to destroy when `destroy` was + called on the parent (as opposed to `destroy!`). + + Fixes #20991. + + *Sean Griffin* + * ActiveRecord::RecordNotFound modified to store model name, primary_key and id of the caller model. It allows the catcher of this exception to make a better decision to what to do with it. For example consider this simple @@ -170,7 +183,7 @@ *Aster Ryan* -* Add `:enum_prefix`/`:enum_suffix` option to `enum` definition. +* Add `:_prefix` and `:_suffix` options to `enum` definition. Fixes #17511, #17415. diff --git a/activerecord/Rakefile b/activerecord/Rakefile index a619204e6f..8ea22fd901 100644 --- a/activerecord/Rakefile +++ b/activerecord/Rakefile @@ -83,15 +83,6 @@ end task "isolated_test_#{adapter}" => ["#{adapter}:env", "test:isolated:#{adapter}"] end -rule '.sqlite3' do |t| - sh %Q{sqlite3 "#{t.name}" "create table a (a integer); drop table a;"} -end - -task :test_sqlite3 => [ - 'test/fixtures/fixture_database.sqlite3', - 'test/fixtures/fixture_database_2.sqlite3' -] - namespace :db do namespace :mysql do desc 'Build the MySQL test databases' diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb index 19f0dca5a6..c7c769b283 100644 --- a/activerecord/lib/active_record/callbacks.rb +++ b/activerecord/lib/active_record/callbacks.rb @@ -290,6 +290,9 @@ module ActiveRecord def destroy #:nodoc: _run_destroy_callbacks { super } + rescue RecordNotDestroyed => e + @_association_destroy_exception = e + false end def touch(*) #:nodoc: diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index e3115abe66..a30945d0ee 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -761,9 +761,9 @@ module ActiveRecord # [<tt>:name</tt>] # The constraint name. Defaults to <tt>fk_rails_<identifier></tt>. # [<tt>:on_delete</tt>] - # Action that happens <tt>ON DELETE</tt>. Valid values are +:nullify+, +:cascade:+ and +:restrict+ + # Action that happens <tt>ON DELETE</tt>. Valid values are +:nullify+, +:cascade+ and +:restrict+ # [<tt>:on_update</tt>] - # Action that happens <tt>ON UPDATE</tt>. Valid values are +:nullify+, +:cascade:+ and +:restrict+ + # Action that happens <tt>ON UPDATE</tt>. Valid values are +:nullify+, +:cascade+ and +:restrict+ def add_foreign_key(from_table, to_table, options = {}) return unless supports_foreign_keys? diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index c0d9d9c1c8..91a13cb0cd 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -75,22 +75,24 @@ module ActiveRecord # # Conversation.where("status <> ?", Conversation.statuses[:archived]) # - # You can use the +:enum_prefix+ or +:enum_suffix+ options when you need - # to define multiple enums with same values. If the passed value is +true+, - # the methods are prefixed/suffixed with the name of the enum. + # You can use the +:_prefix+ or +:_suffix+ options when you need to define + # multiple enums with same values. If the passed value is +true+, the methods + # are prefixed/suffixed with the name of the enum. It is also possible to + # supply a custom value: # - # class Invoice < ActiveRecord::Base - # enum verification: [:done, :fail], enum_prefix: true + # class Conversation < ActiveRecord::Base + # enum status: [:active, :archived], _suffix: true + # enum comments_status: [:active, :inactive], _prefix: :comments # end # - # It is also possible to supply a custom prefix. + # With the above example, the bang and predicate methods along with the + # associated scopes are now prefixed and/or suffixed accordingly: # - # class Invoice < ActiveRecord::Base - # enum verification: [:done, :fail], enum_prefix: :verification_status - # end + # conversation.active_status! + # conversation.archived_status? # => false # - # Note that <tt>:enum_prefix</tt>/<tt>:enum_suffix</tt> are reserved keywords - # and can not be used as an enum name. + # conversation.comments_inactive! + # conversation.comments_active? # => false module Enum def self.extended(base) # :nodoc: @@ -137,8 +139,8 @@ module ActiveRecord def enum(definitions) klass = self - enum_prefix = definitions.delete(:enum_prefix) - enum_suffix = definitions.delete(:enum_suffix) + enum_prefix = definitions.delete(:_prefix) + enum_suffix = definitions.delete(:_suffix) definitions.each do |name, values| # statuses = { } enum_values = ActiveSupport::HashWithIndifferentAccess.new diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 0a6e4ac0bd..09c36d7b4d 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -193,7 +193,7 @@ module ActiveRecord # and #destroy! raises ActiveRecord::RecordNotDestroyed. # See ActiveRecord::Callbacks for further details. def destroy! - destroy || raise(RecordNotDestroyed.new("Failed to destroy the record", self)) + destroy || _raise_record_not_destroyed end # Returns an instance of the specified +klass+ with the attributes of the @@ -548,5 +548,12 @@ module ActiveRecord def verify_readonly_attribute(name) raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name) end + + def _raise_record_not_destroyed + @_association_destroy_exception ||= nil + raise @_association_destroy_exception || RecordNotDestroyed.new("Failed to destroy the record", self) + ensure + @_association_destroy_exception = nil + end end end diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb index 5106f4e127..32d17a1392 100644 --- a/activerecord/lib/active_record/validations/uniqueness.rb +++ b/activerecord/lib/active_record/validations/uniqueness.rb @@ -17,7 +17,9 @@ module ActiveRecord value = map_enum_attribute(finder_class, attribute, value) relation = build_relation(finder_class, table, attribute, value) - relation = relation.where.not(finder_class.primary_key => record.id) if record.persisted? + if record.persisted? && finder_class.primary_key.to_s != attribute.to_s + relation = relation.where.not(finder_class.primary_key => record.id) + end relation = scope_relation(record, table, relation) relation = relation.merge(options[:conditions]) if options[:conditions] diff --git a/activerecord/test/cases/adapters/mysql/quoting_test.rb b/activerecord/test/cases/adapters/mysql/quoting_test.rb index ca476947fc..426b088e2f 100644 --- a/activerecord/test/cases/adapters/mysql/quoting_test.rb +++ b/activerecord/test/cases/adapters/mysql/quoting_test.rb @@ -15,14 +15,14 @@ class MysqlQuotingTest < ActiveRecord::MysqlTestCase def test_quoted_date_precision_for_gte_564 @conn.stubs(:full_version).returns('5.6.4') - @conn.remove_instance_variable(:@version) + @conn.remove_instance_variable(:@version) if @conn.instance_variable_defined?(:@version) t = Time.now.change(usec: 1) assert_match(/\.000001\z/, @conn.quoted_date(t)) end def test_quoted_date_precision_for_lt_564 @conn.stubs(:full_version).returns('5.6.3') - @conn.remove_instance_variable(:@version) + @conn.remove_instance_variable(:@version) if @conn.instance_variable_defined?(:@version) t = Time.now.change(usec: 1) refute_match(/\.000001\z/, @conn.quoted_date(t)) end diff --git a/activerecord/test/cases/adapters/mysql2/quoting_test.rb b/activerecord/test/cases/adapters/mysql2/quoting_test.rb index a49cbba4b4..92221e61bf 100644 --- a/activerecord/test/cases/adapters/mysql2/quoting_test.rb +++ b/activerecord/test/cases/adapters/mysql2/quoting_test.rb @@ -7,14 +7,14 @@ class Mysql2QuotingTest < ActiveRecord::Mysql2TestCase test 'quoted date precision for gte 5.6.4' do @connection.stubs(:full_version).returns('5.6.4') - @connection.remove_instance_variable(:@version) + @connection.remove_instance_variable(:@version) if @connection.instance_variable_defined?(:@version) t = Time.now.change(usec: 1) assert_match(/\.000001\z/, @connection.quoted_date(t)) end test 'quoted date precision for lt 5.6.4' do @connection.stubs(:full_version).returns('5.6.3') - @connection.remove_instance_variable(:@version) + @connection.remove_instance_variable(:@version) if @connection.instance_variable_defined?(:@version) t = Time.now.change(usec: 1) refute_match(/\.000001\z/, @connection.quoted_date(t)) end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index c2ef59d0f7..21e2ee3b11 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -2278,4 +2278,34 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_deprecated { company.clients_of_firm(true) } end + + class AuthorWithErrorDestroyingAssociation < ActiveRecord::Base + self.table_name = "authors" + has_many :posts_with_error_destroying, + class_name: "PostWithErrorDestroying", + foreign_key: :author_id, + dependent: :destroy + end + + class PostWithErrorDestroying < ActiveRecord::Base + self.table_name = "posts" + self.inheritance_column = nil + before_destroy -> { throw :abort } + end + + def test_destroy_does_not_raise_when_association_errors_on_destroy + assert_no_difference "AuthorWithErrorDestroyingAssociation.count" do + author = AuthorWithErrorDestroyingAssociation.first + + assert_not author.destroy + end + end + + def test_destroy_with_bang_bubbles_errors_from_associations + error = assert_raises ActiveRecord::RecordNotDestroyed do + AuthorWithErrorDestroyingAssociation.first.destroy! + end + + assert_instance_of PostWithErrorDestroying, error.record + end end diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb index 2608c84be2..ceca2c8366 100644 --- a/activerecord/test/cases/validations/uniqueness_validation_test.rb +++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb @@ -427,4 +427,23 @@ class UniquenessValidationTest < ActiveRecord::TestCase assert reply.valid? assert topic.valid? end + + def test_validate_uniqueness_of_custom_primary_key + klass = Class.new(ActiveRecord::Base) do + self.table_name = "keyboards" + self.primary_key = :key_number + + validates_uniqueness_of :key_number + + def self.name + "Keyboard" + end + end + + klass.create!(key_number: 10) + key2 = klass.create!(key_number: 11) + + key2.key_number = 10 + assert_not key2.valid? + end end diff --git a/activerecord/test/models/book.rb b/activerecord/test/models/book.rb index 24bfe47bbf..1927191393 100644 --- a/activerecord/test/models/book.rb +++ b/activerecord/test/models/book.rb @@ -10,10 +10,10 @@ class Book < ActiveRecord::Base enum status: [:proposed, :written, :published] enum read_status: {unread: 0, reading: 2, read: 3} enum nullable_status: [:single, :married] - enum language: [:english, :spanish, :french], enum_prefix: :in - enum author_visibility: [:visible, :invisible], enum_prefix: true - enum illustrator_visibility: [:visible, :invisible], enum_prefix: true - enum font_size: [:small, :medium, :large], enum_prefix: :with, enum_suffix: true + enum language: [:english, :spanish, :french], _prefix: :in + enum author_visibility: [:visible, :invisible], _prefix: true + enum illustrator_visibility: [:visible, :invisible], _prefix: true + enum font_size: [:small, :medium, :large], _prefix: :with, _suffix: true def published! super diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index f81ffe1d90..d17270021a 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -32,7 +32,7 @@ class Topic < ActiveRecord::Base end end - has_many :replies, :dependent => :destroy, :foreign_key => "parent_id" + has_many :replies, dependent: :destroy, foreign_key: "parent_id", autosave: true has_many :approved_replies, -> { approved }, class_name: 'Reply', foreign_key: "parent_id", counter_cache: 'replies_count' has_many :unique_replies, :dependent => :destroy, :foreign_key => "parent_id" diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb index f1c6230084..ca48164c54 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -9,7 +9,7 @@ module ActiveSupport #-- # Note that a pending Exclusive lock attempt does not block incoming # Share requests (i.e., we are "read-preferring"). That seems - # consistent with the behavior of +loose_upgrades+, but may be the + # consistent with the behavior of "loose" upgrades, but may be the # wrong choice otherwise: it nominally reduces the possibility of # deadlock by risking starvation instead. class ShareLock @@ -20,47 +20,48 @@ module ActiveSupport # to upgrade share locks to exclusive. - # If +loose_upgrades+ is false (the default), then a thread that - # is waiting on an Exclusive lock will continue to hold any Share - # lock that it has already established. This is safer, but can - # lead to deadlock. - # - # If +loose_upgrades+ is true, a thread waiting on an Exclusive - # lock will temporarily relinquish its Share lock. Being less - # strict, this behavior prevents some classes of deadlocks. For - # many resources, loose upgrades are sufficient: if a thread is - # awaiting a lock, it is not running any other code. - attr_reader :loose_upgrades - - def initialize(loose_upgrades = false) - @loose_upgrades = loose_upgrades - + def initialize super() @cv = new_cond @sharing = Hash.new(0) + @waiting = {} @exclusive_thread = nil @exclusive_depth = 0 end - # Returns false if +no_wait+ is specified and the lock is not + # Returns false if +no_wait+ is set and the lock is not # immediately available. Otherwise, returns true after the lock # has been acquired. - def start_exclusive(no_wait=false) + # + # +purpose+ and +compatible+ work together; while this thread is + # waiting for the exclusive lock, it will yield its share (if any) + # to any other attempt whose +purpose+ appears in this attempt's + # +compatible+ list. This allows a "loose" upgrade, which, being + # less strict, prevents some classes of deadlocks. + # + # For many resources, loose upgrades are sufficient: if a thread + # is awaiting a lock, it is not running any other code. With + # +purpose+ matching, it is possible to yield only to other + # threads whose activity will not interfere. + def start_exclusive(purpose: nil, compatible: [], no_wait: false) synchronize do unless @exclusive_thread == Thread.current - return false if no_wait && busy? + if busy?(purpose) + return false if no_wait - loose_shares = nil - if @loose_upgrades loose_shares = @sharing.delete(Thread.current) + @waiting[Thread.current] = compatible if loose_shares + + begin + @cv.wait_while { busy?(purpose) } + ensure + @waiting.delete Thread.current + @sharing[Thread.current] = loose_shares if loose_shares + end end - - @cv.wait_while { busy? } if busy? - @exclusive_thread = Thread.current - @sharing[Thread.current] = loose_shares if loose_shares end @exclusive_depth += 1 @@ -106,8 +107,10 @@ module ActiveSupport # +no_wait+ is set and the lock is not immediately available, # returns +nil+ without yielding. Otherwise, returns the result of # the block. - def exclusive(no_wait=false) - if start_exclusive(no_wait) + # + # See +start_exclusive+ for other options. + def exclusive(purpose: nil, compatible: [], no_wait: false) + if start_exclusive(purpose: purpose, compatible: compatible, no_wait: no_wait) begin yield ensure @@ -129,8 +132,9 @@ module ActiveSupport private # Must be called within synchronize - def busy? + def busy?(purpose) (@exclusive_thread && @exclusive_thread != Thread.current) || + @waiting.any? { |k, v| k != Thread.current && !v.include?(purpose) } || @sharing.size > (@sharing[Thread.current] > 0 ? 1 : 0) end end diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index fc6f822969..f76ef04f49 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -37,6 +37,13 @@ module ActiveSupport #:nodoc: Dependencies.interlock.loading { yield } end + # Execute the supplied block while holding an exclusive lock, + # preventing any other thread from being inside a #run_interlock + # block at the same time + def self.unload_interlock + Dependencies.interlock.unloading { yield } + end + # :nodoc: # Should we turn on Ruby warnings on the first load of dependent files? @@ -255,12 +262,10 @@ module ActiveSupport #:nodoc: end def load_dependency(file) - Dependencies.load_interlock do - if Dependencies.load? && ActiveSupport::Dependencies.constant_watch_stack.watching? - Dependencies.new_constants_in(Object) { yield } - else - yield - end + if Dependencies.load? && ActiveSupport::Dependencies.constant_watch_stack.watching? + Dependencies.new_constants_in(Object) { yield } + else + yield end rescue Exception => exception # errors from loading file exception.blame_file! file if exception.respond_to? :blame_file! @@ -348,7 +353,7 @@ module ActiveSupport #:nodoc: def clear log_call - Dependencies.load_interlock do + Dependencies.unload_interlock do loaded.clear loading.clear remove_unloadable_constants! diff --git a/activesupport/lib/active_support/dependencies/interlock.rb b/activesupport/lib/active_support/dependencies/interlock.rb index 148212c951..fbeb904684 100644 --- a/activesupport/lib/active_support/dependencies/interlock.rb +++ b/activesupport/lib/active_support/dependencies/interlock.rb @@ -4,21 +4,27 @@ module ActiveSupport #:nodoc: module Dependencies #:nodoc: class Interlock def initialize # :nodoc: - @lock = ActiveSupport::Concurrency::ShareLock.new(true) + @lock = ActiveSupport::Concurrency::ShareLock.new end def loading - @lock.exclusive do + @lock.exclusive(purpose: :load, compatible: [:load]) do yield end end - # Attempt to obtain a "loading" (exclusive) lock. If possible, + def unloading + @lock.exclusive(purpose: :unload, compatible: [:load, :unload]) do + yield + end + end + + # Attempt to obtain an "unloading" (exclusive) lock. If possible, # execute the supplied block while holding the lock. If there is # concurrent activity, return immediately (without executing the # block) instead of waiting. - def attempt_loading - @lock.exclusive(true) do + def attempt_unloading + @lock.exclusive(purpose: :unload, compatible: [:load, :unload], no_wait: true) do yield end end diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb new file mode 100644 index 0000000000..ad41db608b --- /dev/null +++ b/activesupport/test/share_lock_test.rb @@ -0,0 +1,333 @@ +require 'abstract_unit' +require 'concurrent/atomics' +require 'active_support/concurrency/share_lock' + +class ShareLockTest < ActiveSupport::TestCase + def setup + @lock = ActiveSupport::Concurrency::ShareLock.new + end + + def test_reentrancy + thread = Thread.new do + @lock.sharing { @lock.sharing {} } + @lock.exclusive { @lock.exclusive {} } + end + assert_threads_not_stuck thread + end + + def test_sharing_doesnt_block + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_latch| + assert_threads_not_stuck(Thread.new {@lock.sharing {} }) + end + end + + def test_sharing_blocks_exclusive + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + @lock.exclusive(no_wait: true) { flunk } # polling should fail + exclusive_thread = Thread.new { @lock.exclusive {} } + assert_threads_stuck_but_releasable_by_latch exclusive_thread, sharing_thread_release_latch + end + end + + def test_exclusive_blocks_sharing + with_thread_waiting_in_lock_section(:exclusive) do |exclusive_thread_release_latch| + sharing_thread = Thread.new { @lock.sharing {} } + assert_threads_stuck_but_releasable_by_latch sharing_thread, exclusive_thread_release_latch + end + end + + def test_multiple_exlusives_are_able_to_progress + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + exclusive_threads = (1..2).map do + Thread.new do + @lock.exclusive {} + end + end + + assert_threads_stuck_but_releasable_by_latch exclusive_threads, sharing_thread_release_latch + end + end + + def test_sharing_is_upgradeable_to_exclusive + upgrading_thread = Thread.new do + @lock.sharing do + @lock.exclusive {} + end + end + assert_threads_not_stuck upgrading_thread + end + + def test_exclusive_upgrade_waits_for_other_sharers_to_leave + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + in_sharing = Concurrent::CountDownLatch.new + + upgrading_thread = Thread.new do + @lock.sharing do + in_sharing.count_down + @lock.exclusive {} + end + end + + in_sharing.wait + assert_threads_stuck_but_releasable_by_latch upgrading_thread, sharing_thread_release_latch + end + end + + def test_exclusive_matching_purpose + [true, false].each do |use_upgrading| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + exclusive_threads = (1..2).map do + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: :load, compatible: [:load, :unload]) {} + end + end + end + + assert_threads_stuck_but_releasable_by_latch exclusive_threads, sharing_thread_release_latch + end + end + end + + def test_killed_thread_loses_lock + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + thread = Thread.new do + @lock.sharing do + @lock.exclusive {} + end + end + + assert_threads_stuck thread + thread.kill + + sharing_thread_release_latch.count_down + + thread = Thread.new do + @lock.exclusive {} + end + + assert_threads_not_stuck thread + end + end + + def test_exclusive_conflicting_purpose + [true, false].each do |use_upgrading| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + begin + conflicting_exclusive_threads = [ + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: :red, compatible: [:green, :purple]) {} + end + end, + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: :blue, compatible: [:green]) {} + end + end + ] + + assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks + + # This thread will be stuck as long as any other thread is in + # a sharing block. While it's blocked, it holds no lock, so it + # doesn't interfere with any other attempts. + no_purpose_thread = Thread.new do + @lock.exclusive {} + end + assert_threads_stuck no_purpose_thread + + # This thread is compatible with both of the "primary" + # attempts above. It's initially stuck on the outer share + # lock, but as soon as that's released, it can run -- + # regardless of whether those threads hold share locks. + compatible_thread = Thread.new do + @lock.exclusive(purpose: :green, compatible: []) {} + end + assert_threads_stuck compatible_thread + + assert_threads_stuck conflicting_exclusive_threads + + sharing_thread_release_latch.count_down + + assert_threads_not_stuck compatible_thread # compatible thread is now able to squeak through + + if use_upgrading + # The "primary" threads both each hold a share lock, and are + # mutually incompatible; they're still stuck. + assert_threads_stuck conflicting_exclusive_threads + + # The thread without a specified purpose is also stuck; it's + # not compatible with anything. + assert_threads_stuck no_purpose_thread + else + # As the primaries didn't hold a share lock, as soon as the + # outer one was released, all the exclusive locks are free + # to be acquired in turn. + + assert_threads_not_stuck conflicting_exclusive_threads + assert_threads_not_stuck no_purpose_thread + end + ensure + conflicting_exclusive_threads.each(&:kill) + no_purpose_thread.kill + end + end + end + end + + def test_exclusive_ordering + scratch_pad = [] + scratch_pad_mutex = Mutex.new + + load_params = [:load, [:load]] + unload_params = [:unload, [:unload, :load]] + + [load_params, load_params, unload_params, unload_params].permutation do |thread_params| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + threads = thread_params.map do |purpose, compatible| + Thread.new do + @lock.sharing do + @lock.exclusive(purpose: purpose, compatible: compatible) do + scratch_pad_mutex.synchronize { scratch_pad << purpose } + end + end + end + end + + sleep(0.01) + scratch_pad_mutex.synchronize { assert_empty scratch_pad } + + sharing_thread_release_latch.count_down + + assert_threads_not_stuck threads + scratch_pad_mutex.synchronize do + assert_equal [:load, :load, :unload, :unload], scratch_pad + scratch_pad.clear + end + end + end + end + + def test_in_shared_section_incompatible_non_upgrading_threads_cannot_preempt_upgrading_threads + scratch_pad = [] + scratch_pad_mutex = Mutex.new + + upgrading_load_params = [:load, [:load], true] + non_upgrading_unload_params = [:unload, [:load, :unload], false] + + [upgrading_load_params, non_upgrading_unload_params].permutation do |thread_params| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + threads = thread_params.map do |purpose, compatible, use_upgrading| + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: purpose, compatible: compatible) do + scratch_pad_mutex.synchronize { scratch_pad << purpose } + end + end + end + end + + assert_threads_stuck threads + scratch_pad_mutex.synchronize { assert_empty scratch_pad } + + sharing_thread_release_latch.count_down + + assert_threads_not_stuck threads + scratch_pad_mutex.synchronize do + assert_equal [:load, :unload], scratch_pad + scratch_pad.clear + end + end + end + end + + private + + module CustomAssertions + SUFFICIENT_TIMEOUT = 0.2 + + private + + def assert_threads_stuck_but_releasable_by_latch(threads, latch) + assert_threads_stuck threads + latch.count_down + assert_threads_not_stuck threads + end + + def assert_threads_stuck(threads) + sleep(SUFFICIENT_TIMEOUT) # give threads time to do their business + assert(Array(threads).all? { |t| t.join(0.001).nil? }) + end + + def assert_threads_not_stuck(threads) + assert(Array(threads).all? { |t| t.join(SUFFICIENT_TIMEOUT) }) + end + end + + class CustomAssertionsTest < ActiveSupport::TestCase + include CustomAssertions + + def setup + @latch = Concurrent::CountDownLatch.new + @thread = Thread.new { @latch.wait } + end + + def teardown + @latch.count_down + @thread.join + end + + def test_happy_path + assert_threads_stuck_but_releasable_by_latch @thread, @latch + end + + def test_detects_stuck_thread + assert_raises(Minitest::Assertion) do + assert_threads_not_stuck @thread + end + end + + def test_detects_free_thread + @latch.count_down + assert_raises(Minitest::Assertion) do + assert_threads_stuck @thread + end + end + + def test_detects_already_released + @latch.count_down + assert_raises(Minitest::Assertion) do + assert_threads_stuck_but_releasable_by_latch @thread, @latch + end + end + + def test_detects_remains_latched + another_latch = Concurrent::CountDownLatch.new + assert_raises(Minitest::Assertion) do + assert_threads_stuck_but_releasable_by_latch @thread, another_latch + end + end + end + + include CustomAssertions + + def with_thread_waiting_in_lock_section(lock_section) + in_section = Concurrent::CountDownLatch.new + section_release = Concurrent::CountDownLatch.new + + stuck_thread = Thread.new do + @lock.send(lock_section) do + in_section.count_down + section_release.wait + end + end + + in_section.wait + + yield section_release + ensure + section_release.count_down + stuck_thread.join # clean up + end +end diff --git a/guides/source/credits.html.erb b/guides/source/credits.html.erb index 61ea0b44ef..1d995581fa 100644 --- a/guides/source/credits.html.erb +++ b/guides/source/credits.html.erb @@ -28,7 +28,7 @@ Ruby on Rails Guides: Credits <h3 class="section">Rails Guides Authors</h3> <%= author('Ryan Bigg', 'radar', 'radar.png') do %> - Ryan Bigg works as the Community Manager at <a href="http://spreecommerce.com">Spree Commerce</a> and has been working with Rails since 2006. He's the author of <a href="https://leanpub.com/multi-tenancy-rails">Multi Tenancy With Rails</a> and co-author of <a href="http://manning.com/bigg2">Rails 4 in Action</a>. He's written many gems which can be seen on <a href="https://github.com/radar">his GitHub page</a> and he also tweets prolifically as <a href="http://twitter.com/ryanbigg">@ryanbigg</a>. + Ryan Bigg works as a Rails developer at <a href="http://marketplacer.com">Marketplacer</a> and has been working with Rails since 2006. He's the author of <a href="https://leanpub.com/multi-tenancy-rails">Multi Tenancy With Rails</a> and co-author of <a href="http://manning.com/bigg2">Rails 4 in Action</a>. He's written many gems which can be seen on <a href="https://github.com/radar">his GitHub page</a> and he also tweets prolifically as <a href="http://twitter.com/ryanbigg">@ryanbigg</a>. <% end %> <%= author('Oscar Del Ben', 'oscardelben', 'oscardelben.jpg') do %> diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index 573b1f8f69..30449aa670 100644 --- a/railties/CHANGELOG.md +++ b/railties/CHANGELOG.md @@ -1,3 +1,8 @@ +* Add a `--api` option in order to generate plugins that can be added + inside an API application. + + *Robin Dupret* + * Fix `NoMethodError` when generating a scaffold inside a full engine. *Yuji Yaginuma* diff --git a/railties/lib/rails/application/finisher.rb b/railties/lib/rails/application/finisher.rb index f8f92792a7..404e3c3e23 100644 --- a/railties/lib/rails/application/finisher.rb +++ b/railties/lib/rails/application/finisher.rb @@ -86,7 +86,7 @@ module Rails # added in the hook are taken into account. initializer :set_clear_dependencies_hook, group: :all do callback = lambda do - ActiveSupport::Dependencies.interlock.attempt_loading do + ActiveSupport::Dependencies.interlock.attempt_unloading do ActiveSupport::DescendantsTracker.clear ActiveSupport::Dependencies.clear end diff --git a/railties/lib/rails/generators/actions.rb b/railties/lib/rails/generators/actions.rb index 560a553789..b4356f71e0 100644 --- a/railties/lib/rails/generators/actions.rb +++ b/railties/lib/rails/generators/actions.rb @@ -92,11 +92,11 @@ module Rails # file in <tt>config/environments</tt>. # # environment do - # "config.autoload_paths += %W(#{config.root}/extras)" + # "config.action_controller.asset_host = 'cdn.provider.com'" # end # # environment(nil, env: "development") do - # "config.autoload_paths += %W(#{config.root}/extras)" + # "config.action_controller.asset_host = 'localhost:3000'" # end def environment(data=nil, options={}) sentinel = /class [a-z_:]+ < Rails::Application/i diff --git a/railties/lib/rails/generators/rails/plugin/plugin_generator.rb b/railties/lib/rails/generators/rails/plugin/plugin_generator.rb index 66111004aa..0e1326ce4f 100644 --- a/railties/lib/rails/generators/rails/plugin/plugin_generator.rb +++ b/railties/lib/rails/generators/rails/plugin/plugin_generator.rb @@ -17,15 +17,22 @@ module Rails def app if mountable? - directory 'app' - empty_directory_with_keep_file "app/assets/images/#{namespaced_name}" + if api? + directory 'app', exclude_pattern: %r{app/(views|helpers)} + else + directory 'app' + empty_directory_with_keep_file "app/assets/images/#{namespaced_name}" + end elsif full? empty_directory_with_keep_file 'app/models' empty_directory_with_keep_file 'app/controllers' - empty_directory_with_keep_file 'app/views' - empty_directory_with_keep_file 'app/helpers' empty_directory_with_keep_file 'app/mailers' - empty_directory_with_keep_file "app/assets/images/#{namespaced_name}" + + unless api? + empty_directory_with_keep_file "app/assets/images/#{namespaced_name}" + empty_directory_with_keep_file 'app/helpers' + empty_directory_with_keep_file 'app/views' + end end end @@ -82,6 +89,7 @@ task default: :test opts = (options || {}).slice(*PASSTHROUGH_OPTIONS) opts[:force] = force opts[:skip_bundle] = true + opts[:api] = options.api? invoke Rails::Generators::AppGenerator, [ File.expand_path(dummy_path, destination_root) ], opts @@ -176,6 +184,9 @@ task default: :test desc: "If creating plugin in application's directory " + "skip adding entry to Gemfile" + class_option :api, type: :boolean, default: false, + desc: "Generate a smaller stack for API application plugins" + def initialize(*args) @dummy_path = nil super @@ -210,15 +221,15 @@ task default: :test end def create_public_stylesheets_files - build(:stylesheets) + build(:stylesheets) unless api? end def create_javascript_files - build(:javascripts) + build(:javascripts) unless api? end def create_images_directory - build(:images) + build(:images) unless api? end def create_bin_files @@ -305,6 +316,10 @@ task default: :test options[:skip_test].blank? || options[:dummy_path] != 'test/dummy' end + def api? + options[:api] + end + def self.banner "rails plugin new #{self.arguments.map(&:usage).join(' ')} [options]" end diff --git a/railties/lib/rails/generators/rails/plugin/templates/app/controllers/%namespaced_name%/application_controller.rb.tt b/railties/lib/rails/generators/rails/plugin/templates/app/controllers/%namespaced_name%/application_controller.rb.tt index 7157e48c42..7fe4e5034d 100644 --- a/railties/lib/rails/generators/rails/plugin/templates/app/controllers/%namespaced_name%/application_controller.rb.tt +++ b/railties/lib/rails/generators/rails/plugin/templates/app/controllers/%namespaced_name%/application_controller.rb.tt @@ -1,5 +1,5 @@ <%= wrap_in_modules <<-rb.strip_heredoc - class ApplicationController < ActionController::Base + class ApplicationController < ActionController::#{api? ? "API" : "Base"} end rb %> diff --git a/railties/lib/rails/generators/rails/plugin/templates/lib/%namespaced_name%/engine.rb b/railties/lib/rails/generators/rails/plugin/templates/lib/%namespaced_name%/engine.rb index 17afd52177..8938770fc4 100644 --- a/railties/lib/rails/generators/rails/plugin/templates/lib/%namespaced_name%/engine.rb +++ b/railties/lib/rails/generators/rails/plugin/templates/lib/%namespaced_name%/engine.rb @@ -1,6 +1,7 @@ <%= wrap_in_modules <<-rb.strip_heredoc class Engine < ::Rails::Engine #{mountable? ? ' isolate_namespace ' + camelized_modules : ' '} + #{api? ? " config.generators.api_only = true" : ' '} end rb %> diff --git a/railties/test/generators/plugin_generator_test.rb b/railties/test/generators/plugin_generator_test.rb index 73e68863f4..6f5a815173 100644 --- a/railties/test/generators/plugin_generator_test.rb +++ b/railties/test/generators/plugin_generator_test.rb @@ -544,6 +544,60 @@ class PluginGeneratorTest < Rails::Generators::TestCase end end + def test_skipping_useless_folders_generation_for_api_engines + ['--full', '--mountable'].each do |option| + run_generator [destination_root, option, '--api'] + + assert_no_directory "app/assets" + assert_no_directory "app/helpers" + assert_no_directory "app/views" + + FileUtils.rm_rf destination_root + end + end + + def test_application_controller_parent_for_mountable_api_plugins + run_generator [destination_root, '--mountable', '--api'] + + assert_file "app/controllers/bukkits/application_controller.rb" do |content| + assert_match "ApplicationController < ActionController::API", content + end + end + + def test_dummy_api_application_for_api_plugins + run_generator [destination_root, '--api'] + + assert_file "test/dummy/config/application.rb" do |content| + assert_match "config.api_only = true", content + end + end + + + def test_api_generators_configuration_for_api_engines + run_generator [destination_root, '--full', '--api'] + + assert_file "lib/bukkits/engine.rb" do |content| + assert_match "config.generators.api_only = true", content + end + end + + def test_scaffold_generator_for_mountable_api_plugins + run_generator [destination_root, '--mountable', '--api'] + + capture(:stdout) do + `#{destination_root}/bin/rails g scaffold article` + end + + assert_file "app/models/bukkits/article.rb" + assert_file "app/controllers/bukkits/articles_controller.rb" do |content| + assert_match "only: [:show, :update, :destroy]", content + end + + assert_no_directory "app/assets" + assert_no_directory "app/helpers" + assert_no_directory "app/views" + end + protected def action(*args, &block) silence(:stdout){ generator.send(*args, &block) } |