#967 ✓resolved
ojak

Lazy-loaded properties for belong_to relationships causing ArgumentError

Reported by ojak | July 10th, 2009 @ 03:29 PM | in 1.0.0

Description

FK id's are sometimes not loaded for has-belongs_to relationships. Seems like a validity check should be triggered for calls to any properties that have a defined relationship, even if the property type is unknown. See example below for details:

Example

Note that in the irb example below, the first and third commands are identical, but the third command works only after the relationship is loaded.

irb(main):001:0> Book.all(:user_id=>1)
ArgumentError: condition :user_id does not map to a property or relationship in Book

    from /opt/local/lib/ruby/gems/1.8/gems/dm-core-0.10.0/lib/dm-core/query.rb:793:in `assert_valid_conditions'
    ...



irb(main):002:0> Book.all('user.id'=>1) ~ (0.000089) SET sql_auto_is_null = 0 ~ (0.000061) SET SESSION sql_mode = 'ANSI,NO_AUTO_VALUE_ON_ZERO,NO_DIR_IN_CREATE,NO_ENGINE_SUBSTITUTION,NO_UNSIGNED_SUBTRACTION,TRADITIONAL' ~ (0.381100) SELECT "books"."id", "books"."created_at", "books"."updated_at" FROM "books" INNER JOIN "users" ON "users"."id" = "books"."user_id" WHERE "users"."id" = 1 GROUP BY "books"."id", "books"."created_at", "books"."updated_at" ORDER BY "books"."id" => [...]
irb(main):003:0> Book.all(:user_id=>1) ~ (0.023650) SELECT "id", "created_at", "updated_at", "user_id" FROM "books" WHERE "user_id" = 1 ORDER BY "id" => [...]

Expected behavior

From the example above, Book.all(:user_id=>1) should work without any external massaging of DM load order, etc. See merb workaround below for an example of how Merb deals with the issue. I figure this should be handled natively by DM, possibly triggered when a call to an FK id (known relationship) is made that does not have a property assigned to it on the Model.properties.

Merb Workaround

Here is a workaround that reloads property relationship after the models have been loaded:

Merb::BootLoader.after_app_loads do
  # This will get executed after your app's classes have been loaded.
# Ensure that DataMapper relationship's properties are defined DataMapper::Model.descendants.each do |model| model.relationships.each_value { |r| r.child_key } end end

IRC Discussion

This 7/10/09 #datamapper IRC discussion may be helpful:

namelessjon: Book.all(:user => @user) should work (though I'd expect :user_id to work, too)
[12:52pm] ojak: knowtheory: http://pastie.org/541779
[12:53pm] ojak: strange thing is, the first and third commands are the same.
[12:53pm] ojak: only, it only works after its been referenced previously (cached?)
[12:54pm] knowtheory: heh, okay so once an object is loaded, it's found amongst the local object
[12:54pm] knowtheory: and doesn't hit the DB
[12:54pm] knowtheory: that's something that's worth filing a bug over
[12:54pm] ojak: knowtheory: yeah, sounds like it.  shit.
[12:54pm] namelessjon: ojak: Ah.  I thought that bug had gone away in 0.10.0.  But yeah, the property gets lazily declared.  Since at the time the first class is loaded, the parent class might not be.
[12:55pm] namelessjon: so the child class can't know what type of property to create for the fk.
[12:55pm] ojak: namelessjon: ah.  i'd expect that fk id's should be loaded as properties though.
[12:56pm] ojak: namelessjon: hopefully a simple fix of just adding fk id's as properties to the model.
[12:56pm] namelessjon: ojak: How?  The pk of the parent could be a string or a composite key or ...
[12:56pm] namelessjon: ojak: The property is added.  Just, later.
[12:56pm] ojak: namelessjon: too late... 
[12:57pm] namelessjon: ojak: Unfortunately you can't tell ruby 'do something when this constant is defined'
[12:57pm] DireFog: you mean as an event on definition?
[12:58pm] ojak: namelessjon: when a property has a belongs_to, the name of the property is known though?  so shouldn't it at least be added as a property at that point, without a query being required?
[12:59pm] ojak: namelessjon: typo above... "when a model has a belongs_to"
[12:59pm] namelessjon: ojak: No, it isn't known.  If I say belongs_to :user, then without the user class (which might not be loaded at this point) I don't know what the primary key of the User model is.
[12:59pm] randybias|away is now known as randybias.
[1:00pm] namelessjon: ojak: It /might/ be a Serial called id.  But it could just as easily be a String called login.
[1:01pm] ojak: namelessjon: is see, so load order stack isn't dictated by relationships?
[1:01pm] ojak: namelessjon: that is a problem...
[1:02pm] namelessjon: ojak: DM is completely at the mercy of your require statement ordering.
[1:04pm] namelessjon: ojak: I should also say there's a simple work around here:  Define 'property :user_id, Integer' explicitly, and the relationship will use that, assuming User has a Serial key.
[1:06pm] ojak: namelessjon: hmm.  not too dry though.  I'll file a bug.
[1:06pm] ojak: thx.
[1:12pm] namelessjon: ojak: There is also a code snippet that lives in merb that chases up all your DM models and defines all the keys.
[1:12pm] ojak: namelessjon: hmm.  My DM test was using Merb irb, so I wonder if Merb is broken.
[1:13pm] ojak: namelessjon: seems like this functionality should exist in DM?
[1:14pm] randybias is now known as randybias|away.
[1:15pm] namelessjon: ojak: What version of merb?
[1:15pm] ojak: namelessjon: 1.1
[1:15pm] ojak: namelessjon: snusnu fork
[1:16pm] namelessjon: ojak: Ahhh, snusnu removed that snippet.  Since he does the property define thing, it never would have caused him issues.
[1:16pm] namelessjon: ojak: And the code itself got broken by 0.10.0 due to some method redefinitions.
[1:16pm] ojak: namelessjon: no shit?  (what are the odds).
[1:17pm] namelessjon: ojak: 0.10.0 has had almost no effect on the public API, at least.
[1:18pm] namelessjon: ojak: But anyway, it could live in DM.  But when to fire it is still an app specific thing, since it has to be 'after all models are loaded'
[1:19pm] namelessjon: ojak: Anyway, I'll try and find the snippet and update it for 0.10.0
[1:19pm] ojak: namelessjon: belongs_to does make a model aware of a relationship, even if the type is not known, so I figure it should be fired on first request to property that has a know relationship.
[1:19pm] ojak: namelessjon: dunno, just thinkin out load.
[1:19pm] randybias|away is now known as randybias.
[1:20pm] ojak: namelessjon: out loud...
[1:21pm] namelessjon: ojak: Hmm.  You could probably make a case that the property validity checker could also test for relationships.
[1:24pm] ojak: namelessjon_:  could work?  im not intimate with that part of the framework, but I'll put this in the ticket.

Comments and changes to this ticket

  • Jonathan Stott (namelessjon)

    Jonathan Stott (namelessjon) July 10th, 2009 @ 04:00 PM

    • State changed from “new” to “unconfirmed”

    Just as an additional note: Speaking from the merb PoV, it would be reassuring if DataMapper::Model.descendants could be marked as part of the plugin/semi-public API at least.

    Also, I don't know how much of a performance hit is involved, but to expand on what ojak says at the start of the ticket, could the query validity checker not also interrogate the model's relationships for child keys before it declares a property invalid.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) July 10th, 2009 @ 07:07 PM

    • Assigned user set to “Dan Kubb (dkubb)”
    • State changed from “unconfirmed” to “accepted”

    I have no problem with DataMapper::Model.descendants being part of the semipublic API.

    I think this is due to the load ordering of the models. The foreign key inference doesn't happen until Model#assert_valid is called. I was thinking that if there was DataMapper::Model.initialize or something similar, we could do whatever needs to be done after all the models are loaded.

    This should be easy to add to Merb, but not sure if there's a callback in Rails that we can hook into after model loading. If I can find it I'd like to update rails_datamapper and merb_datamapper so that models are initialized right after load, allowing FK inference to be done explicitly rather than lazily.

  • Jonathan Stott (namelessjon)

    Jonathan Stott (namelessjon) July 10th, 2009 @ 07:34 PM

    Wrapping things like the DataMapper::Model.descendants in an initialize method that would do all such things does seem a little cleaner (and clearer) than the present situation, but it still doesn't quite seem right. I guess this comes back to a need for an actual ruby based 'const_defined' hook as we talked about earlier today, which could wait for the classes to load and assign FKs as the models loaded.

    How expensive would it be for the query validation code to also check for potential undefined relationship keys when it received a query? Though I guess there are other places it could need the knowledge too. Like knowing it needed to retrieve the FK when get was called.

  • Martin Gamsjaeger (snusnu)
  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) July 10th, 2009 @ 09:51 PM

    • Assigned user set to “Dan Kubb (dkubb)”
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) July 10th, 2009 @ 11:12 PM

    @Johnathan: It's actually not that expensive in terms of causing a performance issue. Initializing the relationships takes an imperceptible amount of time. The bigger issue is that if we keep it so initialization is lazy there are quite a few pathways into the code that need relationships to be defined. It would be quite gross to use Model#assert_valid in each of those.

    A single initialization point would be much easier to debug and would resolve all of the intermittent errors that are caused by FKs not being inferenced correctly.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 4th, 2009 @ 09:33 PM

    • Milestone changed from 0.10.0 to 0.10.2

    [project:id#20609 not-tagged:"0.10.0" milestone:id#51895 bulk edit command]

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) February 1st, 2010 @ 04:33 PM

    • Milestone changed from 0.10.2 to 1.0.0
  • Dan Kubb (dkubb)
  • Dan Kubb (dkubb)
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) February 2nd, 2010 @ 02:47 AM

    • State changed from “accepted” to “confirmed”
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 26th, 2010 @ 02:10 AM

    • Milestone set to 1.0.0
    • Assigned user set to “Jonathan Stott (namelessjon)”

    Assigning this to namelessjon, who has started work on adding DataMapper.finalize to the DM 1.0.0 API.

  • Jonathan Stott (namelessjon)

    Jonathan Stott (namelessjon) May 27th, 2010 @ 05:07 PM

    • State changed from “confirmed” to “resolved”

    Since DataMapper.finalize went out with RC3, I'm marking this resolved

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Pages