Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require `belongs_to` by default. #18937

Merged
merged 1 commit into from Feb 23, 2015
Merged

Conversation

@simi
Copy link
Contributor

simi commented Feb 14, 2015

Deprecate required option in favor of optional for belongs_to.

closes #18233

@simi
Copy link
Contributor Author

simi commented Feb 14, 2015

@simi simi force-pushed the simi:optional-belongs-to branch 2 times, most recently Feb 14, 2015
@simi
Copy link
Contributor Author

simi commented Feb 14, 2015

I wonder what to do with failing test.

rails generate scaffold LineItems product:references cart:belongs_to
bundle exec rake db:migrate test`

This code fails now, since belongs_to relation is required by default. Should I change generated test for belongs_to to make it pass or should I remove this test since it is not valid with this change?

@simi
simi reviewed Feb 15, 2015
View changes
activerecord/lib/active_record/associations/builder/belongs_to.rb Outdated

def self.define_validations(model, reflection)
if reflection.options.key?(:required)
ActiveSupport::Deprecation.warn('`required` option for belongs_to relation is deprecated and replaced with `optional` option.')

This comment has been minimized.

Copy link
@simi

simi Feb 15, 2015

Author Contributor

Better info will be needed here. Any ideas?

This comment has been minimized.

Copy link
@dhh

dhh Feb 15, 2015

Member

'required: true is now the default for belongs_to and can be removed.' – don't even need to talk about optional here. What they wanted to do is now being done for them.

This comment has been minimized.

Copy link
@simi

simi Feb 15, 2015

Author Contributor

What about when required: false is specified explicitly? Maybe throw deprecation warning only for false value?

This comment has been minimized.

Copy link
@dhh

dhh Feb 15, 2015

Member

That was the old default, which means no client code will have that code, since it doesn’t make any sense.

On Feb 15, 2015, at 12:54 PM, Josef Šimánek notifications@github.com wrote:

In activerecord/lib/active_record/associations/builder/belongs_to.rb #18937 (comment):

@@ -110,5 +110,24 @@ 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)
    
  •    ActiveSupport::Deprecation.warn('`required` option for belongs_to relation is deprecated and replaced with `optional` option.')
    
    What about when required: false is specified explicitly? Maybe throw deprecation warning only for false value?


Reply to this email directly or view it on GitHub https://github.com/rails/rails/pull/18937/files#r24725940.

This comment has been minimized.

Copy link
@simi

simi Feb 15, 2015

Author Contributor

OK, makes sense.

@simi simi force-pushed the simi:optional-belongs-to branch Feb 15, 2015
@dhh
Copy link
Member

dhh commented Feb 15, 2015

You can kill that test and replace it with a test that tests optional: true.

On Feb 14, 2015, at 3:24 AM, Josef Šimánek notifications@github.com wrote:

I wonder what to do with failing test.

rails generate scaffold LineItems product:references cart:belongs_to
bundle exec rake db:migrate test`
This code fails now, since belongs_to relation is required by default. Should I change generated test for belongs_to to make it pass or should I remove this test since it is not valid with this change?


Reply to this email directly or view it on GitHub #18937 (comment).

@simi
Copy link
Contributor Author

simi commented Feb 18, 2015

Should I add also this to generator? rails g scaffold post author:belongs_to:optional ?

@dhh
Copy link
Member

dhh commented Feb 18, 2015

Nah, don’t think that’s necessary. Will be a very uncommon option to use, I think.

On Feb 18, 2015, at 14:30, Josef Šimánek notifications@github.com wrote:

Should I add also this to generator? rails g scaffold post author:belongs_to:optional ?


Reply to this email directly or view it on GitHub.

@simi simi force-pushed the simi:optional-belongs-to branch 2 times, most recently Feb 18, 2015
@simi
Copy link
Contributor Author

simi commented Feb 18, 2015

OK I think my work is almost done here. I'll add also CHANGELOG note soon.

@dhh
Copy link
Member

dhh commented Feb 18, 2015

Awesome. Ping when ready to merge.

On Feb 18, 2015, at 15:35, Josef Šimánek notifications@github.com wrote:

OK I think my work is almost done here. I'll add also CHANGELOG note soon.


Reply to this email directly or view it on GitHub.

@simi simi force-pushed the simi:optional-belongs-to branch 2 times, most recently Feb 18, 2015
@simi
Copy link
Contributor Author

simi commented Feb 18, 2015

I added CHANGELOG note, but it is a little fuzzy to me. Feel free to ping me with better one.

@dhh
dhh reviewed Feb 18, 2015
View changes
activerecord/CHANGELOG.md Outdated
This can by configure by `config.active_record.require_belongs_to` option
or by `optional: true` option on relation.

*Josef Šimánek*

This comment has been minimized.

Copy link
@dhh

dhh Feb 18, 2015

Member

Let's try with:

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.require_belongs_to = true)

This comment has been minimized.

Copy link
@simi

simi Feb 18, 2015

Author Contributor

Actually the final note is not true right now. config.active_record.require_belongs_to = true is set directly in railtie, not in application config. Should I move it to new generated application.rb and let it nil for existing applications? In that case your note will be true.

@dhh
Copy link
Member

dhh commented Feb 18, 2015

Let's update the config option to be a bit clearer: belongs_to_required_by_default = true

@dhh
Copy link
Member

dhh commented Feb 19, 2015

This is also missing the config/initializers/active_record_belongs_to_required_by_default.rb file that's going to set the belongs_to_required_by_default = true setting by default for new apps.

@simi
Copy link
Contributor Author

simi commented Feb 19, 2015

@dhh
Copy link
Member

dhh commented Feb 19, 2015

Ah, we crossed streams. Didn't note that one. Yes, please do go ahead. Thanks!

@jeremy
jeremy reviewed Feb 19, 2015
View changes
activerecord/lib/active_record/associations/builder/belongs_to.rb Outdated
required = model.require_belongs_to
else
required = !reflection.options[:optional]
end

This comment has been minimized.

Copy link
@jeremy

jeremy Feb 19, 2015

Member

I'm finding the control flow here a little difficult to follow. Consider condensing the conditionals and using the :optional option to determine whether to declare the presence validation:

# Disallow conflicting options
if reflection.options.key?(:required) && reflection.options.key?(:optional)
  raise ArgumentError, "complain if we have conflicting options"
# Deprecate :required
elsif reflection.options.key?(:required)
  reflection.options[:optional] = !reflection.options[:required]
  ActiveSupport::Deprecation.warn "switch to `optional: #{reflection.options[:optional]}`"
# Set default :optional option
elsif !reflection.options.key?(:optional)
  reflection.options[:optional] = !model.require_belongs_to
end

super

if !reflection.options[:optional]
  model.validates_presence_of 
end

This comment has been minimized.

Copy link
@simi

simi Feb 19, 2015

Author Contributor

@jeremy I try to rethink this flow, but deprecation was marked as unnecessary by @dhh here.

if reflection.options[:required]
model.validates_presence_of reflection.name, message: :required
end
end

This comment has been minimized.

Copy link
@jeremy

jeremy Feb 19, 2015

Member

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

This comment has been minimized.

Copy link
@dhh

dhh Feb 19, 2015

Member

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).

@jeremy
jeremy reviewed Feb 19, 2015
View changes
activerecord/lib/active_record/core.rb Outdated
@@ -87,6 +87,8 @@ def self.configurations

mattr_accessor :maintain_test_schema, instance_accessor: false

mattr_accessor :require_belongs_to, instance_accessor: false

This comment has been minimized.

Copy link
@jeremy

jeremy Feb 19, 2015

Member

Out of context, I wouldn't know what this "require belongs to" means. Is there a clearer config name?

This comment has been minimized.

Copy link
@simi

simi Feb 19, 2015

Author Contributor

yup, it is on the way

@jeremy
jeremy reviewed Feb 19, 2015
View changes
activerecord/test/cases/associations/required_test.rb Outdated
@@ -34,7 +34,7 @@ class Child < ActiveRecord::Base

test "required belongs_to associations have presence validated" do
model = subclass_of(Child) do
belongs_to :parent, required: true, inverse_of: false,
belongs_to :parent, optional: false, inverse_of: false,

This comment has been minimized.

Copy link
@jeremy

jeremy Feb 19, 2015

Member

If we still support required: true, we should still test it.

@jeremy
jeremy reviewed Feb 19, 2015
View changes
activerecord/lib/active_record/railtie.rb Outdated
@@ -32,6 +32,7 @@ class Railtie < Rails::Railtie # :nodoc:

config.active_record.use_schema_cache_dump = true
config.active_record.maintain_test_schema = true
config.active_record.require_belongs_to = true

This comment has been minimized.

Copy link
@jeremy

jeremy Feb 19, 2015

Member

Since this is in the Railtie, it'll surprisingly affect apps that upgrade to newer Rails. We'll need to generate new apps with it set, but use false as the default to preserve existing app behavior.

This comment has been minimized.

Copy link
@simi

simi Feb 19, 2015

Author Contributor

sure, it is on the way also

@jeremy
Copy link
Member

jeremy commented Feb 19, 2015

Nice work @simi. Pardon the repeated remarks, looks like I missed concurrent feedback too. Thanks!

@simi
Copy link
Contributor Author

simi commented Feb 19, 2015

Nevermind @jeremy. Thanks for review.

I have just one more question. I made change where config.active_record.require_belongs_to = true is generated to application.rb for new apps. But it is only one uncommented (unless :skip_active_record) configuration in that file. Should I left it there or should it be better to move into custom initializer?

@simi
Copy link
Contributor Author

simi commented Feb 19, 2015

I bet it will be better as initializer after inspecting currently generated initializers.

@simi
Copy link
Contributor Author

simi commented Feb 19, 2015

@jeremy @dhh I pushed new commit (I can squash later, just to highlight new code). config.active_record.belongs_to_required_by_default = true is now part of config/initializers/active_record_belongs_to_required_by_default.rb.

This initializer is generated only when ActiveRecord is not skipped and is not added for rake rails:update. I think this is compatible enough to skip deprecation.

cefigueiredo added a commit to cefigueiredo/rails that referenced this pull request Feb 27, 2017
Since rails#18937 `belongs_to` associations implies `required: true` by
default, with that option replaced for `optional: false`.
cefigueiredo added a commit to cefigueiredo/rails that referenced this pull request Feb 27, 2017
Since rails#18937 `belongs_to` associations receive a setting to determine if
it should be or not treated as `required` by default.

While the tests were still passing, it was not evident that the
"default" behaviour for `required` could change in fuction of a setting,
that is set by default for fresh Rails5 apps, but not for upgraded
apps.

This commit try to relate them to make it clear what is the behaviour
expected when the setting is set as `true` or not set.
@Silex
Copy link

Silex commented Jun 28, 2017

I feel this change was a bit of a mistake at the design level... validations and relations are different things, and now we get validations done when defining relations.

I understand the "it's convenient" argument tho.

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

This comment has been minimized.

Copy link
@Silex

Silex Jun 28, 2017

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

This comment has been minimized.

Copy link
@simi

simi Jun 28, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@Silex

Silex Jun 28, 2017

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.

This comment has been minimized.

Copy link
@Silex

Silex Jun 29, 2017

Ok, this was fixed in 803ef74

kellyChex added a commit to q-centrix/health-data-standards that referenced this pull request Jul 20, 2017
The referenced change for rails 5 is making the belongs_to relationships
break validations for our models. This change makes them optional.

rails/rails#18937
@lekansogunle
Copy link

lekansogunle commented Aug 9, 2017

We are only testing for validation of the presence of the association ID, what if the given ID is invalid. And app behaviour is not to validate the existence of such association?
https://robots.thoughtbot.com/referential-integrity-with-foreign-keys

ajarzadon12 added a commit to ajarzadon12/mongo_followable that referenced this pull request Sep 4, 2017
ajarzadon12 added a commit to ajarzadon12/mongo_followable that referenced this pull request Sep 4, 2017
…is required by default in rails 5. (rails/rails#18937)

Revert all followers to old implementation
schneems added a commit to codetriage/CodeTriage that referenced this pull request Jul 16, 2018
everaldo added a commit to CruGlobal/fe that referenced this pull request Nov 9, 2018
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.
Also deprecate required option in favor of optional for belongs_to.
(Pull Request)

rails/rails#18937
jbirdjavi added a commit to CruGlobal/fe that referenced this pull request Dec 4, 2018
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.
Also deprecate required option in favor of optional for belongs_to.
(Pull Request)

rails/rails#18937
jbirdjavi added a commit to CruGlobal/fe that referenced this pull request Jan 16, 2019
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.
Also deprecate required option in favor of optional for belongs_to.
(Pull Request)

rails/rails#18937
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Apr 15, 2019
`belongs_to` association have `required: true` by default
rails#18937 onwards so we don't need it
in the generator template.
We still need the code for required in the command line generator as
it adds `null: false` in the migration.
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Apr 15, 2019
`belongs_to` association have `required: true` by default
rails#18937 onwards so we don't need it
in the generator template.
We still need the code for required in the command line generator as
it adds `null: false` in the migration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.