Skip to content
This repository

Backport 4bc2ae0 to fix #7950 #7983

Merged
merged 2 commits into from over 1 year ago

5 participants

George Brocklehurst Carlos Antonio da Silva Rafael Mendonça França Francesco Rodríguez Aaron Patterson
George Brocklehurst

An unsaved parent that has_many children shouldn't say that its children include any child with a nil foreign key:

child = Child.create(parent_id: nil)
Parent.new.children.include?(child) # => false

Adds a test and fixes it by backporting 4bc2ae0.

See #7950 for the original issue, and #7959 for an attempted fix (before I realised it was already fixed on master) and discussion of backporting.

Carlos Antonio da Silva

Looks like a big change to do in 3-2-stable :), it seems fine but I'd like to confirm with @tenderlove if that's ok to backport.

Also, we'd probably push the same test to master to ensure the behavior won't break anymore there. Thanks!

George Brocklehurst

Thanks Carlos. I assume we just wait for Aaron to OK the patch for 3-2-stable, or if it's too big a change to backport we live with the bug until 4.0? Let me know if there's anything more I can do.

Carlos Antonio da Silva

@georgebrock hey, I talked to @tenderlove and looks ok to backport, but I'd like to ask you to do two small things: Can you cherry pick his commit first, than add your commit with the test later separately? And together with your commit, you can add a changelog entry to Active Record explaining the fix. Let me know if you need anything, thanks!

George Brocklehurst

@carlosantoniodasilva Thanks. Commits are un-squashed, rebased onto the latest 3-2-stable, and I've added a changelog entry.

activerecord/CHANGELOG.md
... ...
@@ -142,6 +142,12 @@
142 142
 
143 143
     *Seamus Abshere*
144 144
 
  145
+*   Calling `include?` on `has_many` associations on unsaved records no longer
2
Francesco Rodríguez Collaborator
frodsan added a note October 30, 2012

New CHANGELOG entries go to the top :)

Thanks @frodsan, I've updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carlos Antonio da Silva

@georgebrock thanks! I'll merge it after 3.2.9 is released (it can't go now because the rc is out).

Carlos Antonio da Silva

@georgebrock github says it cannot be merged automatically anymore, probably a changelog conflict. Can you rebase please? Thanks.

George Brocklehurst

It was just a changelog conflict. Rebased, tests still passing.

Carlos Antonio da Silva

Well, github still says it cannot be automatically merged :smile:.

Also, thinking a little bit about the change, I'm afraid it could break other people's code relying on this. Hopefully not.

and others added some commits February 27, 2012
Aaron Patterson use bind values for join columns
This is a backport of 4bc2ae0.
It fixes bug #7950.

Conflicts:

	activerecord/lib/active_record/relation/calculations.rb
	activerecord/lib/active_record/relation/finder_methods.rb
a3cf03e
George Brocklehurst Test/changelog for has_many bug on unsaved records
See issue #7950.

The previous commit fixes this bug, and is a backport of
4bc2ae0.
b6a2bae
George Brocklehurst

Oops, I rebased onto 3-2-stable from my fork, not from upstream. Sorry about that.

If you think there's a big risk of breaking people's apps we can drop it: Probably better to break things in a major release than a minor patch.

Carlos Antonio da Silva

Now it seems ok to merge. @tenderlove gave an :ok: to backport, and we have backported other similar things like count returning 0 for new records. I think it's probably fine, but I'll just want to get someone else's to review it before merging, thanks!

/cc @josevalim @rafaelfranca

Rafael Mendonça França rafaelfranca commented on the diff November 12, 2012
activerecord/lib/active_record/associations/association_scope.rb
@@ -67,7 +79,10 @@ def add_constraints(scope)
67 79
           conditions = self.conditions[i]
68 80
 
69 81
           if reflection == chain.last
70  
-            scope = scope.where(table[key].eq(owner[foreign_key]))
  82
+            column = column_for(table.table_name, key.to_s)
  83
+            bind_val = bind(scope, column, owner[foreign_key])
  84
+            scope = scope.where(table[key].eq(bind_val))
  85
+            #scope = scope.where(table[key].eq(owner[foreign_key]))
1
Rafael Mendonça França Owner

We have commented code here. If it is not needed please remove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rafael Mendonça França

Looks good to me.

Carlos Antonio da Silva carlosantoniodasilva merged commit b6a2bae into from November 16, 2012
Carlos Antonio da Silva carlosantoniodasilva closed this November 16, 2012
Carlos Antonio da Silva

@georgebrock np, thank you :)

Carlos Antonio da Silva

So, this is apparently giving us some failures under mysql2 that I couldn't figure out at a glance, @georgebrock can you take a look? Otherwise I'll have to revert soon if we can't check what happened, thanks!

George Brocklehurst

Sorry @carlosantoniodasilva, I didn't think to test against Ruby 1.8.7. There's a simple fix for this bug, it's already in master: 08477a6

Carlos Antonio da Silva

@georgebrock thanks. Apparently backporting that commit does make the failing tests pass. I'll just ping @jeremy to ensure I'm not missing anything.

Also, no worries about 1.8.7, the tests fail against all versions in Travis :).

George Brocklehurst

Oh, so they do. They didn't fail for me before the merge on 1.9.3 and I only read the Travis output for 1.8.7 so I assumed the Ruby version was related :)

Carlos Antonio da Silva carlosantoniodasilva referenced this pull request from a commit January 15, 2013
Carlos Antonio da Silva Revert "Merge pull request #7983 from georgebrock/bug7950-squashed"
This reverts commit 88a296d, reversing
changes made to 666a7e3.

Conflicts:
	activerecord/CHANGELOG.md

Reason: this has been resulting in some hard to track bugs and is
introducing a possible breackage in a stable version.
da5e5c5
Carlos Antonio da Silva carlosantoniodasilva referenced this pull request from a commit January 15, 2013
Carlos Antonio da Silva Merge branch 'revert-binds-3-2' into 3-2-stable
This has been resulting in some hard to track bugs and is introducing
a possible breackage in a stable version. The issue it currently "fixes"
should be handled in some other way.

Closes #8743. Related to #7983.
63970dc
Carlos Antonio da Silva

Unfortunately this has been reverted due to causing too many issues in a stable branch, so it's safer to not ship this code in the next release. Please take a look in some other solution to fix the issue if you want, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 2 authors.

Nov 10, 2012
Aaron Patterson use bind values for join columns
This is a backport of 4bc2ae0.
It fixes bug #7950.

Conflicts:

	activerecord/lib/active_record/relation/calculations.rb
	activerecord/lib/active_record/relation/finder_methods.rb
a3cf03e
George Brocklehurst Test/changelog for has_many bug on unsaved records
See issue #7950.

The previous commit fixes this bug, and is a backport of
4bc2ae0.
b6a2bae
This page is out of date. Refresh to see the latest.
6  activerecord/CHANGELOG.md
Source Rendered
... ...
@@ -1,5 +1,11 @@
1 1
 ## Rails 3.2.10 (unreleased)
2 2
 
  3
+*   Calling `include?` on `has_many` associations on unsaved records no longer
  4
+    returns `true` when passed a record with a `nil` foreign key.
  5
+    Fixes #7950.
  6
+
  7
+    *George Brocklehurst*
  8
+
3 9
 *   `AR::Base#attributes_before_type_cast` now returns unserialized values for serialized attributes.
4 10
 
5 11
     *Nikita Afanasenko*
17  activerecord/lib/active_record/associations/association_scope.rb
@@ -33,6 +33,18 @@ def scope
33 33
 
34 34
       private
35 35
 
  36
+      def column_for(table_name, column_name)
  37
+        columns = alias_tracker.connection.schema_cache.columns_hash[table_name]
  38
+        columns[column_name]
  39
+      end
  40
+
  41
+      def bind(scope, column, value)
  42
+        substitute = alias_tracker.connection.substitute_at(
  43
+          column, scope.bind_values.length)
  44
+        scope.bind_values += [[column, value]]
  45
+        substitute
  46
+      end
  47
+
36 48
       def add_constraints(scope)
37 49
         tables = construct_tables
38 50
 
@@ -67,7 +79,10 @@ def add_constraints(scope)
67 79
           conditions = self.conditions[i]
68 80
 
69 81
           if reflection == chain.last
70  
-            scope = scope.where(table[key].eq(owner[foreign_key]))
  82
+            column = column_for(table.table_name, key.to_s)
  83
+            bind_val = bind(scope, column, owner[foreign_key])
  84
+            scope = scope.where(table[key].eq(bind_val))
  85
+            #scope = scope.where(table[key].eq(owner[foreign_key]))
71 86
 
72 87
             if reflection.type
73 88
               scope = scope.where(table[reflection.type].eq(owner.class.base_class.name))
8  activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -20,14 +20,14 @@ def select_all(arel, name = nil, binds = [])
20 20
 
21 21
       # Returns a record hash with the column names as keys and column values
22 22
       # as values.
23  
-      def select_one(arel, name = nil)
24  
-        result = select_all(arel, name)
  23
+      def select_one(arel, name = nil, binds = [])
  24
+        result = select_all(arel, name, binds)
25 25
         result.first if result
26 26
       end
27 27
 
28 28
       # Returns a single value from a record
29  
-      def select_value(arel, name = nil)
30  
-        if result = select_one(arel, name)
  29
+      def select_value(arel, name = nil, binds = [])
  30
+        if result = select_one(arel, name, binds)
31 31
           result.values.first
32 32
         end
33 33
       end
7  activerecord/lib/active_record/relation.rb
@@ -464,7 +464,12 @@ def where_values_hash
464 464
         node.left.relation.name == table_name
465 465
       }
466 466
 
467  
-      Hash[equalities.map { |where| [where.left.name, where.right] }]
  467
+      binds = Hash[bind_values.find_all(&:first).map { |column, v| [column.name, v] }]
  468
+
  469
+      Hash[equalities.map { |where|
  470
+        name = where.left.name
  471
+        [name, binds.fetch(name.to_s) { where.right }]
  472
+      }]
468 473
     end
469 474
 
470 475
     def scope_for_create
7  activerecord/lib/active_record/relation/calculations.rb
@@ -178,7 +178,7 @@ def calculate(operation, column_name, options = {})
178 178
     #
179 179
     def pluck(column_name)
180 180
       column_name = column_name.to_s
181  
-      klass.connection.select_all(select(column_name).arel).map! do |attributes|
  181
+      klass.connection.select_all(select(column_name).arel, nil, bind_values).map! do |attributes|
182 182
         klass.type_cast_attribute(attributes.keys.first, klass.initialize_attributes(attributes))
183 183
       end
184 184
     end
@@ -240,7 +240,8 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
240 240
         query_builder = relation.arel
241 241
       end
242 242
 
243  
-      type_cast_calculated_value(@klass.connection.select_value(query_builder), column_for(column_name), operation)
  243
+      result = @klass.connection.select_value(query_builder, nil, relation.bind_values)
  244
+      type_cast_calculated_value(result, column_for(column_name), operation)
244 245
     end
245 246
 
246 247
     def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
@@ -286,7 +287,7 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
286 287
       relation = except(:group).group(group)
287 288
       relation.select_values = select_values
288 289
 
289  
-      calculated_data = @klass.connection.select_all(relation)
  290
+      calculated_data = @klass.connection.select_all(relation, nil, bind_values)
290 291
 
291 292
       if association
292 293
         key_ids     = calculated_data.collect { |row| row[group_aliases.first] }
4  activerecord/lib/active_record/relation/finder_methods.rb
@@ -199,7 +199,7 @@ def exists?(id = false)
199 199
         relation = relation.where(table[primary_key].eq(id)) if id
200 200
       end
201 201
 
202  
-      connection.select_value(relation, "#{name} Exists") ? true : false
  202
+      connection.select_value(relation, "#{name} Exists", relation.bind_values) ? true : false
203 203
     rescue ThrowResult
204 204
       false
205 205
     end
@@ -332,7 +332,7 @@ def find_one(id)
332 332
 
333 333
       substitute = connection.substitute_at(column, @bind_values.length)
334 334
       relation = where(table[primary_key].eq(substitute))
335  
-      relation.bind_values = [[column, id]]
  335
+      relation.bind_values += [[column, id]]
336 336
       record = relation.first
337 337
 
338 338
       unless record
5  activerecord/lib/active_record/relation/spawn_methods.rb
@@ -22,7 +22,7 @@ def merge(r)
22 22
         end
23 23
       end
24 24
 
25  
-      (Relation::MULTI_VALUE_METHODS - [:joins, :where, :order]).each do |method|
  25
+      (Relation::MULTI_VALUE_METHODS - [:joins, :where, :order, :binds]).each do |method|
26 26
         value = r.send(:"#{method}_values")
27 27
         merged_relation.send(:"#{method}_values=", merged_relation.send(:"#{method}_values") + value) if value.present?
28 28
       end
@@ -31,6 +31,8 @@ def merge(r)
31 31
 
32 32
       merged_wheres = @where_values + r.where_values
33 33
 
  34
+      merged_binds = (@bind_values + r.bind_values).uniq(&:first)
  35
+
34 36
       unless @where_values.empty?
35 37
         # Remove duplicates, last one wins.
36 38
         seen = Hash.new { |h,table| h[table] = {} }
@@ -47,6 +49,7 @@ def merge(r)
47 49
       end
48 50
 
49 51
       merged_relation.where_values = merged_wheres
  52
+      merged_relation.bind_values = merged_binds
50 53
 
51 54
       (Relation::SINGLE_VALUE_METHODS - [:lock, :create_with, :reordering]).each do |method|
52 55
         value = r.send(:"#{method}_value")
7  activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1225,6 +1225,13 @@ def test_included_in_collection
1225 1225
     assert companies(:first_firm).clients.include?(Client.find(2))
1226 1226
   end
1227 1227
 
  1228
+  def test_included_in_collection_for_new_records
  1229
+    client = Client.create(:name => 'Persisted')
  1230
+    assert_nil client.client_of
  1231
+    assert !Firm.new.clients_of_firm.include?(client),
  1232
+           'includes a client that does not belong to any firm'
  1233
+  end
  1234
+
1228 1235
   def test_adding_array_and_collection
1229 1236
     assert_nothing_raised { Firm.find(:first).clients + Firm.find(:all).last.clients }
1230 1237
   end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.