#1124 ✓not-applicable
Avdi Grimm

Model#load fails to find key fields in record hash

Reported by Avdi Grimm | November 14th, 2009 @ 11:53 AM

When looking to see if all key fields are included in a Hash-style record, Model#load calls record.values_at() with an array of Property objects. But since this hash is indexed by straight-up strings or symbols, no fields are ever found. Substituting field_map (which is Property-indexed) for record fixes the problem.

Comments and changes to this ticket

  • Avdi Grimm
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) November 14th, 2009 @ 06:18 PM

    • State changed from “new” to “unconfirmed”

    @Avdi: Are you sure that does what is expected? Do all of dm-core's specs pass with that?

    You'll note a few lines above that, we remap the field to the Property objects:

    http://github.com/avdi/dm-core/blob/7dd570d66ecad188e94761b767ca5a1...

    I've actually been thinking of simplifying the API to just work with Hashes with String keys to keep things simple. Right now we allow Hash with String/Symbol/Property, and Resource objects, which I think is overkill. I understand the original reasoning for the design, but I think it's too much.

  • Avdi Grimm

    Avdi Grimm November 15th, 2009 @ 10:47 AM

    Dan, unless I'm missing something the remapping is done only for field_map (which is a dup of records), NOT for the original records hash. As far as I can tell the old values_at() call could never return any values, because the original records hash remains pristine and keyed by strings. I've confirmed this by stepping through the code.

    I'll let you know when I've checked the specs.

    I would be fine with reducing the API to plain-old string-indexed hashes only; it woudl reduce the cognitive load on adapter implementors (e.g. me).

    My understanding is that the old (0.9.*) API expected adapters to return Resource objects. This would explain why this bug hasn't been caught before - the existing adapters haven't sufficiently exercised the new Hash case.

  • Avdi Grimm

    Avdi Grimm November 15th, 2009 @ 11:25 AM

    After doing some more stepping through, I'm a little less sure.

    Also, I get a hojillion errors when I run the specs, and they take forever. Any tips on setting up to run the DM specs and/or running useful subsets?

  • Avdi Grimm

    Avdi Grimm November 15th, 2009 @ 01:22 PM

    OK, I'm going to say that this isn't an issue after all. Either I was getting spurious behavior from SimpleDB or some other change I made fixed it, but I'm able to use the 0.10.1 gem without problems now. And you're right, my change doesn't do what I thought it did.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) February 5th, 2010 @ 02:06 AM

    • State changed from “unconfirmed” to “not-applicable”

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 »

People watching this ticket

Pages