Prevent DirtyMinder from attempting to wrap nil values.

[emmanuel]

This happens when a resource is lazy-loading a property that includes DirtyMinder: Resource#eager_load iterates over the set of properties to lazy-load and calls Property#set with nil for each before calling Collection#lazy_load. The Property#set call blows up when given nil because DirtyMinder tries to add instance variables to nil in DirtyMinder#wrap_value—@resource & @property. nil, of course, is a frozen object and thus the attempt to add instance variables blows up.

Do the research if you don't believe me :P. Seriously, please do; I'd love to be proven wrong about this. Anyways, nil can't be mutated, thus I don't think anyone cares about tracking nil, so this fix/work-around should not impact functionality at all.

/cc @jpr5

[dkubb]

I think you're right about no one wanting to track nil.

However I don't think nil is frozen. It may appear that way in 1.9, since doing nil.freeze will flip nil.frozen? from false to true (globally. sigh) Does that mean there also needs to be a value.nil? test in there too?

Would it be possible to have some failing specs for this change?

[jpr5]

When you first brought this up ~3 weeks ago (right?) it felt like something abnormal was lurking -- a real property value that shouldn't have been frozen but was, or something janky about the ruby version, or whatever. My first thought was "ooh, that sucks that it broke", but immediately following was "ooh, that was a useful canary, I wonder what it will turn out to be".

I'm in favor of solving the problem - don't hook nil - and keeping the canary (blow up if it's unexpectedly frozen, or for any other reason). At least we could still learn more about any further hidden implications with this approach.

Otherwise, unnecessarily broadening the scope of what doesn't get hooked opens the door to hidden doesn't-do-what-is-expected scenarios, which DirtyMinder was attempting to fix in the first place.

In short, I propose the additional test should be for value.nil? instead of value.frozen?.

[dkubb]

@jpr5 agreed

[emmanuel]

Agreed on all counts—it was ~3 weeks ago I first reported this issue, it was a useful canary which helped to reveal something interesting, and I agree that checking nil? instead of frozen? is a more specific (and better) test.

In my app—running on 1.9—the frozen? check fixed things. I clearly did not understand the behavior of NilClass#frozen? on 1.8 or 1.9, because now that you mention it, I checked 1.8 and 1.9 in IRB, and nil.frozen? returns false in both cases. Something in my app is freezing nil, but I can't be bothered to track down what that might be.

[emmanuel]

Oh yeah, I totally glossed over @dkubb's request for failing specs.

sigh, yes I suppose I can add some coverage to help prevent regression of this obscure failure in the future :P.

[jpr5]

@emmanuel Did this happen?

[emmanuel]

Just got back from a ~2wk vacation, hasn't happened yet. I'll try to make time this week to add some tests.

On Sep 8, 2011, at 10:51 PM, Jordan Ritter wrote:

@emmanuel Did this happen?

Reply to this email directly or view it on GitHub:
#41 (comment)

[solnic]

@jpr5 @emmanuel tl;dr can sombody sum up the status of this pull request? :)

[emmanuel]

This pull request fixes a bug with lazy-loaded complex types (Hash, Array, JSON, etc), but it doesn't include any tests for the fix.

A couple of tests need to be written to prove the fix (it works in my app) and prevent future regression.

On Sep 14, 2011, at 2:10 AM, Piotr Solnica wrote:

@jpr5 @emmanuel tl;dr can sombody sum up the status of this pull request? :)

Reply to this email directly or view it on GitHub:
#41 (comment)

[ghost]

Hi,

I'm not sure if this exactly relates to this issue, but I think DirtyMinder tries to hook wrap Fixnum (probably other Numerics as well) and it gives TypeError.

I believe it's pointless to put a Fixnum in custom type such as Yaml at first place,
however, there're occasions you happen to assign a variety of... variables.

  require 'dm-core'
  require 'dm-types'
  require 'dm-migrations'
  require 'dm-sqlite-adapter'
  DataMapper.setup :default, :adapter  => 'sqlite3',
                             :database => ':memory:'
  class HookerTypeError
    include DataMapper::Resource
    property :id, Serial
    property :value, Yaml
    auto_migrate!
  end
  HookerTypeError.create :value => 1 # raises TypeError

Cheers,
Hama

P.S. I thought about adding a test for this but I wasn't sure if it were written in proper manner, so. :/

[jpr5]

@yoidoreorg,

As you said, it makes no sense to assign a Fixnum to Yaml, however you are right that the issue is in DirtyMinder. AFAICT, this is a problem for any Fixnum or Symbol value assignment (Float, String and Numeric are fine, so in your example :value => "1" or :value => 1.0 will work).

The issue you raise is different from this thread. Please resubmit as a new issue, I will respond with these comments and a commit against current master that solves this problem.

Thanks for the report!