Possible bug in == method and solution #151

Closed
joegoggins opened this Issue Apr 22, 2013 · 8 comments

3 participants

@joegoggins

I'm seeing this behavior for records that include a blank space in their primary keys:

rec1.id == rec2.id 
=> true
rec1.attributes == rec2.attributes
=> true
rec1 == rec2 
=> false

This patch appears to fix the problem:

--- base.rb 2013-04-22 16:29:05.000000000 -0500
+++ base.rb.new 2013-04-22 16:29:00.000000000 -0500
@@ -137,7 +137,7 @@
       end

       def ==(comparison_object)
-        ids.is_a?(Array) ? super(comparison_object) && ids.all? {|id| id.present?} : super(comparison_object)
+        ids.is_a?(Array) ? super(comparison_object) && ids.all? {|id| !id.nil?} : super(comparison_object)
       end

The essential change is id.present? to !id.nil?, the latter seems more correct, but I didn't submit a pull request because I wanted to get feedback/see if there could be broader implications to this tiny change. Let me know if there is anything I can do to help/test/explore/etc.

Thanks,

-joe

@cfis

present? allows blanks strings while nil does not. I have seen situations where a column is NOT NULL so it gets filled in with ''

When you say "include a blank space" do you mean " " or "I have a few blanks spaces in my key"

@joegoggins

An example composite key is: ["bla","bla2"," "," ","bla3"], i.e. the value is a single blank space, " ".

@cfis

Ok - so why does this change help then?

@joegoggins

It makes ActiveRecord behave the way one would expect, i.e.:

rec1.id == rec2.id 
=> true
rec1 == rec2 
=> true

Instead of:

rec1.id == rec2.id 
=> true
rec1 == rec2 
=> false

When the id includes a " " in any of it's values.

Using !.nil? treats " " as data rather than as if it were nil (as .present? does).

The legacy system our ActiveRecord classes are encapsulating use " " as data, that's how I encountered this issue in the first place.

Does that make sense?

@cfis

Sorry to have lost track of this issue. I assume you are still having issues with this?

@joegoggins

No problem, thanks for checking in.

I conjured up a work around in the codebase where it was causing issues, so it's not affecting me anymore.
I filed the issue because it seemed like a bug and this subtle change would improve composite primary keys.

If you'd like to make this tweak and merge it great, if not, I understand--totally your call as the maintainer of this tool.

@cfis

Sorry not to have dealt with this sooner. I have merged in this change, thanks!

@cfis cfis closed this Mar 15, 2014
@cfis cfis pushed a commit that referenced this issue Mar 15, 2014
Charlie Savage Change equality check to fix #151. ef86f48
@cfis cfis pushed a commit that referenced this issue Mar 16, 2014
Charlie Savage Fix for #151. cc76758
@seashell-qd

The change makes below testing fail.

FAIL TestEqual#test_new (0.00s)
Expected # to not be equal to #.
/home/yunpeng/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/bundler/gems/composite_primary_keys-ef86f485a230/test/test_equal.rb:7:in `test_new'

I am thinking whether we should always return [nil, nil, ...] as id for new records?

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