hotwired/turbo-rails

turbo_frame_request_id safe operator in not handled correctly

jwoodrow opened this issue · 2 comments

When rendering a layout from specs, the request value will be nil but using the safe operator followed by the [] method leads to an undefined method [] for nil

We render an axlsx in some of our specs to validate the content of the rendered file like this

ApplicationController.new.render_to_string(
  formats:  [:xlsx],
  handlers: [:axlsx],
  filename:,
  template: 'path/to/view',
  locals:
)

this ends up failing here

request&.headers["Turbo-Frame"]

def turbo_frame_request_id
  request&.headers["Turbo-Frame"]
end

I would have used request&.headers&.dig('Turbo-Frame') to avoid this.

We found this out because we tried setting up rails/mission_control-jobs which depends on turbo-rails (which we don't currently use)

@jwoodrow thank you for opening this issue. Could you share a script that reproduces the behavior you're experiencing? I've shared a reproduction script below that you could modify to suit your needs. For example, you could replace the SystemTest and the HTML beneath the __END__ separator with an ActiveSupport::TestCase test that's closer to the source of your issue.

Based on what you've shared above, I'm unsure how the #[] access is raising a NoMethodError. the & operator should prevent the rest of the expression from executing when request == nil. That means the request object is present, but the #headers is returning nil. I'm not sure what could be the root cause of those circumstances.

Save this snippet to `bug.rb` then execute `ruby bug.rb`
require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  gem "propshaft"
  gem "puma"
  gem "sqlite3"
  gem "turbo-rails"

  gem "capybara"
  gem "cuprite", require: "capybara/cuprite"
end

ENV["DATABASE_URL"] = "sqlite3::memory:"
ENV["RAILS_ENV"] = "test"

require "active_record/railtie"
# require "active_storage/engine"
require "action_controller/railtie"
require "action_view/railtie"
# require "action_mailer/railtie"
# require "active_job/railtie"
require "action_cable/engine"
# require "action_mailbox/engine"
# require "action_text/engine"
require "rails/test_unit/railtie"

class App < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f

  config.root = __dir__
  config.hosts << "example.org"
  config.eager_load = false
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"
  config.consider_all_requests_local = true
  config.action_cable.cable = {"adapter" => "async"}
  config.turbo.draw_routes = false

  Rails.logger = config.logger = Logger.new($stdout)

  routes.append do
    root to: "application#index"
  end
end

Rails.application.initialize!

ActiveRecord::Schema.define do
  create_table :messages, force: true do |t|
    t.text :body, null: false
  end
end

class Message < ActiveRecord::Base
end

class ApplicationController < ActionController::Base
  include Rails.application.routes.url_helpers

  class_attribute :template, default: DATA.read

  def index
    render inline: template, formats: :html
  end
end

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: {js_errors: true}
end

Capybara.configure do |config|
  config.server = :puma, {Silent: true}
  config.default_normalize_ws = true
end

require "rails/test_help"

class TurboSystemTest < ApplicationSystemTestCase
  test "reproduces bug" do
    visit root_path

    assert_text "Loaded with Turbo"
  end
end

__END__
<!DOCTYPE html>
<html>
  <head>
    <%= csrf_meta_tags %>

    <script type="importmap">
      {
        "imports": {
          "@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>"
        }
      }
    </script>

    <script type="module">
      import "@hotwired/turbo-rails"

      addEventListener("turbo:load", () => document.body.innerHTML = "Loaded with Turbo")
    </script>
  </head>

  <body>Loaded without Turbo</body>
</html>

I could try to get you a reproduction script but AFAIK the &. Operator doesn't prevent the rest of the line from executing, it simply makes it so if the previous function call or value returned nil the next function call will be bypassed to return nil. That's why rubocop has a rule saying that if you start using &. You need to use it before each function call in the chain. Seeing as [] is a function call too then this situation happens.