Merge pull request #34788 from gsamokovarov/actionable-errors

Introduce Actionable Errors
author: Kasper Timm Hansen <kaspth@gmail.com> 2019-04-19 15:38:57 +0200
committer: GitHub <noreply@github.com> 2019-04-19 15:38:57 +0200
commit: 10da0a27512e108a5cde3eeba774b01c15f6c43a (patch)
tree: 95f93befe2e9723cade192bc6af06412bf6f6f6f
parent: 3c8ea319d0fa4cd1e707a294ce1be4ffc8cd78b9 (diff)
parent: feaaa7576a0774471e36dc59730a886f623712e6 (diff)
download: rails-10da0a27512e108a5cde3eeba774b01c15f6c43a.tar.gz
rails-10da0a27512e108a5cde3eeba774b01c15f6c43a.tar.bz2
rails-10da0a27512e108a5cde3eeba774b01c15f6c43a.zip
20 files changed, 331 insertions, 3 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 79f6320a04..4109ae7006 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,12 @@
+*   Introduce `ActionDispatch::ActionableExceptions`.
+
+    The `ActionDispatch::ActionableExceptions` middleware dispatches actions
+    from `ActiveSupport::ActionableError` descendants.
+
+    Actionable errors let's you dispatch actions from Rails' error pages.
+
+    *Vipul A M*, *Yao Jie*, *Genadi Samokovarov*
+
 *   Raise an `ArgumentError` if a resource custom param contains a colon (`:`).
 
     After this change it's not possible anymore to configure routes like this:
diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb
index 8f39b88d56..6a4ba9af4a 100644
--- a/actionpack/lib/action_dispatch.rb
+++ b/actionpack/lib/action_dispatch.rb
@@ -53,6 +53,7 @@ module ActionDispatch
     autoload :RequestId
     autoload :Callbacks
     autoload :Cookies
+    autoload :ActionableExceptions
     autoload :DebugExceptions
     autoload :DebugLocks
     autoload :DebugView
diff --git a/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb b/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb
new file mode 100644
index 0000000000..e94cc46603
--- /dev/null
+++ b/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb
@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require "erb"
+require "action_dispatch/http/request"
+require "active_support/actionable_error"
+
+module ActionDispatch
+  class ActionableExceptions # :nodoc:
+    cattr_accessor :endpoint, default: "/rails/actions"
+
+    def initialize(app)
+      @app = app
+    end
+
+    def call(env)
+      request = ActionDispatch::Request.new(env)
+      return @app.call(env) unless actionable_request?(request)
+
+      ActiveSupport::ActionableError.dispatch(request.params[:error].to_s.safe_constantize, request.params[:action])
+
+      redirect_to request.params[:location]
+    end
+
+    private
+      def actionable_request?(request)
+        request.show_exceptions? && request.post? && request.path == endpoint
+      end
+
+      def redirect_to(location)
+        body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(location)}\">redirected</a>.</body></html>"
+
+        [302, {
+          "Content-Type" => "text/html; charset=#{Response.default_charset}",
+          "Content-Length" => body.bytesize.to_s,
+          "Location" => location,
+        }, [body]]
+      end
+  end
+end
diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
index 59113e13f4..0b15c94122 100644
--- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -4,6 +4,8 @@ require "action_dispatch/http/request"
 require "action_dispatch/middleware/exception_wrapper"
 require "action_dispatch/routing/inspector"
 
+require "active_support/actionable_error"
+
 require "action_view"
 require "action_view/base"
 
diff --git a/actionpack/lib/action_dispatch/middleware/debug_view.rb b/actionpack/lib/action_dispatch/middleware/debug_view.rb
index 43c0a84504..a03650254e 100644
--- a/actionpack/lib/action_dispatch/middleware/debug_view.rb
+++ b/actionpack/lib/action_dispatch/middleware/debug_view.rb
@@ -52,5 +52,9 @@ module ActionDispatch
         super
       end
     end
+
+    def protect_against_forgery?
+      false
+    end
   end
 end
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.html.erb
new file mode 100644
index 0000000000..b6c6d2f50d
--- /dev/null
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.html.erb
@@ -0,0 +1,13 @@
+<% actions = ActiveSupport::ActionableError.actions(exception) %>
+
+<% if actions.any? %>
+  <div class="actions">
+    <% actions.each do |action, _| %>
+      <%= button_to action, ActionDispatch::ActionableExceptions.endpoint, params: {
+        error: exception.class.name,
+        action: action,
+        location: request.path
+      } %>
+    <% end %>
+  </div>
+<% end %>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.text.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.text.erb
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.text.erb
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
index bde26f46c2..999e84e4d6 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
@@ -8,7 +8,11 @@
 </header>
 
 <div id="container">
-  <h2><%= h @exception.message %></h2>
+  <h2>
+    <%= h @exception.message %>
+
+    <%= render "rescues/actions", exception: @exception, request: @request %>
+  </h2>
 
   <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx, error_index: 0 %>
   <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show, error_index: 0 %>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb
index 39ea25bdfc..0f78e23b7f 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb
@@ -117,6 +117,10 @@
       background-color: #FFCCCC;
     }
 
+    .button_to {
+      display: inline-block;
+    }
+
     .hidden {
       display: none;
     }
diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
index 6460ca6f8d..32a0b8efeb 100644
--- a/actionpack/test/abstract_unit.rb
+++ b/actionpack/test/abstract_unit.rb
@@ -96,6 +96,7 @@ class ActionDispatch::IntegrationTest < ActiveSupport::TestCase
     RoutedRackApp.new(routes || ActionDispatch::Routing::RouteSet.new) do |middleware|
       middleware.use ActionDispatch::ShowExceptions, ActionDispatch::PublicExceptions.new("#{FIXTURE_LOAD_PATH}/public")
       middleware.use ActionDispatch::DebugExceptions
+      middleware.use ActionDispatch::ActionableExceptions
       middleware.use ActionDispatch::Callbacks
       middleware.use ActionDispatch::Cookies
       middleware.use ActionDispatch::Flash
diff --git a/actionpack/test/dispatch/actionable_exceptions_test.rb b/actionpack/test/dispatch/actionable_exceptions_test.rb
new file mode 100644
index 0000000000..9215a91e9c
--- /dev/null
+++ b/actionpack/test/dispatch/actionable_exceptions_test.rb
@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+
+class ActionableExceptionsTest < ActionDispatch::IntegrationTest
+  Actions = []
+
+  class ActionError < StandardError
+    include ActiveSupport::ActionableError
+
+    action "Successful action" do
+      Actions << "Action!"
+    end
+
+    action "Failed action" do
+      raise "Inaction!"
+    end
+  end
+
+  Noop = -> env { [200, {}, [""]] }
+
+  setup do
+    @app = ActionDispatch::ActionableExceptions.new(Noop)
+
+    Actions.clear
+  end
+
+  test "dispatches an actionable error" do
+    post ActionDispatch::ActionableExceptions.endpoint, params: {
+      error: ActionError.name,
+      action: "Successful action",
+      location: "/",
+    }
+
+    assert_equal ["Action!"], Actions
+
+    assert_equal 302, response.status
+    assert_equal "/", response.headers["Location"]
+  end
+
+  test "cannot dispatch errors if not allowed" do
+    post ActionDispatch::ActionableExceptions.endpoint, params: {
+      error: ActionError.name,
+      action: "Successful action",
+      location: "/",
+    }, headers: { "action_dispatch.show_exceptions" => false }
+
+    assert_empty Actions
+  end
+
+  test "dispatched action can fail" do
+    assert_raise RuntimeError do
+      post ActionDispatch::ActionableExceptions.endpoint, params: {
+        error: ActionError.name,
+        action: "Failed action",
+        location: "/",
+      }
+    end
+  end
+
+  test "cannot dispatch non-actionable errors" do
+    assert_raise ActiveSupport::ActionableError::NonActionable do
+      post ActionDispatch::ActionableExceptions.endpoint, params: {
+        error: RuntimeError.name,
+        action: "Inexistent action",
+        location: "/",
+      }
+    end
+  end
+
+  test "cannot dispatch Inexistent errors" do
+    assert_raise ActiveSupport::ActionableError::NonActionable do
+      post ActionDispatch::ActionableExceptions.endpoint, params: {
+        error: "",
+        action: "Inexistent action",
+        location: "/",
+      }
+    end
+  end
+end
diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb
index 2812b1b614..5ae8a20ae4 100644
--- a/actionpack/test/dispatch/debug_exceptions_test.rb
+++ b/actionpack/test/dispatch/debug_exceptions_test.rb
@@ -5,6 +5,18 @@ require "abstract_unit"
 class DebugExceptionsTest < ActionDispatch::IntegrationTest
   InterceptedErrorInstance = StandardError.new
 
+  class CustomActionableError < StandardError
+    include ActiveSupport::ActionableError
+
+    action "Action 1" do
+      nil
+    end
+
+    action "Action 2" do
+      nil
+    end
+  end
+
   class Boomer
     attr_accessor :closed
 
@@ -92,6 +104,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
         method_that_raises
       when "/nested_exceptions"
         raise_nested_exceptions
+      when %r{/actionable_error}
+        raise CustomActionableError
       else
         raise "puke!"
       end
@@ -589,4 +603,21 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
       end
     end
   end
+
+  test "shows a buttons for every action in an actionable error" do
+    @app = DevelopmentApp
+    Rails.stub :root, Pathname.new(".") do
+      cleaner = ActiveSupport::BacktraceCleaner.new.tap do |bc|
+        bc.add_silencer { |line| line !~ %r{test/dispatch/debug_exceptions_test.rb} }
+      end
+
+      get "/actionable_error", headers: { "action_dispatch.backtrace_cleaner" => cleaner }
+
+      # Assert correct error
+      assert_response 500
+
+      assert_select 'input[value="Action 1"]'
+      assert_select 'input[value="Action 2"]'
+    end
+  end
 end
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index ed0c6d48b8..f20edbeb93 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -4,6 +4,7 @@ require "benchmark"
 require "set"
 require "zlib"
 require "active_support/core_ext/module/attribute_accessors"
+require "active_support/actionable_error"
 
 module ActiveRecord
   class MigrationError < ActiveRecordError #:nodoc:
@@ -128,6 +129,12 @@ module ActiveRecord
   end
 
   class PendingMigrationError < MigrationError #:nodoc:
+    include ActiveSupport::ActionableError
+
+    action "Run pending migrations" do
+      ActiveRecord::Tasks::DatabaseTasks.migrate
+    end
+
     def initialize(message = nil)
       if !message && defined?(Rails.env)
         super("Migrations are pending. To resolve this issue, run:\n\n        rails db:migrate RAILS_ENV=#{::Rails.env}")
diff --git a/activerecord/test/cases/tasks/database_tasks_test.rb b/activerecord/test/cases/tasks/database_tasks_test.rb
index dd4a0b0455..ffe94eee0f 100644
--- a/activerecord/test/cases/tasks/database_tasks_test.rb
+++ b/activerecord/test/cases/tasks/database_tasks_test.rb
@@ -760,7 +760,7 @@ module ActiveRecord
     end
 
     class DatabaseTasksMigrateTest < DatabaseTasksMigrationTestCase
-      def test_migrate_set_and_unset_verbose_and_version_env_vars
+      def test_can_migrate_from_pending_migration_error_action_dispatch
         verbose, version = ENV["VERBOSE"], ENV["VERSION"]
         ENV["VERSION"] = "2"
         ENV["VERBOSE"] = "false"
@@ -772,7 +772,9 @@ module ActiveRecord
         ENV.delete("VERBOSE")
 
         # re-run up migration
-        assert_includes capture_migration_output, "migrating"
+        assert_includes(capture(:stdout) do
+          ActiveSupport::ActionableError.dispatch ActiveRecord::PendingMigrationError, "Run pending migrations"
+        end, "migrating")
       ensure
         ENV["VERBOSE"], ENV["VERSION"] = verbose, version
       end
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 301b0c8822..4c7b134c35 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,35 @@
+*   Introduce `ActiveSupport::ActionableError`.
+
+    Actionable errors let's you dispatch actions from Rails' error pages. This
+    can help you save time if you have a clear action for the resolution of
+    common development errors.
+
+    The de-facto example are pending migrations. Every time pending migrations
+    are found, a middleware raises an error. With actionable errors, you can
+    run the migrations right from the error page. Other examples include Rails
+    plugins that need to run a rake task to setup themselves. They can now
+    raise actionable errors to run the setup straight from the error pages.
+
+    Here is how to define an actionable error:
+
+    ```ruby
+    class PendingMigrationError < MigrationError #:nodoc:
+      include ActiveSupport::ActionableError
+
+      action "Run pending migrations" do
+        ActiveRecord::Tasks::DatabaseTasks.migrate
+      end
+    end
+    ```
+
+    To make an error actionable, include the `ActiveSupport::ActionableError`
+    module and invoke the `action` class macro to define the action. An action
+    needs a name and a procedure to execute. The name is shown as the name of a
+    button on the error pages. Once clicked, it will invoke the given
+    procedure.
+
+    *Vipul A M*, *Yao Jie*, *Genadi Samokovarov*
+
 *   Preserve `html_safe?` status on `ActiveSupport::SafeBuffer#*`.
 
     Before:
diff --git a/activesupport/lib/active_support.rb b/activesupport/lib/active_support.rb
index 5589c71281..9e242ddeaa 100644
--- a/activesupport/lib/active_support.rb
+++ b/activesupport/lib/active_support.rb
@@ -34,6 +34,7 @@ module ActiveSupport
   extend ActiveSupport::Autoload
 
   autoload :Concern
+  autoload :ActionableError
   autoload :CurrentAttributes
   autoload :Dependencies
   autoload :DescendantsTracker
diff --git a/activesupport/lib/active_support/actionable_error.rb b/activesupport/lib/active_support/actionable_error.rb
new file mode 100644
index 0000000000..7db14cd178
--- /dev/null
+++ b/activesupport/lib/active_support/actionable_error.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+module ActiveSupport
+  # Actionable errors let's you define actions to resolve an error.
+  #
+  # To make an error actionable, include the <tt>ActiveSupport::ActionableError</tt>
+  # module and invoke the +action+ class macro to define the action. An action
+  # needs a name and a block to execute.
+  module ActionableError
+    extend Concern
+
+    class NonActionable < StandardError; end
+
+    included do
+      class_attribute :_actions, default: {}
+    end
+
+    def self.actions(error) # :nodoc:
+      case error
+      when ActionableError, -> it { Class === it && it < ActionableError }
+        error._actions
+      else
+        {}
+      end
+    end
+
+    def self.dispatch(error, name) # :nodoc:
+      actions(error).fetch(name).call
+    rescue KeyError
+      raise NonActionable, "Cannot find action \"#{name}\""
+    end
+
+    module ClassMethods
+      # Defines an action that can resolve the error.
+      #
+      #   class PendingMigrationError < MigrationError
+      #     include ActiveSupport::ActionableError
+      #
+      #     action "Run pending migrations" do
+      #       ActiveRecord::Tasks::DatabaseTasks.migrate
+      #     end
+      #   end
+      def action(name, &block)
+        _actions[name] = block
+      end
+    end
+  end
+end
diff --git a/activesupport/test/actionable_error_test.rb b/activesupport/test/actionable_error_test.rb
new file mode 100644
index 0000000000..63046b937c
--- /dev/null
+++ b/activesupport/test/actionable_error_test.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+require "active_support/actionable_error"
+
+class ActionableErrorTest < ActiveSupport::TestCase
+  NonActionableError = Class.new(StandardError)
+
+  class DispatchableError < StandardError
+    include ActiveSupport::ActionableError
+
+    class_attribute :flip1, default: false
+    class_attribute :flip2, default: false
+
+    action "Flip 1" do
+      self.flip1 = true
+    end
+
+    action "Flip 2" do
+      self.flip2 = true
+    end
+  end
+
+  test "returns all action of an actionable error" do
+    assert_equal ["Flip 1", "Flip 2"], ActiveSupport::ActionableError.actions(DispatchableError).keys
+    assert_equal ["Flip 1", "Flip 2"], ActiveSupport::ActionableError.actions(DispatchableError.new).keys
+  end
+
+  test "returns no actions for non-actionable errors" do
+    assert ActiveSupport::ActionableError.actions(Exception).empty?
+    assert ActiveSupport::ActionableError.actions(Exception.new).empty?
+  end
+
+  test "dispatches actions from error and name" do
+    assert_changes "DispatchableError.flip1", from: false, to: true do
+      ActiveSupport::ActionableError.dispatch DispatchableError, "Flip 1"
+    end
+  end
+
+  test "cannot dispatch missing actions" do
+    err = assert_raises ActiveSupport::ActionableError::NonActionable do
+      ActiveSupport::ActionableError.dispatch NonActionableError, "action"
+    end
+
+    assert_equal 'Cannot find action "action"', err.to_s
+  end
+end
diff --git a/railties/lib/rails/application/default_middleware_stack.rb b/railties/lib/rails/application/default_middleware_stack.rb
index 193cc59f3a..9800b19274 100644
--- a/railties/lib/rails/application/default_middleware_stack.rb
+++ b/railties/lib/rails/application/default_middleware_stack.rb
@@ -49,6 +49,7 @@ module Rails
           middleware.use ::Rails::Rack::Logger, config.log_tags
           middleware.use ::ActionDispatch::ShowExceptions, show_exceptions_app
           middleware.use ::ActionDispatch::DebugExceptions, app, config.debug_exception_response_format
+          middleware.use ::ActionDispatch::ActionableExceptions
 
           unless config.cache_classes
             middleware.use ::ActionDispatch::Reloader, app.reloader
diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb
index 4242daf39a..54b2e95d75 100644
--- a/railties/test/application/middleware_test.rb
+++ b/railties/test/application/middleware_test.rb
@@ -38,6 +38,7 @@ module ApplicationTests
         "Rails::Rack::Logger",
         "ActionDispatch::ShowExceptions",
         "ActionDispatch::DebugExceptions",
+        "ActionDispatch::ActionableExceptions",
         "ActionDispatch::Reloader",
         "ActionDispatch::Callbacks",
         "ActiveRecord::Migration::CheckPending",
@@ -70,6 +71,7 @@ module ApplicationTests
         "Rails::Rack::Logger",
         "ActionDispatch::ShowExceptions",
         "ActionDispatch::DebugExceptions",
+        "ActionDispatch::ActionableExceptions",
         "ActionDispatch::Reloader",
         "ActionDispatch::Callbacks",
         "Rack::Head",
author	Kasper Timm Hansen <kaspth@gmail.com>	2019-04-19 15:38:57 +0200
committer	GitHub <noreply@github.com>	2019-04-19 15:38:57 +0200
commit	10da0a27512e108a5cde3eeba774b01c15f6c43a (patch)
tree	95f93befe2e9723cade192bc6af06412bf6f6f6f
parent	3c8ea319d0fa4cd1e707a294ce1be4ffc8cd78b9 (diff)
parent	feaaa7576a0774471e36dc59730a886f623712e6 (diff)
download	rails-10da0a27512e108a5cde3eeba774b01c15f6c43a.tar.gz rails-10da0a27512e108a5cde3eeba774b01c15f6c43a.tar.bz2 rails-10da0a27512e108a5cde3eeba774b01c15f6c43a.zip