Datamapper DataMapper → 0.10.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  MarkMT October 29th, 2009 @ 09:27 AM

  I don't have a very deep knowledge of DM internals, but I've been looking through the code and my assessment of what is happening is the following...

  The association reader (User#address in my case) invokes Associations::OneToOne::Relationship#get which executes -

  relationship.get(source, other_query).first

  'relationship' references a OneToMany relationship embedded in the OneToOne relationship. 'relationship.get' returns a collection, which would ordinarily have one element, and 'first' ensures that only that element is returned.

  In the case I described, because there is no child 'address' object associated with the parent 'user', relationship.get returns an empty collection, []. It is when 'first' is called on this result that the problem I've observed occurs.

  Collection#first includes this snippet...

    collection = if !with_query && (loaded? || lazy_possible?(head, query.limit))
      new_collection(query, super(query.limit))
    else
      all(query)
    end
  

  In the condition expression, !with_query is true, and the first time the association is accessed, both loaded? and lazy_possible?(head, query.limit) are false. However it turns out that the second time the association is accessed, lazy_possible? is true (I haven't dug into that to understand why) and this causes new_collection(query, super(query.limit)) to be executed.

  query.limit is equal to 1. super(query.limit) invokes LazyArray#first on the empty collection [], and it turns out that this returns [nil]. This gets passed into new_collection as the parameter 'resources' :

    def new_collection(query, resources = nil, &block)
      collection = self.class.new(query, &block)
      ...
      if resources
        collection.set(resources)
      end
    ...
    end
  

  The critical point is that within this method, collection.set(resources) will be called if resources is non-nil. In this case, resources is [nil] and so collection.set will be called. However it turns out that inside the implementation of Collection#set, when resources is an Enumerable, its elements are expected to be non-nil, as evidenced by a later attempt to call read_only? in One_to_Many::Collection#inverse_set.

  I don't know enough about how DM is supposed to work to know what the right solution is, but one simple fix is to just modify new_collection above, replacing -

      if resources
  

  with
  

      if resources && resources.all?
  

• You flagged this item as spam.
  

  MarkMT October 30th, 2009 @ 08:51 AM

  I've run into another situation that also fails, with similar, though not identical, behavior but the same underlying cause. This one makes me think that there is a better fix than the one I suggested above that will enable both situations to be addressed in more or less the same way...

  Here is a script that illustrates the second situation - http://gist.github.com/222295

  The problem occurs when I assign a new child object to a parent that initially has no child, and do so using the ||= operator, i.e. equivalent to -

  user.address = user.address || Address.new

  ...so whereas in the case I reported earlier there were two consecutive calls to the association reader, here there is a call to the reader followed by one to the writer.

  In this case a runtime error occurs on the call to the writer. The writer method calls Associations::OneToOne::Relationship#set , which in turn calls Associations::OneToMany::Relationship#set on the OneToMany relationship embedded in the OneToOne relationship. This method executes -

       get!(source).replace(targets)
  

  It turns out that in the particular situation I've described, get!(source) returns a one element array, [nil] (internally this seems to be returned by instance_variable_set, but I haven't dug further to understand why this only happens when the writer is called after the reader).

  Calling 'replace' on this particular value is what causes the exception. This leads (indirectly, via super) to a call to Collection#resources_removed, which executes

       resources.each { |resource| resource_removed(resource) }
  

  where resource_removed is defined in the subclass OneToMany::Collection. Since 'resources' is [nil], the loop gets called once for the nil element. resource_removed calls inverse_set(resource,nil). 'resource' appears in inverse_set as a local variable 'source' and internally the method performs a test

       if source.read_only?
  

  Right here is where the exception occurs, because you can't call read_only on nil.

  This bears a very close resemblance to the problem I reported initially. As I noted earlier, the problem occurs in that case when Collection#set(resources) is called with the argument [nil]. In that case the implementation of 'set' calls Collection#resources_added, which executes

       resources.each { |resource| resource_added(resource) }
  

  where resource_added is defined in the subclass OneToMany::Collection. This an exact parallel to the case above with resources_removed, since like resource_removed, resource_add also calls OneToMany::Collection#inverse_set with a nil resource and fails when attempting to call read_only? on it.

  The call to read_only? was added in a patch about a week ago -

  http://github.com/datamapper/dm-core/commit/88fa9a6424daf819d70f643...

  replacing a previous call to 'frozen?', which didn't have this problem because frozen? is defined on nil.

  Although the two cases I've described are distinct, they both stem from the same underlying cause and manifest themselves in different but similar ways. In the first case I previously suggested intercepting the [nil] argument before calling Collection#set. This solution doesn't apply so readily in the second case. A better solution imo would be to apply parallel fixes for both problems in the two methods resources_added and resources_removed respectively. This can be done simply by replacing

       resources.each { |resource| resource_added(resource) }
  

  with
  

       resources.compact.each { |resource| resource_added(resource) }
  

  in Collection#resources_added and replacing

       resources.each { |resource| resource_removed(resource) }
  

  with
  

       resources.compact.each { |resource| resource_removed(resource) }
  

  in Collection#resources_removed.

• You flagged this item as spam.
  

  MarkMT October 30th, 2009 @ 08:58 AM

  Sorry, there's a copy and paste error above... Collection#resources_added actually does this -

      resources.map { |resource| resource_added(resource) }
  

  not this
  

      resources.each { |resource| resource_added(resource) }
  

  and so the proposed solution for the first case is...

      resources.compact.map { |resource| resource_added(resource) }
  

• 

  Dan Kubb (dkubb) October 30th, 2009 @ 04:07 PM

  • State changed from “new” to “unconfirmed”

  The actual bug is that the resources Array contains a nil at all. A nil value is not permitted inside a Collection, and the resources Array is being used to populate the Collection, so it shouldn't include nil either.

  What I'd suggest doing is checking at the "entry point" and see why the resources Array contains a nil.

• You flagged this item as spam.
  

  MarkMT November 1st, 2009 @ 11:41 PM

  I believe the root problem is in Collection#first...

  This commit - http://github.com/datamapper/dm-core/commit/6a6add48ca6f5dcf85af7ae...

  a few days ago introduced the following expression for the return value that applies the first time OneToMany::Collection#first is called -

      head[0] = collection.to_a.first
  

  In the case where no child object exists, collection.to_a is an empty array [], and [].first returns nil, which gets stored in @head. A subsequent call to super(query.limit) the next time the association is accessed returns @head.first(1), which is the non-empty array [nil] that is passed as the initial resources array to new_collection.

  A subsequent call to lazy_load also transfers the nil value into the collection's @array variable.

  I'm not sure what the ultimate solution is, but backing out of that patch has resolved the issues I was observing.

• 

  Dirkjan Bussink November 2nd, 2009 @ 02:20 PM

  Could you check whether your issue is fixed with this commit:

  http://github.com/datamapper/dm-core/commit/2e8e7fde21170ab43078e42...

  Also added a spec for this case so it won't happen again.

• You flagged this item as spam.
  

  MarkMT November 5th, 2009 @ 02:38 PM

  Thanks Dirkjan. Sorry it's taken me a few days to circle back to this issue. I can confirm that that commit fixes both manifestations of the problem I described.

  FYI I also discovered a case in which simply backing out of the previous patch as I described exposed another problem (presumably related to the one that patch was designed to fix). Your commit does not suffer from this problem, so seems safe.

  BTW, I guess reflecting the limits of my ruby experience it took me a while to realize what happens when none of the conditions in the if construct are satisfied, given that you no longer have a closing else clause. Eventually I figured out that an if statement without a matched condition actually evaluates to nil, which of course is part of why this works.

• 

  Dan Kubb (dkubb) November 6th, 2009 @ 01:41 AM

  • State changed from “unconfirmed” to “resolved”
  • Milestone set to “0.10.2”
  • Assigned user set to “Dirkjan Bussink”