Hello all, I'd like to get a review of ticket #3005: http://dev.rubyonrails.org/ticket/3005 This patch prevents the following kind of errors: class QuoteLine < AR:Base belongs_to :quote end QuoteLine.find_by_product_id(312) SELECT * FROM quote_lines WHERE product_id = <#Quote:0x39420> #quote is defined as a method on AR:Base that quotes values. Since it now returns Quote objects, the SQL generates badly. Instead, the patch will blow up during the loading of quote_line.rb: ActiveRecord::Reflection::ReflectionNameAlreadyTaken: 'quote' is already defined as a method in QuoteLine. You'll have to rename your association. <rest of backtrace here> The patch includes tests, and is ready for trunk. If anybody would be willing to take a look maybe apply ? Thanks ! -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 12/6/05, Francois Beausoleil <francois.beausoleil@gmail.com> wrote:> Hello all, > > I''d like to get a review of ticket #3005: http://dev.rubyonrails.org/ticket/3005I like the general approach, How feasible is it to expand the approach to handle columns called quote? ie. CREATE TABLE somethings id serial, quote text ); -- Cheers Koz
Hello Michael, 2005/12/5, Michael Koziarski <michael@koziarski.com>:> On 12/6/05, Francois Beausoleil <francois.beausoleil@gmail.com> wrote: > > Hello all, > > > > I'd like to get a review of ticket #3005: http://dev.rubyonrails.org/ticket/3005 > > I like the general approach, How feasible is it to expand the > approach to handle columns called quote? > > ie. > > CREATE TABLE somethings > id serial, > quote text > );It would certainly be possible to do so. Where should we do it, though ? I can't find a good place to put such code... Bye, -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
Hi ! 2005/12/5, Michael Koziarski <michael@koziarski.com>:> On 12/6/05, Francois Beausoleil <francois.beausoleil@gmail.com> wrote: > > Hello all, > > > > I'd like to get a review of ticket #3005: http://dev.rubyonrails.org/ticket/3005 > > I like the general approach, How feasible is it to expand the > approach to handle columns called quote?I implemented it, but I don't like it... Here's a taste of what the new code returns: ./script/../config/../vendor/rails/railties/lib/commands/runner.rb:27: Columns 'name', 'transaction', 'quote' have already been taken in models of class Game. You will have to rename your columns to not conflict with pre-existing method names. (ActiveRecord::ColumnNameAlreadyTaken) When I used name in my models, I never expected that to conflict. It's such a common one. Guess what it conflicts with: Class::name. Here's the implementation of the guard method: # Raise ActiveRecord::ColumnNameAlreadyTaken if one of the columns defined # in objects of this class conflict with pre-existing method names. def guard_against_duplicate_column_names # We cast a very wide net here, maybe we should not cast such as wide one ? methods = [ ActiveRecord::Base.methods, ActiveRecord::Base.instance_methods].flatten.compact.uniq duplicates = self.class.columns.select do |column| next false if column.name == self.class.primary_key methods.include?(column.name) end raise ActiveRecord::ColumnNameAlreadyTaken.new(duplicates, self.class) unless duplicates.empty? end Thoughts anyone ? -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On Dec 6, 2005, at 7:07 PM, Francois Beausoleil wrote:> Hi ! > > 2005/12/5, Michael Koziarski <michael@koziarski.com>: >> On 12/6/05, Francois Beausoleil <francois.beausoleil@gmail.com> >> wrote: >>> Hello all, >>> >>> I''d like to get a review of ticket #3005: http:// >>> dev.rubyonrails.org/ticket/3005 >> >> I like the general approach, How feasible is it to expand the >> approach to handle columns called quote? > > I implemented it, but I don''t like it... Here''s a taste of what the > new code returns: > > ./script/../config/../vendor/rails/railties/lib/commands/runner.rb:27: > Columns ''name'', ''transaction'', ''quote'' have already been taken in > models of class Game. You will have to rename your columns to not > conflict with pre-existing method names. > (ActiveRecord::ColumnNameAlreadyTaken) >I didn''t catch what was unlikable about this... it seems friendly to me. Duane Johnson (canadaduane)
2005/12/7, Duane Johnson <duane.johnson@gmail.com>:> > ./script/../config/../vendor/rails/railties/lib/commands/runner.rb:27: > > Columns 'name', 'transaction', 'quote' have already been taken in > > models of class Game. You will have to rename your columns to not > > conflict with pre-existing method names. > > (ActiveRecord::ColumnNameAlreadyTaken) > > > I didn't catch what was unlikable about this... it seems friendly to me.It means I'll have to change my models ! Seriously, #name is a method in Class. So, we can't use it in models. #type is another. It means we can't have Single Table Inheritance working. Okay, #type is deprecated and will go away, but what do we do with it now ? Bye ! -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
> It means I''ll have to change my models ! Seriously, #name is a method > in Class. So, we can''t use it in models. #type is another. It means > we can''t have Single Table Inheritance working.methods = [ ActiveRecord::Base.methods, ActiveRecord::Base.instance_methods].flatten.compact.uniq Why are you taking the class methods as well? Column names won''t conflict with class methods. #type is a problem though ... Perhaps: methods = ActiveRecord::Base.instance_methods - Object.instance_methods -- Cheers Koz
2005/12/7, Michael Koziarski <michael@koziarski.com>:> Why are you taking the class methods as well? Column names won't > conflict with class methods.Because you won't be able to start a transaction on a model instance otherwise: Game.find(1).transaction do # Do stuff on game end I verified which #transaction method would be called. It was ActiveRecord::Base.transaction, a class method. Here's a little test I made: $ type check.rb p = proc do |event, file, line, id, binding, classname| if id.to_s =~ /transaction/ then printf "%8s %s:%-2d %10s %8s\n", event, file, line, id, classname end end g = Game.new set_trace_func p g.transaction do puts "In transaction" end set_trace_func nil $ ruby script\runner "require 'check'" call ./script/../config/../vendor/rails/activerecord/lib/active_record/transactions.rb:117 transaction ActiveRecord::Transactions ... In transaction return ./check.rb:10 transaction ActiveRecord::Transactions::ClassMethods So, we do in fact have to gobble up the class methods. Maybe this is an instance of Ruby being too helpful ? Bye ! -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On Dec 7, 2005, at 2:23 PM, Francois Beausoleil wrote:> So, we do in fact have to gobble up the class methods. Maybe this is > an instance of Ruby being too helpful ?class RubyBeingTooHelpful; end instance = RubyBeingTooHelpful.new :) Sorry, couldn''t resist. Duane
Hi ! 2005/12/7, Duane Johnson <duane.johnson@gmail.com>:> On Dec 7, 2005, at 2:23 PM, Francois Beausoleil wrote: > > So, we do in fact have to gobble up the class methods. Maybe this is > > an instance of Ruby being too helpful ? > > class RubyBeingTooHelpful; end > instance = RubyBeingTooHelpful.newEh eh, nice one :) Seriously though, what should we do ? Leaving off class methods seems like the easiest method so far. Will we be bitten later on ? That's the question we have to ask. Dave did say:> We sidestep the issue. Reserved words are the reason why you can do > attribute access with brackets, ala: > > model[:attr]Of course that can be done. In that case, should auto read/write methods be forbidden for such columns ? In the end, we're trying to keep the principle of least surprise active. What's less suprising: instance.transaction = Transaction.new # NoMethodError - OR - instance.transaction do bla bla end # nothing committed - no unit tests = bug not exposed Bye ! -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
2005/12/7, Francois Beausoleil <francois.beausoleil@gmail.com>:> In the end, we're trying to keep the principle of least surprise > active. What's less suprising: > > instance.transaction = Transaction.new > # NoMethodError > > - OR - > > instance.transaction do > bla > bla > end > > # nothing committed - no unit tests = bug not exposedSo, what do we do ? We could also take the status quo, and just add the patch as-is, meaning relationships would prevent reusing existing method names, but columns are not. Bye ! -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
Hey ! 2005/12/9, Francois Beausoleil <francois.beausoleil@gmail.com>:> 2005/12/7, Francois Beausoleil <francois.beausoleil@gmail.com>: > > In the end, we're trying to keep the principle of least surprise > > active. What's less suprising: > > So, what do we do ? We could also take the status quo, and just add > the patch as-is, meaning relationships would prevent reusing existing > method names, but columns are not.Any takers on this ? -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 12/14/05, Francois Beausoleil <francois.beausoleil@gmail.com> wrote:> Hey ! > > 2005/12/9, Francois Beausoleil <francois.beausoleil@gmail.com>: > > 2005/12/7, Francois Beausoleil <francois.beausoleil@gmail.com>: > > > In the end, we''re trying to keep the principle of least surprise > > > active. What''s less suprising: > > > > So, what do we do ? We could also take the status quo, and just add > > the patch as-is, meaning relationships would prevent reusing existing > > method names, but columns are not. > > Any takers on this ?What if the patch were the ''full bonanza'', combining both of the earlier ones, with a special exception for the ''type'' column, since that one would be a hassle, and is going away soon via a Ruby update anyway? Even better, these checks could also be added to the generator script. Is that too draconian? Personally, I think this can be a source of very frustrating bugs to Rails newcomers, and it would be better to err on the side of caution. --Wilson.
2005/12/14, Wilson Bilkovich <wilsonb@gmail.com>:> What if the patch were the 'full bonanza', combining both of the > earlier ones, with a special exception for the 'type' column, since > that one would be a hassle, and is going away soon via a Ruby update > anyway? > > Even better, these checks could also be added to the generator script. > > Is that too draconian? Personally, I think this can be a source of > very frustrating bugs to Rails newcomers, and it would be better to > err on the side of caution.#type & #name are the major exceptions, I believe. Let me build that patch and submit. Thanks ! -- François Beausoleil http://blog.teksol.info/ _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core