
attributes= now respects method visibility change breaks Model#copy
Reported by Shane Hanna | December 4th, 2008 @ 07:57 PM
http://github.com/sam/dm-core/co...
Model#copy does a create(resource.attributes). #attributes returns an attribute when the reader (not writer) is public.
This means you get an error on Model#copy now when your reader is public but your writer isn't.
I'm not sure what to fix so no patch or spec this time.
Comments and changes to this ticket
-
Dan Kubb (dkubb) December 4th, 2008 @ 11:54 PM
- State changed from unconfirmed to confirmed
I'm thinking that since Model#copy is a special case, we shouldn't use Model#create(resource.attributes) and should instead do it the (slightly) longer form:
def copy(source, destination, query = {}) properties = properties(source) repository(destination) do all(:repository => source).each do |resource| copy = new properties.each { |p| p.set!(copy, p.get!(resource)) } copy.save end end end
This is actually probably better anyway. I don't like to have methods access the read_many method in the adapter anyway. It's preferable to route all requests for it through Model#all anyway.
Shane, would you mind putting together a patch for this with specs, if it's not too much trouble? Probably the biggest reason for this breaking is that the Model#copy method is pretty much unspecced. It would be a good idea to spec normal behavior, as well as making sure that private attributes are copied over (the above code should work for privates), so this regression doesn't happen again. At the same time, I wouldn't spend a ton of time on the specs, just a few of the common cases, since we're updating Model#copy in dkubb/dm-core, and rewriting the specs using a specific style that probably doesn't mesh well with the sam/dm-core specs.
-
Shane Hanna December 5th, 2008 @ 12:16 AM
Yep. It's already passed knock-off time here though and I'm off for a couple cold ones down the pub.
If it's not patched by the time I sober up I'll do it :)
-
Shane Hanna December 8th, 2008 @ 01:31 AM
I'm a little unsure if the tests are any good or even in the right spot.
I've never really used RSpec before (the indirect .should calls smell funny to me) plus private attribute support for #copy is only implied by the other tests success.
http://github.com/shanna/dm-core...
Shane.
-
Dan Kubb (dkubb) January 4th, 2009 @ 11:38 PM
- State changed from confirmed to resolved
Ticket #742 is a dupe of this, and is now resolved.
-
Shane Hanna January 5th, 2009 @ 09:40 PM
- Assigned user changed from Jonathan Stott (namelessjon) to Dan Kubb (dkubb)
My mistake, ticket #742 doesn't resolve the protected attributes issue I don't think.
-
Dan Kubb (dkubb) January 5th, 2009 @ 11:34 PM
- State changed from resolved to accepted
Shane, I mispoke and I think you are right. Let me see what I can do to make it totally bypass private and protected accessors and mutators.
-
Jonathan Stott (namelessjon) February 22nd, 2009 @ 11:50 AM
- Tag changed from dm-core to dm-core, one_file_test
This is still an issue as of 02a93b11ce576361789340f78babf7bd4f796161. See attached one file test.
-
Dan Kubb (dkubb) November 10th, 2009 @ 02:00 AM
- State changed from accepted to resolved
(from [afec5ad19047bbde916057c2ac3a4023fc4a2e2b]) Model#copy bypasses private method visibility
[#695 state:resolved] http://github.com/datamapper/dm-core/commit/afec5ad19047bbde916057c...
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 your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
People watching this ticket
Attachments
Tags
Referenced by
-
742 Model.copy is broken http://datamapper.lighthouseapp....
-
695 attributes= now respects method visibility change breaks Model#copy [#695 state:resolved] http://github.com/datamapper/dm-co...