Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Inconsistencies with MRI regarding hashes and arrays affecting rails #411

Closed
jvshahid opened this Issue · 12 comments

3 participants

@jvshahid

Hi there,

I'm currently working on a drop in replacement gem for ruby-pg under JRuby. While running the active record test suite on the gem, I realized the following inconsistencies between JRuby and MRI:

1) Let's start with the easy one less important:

#!/usr/bin/env ruby

hash = {:foo => :bar}
hash.freeze
hash[:foo] = ":bazz"

Under MRI this code will throw the following error:

./test_hash.rb:5:in `[]=': can't modify frozen Hash (RuntimeError)
    from ./test_hash.rb:5:in `<main>'

Under JRuby it will throw the following:

RuntimeError: can't modify frozen hash
     []= at org/jruby/RubyHash.java:903
  (root) at ./test_hash.rb:5

What's relevant here is the case of Hash vs. hash. Surprisingly, there's a test in rails that test the actual error message and it's not case insensitive. I guess the question now is what's the right course of action here.

2) This is more relevant, since it affects the behavior of active record's associations behavior under JRuby, which is currently faulty for the following reasons:

First the bug, is in Array::delete. Although the ruby docs say that Array::delete(obj) will return obj if an object was found (and deleted) that's equal to obj. But the actual implementation returns the last object in the array that was equal to obj instead of obj (as seen in the source code and docs here). On the other hand JRuby do the right thing and return obj. I know, I said JRuby is doing the right thing, but it's still a difference between the two implementations and MRI is the defacto standard. I also don't know what's the right course of action which is why I'm posting this here to discuss.

A test case to explain the difference in behavior is:

#!/usr/bin/env ruby

class Foo
  attr_reader :name, :age

  def initialize name, age
    @name = name
    @age = age
  end

  def == other
    other.name == name
  end
end

foo1 = Foo.new "John Shahid", 27
foo2 = Foo.new "John Shahid", 28
array = [foo1]
temp = array.delete foo2
puts "#{temp.age} should equal 28"

Output under MRI: 27 should equal 28
Under JRuby: 28 should equal 28

In case you were wondering where this is relevant or someone had the same problem in rails. The problem is in activerecord/lib/active_record/associations/collection_association.rb:422 where the code tries to merge in memory records that could possibly be modified with records that were just retrieved from the db (as shown below). In MRI mem_record will hold the memory record since we're deleting from the memory array, while JRuby will return the object we're trying to delete which is the persisted record. The symptom of this bug is that you loose your memory modifications if you don't save the record to the db, since everytime you merge the persisted data will override what's in memory.

persisted.map! do |record|
  if mem_record = memory.delete(record)
    puts "record.attribute_names & mem_record.attribute_names: #{record.attribute_names & mem_record.attribute_names}"
    puts "mem_record.changes: #{mem_record.changes.keys}"

    ((record.attribute_names & mem_record.attribute_names) - mem_record.changes.keys).each do |name|
      mem_record[name] = record[name]
    end

    mem_record
  else
    record
  end
end

Sorry for this lengthy issue, I tried to be thorough as much as I can. I'm happy to write the specs in ruby-specs and send a patch to make the spec pass. I don't know which implementation should be fixed and want everyone to be on the same page (including the MRI guys if this will involve a change in their implementation) before I go on.

@BanzaiMan
Owner

Please avoid aggregating disparate issues into one ticket. It makes discussion really confusing. I can fixed the first issue (since it's a simple fix to align error message more closely) with aa2e98c, so there is no need to split this one.

@jvshahid

I apologize for aggregating the issues and thanks for the fix. Should we add a spec for this case in ruby spec or it's not worth the effort ?

@BanzaiMan
Owner

It's not necessary. It is a bad idea to rely on error message, but I guess there is no distinction between this kind of RuntimeError (attempts to modify frozen object, a Hash) and others.

@BanzaiMan BanzaiMan closed this issue from a commit
@BanzaiMan BanzaiMan Array#delete should delete the first object *inside* the array,
not the one that is given as the argument to it, even when the objects
are equal via '=='.

This fixes #411.
fe138bf
@BanzaiMan BanzaiMan closed this in fe138bf
@jvshahid

Thanks for fixing those bugs.

I have a problem though. The reason I opened this issue is to discuss which implementation is wrong and try to fix it. The changes you pushed aren't tested which means that someone can change this behavior without knowing in the future.

The second issue, is that the ruby documentation is not right, so either MRI has to change to match the docs or JRuby has to change its behavior (which is the path you chose) and the docs have to be fixed.

@BanzaiMan
Owner

@jvshahid Oh. I see what you are saying now. I apologize for the confusion. I think this is an MRI bug, after all. I'm going to revert the changes, and open a bug with MRI.

@BanzaiMan BanzaiMan reopened this
@BanzaiMan BanzaiMan was assigned
@BanzaiMan
Owner

By the way, the test should go into RubySpec.

@headius
Owner

Looks like the behavior change was explicit and intentional, so we'll need to follow suit with MRI here.

@BanzaiMan
Owner

I expect MRI to retain its current behavior and correct the documentation. Once we have that confirmed, I will re-apply the fix, and propose the spec to RubySpec (or inherit MRI's new test).

@BanzaiMan BanzaiMan referenced this issue from a commit
@BanzaiMan BanzaiMan Array#delete should return the last object *inside* the array, not th…
…e one that is given as the argument to it, even when the objects are equal via '=='.

This addresses #411.
843bd78
@BanzaiMan
Owner

Re-pushed the fix. Spec will be pushed to RubySpec.

@BanzaiMan BanzaiMan closed this
@jvshahid

Thanks.

@prathamesh-sonpatki prathamesh-sonpatki referenced this issue from a commit in prathamesh-sonpatki/jruby
@BanzaiMan BanzaiMan Array#delete should delete the first object *inside* the array,
not the one that is given as the argument to it, even when the objects
are equal via '=='.

This fixes #411.
ff36ddb
@prathamesh-sonpatki prathamesh-sonpatki referenced this issue from a commit in prathamesh-sonpatki/jruby
@BanzaiMan BanzaiMan Regression test for #411, based on test given by @jvshahid 2f71c08
@prathamesh-sonpatki prathamesh-sonpatki referenced this issue from a commit in prathamesh-sonpatki/jruby
@BanzaiMan BanzaiMan Revert "Regression test for #411, based on test given by @jvshahid"
This reverts commit 5916a45.
1cb30e8
@prathamesh-sonpatki prathamesh-sonpatki referenced this issue from a commit in prathamesh-sonpatki/jruby
@BanzaiMan BanzaiMan Array#delete should return the last object *inside* the array, not th…
…e one that is given as the argument to it, even when the objects are equal via '=='.

This addresses #411.
4bd997f
@martinott martinott referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@martinott martinott referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@martinott martinott referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@martinott martinott referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.