inertiajs / inertia-rails

The Rails adapter for Inertia.js.
https://inertia-rails.dev/
MIT License
574 stars 45 forks source link

Shared data can conflict with exception handler #20

Closed ledermann closed 2 years ago

ledermann commented 4 years ago

The current implementation of inertia_share is based on before_action, so the shared data is executed by all means before the action is processed. Even if the controller action raises an exception, which I don't think is a good thing.

Think of this application using a custom exception handler to response with Inertia for exceptions, like this:

# app/config/application.rb
module MyApplication
  class Application < Rails::Application
    # ...
    config.exceptions_app = ->(env) { ExceptionsController.action(:show).call(env) }
  end
end

# app/controllers/exceptions_controller.rb
class ExceptionsController < ActionController::Base
  def show
    render inertia: 'Error', props: {
      status: request.path_info[1..-1].to_i
    }
  end
end

This works fine, see my my demo Rails app PingCRM for a live example: https://pingcrm.ledermann.dev

Now, lets assume an action raises an error, like this:

class ApplicationController < ActionController::Base
  inertia_share do
    # complex logic here
  end
end

class DashboardController < ApplicationController
  inertia_share do
    # more complex logic here
  end

  def index
    1/0
    render inertia: 'Dashboard', props: {}
  end
end

When the error occurs, the exception handler strikes in and renders the Error Page instead of the Dashboard page, but the shared data of ApplicationController and DashboardController is included as props. too. This is not optimal, because in case of an exception the response should be as simple as possible.

I would like to propose again to merge my PR #17 because it has not this issue.

ledermann commented 4 years ago

I found an example demonstrating a more serious conflict:

class DashboardController < ApplicationController
  # Define shared data with a block calling a method which exists in THIS controller only
  inertia_share something: -> { foo }

  def index
    # within this method an exception is raised
    raise RuntimeError
  end

  private 
  def foo
    # something
  end
end

Now, bad things happen when user goes to /dashboard:

  1. Before the index method is executed, the inertia_share block is added to the global list of shared data blocks.
  2. Now the index method runs and the exception is raised.
  3. The exception handler strikes in and tries to render the Error page. To render, all shared data blocks are executed, but this fails because it can not access the foo method, which is defined in DashboardController, not ExceptionController.

My PR #17 fixes things like this. @bknoles and @BrandonShar, please think again about merging #17 :-)

bknoles commented 4 years ago

I wrote this in #17 , but reiterating here:

The second situation is definitely something that should be fixed... but on the first example, I think I would prefer to have the shared data available within the ExceptionsController.

BrandonShar commented 2 years ago

Closing due to inactivity. Feel free to re-open if we missed an issue that still exists!