inertiajs / inertia-rails

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

Intermittent 302 on DELETE requests #58

Closed christoomey closed 3 years ago

christoomey commented 3 years ago

I recently ran into an issue where the "Sign Out" button of my app would occasionally result in an inertia modal w/ 404 error page.

I opened this as https://github.com/inertiajs/inertia/issues/419 on the main Inertia repo, but they kindly helped me dig a bit deeper and I've now confirmed that the issue is that response status code.

At this point I'm guessing the issue is somewhere in my app / middleware stack, but I wanted to share here in case you folks have any thoughts as to how this might be happening.


I'm specifically running into this with the "Sign Out" button in my app. The problem flow is.

I've found this tricky to reproduce as most cases send the correct 303 status code, but I have been able to reproduce it.

For reference, here's my middleware stack.

Rack middleware stack ``` use Rack::Cors use Honeybadger::Rack::UserInformer use Honeybadger::Rack::UserFeedback use Honeybadger::Rack::ErrorNotifier use Rack::Rewrite use ActionDispatch::SSL use Rack::Sendfile use ActionDispatch::Static use Rack::Deflater use ActionDispatch::Executor use ActiveSupport::Cache::Strategy::LocalCache::Middleware use RailsAutoscaleAgent::Middleware use Rack::Runtime use Rack::MethodOverride use ActionDispatch::RequestId use Rack::Timeout use RequestStore::Middleware use ActionDispatch::RemoteIp use Rails::Rack::Logger use ActionDispatch::ShowExceptions use ActionDispatch::DebugExceptions use Rollbar::Middleware::Rails::RollbarMiddleware use ActionDispatch::Callbacks use ActionDispatch::Cookies use ActionDispatch::Session::CookieStore use ActionDispatch::Flash use ActionDispatch::ContentSecurityPolicy::Middleware use Rack::Head use Rack::ConditionalGet use Rack::ETag use Rack::TempfileReaper use Clearance::RackSession use Warden::Manager use Coverband::BackgroundMiddleware use Rack::Attack use ScoutApm::Middleware use InertiaRails::Middleware run MyApp::Application.routes ```

Any thoughts? (I'll also certainly understand if you want to close this under the assumption that the problem is on my side, but figured it was worth an ask).

bknoles commented 3 years ago

Intermittent issues are the best! 😄

We haven't seen anything like this before, but there definitely could be something unexpected happening in the middleware.

Since InertiaRails is last on the middleware list, any of the middlewares above it might alter the status code. Would you be able to debug inside the gem within your project so we can verify if this is an InertiaRails problem?

I'd want to see if the status is a 303 after this line runs, during the sign out request that fails as you described.

https://github.com/inertiajs/inertia-rails/blob/master/lib/inertia_rails/middleware.rb#L18

something like:

def call(env)
      @env = env

      status, headers, body = @app.call(env)

      # ... bunch of skipped code here

      status = 303 if inertia_non_post_redirect?(status)

      # Cool kids will probably use an actual debugger
      puts "HEY THERE THE STATUS IS: #{status}"

      return stale_inertia_get? ? force_refresh(request) : [status, headers, body]
end
christoomey commented 3 years ago

@bknoles thanks the notes! Unfortunately, I've only been able to reproduce on production. Staging should be identical but haven't been able to trip it there, nor in development, thus limiting debugging options.

Looking at the Inertia rails code I'd be very surprised if anything in there was getting mixed up, with the small caveat that it's possible some other middleware is mucking with headers and thus inertia-rails gets confused. But again, unlikely.

Anyway, I figured I'd ask on the off chance that it rang any bells for y'all, but since it doesn't I'm going to close this. I'll be sure to comment or re-open if I do find any answers.

Thanks again!

reinvanimschoot commented 3 years ago

@christoomey Did you ever find the answer to this problem? I'm encountering the exact same issue you were facing, intermittent 302's among a vast majority of 303's.

christoomey commented 3 years ago

Unfortunately I have not figured anything out on this front re: the 302 / 303 variation.

I ended up working around this by introducing a NonInertiaForm for the handful of cases where either I'm working against an existing endpoint or otherwise want to force a full page reload (generally auth stuff). Here's the related Svelte code (somewhat specific to Rails' form handling quirks) in case you're interested:

<script lang="ts">
  export let action: string
  export let method: 'delete' | 'put' | 'patch' | undefined = undefined

  let csrfToken: string
  $: csrfToken = decodeURIComponent(
    document.cookie
      .match('(^|;)\\s*' + 'XSRF-TOKEN' + '\\s*=\\s*([^;]+)')
      ?.pop() || ''
  )
</script>

<form {action} method="post" class={$$props.class}>
  {#if method}
    <input type="hidden" value={method} name="_method" />
  {/if}
  <input type="hidden" name="authenticity_token" value={csrfToken} />

  <slot />
</form>

Note for anyone reading this, this is 100% about some inconsistency in how Rails is setting the status code, and not at all the fault of Inertia which remains an utterly stellar project in my mind.

caifara commented 3 years ago

@christoomey: @reinvanimschoot and I happened to run into this problem and could pinpoint it to a threading bug in the middleware code. See the test in #70.

christoomey commented 3 years ago

@caifara that's exciting! Thanks for sharing :)

bknoles commented 3 years ago

Released in a v1.11.1!

https://rubygems.org/gems/inertia_rails/versions/1.11.1

Thanks again @caifara !

Note for anyone reading this, this is 100% about some inconsistency in how Rails is setting the status code, and not at all the fault of Inertia which remains an utterly stellar project in my mind.

@christoomey I guess we can take back the blame from Rails! 😅 Very happy to see this issue resolved.

christoomey commented 3 years ago

I mean, at the end of the day I love both Inertia and Rails, so I would have been open to any outcome here :). Either way, glad to see this resolved.