belongs_to should default to required: true

[dhh]

Almost every belongs_to declaration seems to be a required association. It's rare that you allow your foreign key to be nil. So let's have required: true be the default and required: false be the optional turn-off.

[simi]

I can take this, but I'm afraid this can cause a lot of harm.

[dhh]

Rocking. I think the main issue will be thinking about whether this has any backwards incompatibility issues. Like if you upgrade an app that already declares validates_presence_of or something. I don’t think it should be a problem, since the validation should just overwrite in that case, I believe. But something to be on the lookout for. 👍

On Dec 28, 2014, at 8:01 PM, Josef Šimánek notifications@github.com wrote:

I can take this.

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

[simi]

My fears were fulfilled :(

For this naive implementation (actually it should be changed somewhere in the reflection):

diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb
index 954ea38..e0edc28 100644
--- a/activerecord/lib/active_record/associations/builder/belongs_to.rb
+++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -23,6 +23,11 @@ module ActiveRecord::Associations::Builder
       add_counter_cache_methods mixin
     end

+    def self.define_validations(model, reflection)
+      reflection.options[:required] ||= true
+      super
+    end
+
     private

     def self.add_counter_cache_methods(mixin)

I'm getting (because unsaved objects) a lot failing tests:

4305 runs, 10485 assertions, 117 failures, 508 errors, 2 skips

Since belongs_to is heavily used in test suite:

grep -r 'belongs_to' test/ | wc -l
505

it is not good idea to add require: false everywhere or construct complex relations in setup phases in tests IMHO.

[al2o3cr]

@simi - the implementation given there is incorrect, since ||= will replace an explicit false with true :(

If we make this the default, we should include a warning that it alone isn't sufficient to maintain referential integrity - it suffers from the same sort of race condition as validates_uniqueness_of. Example:

Thread A                               | Thread B
p = Post.new                           |
p.blog_id = 5                          |
p.save                                 |
  BEGIN                                |
    validation verifies that blog with |
      id=5 exists                      |
                                       |
                                       | Blog.find(5).destroy
    INSERT INTO posts etc.             |
  COMMIT                               |

p.save returns true, but the record is now orphaned since the corresponding row
in the blogs table no longer exists.

I'm not personally a huge fan of options where the default is true, since it leaves "nil is different from false" rakes around for people to step on (as above :) ). Maybe required should eventually be inverted and replaced with optional? Associations that wanted to opt-out of the validation could declare optional: true.

[simi]

@al2o3cr sure, this is really bad implementation. But since there is no usage of required: false in whole test suite, it should not be problem for my arguments.

optional sounds good, but I'm still not sure if this is good default behavior.

[dhh]

I like optional: true 👍

On Dec 29, 2014, at 5:32, Josef Šimánek notifications@github.com wrote:

@al2o3cr sure, this is really bad implementation. But since there is no usage of required: false in whole test suite, it should not be problem for my arguments.

optional sounds good, but I'm still not sure if this is good default behavior.

—
Reply to this email directly or view it on GitHub.

[dhh]

Matt, this is to give you the best error message in the majority of the cases. You’ll still need to back it up with a unique constraint in the DB to prevent what you mention. But such an error would raise an exception, not a validation error. So the less that needs to happen, the better.

On Dec 29, 2014, at 5:23, Matt Jones notifications@github.com wrote:

@simi - the implementation given there is incorrect, since ||= will replace an explicit false with true :(

If we make this the default, we should include a warning that it alone isn't sufficient to maintain referential integrity - it suffers from the same sort of race condition as validates_uniqueness_of. Example:

Thread A | Thread B
p = Post.new |
p.blog_id = 5 |
p.save |
BEGIN |
validation verifies that blog with |
id=5 exists |
|
| Blog.find(5).destroy
INSERT INTO posts etc. |
COMMIT |

p.save returns true, but the record is now orphaned since the corresponding row
in the blogs table no longer exists.

I'm not personally a huge fan of options where the default is true, since it leaves "nil is different from false" rakes around for people to step on (as above :) ). Maybe required should eventually be inverted and replaced with optional? Associations that wanted to opt-out of the validation could declare optional: true.

—
Reply to this email directly or view it on GitHub.

[simi]

I think it will be better to introduce new relation based on belongs_to just with another defaults. I'm not sure about the name. Let me think about it for a while.

[sgrif]

Since we're dropping support for Ruby 1.9, why not just use keyword arguments to set the default?

[sgrif]

This would also constitute a major breaking change for most apps, so we should probably have a deprecation cycle.

[chancancode]

We would need to pair this with a corresponding change in migration generator, correct? We might want to wait till we figured out what we to do with that (hopefully soon)?

[dhh]

This is not worth introducing another name than belongs_to for. For migration purposes, we can have a global config switch like we do in so many other cases. config.active_record.belongs_to_validates_presence = true or whatever.

On Dec 29, 2014, at 10:37, Godfrey Chan notifications@github.com wrote:

We would need to pair this with a corresponding change in migration generator, correct? We might want to wait till we figured out what we to do with that (hopefully soon)?

—
Reply to this email directly or view it on GitHub.

[simi]

Global switch sounds good. But what about AR test suite? Should that switch it be turned off for test suite or should be optional: true or required: false added everywhere? I think it is not good idea to force creating of complex db objects for AR test suite since it will slow it down a lot IMHO.