reserved methods for property getter
Reported by Lin Jen-Shin | January 8th, 2009 @ 10:45 AM | in 1.0.0
It should not override internal methods for property getter.
I notice that there's a guard around defining property getter in dkubb/dm-core, which is "unless model.instance_methods(false).include? (getter)". However, this didn't help much since instance_methods(false) would return only instance methods for client model (e.g. User). In addition, Ruby 1.8 would return method names in String, not Symbol, so it's always false in Ruby 1.8. (and Ruby 1.9 returns symbols.)
But change this to "unless model.instance_methods(true).include? (getter)" is too aggressive since you cannot use "property :id" due to there's Object#id in Ruby 1.8 which is deprecated but not removed in this manner. I found this problem when I defined a property called :model, and I got stack overflow simply because calling User.first (it shocks me...).
Other property names like :send, :class, :save, :original_values, etc. would cause different errors in different places. It's hard to find out which name was used internally. I think perhaps we should keep a list of "reserved methods" for property getter, and warn people when they touched reserved methods or raise an error, or silently ignore them just like current guard "unless model.instance_methods(false).include?(getter)" ?
I could try to search for the list if it should be kept.
cheers, Jen-Shin
Comments and changes to this ticket
-
Dan Kubb (dkubb) January 8th, 2009 @ 01:25 PM
- State changed from “unconfirmed” to “confirmed”
I'm fine too with warning (at first) and then raising exceptions when there are conflicts with stuff defined in DataMapper::Resource, as well as core Ruby methods.
-
Dan Kubb (dkubb) February 21st, 2009 @ 03:14 AM
- Tag changed from “property” to “dm-core, property”
Jen-Shin, if you'd like to come up with an approach for doing this safely please do.
What I was considering was making a utility method where you could pass in the name of the method to create, and some text, and it would only create the method in the target model if it was "safe" to do so.
-
Lin Jen-Shin February 22nd, 2009 @ 12:14 AM
What I would like to see is "property" would protect from overriding important methods by default, by warning or silently ignore, or even raise exception is ok with me.
Perhaps another option for property? e.g. "property(:model, String, :getter => [:force, :warning, :ignore].sample)". :force to tell just override it, :warning means putting an warning on Logger, and :ignore to define only "safe" getter. Then the current behavior would fall in :force/:ignore.
-
Dan Kubb (dkubb) May 31st, 2009 @ 12:40 AM
- State changed from “confirmed” to “accepted”
- Assigned user set to “Dan Kubb (dkubb)”
-
Dan Kubb (dkubb) June 7th, 2009 @ 01:30 AM
- Milestone set to “0.10.1”
Lin, I have updated dm-core/next so that the property getter/setter will only be added if it isn't defined inside DM::Resource, or any parent model.
I still think the idea approach at this point would be to model what the Merb guys did and raise an exception unless a method (like override(:method_name)) is used to notify DM that you know what you're doing.
If you're interested in submitting a patch to this effect, I would gladly accept it. Otherwise I will schedule this to happen after 0.10 is released.
-
Lin Jen-Shin June 7th, 2009 @ 05:28 AM
It's a little different here with Merb. DataMapper is generating method automatically, but Merb isn't. Tell me if I was wrong, I am reading this:
http://merbivore.com/documentation/current/doc/rdoc/stack/index.htm...!
I don't think it would make sense if the generated getter/setter should override internal method under any circumstances, but we could generalize this issue to the same level as Merb here. That is use the same approach (method_added) with Merb, preventing from overriding any resource_methods, unless there's a override!(:method) declaration before method added.
Then it should raise an exception if getter/setter is going to override resource_methods, and we don't have to guard resource_method_defined? in Property#create_reader/writer_for.
I could try to produce this patch in recent days of my free time. Many thanks for your effort, Dan.
-
Lin Jen-Shin June 8th, 2009 @ 03:15 AM
I've uploaded 4 commits totally, 3 to dm-core/next, 1 to extlib/next. Please review the commits here:
http://github.com/godfat/dm-core/commits/next
http://github.com/godfat/extlib/commit/832e4aaba04465cae73ff975999b...I ran the following spec:
extlib, dm-core/in_memory, yaml, sqlite3_memory adapter.
All passed on my test machine. (running full spec cost too much time and computer resource...)Pick what you like, rename if you wish, tell me what you thought, many thanks!
-
Dan Kubb (dkubb) October 4th, 2009 @ 09:33 PM
- Milestone changed from “0.10.1” to “0.10.2”
[project:id#20609 not-tagged:"0.10.0" not-tagged:"0.10.1" milestone:id#51895 bulk edit command]
-
Dan Kubb (dkubb) February 1st, 2010 @ 04:33 PM
- Milestone changed from “0.10.2” to “1.0.0”
-
-
-
Dan Kubb (dkubb) February 2nd, 2010 @ 02:47 AM
- State changed from “accepted” to “confirmed”
-
Dan Kubb (dkubb) February 3rd, 2010 @ 03:45 AM
- State changed from “confirmed” to “resolved”
- Milestone set to “1.0.0”
This problem has been fixed in dm-core edge a while ago. Marking this as 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.
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)
Lin Jen-Shin