aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--actionpack/lib/action_dispatch/http/url.rb57
-rw-r--r--actionpack/lib/action_dispatch/journey/formatter.rb2
-rw-r--r--actionpack/lib/action_dispatch/routing/route_set.rb41
-rw-r--r--actionpack/test/journey/router_test.rb39
-rw-r--r--activemodel/lib/active_model/secure_password.rb4
-rw-r--r--activerecord/CHANGELOG.md20
-rw-r--r--activerecord/lib/active_record/associations/builder/belongs_to.rb2
-rw-r--r--activerecord/lib/active_record/autosave_association.rb5
-rw-r--r--activerecord/test/cases/associations/belongs_to_associations_test.rb7
-rw-r--r--activerecord/test/cases/autosave_association_test.rb13
10 files changed, 109 insertions, 81 deletions
diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb
index c9860af909..a6c17f50a5 100644
--- a/actionpack/lib/action_dispatch/http/url.rb
+++ b/actionpack/lib/action_dispatch/http/url.rb
@@ -30,19 +30,25 @@ module ActionDispatch
end
def url_for(options)
+ unless options[:host] || options[:only_path]
+ raise ArgumentError, 'Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true'
+ end
+
path = options[:script_name].to_s.chomp("/")
path << options[:path].to_s
- result = build_host_url(options)
-
if options[:trailing_slash]
if path.include?('?')
- result << path.sub(/\?/, '/\&')
+ path.sub!(/\?/, '/\&')
else
- result << path.sub(/[^\/]\z|\A\z/, '\&/')
+ path.sub!(/[^\/]\z|\A\z/, '\&/')
end
- else
- result << path
+ end
+
+ result = path
+
+ unless options[:only_path]
+ result.prepend build_host_url(options)
end
if options.key? :params
@@ -61,28 +67,25 @@ module ActionDispatch
private
def build_host_url(options)
- unless options[:host] || options[:only_path]
- raise ArgumentError, 'Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true'
+ if match = options[:host].match(HOST_REGEXP)
+ options[:protocol] ||= match[1] unless options[:protocol] == false
+ options[:host] = match[2]
+ options[:port] = match[3] unless options.key?(:port)
end
- result = ""
+ options[:protocol] = normalize_protocol(options)
+ options[:host] = normalize_host(options)
+ options[:port] = normalize_port(options)
- unless options[:only_path]
- if match = options[:host].match(HOST_REGEXP)
- options[:protocol] ||= match[1] unless options[:protocol] == false
- options[:host] = match[2]
- options[:port] = match[3] unless options.key?(:port)
- end
-
- options[:protocol] = normalize_protocol(options)
- options[:host] = normalize_host(options)
- options[:port] = normalize_port(options)
+ result = options[:protocol]
- result << options[:protocol]
- result << rewrite_authentication(options)
- result << options[:host]
- result << ":#{options[:port]}" if options[:port]
+ if options[:user] && options[:password]
+ result << "#{Rack::Utils.escape(options[:user])}:#{Rack::Utils.escape(options[:password])}@"
end
+
+ result << options[:host]
+ result << ":#{options[:port]}" if options[:port]
+
result
end
@@ -94,14 +97,6 @@ module ActionDispatch
(options[:subdomain] == true || !options.key?(:subdomain)) && options[:domain].nil?
end
- def rewrite_authentication(options)
- if options[:user] && options[:password]
- "#{Rack::Utils.escape(options[:user])}:#{Rack::Utils.escape(options[:password])}@"
- else
- ""
- end
- end
-
def normalize_protocol(options)
case options[:protocol]
when nil
diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb
index 57f0963731..aa8d5a4d79 100644
--- a/actionpack/lib/action_dispatch/journey/formatter.rb
+++ b/actionpack/lib/action_dispatch/journey/formatter.rb
@@ -12,7 +12,7 @@ module ActionDispatch
@cache = nil
end
- def generate(type, name, options, recall = {}, parameterize = nil)
+ def generate(name, options, recall = {}, parameterize = nil)
constraints = recall.merge(options)
missing_keys = []
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index e699419f23..01378ae0ec 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -226,21 +226,23 @@ module ActionDispatch
end
def call(t, args)
- options = t.url_options.merge @options
- hash = handle_positional_args(t, args, options, @segment_keys)
+ controller_options = t.url_options
+ options = controller_options.merge @options
+ hash = handle_positional_args(controller_options, args, options, @segment_keys)
t._routes.url_for(hash)
end
- def handle_positional_args(t, args, result, keys)
+ def handle_positional_args(controller_options, args, result, path_params)
inner_options = args.extract_options!
if args.size > 0
- if args.size < keys.size - 1 # take format into account
- keys -= t.url_options.keys
- keys -= result.keys
+ if args.size < path_params.size - 1 # take format into account
+ path_params -= controller_options.keys
+ path_params -= result.keys
end
- keys -= inner_options.keys
- result.merge!(Hash[keys.zip(args)])
+ path_params.each { |param|
+ result[param] = inner_options[param] || args.shift
+ }
end
result.merge!(inner_options)
@@ -598,7 +600,7 @@ module ActionDispatch
# Generates a path from routes, returns [path, params].
# If no route is generated the formatter will raise ActionController::UrlGenerationError
def generate
- @set.formatter.generate(:path_info, named_route, options, recall, PARAMETERIZE)
+ @set.formatter.generate(named_route, options, recall, PARAMETERIZE)
end
def different_controller?
@@ -671,15 +673,18 @@ module ActionDispatch
RESERVED_OPTIONS.each { |ro| path_options.delete ro }
path, params = generate(path_options, recall)
- params.merge!(options[:params] || {})
-
- ActionDispatch::Http::URL.url_for(options.merge!({
- :path => path,
- :script_name => script_name,
- :params => params,
- :user => user,
- :password => password
- }))
+
+ if options.key? :params
+ params.merge! options[:params]
+ end
+
+ options[:path] = path
+ options[:script_name] = script_name
+ options[:params] = params
+ options[:user] = user
+ options[:password] = password
+
+ ActionDispatch::Http::URL.url_for(options)
end
def call(env)
diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb
index e54b64e0f3..dfd5ed8471 100644
--- a/actionpack/test/journey/router_test.rb
+++ b/actionpack/test/journey/router_test.rb
@@ -160,7 +160,7 @@ module ActionDispatch
]
assert_raises(ActionController::UrlGenerationError) do
- @formatter.generate(:path_info, nil, { :id => '10' }, { })
+ @formatter.generate(nil, { :id => '10' }, { })
end
end
@@ -169,11 +169,11 @@ module ActionDispatch
Router::Strexp.new("/foo/:id", { :id => /\d+/ }, ['/', '.', '?'], false)
]
- path, _ = @formatter.generate(:path_info, nil, { :id => '10' }, { })
+ path, _ = @formatter.generate(nil, { :id => '10' }, { })
assert_equal '/foo/10', path
assert_raises(ActionController::UrlGenerationError) do
- @formatter.generate(:path_info, nil, { :id => 'aa' }, { })
+ @formatter.generate(nil, { :id => 'aa' }, { })
end
end
@@ -182,13 +182,13 @@ module ActionDispatch
Router::Strexp.new("/foo(/:id)", {:id => /\d/}, ['/', '.', '?'], false)
]
- path, _ = @formatter.generate(:path_info, nil, { :id => '10' }, { })
+ path, _ = @formatter.generate(nil, { :id => '10' }, { })
assert_equal '/foo/10', path
- path, _ = @formatter.generate(:path_info, nil, { }, { })
+ path, _ = @formatter.generate(nil, { }, { })
assert_equal '/foo', path
- path, _ = @formatter.generate(:path_info, nil, { :id => 'aa' }, { })
+ path, _ = @formatter.generate(nil, { :id => 'aa' }, { })
assert_equal '/foo/aa', path
end
@@ -199,7 +199,7 @@ module ActionDispatch
@router.routes.add_route nil, path, {}, {}, route_name
error = assert_raises(ActionController::UrlGenerationError) do
- @formatter.generate(:path_info, route_name, { }, { })
+ @formatter.generate(route_name, { }, { })
end
assert_match(/missing required keys: \[:id\]/, error.message)
@@ -287,14 +287,14 @@ module ActionDispatch
def test_required_part_in_recall
add_routes @router, [ "/messages/:a/:b" ]
- path, _ = @formatter.generate(:path_info, nil, { :a => 'a' }, { :b => 'b' })
+ path, _ = @formatter.generate(nil, { :a => 'a' }, { :b => 'b' })
assert_equal "/messages/a/b", path
end
def test_splat_in_recall
add_routes @router, [ "/*path" ]
- path, _ = @formatter.generate(:path_info, nil, { }, { :path => 'b' })
+ path, _ = @formatter.generate(nil, { }, { :path => 'b' })
assert_equal "/b", path
end
@@ -304,7 +304,7 @@ module ActionDispatch
"/messages/:id(.:format)"
]
- path, _ = @formatter.generate(:path_info, nil, { :id => 10 }, { :action => 'index' })
+ path, _ = @formatter.generate(nil, { :id => 10 }, { :action => 'index' })
assert_equal "/messages/index/10", path
end
@@ -315,7 +315,7 @@ module ActionDispatch
params = { :controller => "tasks", :format => nil }
extras = { :action => 'lol' }
- path, _ = @formatter.generate(:path_info, nil, params, extras)
+ path, _ = @formatter.generate(nil, params, extras)
assert_equal '/tasks', path
end
@@ -327,7 +327,7 @@ module ActionDispatch
@router.routes.add_route @app, path, {}, {}, {}
- path, _ = @formatter.generate(:path_info, nil, Hash[params], {})
+ path, _ = @formatter.generate(nil, Hash[params], {})
assert_equal '/', path
end
@@ -340,7 +340,6 @@ module ActionDispatch
[:action, "show"] ]
@formatter.generate(
- :path_info,
nil,
Hash[params],
{},
@@ -354,7 +353,7 @@ module ActionDispatch
@router.routes.add_route @app, path, {}, {}, {}
path, params = @formatter.generate(
- :path_info, nil, {:id=>1, :controller=>"tasks", :action=>"show"}, {})
+ nil, {:id=>1, :controller=>"tasks", :action=>"show"}, {})
assert_equal '/tasks/show', path
assert_equal({:id => 1}, params)
end
@@ -363,8 +362,8 @@ module ActionDispatch
path = Path::Pattern.new '/:controller(/:action)'
@router.routes.add_route @app, path, {}, {}, {}
- path, _ = @formatter.generate(:path_info,
- nil, { :controller => "tasks",
+ path, _ = @formatter.generate(nil,
+ { :controller => "tasks",
:action => "a/b c+d",
}, {})
assert_equal '/tasks/a%2Fb%20c+d', path
@@ -374,7 +373,7 @@ module ActionDispatch
path = Path::Pattern.new '/:controller(/:action)'
@router.routes.add_route @app, path, {}, {}, {}
- path, _ = @formatter.generate(:path_info,
+ path, _ = @formatter.generate(
nil, { :controller => "admin/tasks",
:action => "a/b c+d",
}, {})
@@ -385,7 +384,7 @@ module ActionDispatch
path = Path::Pattern.new '/:controller(/:action)'
@router.routes.add_route @app, path, {}, {}, {}
- path, params = @formatter.generate(:path_info,
+ path, params = @formatter.generate(
nil, { :id => 1,
:controller => "tasks",
:action => "show",
@@ -399,7 +398,7 @@ module ActionDispatch
path = Path::Pattern.new '/:controller(/:action(/:id))'
@router.routes.add_route @app, path, {}, {}, {}
- path, params = @formatter.generate(:path_info,
+ path, params = @formatter.generate(
nil,
{:controller =>"tasks", :id => 10},
{:action =>"index"})
@@ -411,7 +410,7 @@ module ActionDispatch
path = Path::Pattern.new '/:controller(/:action)'
@router.routes.add_route @app, path, {}, {}, {}
- path, params = @formatter.generate(:path_info,
+ path, params = @formatter.generate(
"tasks",
{:controller=>"tasks"},
{:controller=>"tasks", :action=>"index"})
diff --git a/activemodel/lib/active_model/secure_password.rb b/activemodel/lib/active_model/secure_password.rb
index 0c3ed9e8ca..4033eb5808 100644
--- a/activemodel/lib/active_model/secure_password.rb
+++ b/activemodel/lib/active_model/secure_password.rb
@@ -52,8 +52,6 @@ module ActiveModel
raise
end
- attr_reader :password
-
include InstanceMethodsOnActivation
if options.fetch(:validations, true)
@@ -92,6 +90,8 @@ module ActiveModel
BCrypt::Password.new(password_digest) == unencrypted_password && self
end
+ attr_reader :password
+
# Encrypts the password into the +password_digest+ attribute, only if the
# new password is not blank.
#
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 55a3cbfd60..283ca33913 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
+* Change belongs_to touch to be consistent with timestamp updates
+
+ If a model is set up with a belongs_to: touch relatinoship the parent
+ record will only be touched if the record was modified. This makes it
+ consistent with timestamp updating on the record itself.
+
+ *Brock Trappitt*
+
* Fixed the inferred table name of a HABTM auxiliar table inside a schema.
Fixes #14824
@@ -434,12 +442,6 @@
*Cody Cutrer*, *Steve Rice*, *Rafael Mendonça Franca*
-* Save `has_one` association even if the record doesn't changed.
-
- Fixes #14407.
-
- *Rafael Mendonça França*
-
* Use singular table name in generated migrations when
`ActiveRecord::Base.pluralize_table_names` is `false`.
@@ -518,6 +520,12 @@
*Troy Kruthoff*, *Lachlan Sylvester*
+* Only save has_one associations if record has changes.
+ Previously after save related callbacks, such as `#after_commit`, were triggered when the has_one
+ object did not get saved to the db.
+
+ *Alan Kennedy*
+
* Allow strings to specify the `#order` value.
Example:
diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb
index 47cc1f4b34..3998aca23e 100644
--- a/activerecord/lib/active_record/associations/builder/belongs_to.rb
+++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -103,7 +103,7 @@ module ActiveRecord::Associations::Builder
BelongsTo.touch_record(record, foreign_key, n, touch)
}
- model.after_save callback
+ model.after_save callback, if: :changed?
model.after_touch callback
model.after_destroy callback
end
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 80cf7572df..1a4d2957ec 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -381,15 +381,16 @@ module ActiveRecord
def save_has_one_association(reflection)
association = association_instance_get(reflection.name)
record = association && association.load_target
+
if record && !record.destroyed?
autosave = reflection.options[:autosave]
if autosave && record.marked_for_destruction?
record.destroy
- else
+ elsif autosave != false
key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id
- if autosave != false && (autosave || new_record? || record_changed?(reflection, record, key))
+ if (autosave && record.changed_for_autosave?) || new_record? || record_changed?(reflection, record, key)
unless reflection.through_reflection
record[reflection.foreign_key] = key
end
diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb
index 3b484a0d64..dc7314b450 100644
--- a/activerecord/test/cases/associations/belongs_to_associations_test.rb
+++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -369,6 +369,13 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
assert_queries(2) { line_item.update amount: 10 }
end
+ def test_belongs_to_with_touch_option_on_empty_update
+ line_item = LineItem.create!
+ Invoice.create!(line_items: [line_item])
+
+ assert_queries(0) { line_item.save }
+ end
+
def test_belongs_to_with_touch_option_on_destroy
line_item = LineItem.create!
Invoice.create!(line_items: [line_item])
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index f7584c3a51..09892d50ba 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -683,10 +683,23 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
end
end
+ @ship.pirate.catchphrase = "Changed Catchphrase"
+
assert_raise(RuntimeError) { assert !@pirate.save }
assert_not_nil @pirate.reload.ship
end
+ def test_should_save_changed_has_one_changed_object_if_child_is_saved
+ @pirate.ship.name = "NewName"
+ assert @pirate.save
+ assert_equal "NewName", @pirate.ship.reload.name
+ end
+
+ def test_should_not_save_changed_has_one_unchanged_object_if_child_is_saved
+ @pirate.ship.expects(:save).never
+ assert @pirate.save
+ end
+
# belongs_to
def test_should_destroy_a_parent_association_as_part_of_the_save_transaction_if_it_was_marked_for_destroyal
assert !@ship.pirate.marked_for_destruction?
generated by cgit v1.2.3 (git 2.25.1) at 2024-10-14 13:49:33 +0000