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

Refactor AM::AttributeMethods + AR::AttributeMethods using module builders #31394

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@shioyama
Copy link
Contributor

shioyama commented Dec 11, 2017

( This is a "lite" version of #30895 )

r? @matthewd

@matthewd This is in response to your comment. This implementation does not add any more modules than would currently be added, so that issue is gone.

I've basically implemented what I described later in that thread.

Summary

This is a large and substantial refactor of two core modules, ActiveModel::AttributeMethods and ActiveRecord::AttributeMethods. The inspiration for this PR is this blog post on the module builder pattern which I wrote a few months ago.

There are a few issues motivating this refactor, the main one being that AR::AttributeMethods and AM::AttributeMethods are heavily coupled, in such a way that (among other things) it makes it impossible to use them (or any other dependent modules like AM/AR Dirty) alongside each other. This is like a straightjacket on AR::Core models since it means you lose any flexibility that AM::AttributeMethods tries to give you.

I came face to face with this coupling when I first tried to make a change to AM::AttributeMethods in #30895, changing some private methods, and found that AR::AttributeMethods completely blew up because it depended on generated_attribute_methods being defined.

The other (related) motivation is to make these modules more encapsulated. Currently, both modules define many methods on the including class in order to "bootstrap" dynamic method definition, e.g. define_proxy_call, matched_attribute_method, generated_attribute_methods, attribute_method_matchers_matching, attribute_method_matchers_cache, etc. They also depend on class attributes like attribute_method_matchers.

Outline

Within the constraints of the current API, this PR attempts to overcome these issues by extracting much of the "method-defining" logic into two module builders, ActiveModel::AttributeMethodsBuilder and ActiveRecord::AttributeMethodsBuilder. These classes each subclass Module so they can define modules themselves, and since they contain all the logic they need to define methods on themselves (including their own matchers), they avoiding the coupling mentioned above. Also since the AR module builder is a subclass of the AM module builder, the relationship between the two of them is clearer (instead of overriding class methods on the model class, we override class methods on the module builder class).

By defining these builders and keeping the core functionality encapsulated within them, they become much more versatile. If they are used in isolation (using AM::AttributeMethodsBuilder.new etc), you can actually define ActiveModel attribute methods alongside ActiveRecord attribute methods on the same model. And you can also go further and define your own attribute methods builder that subclasses either of these classes.

Here's an example of how you could define attribute methods without including ActiveModel::AttributeMethods into a class, by including an instance of ActiveModel::AttributeMethodsBuilder like this:

class Person
  mod = ActiveModel::AttributeMethodsBuilder.new

  include mod

  mod.affix prefix: 'reset_', suffix: '_to_default!'
  mod.suffix '_contrived?'
  mod.prefix 'clear_'
  mod.define_attribute_methods :name

  attr_accessor :name

  def attributes
    { 'name' => @name }
  end

  private

  def attribute_contrived?(attr)
    true
  end

  def clear_attribute(attr)
    send("#{attr}=", nil)
  end

  def reset_attribute_to_default!(attr)
    send("#{attr}=", 'Default Name')
  end
end

This usage does not put any class methods or any class attributes onto the including class (everything needed is defined on the module instance mod).

In order to satisfy existing specs and usage, I left the class attribute attribute_aliases and defined a new attribute_method_builders (and method attribute_method_builder), but these are really "sugar" to maintain the current API; the core code is all encapsulated in the module builders.

Other considerations

There's a lot more here, I'm happy to go through the changes and explain whatever needs to be explained.

I am aware that any impact on performance will rule out any change like this, which is why I closed #30895. This time around I have been careful to check that there is no performance impact (that I can detect at any rate).

I have only tested locally against MRI ruby, but it is possible that other implementations may be impacted since I change one def into a define_method (for the respond_to override). I'm happy to discuss this part of the code.

The other main concern was the number of modules added in #30895. This PR does not grow the number of modules so this should not be a concern this time around.

shioyama added some commits Oct 28, 2017

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Dec 11, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@shioyama shioyama changed the title (This is a "lite version" of #30895.) Refactor AM::AttributeMethods + AR::AttributeMethods using module builders Dec 11, 2017

shioyama added some commits Dec 11, 2017

@shioyama shioyama force-pushed the shioyama:attribute_methods_module_builder_lite branch to d3f07fa Dec 11, 2017

Remove define_method_missing call from ActiveModel::AttributeMethodsB…
…uilder

AM::AttributeMethodsBuilder doesn't need this, and it's better to only
define +method_missing+ and +respond_to+ in modules expicitly.
@@ -9,7 +9,8 @@ module AttributeMethods
include ActiveModel::AttributeMethods

included do
initialize_generated_modules
@attribute_methods_builder = nil

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 12, 2017

Member

This is a smell that that memoization is happening too early. Now this implementation has to know about an internal instance variable in the AM module.

# end
# end
#
class AttributeMethodsBuilder < Module

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 12, 2017

Member

I don't think we should make this module public API. The public API is ActiveModel::AttributeMethods

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

Fair enough. Should I snug the builders into the same files as AM::AttributeMethods / AR:AttributeMethods, or can I just remove the docs and put a # :nodoc: in there?

@sgrif sgrif self-assigned this Dec 12, 2017

@@ -9,7 +9,7 @@ module AttributeMethods
include ActiveModel::AttributeMethods

included do
initialize_generated_modules
_attribute_methods_builder

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

@rafaelfranca I've removed the memoization resetter here, got around the problem by explicitly including AttributeMissingMethods in AM::AttributeMethods.

Actually specs pass without this line as well, but I'm leaving in because I think it's safer all-around to ensure that the module is immediately included in the ancestors, rather than waiting for the first call to prefix/suffix/define_attribute_methods.

@shioyama

This comment has been minimized.

Copy link
Contributor

shioyama commented Dec 13, 2017

@sgrif @rafaelfranca I'm happy to explain in more detail the considerations going into everything here since I'm aware there will be wariness to incorporate a large and deep change like this. I have gone through a lot of alternatives before coming to this code (including #30895), and tried as much as possible to weigh the potential issues (performance, regressions, complexity, etc.)

That said, I'm very receptive to changing the structure. The one thing that I want to maintain, however, is decoupling of all the elements here, especially the idea that these builders should have everything they need to be used in isolation (even if they are in the private API). This is why, for example, I have the ActiveModel::AttributeMissingMethods, since the methods in here are required to use the builders in isolation. If these modules end up requiring stuff outside themselves, then this is kind of pointless.

Unfortunately this is hard to do with the ActiveRecord builder, given how tightly coupled a lot of things already are. But I hope this could be a step toward making the elements of AR more de-coupled.

end
# Note: method_missing overrides are only defined once the builder is
# first used, to avoid unnecessarily increasing execution time of
# +respond_to?+ in case the builder has no matchers on it.

This comment has been minimized.

@matthewd

matthewd Dec 13, 2017

Member

Could this be left up to the builder itself, to define method_missing when it actually has a matcher?

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

Yes it could, and I hesitated a lot about this. I opted to let AM::AttributeMethods decide when to define them so that the builders could in theory be used without the method missing overrides, but maybe that's overthinking it.

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

Actually the reason I don't do this is because there is a situation where we want to include a builder with no method missing/respond_to on it. This is in AR::Base, when defining prefixes and suffixes. We duplicate this builder in inherited classes (i.e. all model classes) and then define mm/respond_to on this duplicated module, but not on the original one since this would be unnecessary and cause an unnecessary code path.

This is actually the most tricky part of this code, and it's tricky mainly because I want to: 1) comply with the current API, 2) decouple core logic into the builders, and 3) avoid any extra method calls. Satisfying these three together is hard, but can be done at the cost of these kinds of subtle considerations.

This comment has been minimized.

@matthewd

matthewd Dec 13, 2017

Member

Does the module need to be included at all in those cases? It needs to be there to gather definitions, and to be duplicated and included by non-abstract children... but maybe doesn't need to be in the inheritance chain?

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

I've lost track of which line you're referencing, but for ActiveRecord, it needs to be included in every non-abstract, non-base class in order to define methods (attribute methods + method_missing/respond_to) on that class. For abstract and base classes, we don't need to include it since we never instantiate those classes (so we don't actually need the methods).

For ActiveModel, nothing is included until you actually call a prefix/suffix/define method. This by the way implies that one subtle corner case of the current code is not maintained: the case where you include ActiveModel::AttributeMethods, define an attributes method with some attributes, define an attribute method to handle the getter, and then try calling one of the keys as a method. Currently this will be caught by method_missing, but with this PR change, mm/respond_to will not be defined until you actually add at least one prefix/suffix/affix or call define_attribute_methods.

This was a trade-off because I consider this a very rare case and it significantly complicates implementation, for little benefit (there is no impact on AR::Base models from this).

# AM::AttributeMethods. However, since this class depends on methods in
# it, we should ensure that it is included into any module that includes
# instances of the builder.
model_class.include AttributeMissingMethods

This comment has been minimized.

@matthewd

matthewd Dec 13, 2017

Member

What about including it into the module itself, in initialize?

I'm also unclear about why this is in both places: the above comment explains why it should be here, and not only in AM::AttributeMethods... but why can't it only be here? I'm probably missing something obvious about the relationship.

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

It's included explicitly in AM::AttributeMethods because of the dependency on when these methods are included into the heirarchy tree. If you don't explicitly include this in AM::AttributeMethods, then these methods end up getting included higher up and stuff breaks in AR (but not in AM).

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

About including it in initialize, this seems to work as well (just testing now locally). I think I had it in includes for a reason that is not relevant anymore. I can move it.

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

I just moved it in 120fc31

@shioyama

This comment has been minimized.

Copy link
Contributor

shioyama commented Dec 13, 2017

In case anyone is wondering: I need to add a spec checking that AR models have only one builder (or one per subclass level, for STI models) in their ancestors, to avoid unnecessary code paths through multiple respond_to overrides. This is the most tricky part of this. There doesn't seem to be a large impact from changing a def to a define_method for respond_to?, but if you're not careful you end up with more overrides than necessary which can impact performance slightly.

Other than respond_to? I don't see anywhere that performance is impacted.

shioyama added some commits Dec 13, 2017

Short-circuit respond_to? conditional if we've included private methods
If respond_to? was called with include_private_methods, and we've called
super, then we've checked that the method is not defined anywhere
(private or public) so we can safely return true here.
# been defined on it. We use _attribute_methods_builder here to avoid
# defining method_missing/respond_to on the inherited builder, which
# would be unnecessary since we define them on the duplicated module.
builder = _attribute_methods_builder.dup

This comment has been minimized.

@shioyama

shioyama Dec 13, 2017

Contributor

Note to self: duping an included module builder instance can have some very subtle issues. If we've already defined method_missing and respond_to?, the builder in the closure will be the original module, not the newly-duped one. So if we go on to add new matchers to the builder we duped, they will not be picked up in those methods.

Two ways to solve this:

  • don't define those methods on the original module (so it can be included without incurring an extra code path)
  • don't include the original module (so it can define the methods and they won't actually affect the code path)

Still working through the implications of both.

@shioyama shioyama closed this Feb 26, 2018

@shioyama

This comment has been minimized.

Copy link
Contributor

shioyama commented Feb 26, 2018

Closing this since it's stale now. I'd really like to figure out how to get this incorporated in some form... I'll keep working on it.

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