Skip to content

Commit

Permalink
Require belongs_to by default.
Browse files Browse the repository at this point in the history
Deprecate `required` option in favor of `optional` for belongs_to.
  • Loading branch information
simi committed Feb 21, 2015
1 parent bab3c7c commit 6576f73
Show file tree
Hide file tree
Showing 14 changed files with 166 additions and 10 deletions.
7 changes: 7 additions & 0 deletions activerecord/CHANGELOG.md
@@ -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
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
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
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
end
end
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
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
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
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.

#### 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
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
@@ -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
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
@@ -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
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
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

0 comments on commit 6576f73

Please sign in to comment.