Hi all! I have come into a strange issue in Rails that I am hoping someone can shed some light. To make a very long story short, I have been researching how to override ActiveRecord::Base#read_attribute and write_attribute to perform security checks at the model level (influenced by the ModelSecurity generator). Shortly after implementing some code to check this behavior, I began to see undefined behavior from rails. The first request to read an attribute would be denied correctly, however further requests would proceed without fail. After a lot of searching, I think I have traced the problem down to the generated read methods. When ActiveRecord::Base#generate_read_methods is false, all is well. When its true, my migraine returns. More research, reading, and I come to this curiosity... ActiveRecord::Base#define_read_method(symbol, attr_name, column) generates reader code for an attribute after Rails realizes there should be a reader method for it, correct? But the generated reader method body does not call read_attribute... instead it accesses the attribute directly and performs a type cast (if necessary) Is this intended? Am I missing something that read_attribute is doing that is not appropriate for a defined attribute reader method? It''s near trivial to draft a patch for this - however, I wanted to make sure I wasn''t vastly underestimating something. Comments? Thanks everyone! -- Jeff Hall Technology Consultant Digital Maelstrom Email: jeff.hall@digitalmaelstrom.net Jabber/XMPP: jeff.hall@digitalmaelstrom.net
I agree that seems odd. I can''t see any reason not to use read_attribute there, as it will type cast if necessary. define_read_method could be simplified and Column#type_cast_code should be unnecesary.... perhaps. Currently, define_read_method uses @attributes.has_key? to verify that the attribute is present and raises an exception if it''s not. You''d probably want to leave that check in there. Why don''t you make a patch and see if it breaks anything :) -Jonathan. On 5/10/06, Jeff Hall <jeff.hall@digitalmaelstrom.net> wrote:> Hi all! > > I have come into a strange issue in Rails that I am hoping someone > can shed some light. > > To make a very long story short, I have been researching how to > override ActiveRecord::Base#read_attribute and write_attribute to > perform security checks at the model level (influenced by the > ModelSecurity generator). Shortly after implementing some code to > check this behavior, I began to see undefined behavior from rails. > The first request to read an attribute would be denied correctly, > however further requests would proceed without fail. > > After a lot of searching, I think I have traced the problem down to > the generated read methods. When > ActiveRecord::Base#generate_read_methods is false, all is well. When > its true, my migraine returns. > > More research, reading, and I come to this curiosity... > > ActiveRecord::Base#define_read_method(symbol, attr_name, column) > generates reader code for an attribute after Rails realizes there > should be a reader method for it, correct? But the generated reader > method body does not call read_attribute... instead it accesses the > attribute directly and performs a type cast (if necessary) > > Is this intended? Am I missing something that read_attribute is > doing that is not appropriate for a defined attribute reader method? > > It''s near trivial to draft a patch for this - however, I wanted to > make sure I wasn''t vastly underestimating something. > Comments? > > Thanks everyone! > -- > Jeff Hall > Technology Consultant > Digital Maelstrom > > Email: jeff.hall@digitalmaelstrom.net > Jabber/XMPP: jeff.hall@digitalmaelstrom.net > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
Marcel Molina Jr.
2006-May-10 01:19 UTC
Re: Issue in ActiveRecord generated reader methods
On Tue, May 09, 2006 at 07:41:41PM -0500, Jeff Hall wrote:> ActiveRecord::Base#define_read_method(symbol, attr_name, column) > generates reader code for an attribute after Rails realizes there > should be a reader method for it, correct? But the generated reader > method body does not call read_attribute... instead it accesses the > attribute directly and performs a type cast (if necessary) > > Is this intended? Am I missing something that read_attribute is > doing that is not appropriate for a defined attribute reader method? > > It''s near trivial to draft a patch for this - however, I wanted to > make sure I wasn''t vastly underestimating something. > Comments?Stefan Kaes implemented the reader method option for performance reasons. He would know if his chosen implementation is intentional (for performance) rather than using read_attribute or incidental. marcel -- Marcel Molina Jr. <marcel@vernix.org>
Jonathan, Patch doesn''t appear to break anything. All the unit tests run green. However, Marcel suggested that Stefan implemented this method intentionally for performance reasons. I will submit the patch as a Trac ticket - maybe then someone more skilled at benchmarking than I can see if any noticable performance hit is incurred. Perhaps then my nightmare with this code section will end. Thanks - cheers! -- Jeff ----- Original Message ----- From: Jonathan Viney <jonathan.viney@gmail.com> To: rails-core@lists.rubyonrails.org Sent: Tuesday, May 9, 2006 8:12:13 PM GMT-0600 Subject: Re: [Rails-core] Issue in ActiveRecord generated reader methods I agree that seems odd. I can''t see any reason not to use read_attribute there, as it will type cast if necessary. define_read_method could be simplified and Column#type_cast_code should be unnecesary.... perhaps. Currently, define_read_method uses @attributes.has_key? to verify that the attribute is present and raises an exception if it''s not. You''d probably want to leave that check in there. Why don''t you make a patch and see if it breaks anything :) -Jonathan. On 5/10/06, Jeff Hall <jeff.hall@digitalmaelstrom.net> wrote:> Hi all! > > I have come into a strange issue in Rails that I am hoping someone > can shed some light. > > To make a very long story short, I have been researching how to > override ActiveRecord::Base#read_attribute and write_attribute to > perform security checks at the model level (influenced by the > ModelSecurity generator). Shortly after implementing some code to > check this behavior, I began to see undefined behavior from rails. > The first request to read an attribute would be denied correctly, > however further requests would proceed without fail. > > After a lot of searching, I think I have traced the problem down to > the generated read methods. When > ActiveRecord::Base#generate_read_methods is false, all is well. When > its true, my migraine returns. > > More research, reading, and I come to this curiosity... > > ActiveRecord::Base#define_read_method(symbol, attr_name, column) > generates reader code for an attribute after Rails realizes there > should be a reader method for it, correct? But the generated reader > method body does not call read_attribute... instead it accesses the > attribute directly and performs a type cast (if necessary) > > Is this intended? Am I missing something that read_attribute is > doing that is not appropriate for a defined attribute reader method? > > It''s near trivial to draft a patch for this - however, I wanted to > make sure I wasn''t vastly underestimating something. > Comments? > > Thanks everyone! > -- > Jeff Hall > Technology Consultant > Digital Maelstrom > > Email: jeff.hall@digitalmaelstrom.net > Jabber/XMPP: jeff.hall@digitalmaelstrom.net > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core -- Jeff Hall Technology Consultant Digital Maelstrom Email: jeff.hall@digitalmaelstrom.net Jabber/XMPP: jeff.hall@digitalmaelstrom.net
> Inline type casting tailored to each attribute''s column type is > faster than does-it-all read_attribute. It''s for performance alone:Jeremy, This is a very good point. However, I am concerned about the origin of this whole discussion. Is it desirable that if the behavior of read_attribute were to change - the new behavior would trigger on the first access to a reader method, but successive calls to the reader would ignore the new behavior entirely. That seems to me it could be the source of immense pain for the sake of performance (perhaps just my pain, but I now know how to address the issue) Thoughts? -- Jeff Hall
> That way you can simply override read_attribute_before_type_cast > and it will affect both read_attribute and the generated read_methods. Thoughts?As I see it, the inconsistancy exists in one of two places. If typecasting as performed by read_attribute really impacts performance undesirbly... then the call made by method_missing to return the attribute value during the reader''s first call should be changed to match what is used in the generated reader methods. Does this stand to reason? -Jeff
Sounds good. I can''t see any obvious reason why the current implementation would be a lot faster than simply using read_attribute. I''ll have a look at the ticket once you''ve posted it. -Jonathan. On 5/10/06, Jeffrey Hall <jeff.hall@digitalmaelstrom.net> wrote:> Jonathan, > > Patch doesn''t appear to break anything. All the unit tests run green. > > However, Marcel suggested that Stefan implemented this method intentionally for performance reasons. I will submit the patch as a Trac ticket - maybe then someone more skilled at benchmarking than I can see if any noticable performance hit is incurred. Perhaps then my nightmare with this code section will end. > > Thanks - cheers! > > -- Jeff > > ----- Original Message ----- > From: Jonathan Viney <jonathan.viney@gmail.com> > To: rails-core@lists.rubyonrails.org > Sent: Tuesday, May 9, 2006 8:12:13 PM GMT-0600 > Subject: Re: [Rails-core] Issue in ActiveRecord generated reader methods > > I agree that seems odd. I can''t see any reason not to use > read_attribute there, as it will type cast if necessary. > define_read_method could be simplified and Column#type_cast_code > should be unnecesary.... perhaps. > > Currently, define_read_method uses @attributes.has_key? to verify that > the attribute is present and raises an exception if it''s not. You''d > probably want to leave that check in there. > > Why don''t you make a patch and see if it breaks anything :) > > -Jonathan. > > On 5/10/06, Jeff Hall <jeff.hall@digitalmaelstrom.net> wrote: > > Hi all! > > > > I have come into a strange issue in Rails that I am hoping someone > > can shed some light. > > > > To make a very long story short, I have been researching how to > > override ActiveRecord::Base#read_attribute and write_attribute to > > perform security checks at the model level (influenced by the > > ModelSecurity generator). Shortly after implementing some code to > > check this behavior, I began to see undefined behavior from rails. > > The first request to read an attribute would be denied correctly, > > however further requests would proceed without fail. > > > > After a lot of searching, I think I have traced the problem down to > > the generated read methods. When > > ActiveRecord::Base#generate_read_methods is false, all is well. When > > its true, my migraine returns. > > > > More research, reading, and I come to this curiosity... > > > > ActiveRecord::Base#define_read_method(symbol, attr_name, column) > > generates reader code for an attribute after Rails realizes there > > should be a reader method for it, correct? But the generated reader > > method body does not call read_attribute... instead it accesses the > > attribute directly and performs a type cast (if necessary) > > > > Is this intended? Am I missing something that read_attribute is > > doing that is not appropriate for a defined attribute reader method? > > > > It''s near trivial to draft a patch for this - however, I wanted to > > make sure I wasn''t vastly underestimating something. > > Comments? > > > > Thanks everyone! > > -- > > Jeff Hall > > Technology Consultant > > Digital Maelstrom > > > > Email: jeff.hall@digitalmaelstrom.net > > Jabber/XMPP: jeff.hall@digitalmaelstrom.net > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > -- > Jeff Hall > Technology Consultant > Digital Maelstrom > > Email: jeff.hall@digitalmaelstrom.net > Jabber/XMPP: jeff.hall@digitalmaelstrom.net > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
> But, I don''t think that will solve your original problem of needing to > be able to override the read methods ... ?Haha - well as far as I can tell, my problem lies only in the fact that the two calls are different. My code works on the first reader call and breaks on any successive calls. If the generated reader methods use read_attribute, then there is a single common place to change the behavior of ActiveRecord to read attributes. Savy? I just was not aware if there is a specific reason they differ. -- Jeff
On May 9, 2006, at 6:12 PM, Jonathan Viney wrote:> I agree that seems odd. I can''t see any reason not to use > read_attribute there, as it will type cast if necessary. > define_read_method could be simplified and Column#type_cast_code > should be unnecesary.... perhaps.Inline type casting tailored to each attribute''s column type is faster than does-it-all read_attribute. It''s for performance alone: you may disable generated readers with no loss of functionality (though you may consider generating your own..) jeremy
Thanks a bunch for all your input. If I have further issues, I''m sure that I will come running back! :) -- Jeff
So it simply puts the type casting code into the definition of each read method to avoid calling Column#type_cast_code each time the read is called? How much difference does that really make? Another solution to this may be to change read_attribute to use read_attribute_before_type_cast rather than looking up @attributes directly. Line 1825 of base.rb changes from if !(value = @attributes[attr_name]).nil? to: if !(value = read_attribute_before_type_cast(attr_name)).nil? And similarly the generated reader to use read_attribute_before_type_cast instead of @attributes. That way you can simply override read_attribute_before_type_cast and it will affect both read_attribute and the generated read_methods. Thoughts? -Jonathan On 5/10/06, Jeremy Kemper <jeremy@bitsweat.net> wrote:> On May 9, 2006, at 6:12 PM, Jonathan Viney wrote: > > I agree that seems odd. I can''t see any reason not to use > > read_attribute there, as it will type cast if necessary. > > define_read_method could be simplified and Column#type_cast_code > > should be unnecesary.... perhaps. > > Inline type casting tailored to each attribute''s column type is > faster than does-it-all read_attribute. It''s for performance alone: > you may disable generated readers with no loss of functionality > (though you may consider generating your own..) > > jeremy > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
Yeah, it is a bit inconsistant that the first call ends up executing different code than a subsequent call. method_missing should probably be changed to address this. But, I don''t think that will solve your original problem of needing to be able to override the read methods ... ? -Jonathan. On 5/10/06, Jeffrey Hall <jeff.hall@digitalmaelstrom.net> wrote:> > That way you can simply override read_attribute_before_type_cast > > and it will affect both read_attribute and the generated read_methods. Thoughts? > > As I see it, the inconsistancy exists in one of two places. If typecasting as performed by read_attribute really impacts performance undesirbly... then the call made by method_missing to return the attribute value during the reader''s first call should be changed to match what is used in the generated reader methods. > > Does this stand to reason? > -Jeff > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
Jeff Hall wrote:> Hi all! > > I have come into a strange issue in Rails that I am hoping someone can > shed some light. > > To make a very long story short, I have been researching how to > override ActiveRecord::Base#read_attribute and write_attribute to > perform security checks at the model level (influenced by the > ModelSecurity generator). Shortly after implementing some code to > check this behavior, I began to see undefined behavior from rails. > The first request to read an attribute would be denied correctly, > however further requests would proceed without fail. > > After a lot of searching, I think I have traced the problem down to > the generated read methods. When > ActiveRecord::Base#generate_read_methods is false, all is well. When > its true, my migraine returns. > > More research, reading, and I come to this curiosity... > > ActiveRecord::Base#define_read_method(symbol, attr_name, column) > generates reader code for an attribute after Rails realizes there > should be a reader method for it, correct? But the generated reader > method body does not call read_attribute... instead it accesses the > attribute directly and performs a type cast (if necessary) > > Is this intended? Am I missing something that read_attribute is doing > that is not appropriate for a defined attribute reader method?The chosen method is intentional, for performance reasons. The savings are due to 3 factors: 1. method_missing is avoided. 2. analysis of the method name inside method_missing is avoided 3. case analysis of the column type is avoided. for string columns, which are probably the vast majority of columns in any app, this means that the generated code will simply access the attributes hash. And this is way faster than doing this analysis over and over again. Taken together, this saves a *lot* of Ruby code being executed on each attribute access. Now the first 2 improvements would still be in place if the generated code would simply call read_attribute. But step 3 is very costly and should be avoided, if possible. I suggest that if you want to modify the generated reader code for your app, go ahead and monkey patch the method "define_read_method". I introduced this method specifically to be able to override it. I do that in my own app as well (but for different reasons). However, I strongly resist to introduce this change into core.> > It''s near trivial to draft a patch for this - however, I wanted to > make sure I wasn''t vastly underestimating something. > Comments?You''re underestimating the performance impact. I suggest you turn off reader generation and measure the impact using my railsbench package. Cheers, -- stefan
For performance reasons, the generated read method does not use read_attribute, so I doubt that it will be changed. Changing both read_attribute and the generated read method to use read_attribute_before_type_cast gives you a single point where you can intercept all read methods and shouldn''t hit performance to any noticeable degree. As Jeremy pointed out, you can simply turn off the generation of read methods causing read_attribute to always be called, but you lose some performance. -Jonathan. On 5/10/06, Jeffrey Hall <jeff.hall@digitalmaelstrom.net> wrote:> > But, I don''t think that will solve your original problem of needing to > > be able to override the read methods ... ? > > Haha - well as far as I can tell, my problem lies only in the fact that the two calls are different. My code works on the first reader call and breaks on any successive calls. If the generated reader methods use read_attribute, then there is a single common place to change the behavior of ActiveRecord to read attributes. Savy? > > I just was not aware if there is a specific reason they differ. > > -- Jeff > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >