New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

association methods are now generated in modules #3636

Merged
merged 6 commits into from Nov 29, 2011

Conversation

Projects
None yet
9 participants
@joshsusser
Contributor

joshsusser commented Nov 15, 2011

Instead of generating association methods directly in the model
class, they are generated in an anonymous module which
is then included in the model class. There is one such module
for each association. The only subtlety is that the
generated_attributes_methods module (from ActiveModel) must
be forced to be included before association methods are created
so that attribute methods will not shadow association methods.

The advantage to this approach is that it is now possible to override
a generated method and still call the original generated method via super.

This change is relatively straightforward from a code organization
perspective, but it has a high potential for causing grief in applications
that make assumptions about how association methods are generated
or ordering of method definitions. Therefore it probably needs some
stress-testing in real applications to see how it goes.

@justinko

This comment has been minimized.

Show comment
Hide comment
@justinko

justinko Nov 15, 2011

Contributor

Oh man this is much cleaner. Nice stuff.

Contributor

justinko commented Nov 15, 2011

Oh man this is much cleaner. Nice stuff.

@maxim

This comment has been minimized.

Show comment
Hide comment
@maxim

maxim Nov 15, 2011

Contributor

I agree, this always felt awkward from practical perspective. Although depends how you look at it. attr_accessor (and the like) don't include modules either. Wouldn't it be less surprising to let associations behave as accessor declarations? Or why not go further and make accessor methods into modules as well? Just curious where would the line be.

Contributor

maxim commented Nov 15, 2011

I agree, this always felt awkward from practical perspective. Although depends how you look at it. attr_accessor (and the like) don't include modules either. Wouldn't it be less surprising to let associations behave as accessor declarations? Or why not go further and make accessor methods into modules as well? Just curious where would the line be.

@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 15, 2011

Contributor

@maxim: Surprise! Attribute accessor methods are already generated in their own module. I wasn't aware of that either until I dug into the code for this change. With this change, association methods will be consistent with that.

Contributor

joshsusser commented Nov 15, 2011

@maxim: Surprise! Attribute accessor methods are already generated in their own module. I wasn't aware of that either until I dug into the code for this change. With this change, association methods will be consistent with that.

@courtenay

This comment has been minimized.

Show comment
Hide comment
@courtenay

courtenay Nov 15, 2011

Contributor

Any time you can get rid of code that looks like class_eval <<-RUBY, __FILE__, __LINE__ + 1 you're having a good day.

Contributor

courtenay commented Nov 15, 2011

Any time you can get rid of code that looks like class_eval <<-RUBY, __FILE__, __LINE__ + 1 you're having a good day.

@maxim

This comment has been minimized.

Show comment
Hide comment
@maxim

maxim Nov 15, 2011

Contributor

@joshsusser Weird, doesn't seem to work for me. Unless it's something on the master?

(Trip is an existing ActiveRecord model)

Loading development environment (Rails 3.1.1)
>> class Trip
>> attr_accessor :foo
>> def foo
>> super
>> end
>> end
=> nil
>> Trip.new.foo
NoMethodError: super: no superclass method `foo' for #<Trip:0x007ff54ef8c8c0>
Contributor

maxim commented Nov 15, 2011

@joshsusser Weird, doesn't seem to work for me. Unless it's something on the master?

(Trip is an existing ActiveRecord model)

Loading development environment (Rails 3.1.1)
>> class Trip
>> attr_accessor :foo
>> def foo
>> super
>> end
>> end
=> nil
>> Trip.new.foo
NoMethodError: super: no superclass method `foo' for #<Trip:0x007ff54ef8c8c0>
@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 15, 2011

Contributor

@maxim: Maybe attr_accessor doesn't work the same way as accessor methods for actual attributes in the database. I'd have to think about whether it makes sense to modularize attr_accessor methods too.

Contributor

joshsusser commented Nov 15, 2011

@maxim: Maybe attr_accessor doesn't work the same way as accessor methods for actual attributes in the database. I'd have to think about whether it makes sense to modularize attr_accessor methods too.

@maxim

This comment has been minimized.

Show comment
Hide comment
@maxim

maxim Nov 15, 2011

Contributor

@joshsusser not arguing against it though, just curious. It's "simple vs easy" type of thing.

Contributor

maxim commented Nov 15, 2011

@joshsusser not arguing against it though, just curious. It's "simple vs easy" type of thing.

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Nov 15, 2011

@joshsusser It's better to create one such module for a model and then define new readers/writers every time an association is created and include it again. This way you will avoid having a lot of anonymous modules.

solnic commented Nov 15, 2011

@joshsusser It's better to create one such module for a model and then define new readers/writers every time an association is created and include it again. This way you will avoid having a lot of anonymous modules.

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Nov 15, 2011

Member

Thanks for doing this - have been meaning to for a while and never got around to it. Will review within the next week.

Member

jonleighton commented Nov 15, 2011

Thanks for doing this - have been meaning to for a while and never got around to it. Will review within the next week.

@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 15, 2011

Contributor

Since @solnic and others have asked... the reason for using a module for each association instead of one module for all of them is that it was the simplest way to code it, and there doesn't seem to be any cost for having lots of modules except for a bit of memory used. I believe all current Ruby implementations optimize method lookup so that having a deep inheritance hierarchy costs little to nothing at runtime.

Contributor

joshsusser commented Nov 15, 2011

Since @solnic and others have asked... the reason for using a module for each association instead of one module for all of them is that it was the simplest way to code it, and there doesn't seem to be any cost for having lots of modules except for a bit of memory used. I believe all current Ruby implementations optimize method lookup so that having a deep inheritance hierarchy costs little to nothing at runtime.

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Nov 15, 2011

@joshsusser the reason I pointed this out was that we did this optimization in DataMapper project and as far as I remember it had a noticeable impact on specs performance. It probably doesn't really matter until you create A LOT of objects. Nevertheless it's a fantastic improvement. I hope it'll get merged in soon.

solnic commented Nov 15, 2011

@joshsusser the reason I pointed this out was that we did this optimization in DataMapper project and as far as I remember it had a noticeable impact on specs performance. It probably doesn't really matter until you create A LOT of objects. Nevertheless it's a fantastic improvement. I hope it'll get merged in soon.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 15, 2011

Contributor

Sweet! Also +1 for compiling all associations into one module. In an app with 30 models and about ~100 associations, it will probably make a difference (including in boot time, vide the benchmarks that showed a rails app with 100 scaffolds on boot was slow mainly because of loading and including 100 helpers).

Contributor

josevalim commented Nov 15, 2011

Sweet! Also +1 for compiling all associations into one module. In an app with 30 models and about ~100 associations, it will probably make a difference (including in boot time, vide the benchmarks that showed a rails app with 100 scaffolds on boot was slow mainly because of loading and including 100 helpers).

@JEG2

This comment has been minimized.

Show comment
Hide comment
@JEG2

JEG2 Nov 15, 2011

It's worth noting that things like attr_accessor and this pull request are different use cases.

For example, if you wanted to use attr_reader (a cousin of attr_accessor, which are both provided by Ruby, not Rails) it is because you are defining a class and want to take a shortcut instead of writing out a rote method definition. This shortcut works when you just want to access the instance variable. If you wanted to do more, say cast the value on the way out, you would just skip the shortcut and define the method normally. It doesn't make sense to have Ruby define the wrong version and you override it in the same class definition.

Now, the code Josh changed is a different scenario. Rails is giving you ways to describe your schema. It will then use that description to build the needed interface methods. It does make perfect sense that you could then need to override those, adding additional behavior to the raw database access Rails is providing.

That's why this change from Josh is such a great idea. I'm all for it.

JEG2 commented Nov 15, 2011

It's worth noting that things like attr_accessor and this pull request are different use cases.

For example, if you wanted to use attr_reader (a cousin of attr_accessor, which are both provided by Ruby, not Rails) it is because you are defining a class and want to take a shortcut instead of writing out a rote method definition. This shortcut works when you just want to access the instance variable. If you wanted to do more, say cast the value on the way out, you would just skip the shortcut and define the method normally. It doesn't make sense to have Ruby define the wrong version and you override it in the same class definition.

Now, the code Josh changed is a different scenario. Rails is giving you ways to describe your schema. It will then use that description to build the needed interface methods. It does make perfect sense that you could then need to override those, adding additional behavior to the raw database access Rails is providing.

That's why this change from Josh is such a great idea. I'm all for it.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 15, 2011

Contributor

@JEG2 agreed. Exactly my thoughts.

Contributor

josevalim commented Nov 15, 2011

@JEG2 agreed. Exactly my thoughts.

@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 15, 2011

Contributor

Doing an optimization to bundle all accessor methods in a single module wouldn't be too terrible, but I'd like to prove that this change won't cause problems first before investing that effort. I already have some ideas how to do that optimization, shouldn't be too hard.

Contributor

joshsusser commented Nov 15, 2011

Doing an optimization to bundle all accessor methods in a single module wouldn't be too terrible, but I'd like to prove that this change won't cause problems first before investing that effort. I already have some ideas how to do that optimization, shouldn't be too hard.

@maxim

This comment has been minimized.

Show comment
Hide comment
@maxim

maxim Nov 15, 2011

Contributor

@JEG2 I like this perspective. No more concerns here. : )

Contributor

maxim commented Nov 15, 2011

@JEG2 I like this perspective. No more concerns here. : )

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Nov 15, 2011

Member

@joshsusser

I agree with what others have said about using only one module. Ideally we'd have a single module that we can just throw methods into wherever in Active Record they get generated. As you said, the attributes stuff already generates methods in its own module. There's also stuff like nested attributes, composed_of, etc, that all generate methods, and would all benefit from this sort of change in the future.

So I wonder if you could incorporate into your patch a change that defines a method on ActiveRecord::Base which generates/includes this module (like the generated_attribute_methods method in ActiveModel::AttributeMethods). Then, we could redefine generated_attribute_methods in AR to reference this module also.

Perhaps you can generate this module in such a way that it is a named constant, so that it's obvious what it is when people inspect Model.ancestors.

Please also add a CHANGELOG entry for this.

It would also be great to add to the docs to indicate to users that they can extend the generated methods using super.

Member

jonleighton commented Nov 15, 2011

@joshsusser

I agree with what others have said about using only one module. Ideally we'd have a single module that we can just throw methods into wherever in Active Record they get generated. As you said, the attributes stuff already generates methods in its own module. There's also stuff like nested attributes, composed_of, etc, that all generate methods, and would all benefit from this sort of change in the future.

So I wonder if you could incorporate into your patch a change that defines a method on ActiveRecord::Base which generates/includes this module (like the generated_attribute_methods method in ActiveModel::AttributeMethods). Then, we could redefine generated_attribute_methods in AR to reference this module also.

Perhaps you can generate this module in such a way that it is a named constant, so that it's obvious what it is when people inspect Model.ancestors.

Please also add a CHANGELOG entry for this.

It would also be great to add to the docs to indicate to users that they can extend the generated methods using super.

@jonleighton

View changes

Show outdated Hide outdated ...record/lib/active_record/associations/builder/has_and_belongs_to_many.rb
@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Nov 15, 2011

Member

Another thing: please could you add an explicit test that extending one of these methods via super actually works.

Member

jonleighton commented Nov 15, 2011

Another thing: please could you add an explicit test that extending one of these methods via super actually works.

@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 15, 2011

Contributor

@jonleighton: Good feedback. I think putting all the association methods in one module is a good way to go. If we're comfortable with this level of change and the potential for disruption I can go ahead and work on that. But I do not think it's a good idea to combine all AR generated methods into a single module for a model class. That will force a particular order for generating methods that may have name clashes. The example in the Rails test fixtures is computers(:workstation).developer - "developer" is the name of both the foreign key field and the belongs_to association. The current order in which those methods get generated breaks the association unless the association methods model/class inherits from the attribute methods module. I think it's better to have a module for attribute methods and a different module for association methods. Offhand I'm not sure what to do for the other kinds of generated methods you mentioned - they might work in one of those modules, or might want to go in another. But I think some kind of precedence ordering should be used, not just the temporal order of method generation.

Contributor

joshsusser commented Nov 15, 2011

@jonleighton: Good feedback. I think putting all the association methods in one module is a good way to go. If we're comfortable with this level of change and the potential for disruption I can go ahead and work on that. But I do not think it's a good idea to combine all AR generated methods into a single module for a model class. That will force a particular order for generating methods that may have name clashes. The example in the Rails test fixtures is computers(:workstation).developer - "developer" is the name of both the foreign key field and the belongs_to association. The current order in which those methods get generated breaks the association unless the association methods model/class inherits from the attribute methods module. I think it's better to have a module for attribute methods and a different module for association methods. Offhand I'm not sure what to do for the other kinds of generated methods you mentioned - they might work in one of those modules, or might want to go in another. But I think some kind of precedence ordering should be used, not just the temporal order of method generation.

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Nov 15, 2011

Member

Ok, yes, I agree with you about having the attributes module higher up the ancestor chain than the associations module, because we always want attributes to get overridden by other stuff if necessary.

But I think it's okay in theory to use the same module for associations, nested attributes, composed of etc. If those things are clashing with each other I actively want us to be redefining methods in such a way as to trigger a warning in ruby. So maybe you can name you single-module thinger in a generic way in anticipation of the flood of pull requests we are imminently going to receive for those other things.

Member

jonleighton commented Nov 15, 2011

Ok, yes, I agree with you about having the attributes module higher up the ancestor chain than the associations module, because we always want attributes to get overridden by other stuff if necessary.

But I think it's okay in theory to use the same module for associations, nested attributes, composed of etc. If those things are clashing with each other I actively want us to be redefining methods in such a way as to trigger a warning in ruby. So maybe you can name you single-module thinger in a generic way in anticipation of the flood of pull requests we are imminently going to receive for those other things.

joshsusser added some commits Nov 15, 2011

association methods are now generated in modules
Instead of generating association methods directly in the model
class, they are generated in an anonymous module which
is then included in the model class. There is one such module
for each association. The only subtlety is that the
generated_attributes_methods module (from ActiveModel) must
be forced to be included before association methods are created
so that attribute methods will not shadow association methods.
@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 16, 2011

Contributor

I took a crack at generating all association methods in a single module per model class. It was pretty easy, right up until I ran into the has_and_belongs_to_many destroy_associations hook. HABTM associations have been generating a module per association for quite a while. They do it so that multiple HABTMs in a class can all have their destroy_associations hooks run in series. Note that all those methods have the same name - they are all in separate modules that are ancestors of the same class, and each one calls the next using super. So to change this patch to use one module per class, I'll have to come up with another way to handle this case. Either that or we can punt on this one case - since HABTM isn't used very often, maybe multiple modules won't offend anyone.

Also, I wanted to respond to a few comments above.

  1. Programmatically creating modules is not the same as loading a bunch of helper modules from the file system so there shouldn't be any significant impact to startup time. I haven't noticed any running tests.

  2. As I mentioned before, having a bunch of modules should have negligible impact on performance. All current Ruby VMs optimize method dispatch so inheritance depth doesn't matter, except maybe while you have cold caches the first time you run a piece of code. That might have a tiny impact on how long tests take to run, but I doubt it would be statistically significant. If you think you are measuring a performance impact from number of modules, you're probably noticing something else.

Anyway, I did add an explicit test that shows a model method using super to call the association method.

Contributor

joshsusser commented Nov 16, 2011

I took a crack at generating all association methods in a single module per model class. It was pretty easy, right up until I ran into the has_and_belongs_to_many destroy_associations hook. HABTM associations have been generating a module per association for quite a while. They do it so that multiple HABTMs in a class can all have their destroy_associations hooks run in series. Note that all those methods have the same name - they are all in separate modules that are ancestors of the same class, and each one calls the next using super. So to change this patch to use one module per class, I'll have to come up with another way to handle this case. Either that or we can punt on this one case - since HABTM isn't used very often, maybe multiple modules won't offend anyone.

Also, I wanted to respond to a few comments above.

  1. Programmatically creating modules is not the same as loading a bunch of helper modules from the file system so there shouldn't be any significant impact to startup time. I haven't noticed any running tests.

  2. As I mentioned before, having a bunch of modules should have negligible impact on performance. All current Ruby VMs optimize method dispatch so inheritance depth doesn't matter, except maybe while you have cold caches the first time you run a piece of code. That might have a tiny impact on how long tests take to run, but I doubt it would be statistically significant. If you think you are measuring a performance impact from number of modules, you're probably noticing something else.

Anyway, I did add an explicit test that shows a model method using super to call the association method.

@emmanuel

This comment has been minimized.

Show comment
Hide comment
@emmanuel

emmanuel Nov 16, 2011

@joshsusser

Nice work!

You might consider making the module 'smart', ie., don't just call Module#define_method on it over and over, give the Module itself methods that define the desired methods in the including (target) class/module. I've just started experimenting with this technique.

It's a bit of a brain-twist at first: you define Module instance methods which in turn dynamically define instance methods in the including class, but it works great. Most importantly, it lets you collect the dynamic method definition logic in one place and name it with intention-revealing selectors :D.

Also worth noting: if you define #inspect on the module, you'll get meaningful output from FooModel.ancestors (more meaningful than #<Module 0x00000>).

emmanuel commented Nov 16, 2011

@joshsusser

Nice work!

You might consider making the module 'smart', ie., don't just call Module#define_method on it over and over, give the Module itself methods that define the desired methods in the including (target) class/module. I've just started experimenting with this technique.

It's a bit of a brain-twist at first: you define Module instance methods which in turn dynamically define instance methods in the including class, but it works great. Most importantly, it lets you collect the dynamic method definition logic in one place and name it with intention-revealing selectors :D.

Also worth noting: if you define #inspect on the module, you'll get meaningful output from FooModel.ancestors (more meaningful than #<Module 0x00000>).

@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 16, 2011

Contributor

@emmanuel: Cleaning that up is next on my list. I thought about enhancing the module behavior to show the association name. I at least want to make define_method public so there aren't all those sends making the code ugly.

Contributor

joshsusser commented Nov 16, 2011

@emmanuel: Cleaning that up is next on my list. I thought about enhancing the module behavior to show the association name. I at least want to make define_method public so there aren't all those sends making the code ugly.

@emmanuel

This comment has been minimized.

Show comment
Hide comment
@emmanuel

emmanuel Nov 16, 2011

Making Module#define_method public would help the code read a bit more clearly.

That said, I think it's better still to treat the Module (@mixin) as an object with its own encapsulation. In this case, its responsibility is to define methods according to certain params (ie., Association::Builder#define_*).

In any case, a win for ActiveRecord.

emmanuel commented Nov 16, 2011

Making Module#define_method public would help the code read a bit more clearly.

That said, I think it's better still to treat the Module (@mixin) as an object with its own encapsulation. In this case, its responsibility is to define methods according to certain params (ie., Association::Builder#define_*).

In any case, a win for ActiveRecord.

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Nov 16, 2011

Member

I think just leave the destroy_associations hook as it is for now, we can fix that up at a later date.

Member

jonleighton commented Nov 16, 2011

I think just leave the destroy_associations hook as it is for now, we can fix that up at a later date.

avoid warnings
This change uses Module.redefine_method as defined in ActiveSupport.
Making Module.define_method public would be as clean in the code, and
would also emit warnings when redefining an association. That is pretty
messy given current tests, so I'm leaving it for someone else to decide
what approach is better.
@joshsusser

This comment has been minimized.

Show comment
Hide comment
@joshsusser

joshsusser Nov 27, 2011

Contributor

I think this change is ready to go. One named module for all associations, docs, and changelog. There is still a separate module for each habtm destroy_associations method, as we agreed. There's a discussion to be had about whether to emit warnings when redefining an association, but that's probably a different change than this one. For now, I fell back to using Module.redefine_method since it is consistent with the old way of defining association methods.

Contributor

joshsusser commented Nov 27, 2011

I think this change is ready to go. One named module for all associations, docs, and changelog. There is still a separate module for each habtm destroy_associations method, as we agreed. There's a discussion to be had about whether to emit warnings when redefining an association, but that's probably a different change than this one. For now, I fell back to using Module.redefine_method since it is consistent with the old way of defining association methods.

super
end
def generated_feature_methods

This comment has been minimized.

@josevalim

josevalim Nov 28, 2011

Contributor

Do we set this constant in order to have a readable output on Model.ancestors?

@josevalim

josevalim Nov 28, 2011

Contributor

Do we set this constant in order to have a readable output on Model.ancestors?

This comment has been minimized.

@josevalim

josevalim Nov 28, 2011

Contributor

If I am not wrong, we were used to do the same thing for generated_attribute_methods and it caused memory leaks in development. I will try to find the relevant commits.

@josevalim

josevalim Nov 28, 2011

Contributor

If I am not wrong, we were used to do the same thing for generated_attribute_methods and it caused memory leaks in development. I will try to find the relevant commits.

This comment has been minimized.

@joshsusser

joshsusser Nov 28, 2011

Contributor

Yes, the module gets named MyModel::GeneratedFeatureMethods. But ActiveModel doesn't name the attributes module.

@joshsusser

joshsusser Nov 28, 2011

Contributor

Yes, the module gets named MyModel::GeneratedFeatureMethods. But ActiveModel doesn't name the attributes module.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 28, 2011

Contributor

Awesome, I took a quick look, everything looks great to me!

Contributor

josevalim commented Nov 28, 2011

Awesome, I took a quick look, everything looks great to me!

jonleighton added a commit that referenced this pull request Nov 29, 2011

Merge pull request #3636 from joshsusser/master
association methods are now generated in modules

@jonleighton jonleighton merged commit 2169603 into rails:master Nov 29, 2011

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Nov 29, 2011

Member

merged! :)

Member

jonleighton commented Nov 29, 2011

merged! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment