Model#load sets a resource in the identity map without loading key values in case of a custom type
Reported by Piotr Solnica (solnic) | October 15th, 2008 @ 09:54 AM
Here's the situation:
class ReversingString < DataMapper::Type
primitive String
size 10
def self.load(value, property)
value.nil? ? nil : value.reverse
end
def self.dump(value, property)
value.nil? ? nil : value.reverse
end
end
class Zoo
include DataMapper::Resource
property :name, ReversingString, :key => true
auto_migrate!
end
@zoo = Zoo.create(:name => 'Madagascar')
p @zoo.repository.identity_map(@zoo.model).get(['Madagascar']) # #<Zoo name="Madagascar">
@zoo.repository.identity_map(@zoo.model).delete(['Madagascar'])
@zoo = Zoo.get(@zoo.name)
p @zoo.repository.identity_map(@zoo.model).get(['Madagascar']) # nil
p @zoo.repository.identity_map(@zoo.model).get(['Madagascar'.reverse]) # #<Zoo name="Madagascar">
It looks like Model#get uses a dumped version of the "name" property when setting up an identity map key, which is not correct...So we end up having '["racsagadaM"]' as the key for our resource.
Comments and changes to this ticket
-
Piotr Solnica (solnic) October 16th, 2008 @ 06:38 AM
- Title changed from “Model#load sets a resource in the identity map without loading key values in case of a custom type” to “[PATCH] Model#load sets a resource in the identity map without loading key values in case of a custom type”
- Tag changed from “bug, dm-core” to “bug, dm-core, patch”
I'm attaching a patch with unit and integration specs which fixes the problem
-
Dan Kubb (dkubb) December 5th, 2008 @ 05:04 AM
- Assigned user cleared.
- State changed from “new” to “confirmed”
-
Dan Kubb (dkubb) January 8th, 2009 @ 04:50 AM
- State changed from “confirmed” to “accepted”
- Assigned user set to “Dan Kubb (dkubb)”
-
Piotr Solnica (solnic) January 8th, 2009 @ 05:00 AM
Hey Dan, my patch doesn't apply to your branch because the structure of specs is different now, I can make a new one with new specs if you want
-
Dan Kubb (dkubb) January 8th, 2009 @ 05:04 AM
Piotr, you may if you wish. See the specs in spec/public/shared/collection_shared_spec.rb for an example.
I am not sure if there is a Model#load section, but if there isn't, feel free to put your specs inside that block (like the Collection specs). Sorry that I don't have too much docs to provide outlining the style.. I will have them soon, but for now, just try your best.. I can always tweak them a bit if needed -- the important thing is that the idea make it in.
-
Piotr Solnica (solnic) January 8th, 2009 @ 02:49 PM
Model#load appears to be a semipublic method, shouldn't it be speced somewhere in the spec/semipublic directory?
BTW, why there is no spec/semipublic/model.rb? You're not going to have specs for the Model's semipublic methods?
-
Dan Kubb (dkubb) January 8th, 2009 @ 05:12 PM
Model itself isn't fully specced directly yet, but it does have sufficient coverage from elsewhere that I'm fairly confident that changes can be made to it relatively safely.
And yes, Model#load will be specced the spec/semipublic/model.rb file, feel free to create one and make your additions there.
-
Piotr Solnica (solnic) January 11th, 2009 @ 07:32 AM
Here's a new patch, I created spec/semipublic/model_spec.rb and put my spec there. I wasn't able to reproduce the issue using in_memory_adapter, so the spec is inside "supported_by :postgres, :mysql, :sqlite3_fs" block.
-
Dirkjan Bussink January 12th, 2009 @ 01:48 AM
You shouldn't put it in a support_by :postgres, :mysql block, because the spec doesn't fail with the in_memory_adapter.
A spec specifies how it should work, they shouldn't be just a bunch of bugs reported. So therefore it should really be just a supported_by :all.
-
Piotr Solnica (solnic) January 12th, 2009 @ 03:16 AM
It doesn't fail with the in_memory_adapter b/c that adapter has problems when it comes to keys and custom types. For instance Model#get doesn't find a resource if you don't manually dump the value of its key, this means that in my example I would have to do @zoo = Zoo.get(@zoo.name.reverse) to get the resource.
I have to fix the in_memory_adapter in order to put the spec inside "supported_by :all".
-
Dan Kubb (dkubb) January 12th, 2009 @ 01:27 PM
- Assigned user changed from “Dan Kubb (dkubb)” to “Piotr Solnica (solnic)”
@Piotr: I think what Dirkjan means is that the intention is that the spec works with all the adapters. If in reality it doesn't work with all of them, better to use the pending_if helper to mark it as pending for that specific adapter.
There are alot of cases in DM where stuff works fine with the in-memory adapter, but doesn't with the other adapters and vice-versa. If we just used supported_by to filter out the non-working adapters, and make a note to work on it later, it'd be really easy to forget or get side tracked with something and for the bug to persist. Instead we mark the spec as pending based on a specific condition being true (what pending_if does), to remind us that something still needs to be fixed.
-
Piotr Solnica (solnic) January 12th, 2009 @ 04:08 PM
Thanks for the explanation.
So after all the spec is inside "supported_by :all", a new version of patch is attached.
-
Piotr Solnica (solnic) June 23rd, 2009 @ 03:56 PM
- Title changed from “[PATCH] Model#load sets a resource in the identity map without loading key values in case of a custom type” to “Model#load sets a resource in the identity map without loading key values in case of a custom type”
-
Dan Kubb (dkubb) June 23rd, 2009 @ 04:00 PM
@Piotr: Did you mean to upload a patch? Your comment says you did, but I can't find any files attached to this ticket.
-
Piotr Solnica (solnic) June 23rd, 2009 @ 04:07 PM
- Tag changed from “bug, dm-core, patch” to “bug, dm-core”
I've removed the patch since it no longer applies. I can see Model#load has been changed recently, unfortunately the bug is not fixed. The only thing which should happen is that key_values should be loaded by their custom types before they are used as a key in the IdentityMap. Now I'm not sure where in the code this should be placed. I can work on this but I need some guidelines :)
-
Piotr Solnica (solnic) February 5th, 2010 @ 05:32 AM
- State changed from “accepted” to “not-applicable”
After all (it's been months haha) I think the adapter should return properly typecasted keys which is exactly what I've done in mongodb adapter where the problem described in this ticket was applicable.
-
Dan Kubb (dkubb) February 5th, 2010 @ 02:51 PM
Yeah I would agree. I can understand why the original design choice was to make DM coerce everything from the adapters, but it just doesn't work very well in practice. I think it's ok to provide default implementations for the simple cases, but the complex cases is where it breaks down, and ultimately adapters need to be responsible for coercion of every value they return to DM.
-
Paul Walker September 13th, 2010 @ 04:15 PM
- Milestone order changed from “0” to “0”
So, I guess an artifact with this is that models I retrieve that are made up of a composite key of UUID and Enum from dm-types have their persisted_state set to Immutable in the latest version of DM. (MySql adapter on Jruby) This used to work in the 0.10.8 release.
-
Paul Walker September 13th, 2010 @ 04:17 PM
Can we get related tickets created on the adapters before this gets lost in the not-applicable que?
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.
Create new ticket
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Dan Kubb (dkubb)
Paul Walker
Piotr Solnica (solnic)
Rick