Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
* `belongs_to` will now trigger a validation error by default if the association is not present.
You can turn this off on a per-association basis with `optional: true`.
(Note this new default only applies to new Rails apps that will be generated with
`config.active_record.belongs_to_required_by_default = true` in initializer.)

*Josef Šimánek*

* Fixed ActiveRecord::Relation#becomes! and changed_attributes issues for type column

Fixes #17139.
Expand Down
8 changes: 7 additions & 1 deletion activerecord/lib/active_record/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1520,10 +1520,16 @@ def has_one(name, scope = nil, options = {})
# object that is the inverse of this <tt>belongs_to</tt> association. Does not work in
# combination with the <tt>:polymorphic</tt> options.
# See ActiveRecord::Associations::ClassMethods's overview on Bi-directional associations for more detail.
# [:optional]
# When set to +true+, the association will not have its presence validated.
# [:required]
# When set to +true+, the association will also have its presence validated.
# This will validate the association itself, not the id. You can use
# +:inverse_of+ to avoid an extra query during validation.
# NOTE: <tt>required</tt> is set to <tt>true</tt> by default and is deprecated. If
# you don't want to have association presence validated, use <tt>optional: true</tt>.
#
#
#
# Option examples:
# belongs_to :firm, foreign_key: "client_of"
Expand All @@ -1536,7 +1542,7 @@ def has_one(name, scope = nil, options = {})
# belongs_to :post, counter_cache: true
# belongs_to :comment, touch: true
# belongs_to :company, touch: :employees_last_updated_at
# belongs_to :user, required: true
# belongs_to :user, optional: true
def belongs_to(name, scope = nil, options = {})
reflection = Builder::BelongsTo.build(self, name, scope, options)
Reflection.add_reflection self, name, reflection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ def self.macro
end

def self.valid_options(options)
super + [:foreign_type, :polymorphic, :touch, :counter_cache]
super + [:foreign_type, :polymorphic, :touch, :counter_cache, :optional]
end

def self.valid_dependent_options
Expand Down Expand Up @@ -110,5 +110,23 @@ def self.add_destroy_callbacks(model, reflection)
name = reflection.name
model.after_destroy lambda { |o| o.association(name).handle_dependency }
end

def self.define_validations(model, reflection)
if reflection.options.key?(:required)
reflection.options[:optional] = !reflection.options.delete(:required)
end

if reflection.options[:optional].nil?
required = model.belongs_to_required_by_default
else
required = !reflection.options[:optional]
end

super

if required
model.validates_presence_of reflection.name, message: :required
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,12 @@ def self.valid_dependent_options
def self.add_destroy_callbacks(model, reflection)
super unless reflection.options[:through]
end

def self.define_validations(model, reflection)
super
if reflection.options[:required]
model.validates_presence_of reflection.name, message: :required
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll be odd to switch to optional: on belongs_to but keep required: for has_one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. belongs_to to me strongly implies a owner/ownee relationship. Doesn't make sense in most cases to say I'm an ownee without a owner. But the other case does make sense. That the owner may or may not have an ownee. (Excuse the made-up word).

end
end
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,5 @@ def create_#{name}!(*args, &block)
end
CODE
end

def self.define_validations(model, reflection)
super
if reflection.options[:required]
model.validates_presence_of reflection.name, message: :required
end
end
end
end
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ def self.configurations

mattr_accessor :maintain_test_schema, instance_accessor: false

mattr_accessor :belongs_to_required_by_default, instance_accessor: false

class_attribute :default_connection_handler, instance_writer: false

def self.connection_handler
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,56 @@ def test_belongs_to_with_primary_key_joins_on_correct_column
end
end

def test_optional_relation
original_value = ActiveRecord::Base.belongs_to_required_by_default
ActiveRecord::Base.belongs_to_required_by_default = true

model = Class.new(ActiveRecord::Base) do
self.table_name = "accounts"
def self.name; "Temp"; end
belongs_to :company, optional: true
end

account = model.new
assert account.valid?
ensure
ActiveRecord::Base.belongs_to_required_by_default = original_value
end

def test_not_optional_relation
original_value = ActiveRecord::Base.belongs_to_required_by_default
ActiveRecord::Base.belongs_to_required_by_default = true

model = Class.new(ActiveRecord::Base) do
self.table_name = "accounts"
def self.name; "Temp"; end
belongs_to :company, optional: false
end

account = model.new
refute account.valid?
assert_equal [{error: :blank}], account.errors.details[:company]
ensure
ActiveRecord::Base.belongs_to_required_by_default = original_value
end

def test_required_belongs_to_config
original_value = ActiveRecord::Base.belongs_to_required_by_default
ActiveRecord::Base.belongs_to_required_by_default = true

model = Class.new(ActiveRecord::Base) do
self.table_name = "accounts"
def self.name; "Temp"; end
belongs_to :company
end

account = model.new
refute account.valid?
assert_equal [{error: :blank}], account.errors.details[:company]
ensure
ActiveRecord::Base.belongs_to_required_by_default = original_value
end

def test_default_scope_on_relations_is_not_cached
counter = 0

Expand Down
5 changes: 5 additions & 0 deletions guides/source/association_basics.md
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,7 @@ The `belongs_to` association supports these options:
* `:polymorphic`
* `:touch`
* `:validate`
* `:optional`

##### `:autosave`

Expand Down Expand Up @@ -956,6 +957,10 @@ end

If you set the `:validate` option to `true`, then associated objects will be validated whenever you save this object. By default, this is `false`: associated objects will not be validated when this object is saved.

##### `:optional`

If you set the `:optional` option to `true`, then associated object will be validated for presence. By default, this is `false`: associated objects will be validated for presence.
Copy link

@Silex Silex Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "then associated object will NOT be validated for presence"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: By default, this is false: associated objects will be validated for presence.

Copy link

@Silex Silex Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about the first part. Look more closely, the sentence says the same thing twice:

If you set the `:optional` option to `true`, then associated object 
will be validated for presence. By default, this is `false`: associated objects 
will be validated for presence.

I took the original sentence and just modified the alignement so it's visually clear they are the same.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this was fixed in 803ef74


#### Scopes for `belongs_to`

There may be times when you wish to customize the query used by `belongs_to`. Such customizations can be achieved via a scope block. For example:
Expand Down
2 changes: 2 additions & 0 deletions guides/source/configuring.md
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ All these configuration options are delegated to the `I18n` library.
`config/environments/production.rb` which is generated by Rails. The
default value is true if this configuration is not set.

* `config.active_record.belongs_to_required_by_default` is a boolean value and controls whether `belongs_to` association is required by default.

The MySQL adapter adds one additional configuration option:

* `ActiveRecord::ConnectionAdapters::MysqlAdapter.emulate_booleans` controls whether Active Record will consider all `tinyint(1)` columns in a MySQL database to be booleans and is true by default.
Expand Down
15 changes: 15 additions & 0 deletions railties/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
* Add `config/initializers/active_record_belongs_to_required_by_default.rb`

Newly generated Rails apps have a new initializer called
`active_record_belongs_to_required_by_default.rb` which sets the value of
the configuration option `config.active_record.belongs_to_requred_by_default`
to `true` when ActiveRecord is not skipped.

As a result, new Rails apps require `belongs_to` association on model
to be valid.

This initializer is *not* added when running `rake rails:update`, so
old apps ported to Rails 5 will work without any change.

*Josef Šimánek*

* `delete` operations in configurations are run last in order to eliminate
'No such middleware' errors when `insert_before` or `insert_after` are added
after the `delete` operation for the middleware being deleted.
Expand Down
11 changes: 11 additions & 0 deletions railties/lib/rails/generators/rails/app/app_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ def config
def config_when_updating
cookie_serializer_config_exist = File.exist?('config/initializers/cookies_serializer.rb')
callback_terminator_config_exist = File.exist?('config/initializers/callback_terminator.rb')
active_record_belongs_to_required_by_default_config_exist = File.exist?('config/initializers/active_record_belongs_to_required_by_default.rb')

config

Expand All @@ -99,6 +100,10 @@ def config_when_updating
unless cookie_serializer_config_exist
gsub_file 'config/initializers/cookies_serializer.rb', /json/, 'marshal'
end

unless active_record_belongs_to_required_by_default_config_exist
remove_file 'config/initializers/active_record_belongs_to_required_by_default.rb'
end
end

def database_yml
Expand Down Expand Up @@ -258,6 +263,12 @@ def delete_assets_initializer_skipping_sprockets
end
end

def delete_active_record_initializers_skipping_active_record
if options[:skip_active_record]
remove_file 'config/initializers/active_record_belongs_to_required_by_default.rb'
end
end

def finish_template
build(:leftovers)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Be sure to restart your server when you modify this file.

# Require `belongs_to` associations by default.
Rails.application.config.active_record.belongs_to_required_by_default = true
5 changes: 4 additions & 1 deletion railties/test/application/rake_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,10 @@ def test_scaffold_tests_pass_by_default
assert_no_match(/Errors running/, output)
end

def test_scaffold_with_references_columns_tests_pass_by_default
def test_scaffold_with_references_columns_tests_pass_when_belongs_to_is_optional
app_file "config/initializers/active_record_belongs_to_required_by_default.rb",
"Rails.application.config.active_record.belongs_to_required_by_default = false"

output = Dir.chdir(app_path) do
`rails generate scaffold LineItems product:references cart:belongs_to;
bundle exec rake db:migrate test`
Expand Down
33 changes: 33 additions & 0 deletions railties/test/generators/app_generator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,38 @@ def test_rails_update_set_the_cookie_serializer_to_marchal_if_it_is_not_already_
assert_file("#{app_root}/config/initializers/cookies_serializer.rb", /Rails\.application\.config\.action_dispatch\.cookies_serializer = :marshal/)
end

def test_rails_update_does_not_create_active_record_belongs_to_required_by_default
app_root = File.join(destination_root, 'myapp')
run_generator [app_root]

FileUtils.rm("#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb")

Rails.application.config.root = app_root
Rails.application.class.stubs(:name).returns("Myapp")
Rails.application.stubs(:is_a?).returns(Rails::Application)

generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell
generator.send(:app_const)
quietly { generator.send(:update_config_files) }
assert_no_file "#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb"
end

def test_rails_update_does_not_remove_active_record_belongs_to_required_by_default_if_already_present
app_root = File.join(destination_root, 'myapp')
run_generator [app_root]

FileUtils.touch("#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb")

Rails.application.config.root = app_root
Rails.application.class.stubs(:name).returns("Myapp")
Rails.application.stubs(:is_a?).returns(Rails::Application)

generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell
generator.send(:app_const)
quietly { generator.send(:update_config_files) }
assert_file "#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb"
end

def test_application_names_are_not_singularized
run_generator [File.join(destination_root, "hats")]
assert_file "hats/config/environment.rb", /Rails\.application\.initialize!/
Expand Down Expand Up @@ -309,6 +341,7 @@ def test_generator_without_skips
def test_generator_if_skip_active_record_is_given
run_generator [destination_root, "--skip-active-record"]
assert_no_file "config/database.yml"
assert_no_file "config/initializers/active_record_belongs_to_required_by_default.rb"
assert_file "config/application.rb", /#\s+require\s+["']active_record\/railtie["']/
assert_file "test/test_helper.rb" do |helper_content|
assert_no_match(/fixtures :all/, helper_content)
Expand Down