after_initialize/after_find misfeature

rubyonrails-core
1

I'm sure it's deliberate that after_initialize gets called after an object is instantiated from the database, but I'm equally sure that it's a bad idea - if nothing else, initialize doesn't get called anywhere in the instantiate method chain.

More importantly though, it's a huge performance hit for any programmer who wants to do something like setting defaults on their object before it gets written to the database, or even checked for validity. At present, the only way to get callback behaviour that triggers only when an object is first made is to do something like:

def after_initialize   if new_record?     do_stuff   end end

But now she is paying the cost of a method call not only when an object is initialized (which is fine, because the method actually _does_) something, but also every time objects of that class are pulled from the database (an eventuality which is already handled by the after_find callback.

Being strict and only calling after_initialize from initialize_with_callbacks and leaving instantiate_with_callbacks only calling after_find can only make after_initialize more useful. If someone really does need behaviour common to both points in an object's lifecycle, it's easy enough to do:

after_find :common_behaviour after_initialize :common_behaviour and god is in his heaven and all is right with the world.

If the cost of changing the API in this way is deemed too high, please at least consider adding some callback that really is only called after initialization and not after instantiation. It's too useful a place to ignore.

2

More importantly though, it's a huge performance hit for any programmer who wants to do something like setting defaults on their object before it gets written to the database, or even checked for validity.

We have before_validation for this case.

If the cost of changing the API in this way is deemed too high, please at least consider adding some callback that really is only called after initialization and not after instantiation. It's too useful a place to ignore.

Taking the case of

1: @customer = Customer.new(params[:customer]) 2: @customer.do_something 3: @customer.save!

What we currently don't have is a hook that will fire before line 2, but not when retrieving from a database. Providing defaults before validation was a common case, and I believe that's where the before_validation hook came from. What other use cases do you have in mind?

The name after_initialize is probably a little misleading because it fires even when an object is retrieved from the database. Strictly speaking this is 'initialization', but from a user's perspective it's 'finding' or 'retrieval'. Perhaps at the very least we could give it a less confusing, more hazardous sounding name.

3

> More importantly though, it's a huge performance hit for any > programmer who wants to do something like setting defaults on their > object before it gets written to the database, or even checked for > validity.

We have before_validation for this case.

No, you don't.

class Foo < ActiveRecord::Base   def before_validate     self.attrib_with_default ||= 99   end end

p Foo.new.attrib_with_default # nil

The default doesn't get set until the object is about to be validated, but sometimes you need that default to be set correctly so that you can use an object before it's either validated or saved. And are you really suggesting that everyone should call 'valid?' on their objects if they want them to get their correct defaults set? Default setting belongs in the initialization phase, not in the validation phase.

> If the cost of changing the API in this way is deemed too high, please > at least consider adding some callback that really is only called > after initialization and not after instantiation. It's too useful a > place to ignore.

Taking the case of

1: @customer = Customer.new(params[:customer]) 2: @customer.do_something 3: @customer.save!

What we currently don't have is a hook that will fire before line 2, but not when retrieving from a database. Providing defaults before validation was a common case, and I believe that's where the before_validation hook came from. What other use cases do you have in mind?

Well, the obvious case is the case where you're implementing new in your controller. The typical boiler plate goes:

def new   @customer = Customer.new end

That @customer is never going to be validated, but it does make sense for it to have its defaults (which belong in the model and not the controller or the view) set correctly so that 'new.rhtml' doesn't need any knowledge of any default values when rendering the form.

The name after_initialize is probably a little misleading because it fires even when an object is retrieved from the database. Strictly speaking this is 'initialization', but from a user's perspective it's 'finding' or 'retrieval'. Perhaps at the very least we could give it a less confusing, more hazardous sounding name.

But 'after_initialize' implies "call this after you call the instance's initialize method", and it's apparent that, in the case of object retrieval, the initialize method simply isn't called (which makes sense, because the object is only initialized once, when it got created by calling its class's new method.)

4

Really hear you on that one.

If we need to deal with default values in a better way, why not something like...

class Foo < ActiveRecord::Base   default :attrib, :to => 1   default :another_attrib, :to => :another_attrib_defaulter

  protected   def another_attrib_defaulter     # ...   end end

The second parameter being a hash for some readability and potentially for some more features...

Thoughts?

5

Does default belong in the schema as it is essentially a db functionality? Or is there a case for more complex conditional logic?

In the latter case, what about using an overloaded attr reader instead?

courtenay

6

No it isn't. Consider the use case of the newly created 'template' object that's used by new.rhtml in some generic controller; this never goes near the database, but still needs to have its defaults correctly set.

7

def new @customer = Customer.new end

That @customer is never going to be validated, but it does make
sense for it to have its defaults (which belong in the model and not the controller or the view) set correctly so that 'new.rhtml' doesn't need any knowledge of any default values when rendering the form.

Really hear you on that one.

If we need to deal with default values in a better way, why not something like...

class Foo < ActiveRecord::Base default :attrib, :to => 1 default :another_attrib, :to => :another_attrib_defaulter

protected def another_attrib_defaulter   # ... end end

The second parameter being a hash for some readability and
potentially for some more features...

Thoughts?

Does default belong in the schema as it is essentially a db functionality?

No it isn't. Consider the use case of the newly created 'template' object that's used by new.rhtml in some generic controller; this never goes near the database, but still needs to have its defaults correctly set.

Right, that's the point i was missing.. but couldn't you do that as an
overloaded reader?

   def name      read_attribute[:name] || "default name"    end

I do like the idea of wrapping that functionality in your syntax as
above, makes it a little quicker

courtenay

8

The default doesn't get set until the object is about to be validated, but sometimes you need that default to be set correctly so that you can use an object before it's either validated or saved. And are you really suggesting that everyone should call 'valid?' on their objects if they want them to get their correct defaults set? Default setting belongs in the initialization phase, not in the validation phase.

You'll note that this is line two mentioned below. Your original email cited "before checked for validity", and before_validation does that. Either way, we're talking past one another here.

But 'after_initialize' implies "call this after you call the instance's initialize method", and it's apparent that, in the case of object retrieval, the initialize method simply isn't called (which makes sense, because the object is only initialized once, when it got created by calling its class's new method.)

This thin and literal interpretation of the word 'initialize' makes sense only to those who care deeply about the implementation of AR. If we were to refactor Base.instantiate(record) to use .new, I'm not sure that many users would care, nor would they expect different callbacks to be called.

I'm not rejecting the need for defaults beyond what the schema provides, merely suggesting that there's probably a better name out there for the callbacks to achieve that behaviour.

9

In the latter case, what about using an overloaded attr reader instead?

you can use this in some situations, but if you have interdependent defaults for three or four fields it can get really confusing. Better to have that logic put in one common place, and a callback is probably the right place for it.

10

> But 'after_initialize' implies "call this after you call the > instance's initialize method", and it's apparent that, in the case of > object retrieval, the initialize method simply isn't called (which > makes sense, because the object is only initialized once, when it got > created by calling its class's new method.)

This thin and literal interpretation of the word 'initialize' makes sense only to those who care deeply about the implementation of AR. If we were to refactor Base.instantiate(record) to use .new, I'm not sure that many users would care, nor would they expect different callbacks to be called.

I really do have to take issue with this; I would argue that the current behaviour of after_initialize is based on a 'thin and literal interpretation of the word "initialize"'. Decanting an object from storage is not initialization; initialization is what happens when an object is first created, back before it got poured into the storage jar.

Consider a woollen gansey's life cycle. It starts off as a length of wool which is then knitted (initialized) on circular needles into a delightful seemless garment. During cold weather, I wear it a lot. During warm weather, I put it in a drawer (storage). When it gets cold again, I don't take the gansey out of the drawer and knit it again from scratch.

Conceptually, that's what's happening with my active record object. It's knitted, shoved into the drawer and pulled out again as needed. It only gets knitted once. Of course, what _really_ happens is that my 'gansey' is stored in the teleporter's pattern buffers (okay, if you insist, we'll call it a database) and reconstituted from an entirely new set of atoms when I need it later. But that doesn't matter. As far as the wearer is concerned, it's the same gansey.

Plus, there's the argument from orthogonality... If after_initialize only fires when the gansey is first knitted, then I can add behaviour that fires after knitting and drawer removal by doing:

after_find :do_something after_initialize :do_something

But when after_initialize fires after find as well, I have to write

after_initialize do |obj|   if obj.new_record?     ...   end end

and pay the cost of the method call every time I get a gansey out of the drawer anyway. No fun.

11

I really do have to take issue with this; I would argue that the current behaviour of after_initialize is based on a 'thin and literal interpretation of the word "initialize"'. Decanting an object from storage is not initialization; initialization is what happens when an object is first created, back before it got poured into the storage jar.

I can buy your argument, but if I do so, there doesn't seem to be a need for after_initialize:

def initialize(attrs)   do_stuff   super end

All we'd be doing is reinventing an initialize method which hid the arguments. Doesn't seem particularly necessary, especially at the cost of backwards compatibility for the people who use after_initialize at present.

12

Have you actually tried that? I have. It's a nightmare. It's long enough ago now that I can't remember exactly what the nightmare was, but it was definitely no fun at all.

A cursory examination ActiveRecord::Base suggests that it's because the various attributes don't work until after '@attributes = attributes_from_column_definition' has been evaluated, and that gets evaluated when you call @super.

I accept that there are people who rely on the current behaviour of after_initialize, so the trick will probably be to come up with a good name for the callback. 'after_new'?

Another option might be to turn ActiveRecord::Base#initialize into a template method along the lines of:

  def initialize(attributes = nil)     @attributes = attributes_from_column_definition     @new_record = true     ensure_proper_type     initialize_defaults if self.respond_to?(:initialize_defaults)     self.attributes = attributes unless attributes.nil?     yield self if block_given?   end

which has the advantage of making the callback after the bones of the object are in place, but before any @attributes are assigned to.

13

I wrote a plugin a while ago that does exactly that:

http://svn.viney.net.nz/things/rails/plugins/active_record_defaults/

-Jonathan.

14

If you want the equivalent of after_initialize, wouldn't you want to do_stuff after calling super? In any case, if you're feeling funky, you can do:

def initialize(attrs = {}, &block)   attrs[:funky_default] ||= 'my funky default'   super end

But I much prefer:

def initialize(attrs = {}, &block)   super   self.funky_default ||= 'my funky default' end

Tom

15

If you want the equivalent of after_initialize, wouldn't you want to do_stuff after calling super? In any case, if you're feeling funky, you can do:

def initialize(attrs = {}, &block)   attrs[:funky_default] ||= 'my funky default'   super end

But I much prefer:

def initialize(attrs = {}, &block)   super   self.funky_default ||= 'my funky default' end

Sorry, that's what I meant, if you do stuff before @attributes is initialized you'll get some nasty surprises. Without cases which can't be solved by this pattern, I'm not sure that adding a new callback is justified.

16

Michael Koziarski wrote:

If you want the equivalent of after_initialize, wouldn't you want to do_stuff after calling super? In any case, if you're feeling funky, you can do:

def initialize(attrs = {}, &block)   attrs[:funky_default] ||= 'my funky default'   super end

But I much prefer:

def initialize(attrs = {}, &block)   super   self.funky_default ||= 'my funky default' end

Sorry, that's what I meant, if you do stuff before @attributes is initialized you'll get some nasty surprises. Without cases which can't be solved by this pattern, I'm not sure that adding a new callback is justified.

Sorry but I must respectfully disagree. This issue caused me plenty of confusion when I started out with rails, I've seen lots of other people being confused about it, and you even admit here that if you don't do it right you'll get "nasty surprises". Any good framework should try to avoid the potential for nasty surprises where possible.

Now, Jonathan's plugin mentioned earlier in this thread sounds like a pretty good solution, but I had never heard of it until now. Maybe core could consider adopting that. But, I would say that setting default values for model objects is an *extremely* common practice... needed on just about every project. It seems like one way or another it would be nice to help rails developers avoid some gotchas with something that's completely straightforward in most other languages/frameworks.

Ben

17

But, I would say that setting default values for model objects is an *extremely* common practice... needed on just about every project.

Just wondering how did you achieve it previous to hearing about Jonathan's plugin ?

Why add a new callback when all you need is 4 lines of documentation on how to do it ?

18

Pratik wrote:

But, I would say that setting default values for model objects is an *extremely* common practice... needed on just about every project.

Just wondering how did you achieve it previous to hearing about Jonathan's plugin ?

Why add a new callback when all you need is 4 lines of documentation on how to do it ?

I do the after_initialize with an "if new_record?" in it... got that from a great post by Wes Gamble a while back where he outlined all the gotchas with setting default values. It was actually my impression from that post that one could *not* override initialize.

So, my point really is that needing to override after_initialize, but remember to put the check for a new_record in there (plus that fact that it's run on every load) is silly... and counter-intuitive. But, if overriding initialize is really completely safe and won't cause lots of heartache then fine. I'm not one to stampede to adding stuff to the framework.

I guess the question is, how bad a deal is it for people who miss the four lines of documentation versus how much of a change is to to provide a simple, obvious way to set model defaults. This is just one of those things that I need to explain to rails newcomers that seems silly and a bit embarrassing.

Anyway, I think I'll definitely check out Jonathan's plugin on my next app... looks all nice and "DSL-ish". :slight_smile:

Ben

19

The problem with the plugin approach here is that it's monkeypatching a pretty fundamental method, and undocumented, method. And that makes it fragile; if the implementation of initialize ever changes (and there's no reason that it shouldn't), the plugin breaks.

Still, if it's the only game in town...

20

Piers Cawley wrote:

Anyway, I think I'll definitely check out Jonathan's plugin on my next app... looks all nice and "DSL-ish". :slight_smile:

The problem with the plugin approach here is that it's monkeypatching a pretty fundamental method, and undocumented, method. And that makes it fragile; if the implementation of initialize ever changes (and there's no reason that it shouldn't), the plugin breaks.

Still, if it's the only game in town...

Hmm, yeah that's a good point... always the risk with monkeypatching.

So, Piers, you're the one that started this whole thing... if it really is safe to do:

class MyModel < ActiveRecord::Base    def initialize(attrs = {}, &block)      super      # all my initialization stuff here    end end

and this is documented in the AR::Base docs, then this does kind of seem like enough. I mean, it's what I did all the time in my java life.

I'd just swear that someone told me the holy beasts of doom would swarm down upon me if I ever tried to override AR::Base#initialize.

Ben