Skip to content
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

Add `action_controller_base` and `action_controller_api` hooks to mitigate differences between them. Also document Active Support on_load hooks #28402

Merged
merged 2 commits into from Apr 11, 2017

Conversation

@jules2689
Copy link
Contributor

jules2689 commented Mar 13, 2017

Summary

Active Support allows specific hooks to be called to mitigate loading code from boot time. These hooks include action_controller, however this is not specific enough.

As evidenced by these issues, action_controller is called if the controller is an instance of ActionController::Base or ActionController::API.

The issue occurs due to the latter having differing implementation and not including methods such as helper_method.

The Fix

To mitigate this, implement 2 new hooks action_controller_base and action_controller_api that can be hooked into instead of one overarching hook.

I also noticed the lack of documentation for this topic, so I added a page on it and included the use cases, examples, and list of all available hooks.

Fixes #27013

cc @rafaelfranca

@rails-bot
Copy link

rails-bot commented Mar 13, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (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.

@jules2689
Copy link
Contributor Author

jules2689 commented Mar 13, 2017

I also added a changelog entry, but it wasn't clear to me what I was supposed to do to the title of the PR.

Copy link
Member

maclover7 left a comment

Added some grammar comments for the docs. Thanks for the PR!!

guides/source/active_support_on_load_hooks.md Outdated

Active Support is the Ruby on Rails component responsible for providing Ruby language extensions, utilities, and other transversal stuff.

Rails Code can often be referenced on load of an application. By loading code like `ActiveRecord::Base` on load, you are loading entire frameworks which may slow down your boot time and potentially cause conflicts.

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

Code --> code

guides/source/active_support_on_load_hooks.md Outdated

This snippet means that when this file is loaded, it will encounter `ActiveRecord::Base` right away. This causes Ruby to look for the definition of that constant and then will require it. This causes the entire Active Record framework to be loaded on boot.

`ActiveSupport.on_load` is a mechanism that can be used to defer the loading of code until it is actually required. The snippet above can be changed to:

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

required --> loaded

This comment has been minimized.

Copy link
@jules2689

jules2689 Mar 13, 2017

Author Contributor

Hrm, I had meant more along the lines of required to be loaded. Would that make sense to you?

guides/source/active_support_on_load_hooks.md Outdated

This snippet means that when this file is loaded, it will encounter `ActiveRecord::Base` right away. This causes Ruby to look for the definition of that constant and then will require it. This causes the entire Active Record framework to be loaded on boot.

`ActiveSupport.on_load` is a mechanism that can be used to defer the loading of code until it is actually loaded. The snippet above can be changed to:

This comment has been minimized.

Copy link
@jules2689

jules2689 Mar 13, 2017

Author Contributor

resurfacing #28402 (comment)
cc @maclover7

Hrm, I had meant more along the lines of required to be loaded. Would that make sense to you?

Copy link
Member

rafaelfranca left a comment

Awesome documentation. I'll @fxn's comment about the new guide to merge.

guides/source/documents.yaml Outdated
@@ -72,6 +72,10 @@
url: active_support_core_extensions.html
description: This guide documents the Ruby core extensions defined in Active Support.
-
name: Active Support On Load Hooks

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 13, 2017

Member

Maybe instead of adding a new guide we should put it as a topic the initialization.html guide? @fxn WDYT?

This comment has been minimized.

Copy link
@fxn

fxn Mar 22, 2017

Member

Agree, too narrow for a dedicated guide.

The initialization guide is basically abandoned though. It was started some time ago, but it has a code walkthrough approach that I do not like, because I'd prefer that guide to be the bird's eye view of what runs when Rails developers need to know. Would be closer to what I covered in this talk.

I believe the main use case from a developer point of view is railties/engines, @rafaelfranca what do you think about adding that to the engines guides meanwhile?

I believe the motivation in the proposed docs is a bit off also. It says "you could load classes from the framework, but would be costly, so you have this trick to lazy load". It has an implicit assumption that this is optional. I think it would better say: "Rails is responsible for what is loaded when, you should not load frameworks by yourself because that violates the contract, on_load is the API to integrate with the Rails boot process".

This comment has been minimized.

Copy link
@jules2689

jules2689 Mar 22, 2017

Author Contributor

"Rails is responsible for what is loaded when, you should not load frameworks by yourself because that violates the contract, on_load is the API to integrate with the Rails boot process".

Nice, yea, I can add that tone no problem 👍

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 28, 2017

Member

Awesome! @jules2689 could you move it to the engines guide?

Copy link
Member

maclover7 left a comment

Few more grammar comments. Coming along very nicely!!

activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,15 @@
* Add `action_controller_api` and `action_controller_base` hooks to be called in `ActiveSupport.on_load`

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

Can we change this to the below?

Add action_controller_api and action_controller_base load hooks

activesupport/CHANGELOG.md Outdated
* Add `action_controller_api` and `action_controller_base` hooks to be called in `ActiveSupport.on_load`

`ActionController::Base` and `ActionController::API` have differing implementations. This means that
the one umbrella hook `action_controller` was not able to address certain situations where a method

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

was --> is

activesupport/CHANGELOG.md Outdated

`ActionController::Base` and `ActionController::API` have differing implementations. This means that
the one umbrella hook `action_controller` was not able to address certain situations where a method
may not exist in the implementation.

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

in the --> in a certain

guides/source/active_support_on_load_hooks.md Outdated

Rails code can often be referenced on load of an application. By loading code like `ActiveRecord::Base` on load, you are loading entire frameworks which may slow down your boot time and potentially cause conflicts.

In this guide, you will learn how to mitigate boot time slow down and potential conflicts arsing from referencing frameworks too early.

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

arsing typo?

This comment has been minimized.

Copy link
@jules2689

jules2689 Mar 13, 2017

Author Contributor

😆 woops

guides/source/active_support_on_load_hooks.md Outdated

* What `on_load` hooks are
* Modifying code to use `on_load` hooks
* The available hooks inside the Rails framework for reference.

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

✂️ for reference (end after framework)

guides/source/active_support_on_load_hooks.md Outdated
What are `on_load` hooks?
-------------------------

Since Ruby is a dynamic language, some code will cause frameworks to load. Take this snippet for instance:

This comment has been minimized.

Copy link
@maclover7

maclover7 Mar 13, 2017

Member

cause different Rails frameworks

guides/source/active_support_on_load_hooks.md Outdated

| Class | Available Hooks |
| --------------------------------- | ------------------------------------ |
| `ActiveRecord::Base` | `active_record` |

This comment has been minimized.

Copy link
@betesh

betesh Mar 28, 2017

Contributor

You're missing a few: action_cable, action_view_test_case, i18n (not technically a Rails module but a load hook that ActiveSupport runs and that client libraries should use if they need to set a locale or add custom i18n load paths).

You may also want to document here before_configuration, before_initialize, before_eager_load, and after_initialize. They are documented elsewhere, and aren't intended to prevent early loading of components, but they are load hooks nonetheless.

Also, I'd find this much more readable if it were sorted alphabetically, and in particular if classes in the same namespace were grouped consecutively.

@jules2689
Copy link
Contributor Author

jules2689 commented Apr 10, 2017

@fxn @betesh see latest commit for the changes you requested. Thoughts?

@rafaelfranca
Copy link
Member

rafaelfranca commented Apr 10, 2017

Could you squash your commits in two? One for the new hooks and the changelogs entry. The other to the guides changes.

jules2689 added 2 commits Mar 13, 2017
@jules2689
Copy link
Contributor Author

jules2689 commented Apr 10, 2017

@rafaelfranca rafaelfranca merged commit afb41fb into rails:master Apr 11, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca
Copy link
Member

rafaelfranca commented Apr 11, 2017

Awesome work!

@jules2689 jules2689 deleted the jules2689:action-controller-api-base-hooks branch Apr 11, 2017
rafaelfranca added a commit that referenced this pull request Apr 11, 2017
…ooks

Add `action_controller_base` and `action_controller_api` hooks to mitigate differences between them. Also document Active Support on_load hooks
@rafaelfranca
Copy link
Member

rafaelfranca commented Apr 11, 2017

Backported in b9a5fd7

maclover7 added a commit that referenced this pull request Apr 11, 2017
Was looking through #28402, and realized the CHANGELOG.md entry is in the wrong
place. Sorry we didn't catch this during code review 😢

[ci skip]
maclover7 added a commit that referenced this pull request Apr 11, 2017
Was looking through #28402, and realized the CHANGELOG.md entry is in the wrong
place. Sorry we didn't catch this during code review 😢

[ci skip]
thermistor added a commit to thermistor/crystalmeta that referenced this pull request Oct 14, 2017
- Use before_action now instead of before_filter as latter is removed in
  rails 5.1.
- Use new load hook `:action_controller_base` so `helper_method` call
  happens only on ActionController::Base and not on ActionController::API
  as well. See rails/rails#28402
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Mar 18, 2018
- Remove
  ```
  *   Namespace error pages' CSS selectors to stop the styles from bleeding
      into other pages when using Turbolinks.
      ([Pull Request](rails#28814))
  ```
  since it was backported to
  `5-1-stable` by 50d5baf
  and
  `5-0-stable` by d1c4a39.

- Remove
  ```
  *   Allow irb options to be passed from `rails console` command.
      ([Pull Request](rails#29010))
  ```
  since it was backported to
  `5-1-stable` by e91b483.

- Remove
  ```
  *   Load environment file in `dbconsole` command.
      ([Pull Request](rails#29725))
  ```
  since it was backported to
  `5-1-stable` by 7f93428.

- Remove
  ```
  *   Gemfile for new apps: upgrade redis-rb from ~> 3.0 to 4.0.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Fix minitest rails plugin.
      The custom reporters are added only if needed.
      This will fix conflicts with others plugins.
      ([Commit](rails@ac99916))
  ```
  since it was backported to
  `5-1-stable` by caa7695.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Add `action_controller_api` and `action_controller_base` load hooks to be
      called in `ActiveSupport.on_load`.
      ([Pull Request](rails#28402))
  ```
  since it was backported to
  `5-1-stable` by b9a5fd7.

- Remove
  ```
  *   `driven_by` now registers poltergeist and capybara-webkit.
      ([Pull Request](rails#29315))
  ```
  since it was backported to
  `5-1-stable` by c5dd451.

- Remove
  ```
  *   Fallback `ActionController::Parameters#to_s` to `Hash#to_s`.
      ([Pull Request](rails#29630))
  ```
  since it was backported to
  `5-1-stable` by c1014e4
  and
  `5-0-stable` by 0e71fc3.

- Remove
  ```
  *   Make `take_failed_screenshot` work within engine.
      ([Pull Request](rails#30421))
  ```
  since it was backported to
  `5-1-stable` by 595a231.

- Remove
  ```
  *   Fix optimized url helpers when using relative url root.
      ([Pull Request](rails#31261))
  ```
  since it was backported to
  `5-1-stable` by e9b7795.

- Remove
  ```
  *   Update `distance_of_time_in_words` helper to display better error messages
      for bad input.
      ([Pull Request](rails#20701))
  ```
  since it was backported to
  `5-1-stable` by 2c97fbf.

- Remove
  ```
  *   Generate field ids in `collection_check_boxes` and
      `collection_radio_buttons`.
      ([Pull Request](rails#29412))
  ```
  since it was backported to
  `5-1-stable` by 2d8c10a.

- Remove
  ```
  *   Fix issues with scopes and engine on `current_page?` method.
      ([Pull Request](rails#29503))
  ```
  since it was backported to
  `5-1-stable` by 2135daf.

- Remove
  ```
  *   Bring back proc with arity of 1 in `ActionMailer::Base.default` proc
      since it was supported in Rails 5.0 but not deprecated.
      ([Pull Request](rails#30391))
  ```
  since it was backported to
  `5-1-stable` by b2bedb1.

- Remove
  ```
  *   Add type caster to `RuntimeReflection#alias_name`.
      ([Pull Request](rails#28961))
  ```
  since it was backported to
  `5-1-stable` by f644e7a.

- Remove
  ```
  *   Loading model schema from database is now thread-safe.
      ([Pull Request](rails#29216))
  ```
  since it was backported to
  `5-1-stable` by 02926cf.
  and
  `5-0-stable` by 84bcfe5

- Remove
  ```
  *   Fix destroying existing object does not work well when optimistic locking
      enabled and `locking_column` is null in the database.
      ([Pull Request](rails#28926))
  ```
  since it was backported to
  `5-1-stable` by e498052.

- Remove
  ```
  *   `ActiveRecord::Persistence#touch` does not work well
      when optimistic locking enabled and `locking_column`,
      without default value, is null in the database.
      ([Pull Request](rails#28914))
  ```
  since it was backported to
  `5-1-stable` by 1e2f63d.

- Remove
  ```
  *   Previously, when building records using a `has_many :through` association,
      if the child records were deleted before the parent was saved,
      they would still be persisted. Now, if child records are deleted
      before the parent is saved on a `has_many :through` association,
      the child records will not be persisted.
      ([Pull Request](rails#29593))
  ```
  since it was backported to
  `5-1-stable` by a22c39e.

- Remove
  ```
  *   Query cache was unavailable when entering the `ActiveRecord::Base.cache`
      block without being connected.
      ([Pull Request](rails#29609))
  ```
  since it was backported to
  `5-1-stable` by fd6c8cd
  and
  `5-0-stable` by 9f2532b.

- Remove
  ```
  *   `Relation#joins` is no longer affected by the target model's
      `current_scope`, with the exception of `unscoped`.
      ([Commit](rails@5c71000))
  ```
  since it was backported to
  `5-1-stable` by 3630d63.

- Remove
  ```
  *   Fix `unscoped(where: [columns])` removing the wrong bind values.
      ([Pull Request](rails#29780))
  ```
  since it was backported to
  `5-1-stable` by d378fcb.

- Remove
  ```
  *   When a `has_one` association is destroyed by `dependent: destroy`,
      `destroyed_by_association` will now be set to the reflection, matching the
      behaviour of `has_many` associations.
      ([Pull Request](rails#29855))
  ```
  since it was backported to
  `5-1-stable` by 8254a8b.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT`
      to keep the existing select list.
      ([Pull Request](rails#29848))
  ```
  since it was backported to
  `5-1-stable` by 0e8d4ed.

- Remove
  ```
  *   Ensure `sum` honors `distinct` on `has_many :through` associations.
      ([Commit](rails@566f1fd))
  ```
  since it was backported to
  `5-1-stable` by c0a1dc2.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` for `GROUP BY` with `ORDER BY` and `LIMIT`.
      ([Commit](rails@5668dc6))
  ```
  since it was backported to
  `5-1-stable` by 87ca68e.

- Remove
  ```
  *   MySQL: Don't lose `auto_increment: true` in the `db/schema.rb`.
      ([Commit](rails@9493d45))
  ```
  since it was backported to
  `5-1-stable` by 8b6e694.

- Remove
  ```
  *   Fix longer sequence name detection for serial columns.
      ([Pull Request](rails#28339))
  ```
  since it was backported to
  `5-1-stable` by af9c170
  and
  `5-0-stable` by 7025b1d.

- Remove
  ```
  *   Fix `bin/rails db:setup` and `bin/rails db:test:prepare` create wrong
      ar_internal_metadata's data for a test database.
      ([Pull Request](rails#30579))
  ```
  since it was backported to
  `5-1-stable` by bb67b5f
  and
  `5-0-stable` by 60437e6.

- Remove
  ```
  *   Fix conflicts `counter_cache` with `touch: true` by optimistic locking.
      ([Pull Request](rails#31405))
  ```
  since it was backported to
  `5-1-stable` by 5236dda.

- Remove
  ```
  *   Fix `count(:all)` to correctly work `distinct` with custom SELECT list.
      ([Commit](rails@c6cd9a5))
  ```
  since it was backported to
  `5-1-stable` by 6beb4de.

- Remove
  ```
  *   Fix to invoke callbacks when using `update_attribute`.
      ([Commit](rails@732aa34))
  ```
  since it was backported to
  `5-1-stable` by 6346683.

- Remove
  ```
  *   Use `count(:all)` in `HasManyAssociation#count_records` to prevent invalid
      SQL queries for association counting.
      ([Pull Request](rails#27561))
  ```
  since it was backported to
  `5-1-stable` by eef3c89.

- Remove
  ```
  *   Fix `count(:all)` with eager loading and having an order other than
      the driving table.
      ([Commit](rails@ebc09ed))
  ```
  since it was backported to
  `5-1-stable` by 6df9b69.

- Remove
  ```
  *   PostgreSQL: Allow pg-1.0 gem to be used with Active Record.
      ([Pull Request](rails#31671))
  ```
  since it was backported to
  `5-1-stable` by a9c06f6.

- Remove
  ```
  *   Fix that after commit callbacks on update does not triggered
      when optimistic locking is enabled.
      ([Commit](rails@7f9bd03))
  ```
  since it was backported to
  `5-1-stable` by aaee10e.

- Remove
  ```
  *   Fix regression in numericality validator when comparing Decimal and Float
      input values with more scale than the schema.
      ([Pull Request](rails#28584))
  ```
  since it was backported to
  `5-1-stable` by 5b1c3e5.
  Note that there was incorrect link to PR,
  original PR is rails#29249.

- Remove
  ```
  *   Fix to working before/after validation callbacks on multiple contexts.
      ([Pull Request](rails#31483))
  ```
  since it was backported to
  `5-1-stable` by 0f7046a.

- Remove
  ```
  *   Fix implicit coercion calculations with scalars and durations.
      ([Pull Request](rails#29163),
      [Pull Request](rails#29971))
  ```
  since it was backported to
  `5-1-stable` by 51ea27c,
                  4d82e2a.

- Remove
  ```
  *   Fix modulo operations involving durations.
      ([Commit](rails@a54e13b))
  ```
  since it was backported to
  `5-1-stable` by 233fa7e.

- Remove
  ```
  *   Return all mappings for a timezone identifier in `country_zones`.
      ([Commit](rails@cdce6a7))
  ```
  since it was backported to
  `5-1-stable` by 0222ebb.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.
  Related to rails#32252.

Related to rails#32222, rails#32222 (comment).
Follow up a489cc8.
tute added a commit to merit-gem/merit that referenced this pull request Jul 23, 2018
Previously merit was initialised once when action_controller was loaded,
but that could happen either for base or for API, and if the app was
configured for the other one Merit wouldn't run.

After rails/rails#28402 we can specifically hook
in to web or API, thus removing the need for the `run_once` option.

Thank you for your work on this, @jamesjefferies.

[fixes #297]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.