From 372ffe5022258260804d1c0c0698c9479f386b3c Mon Sep 17 00:00:00 2001 From: Sebastian van Hesteren Date: Thu, 12 Sep 2024 14:19:02 +0200 Subject: [PATCH] Add support for Rails 7.2 (#45) * Update pbbuilder and fix show_exceptions? errors. * Fix single test call * Handle the 3 show_exceptions settings in the exception_handling module * Set show_exceptions in test to default none. * Add changelog entry * Improve comments * Get updated pbbulder and reject twirp version 1.11 * Remove .ruby-version in favor of multi version testing * yank pry out --- .github/workflows/test.yml | 12 +++++++++--- .ruby-version | 1 - CHANGELOG.md | 6 ++++-- Gemfile | 1 - Rakefile | 23 +++++++++++++++++++++-- lib/rails_twirp/exception_handling.rb | 14 ++++++++++++-- lib/rails_twirp/version.rb | 2 +- rails_twirp.gemspec | 9 ++++----- test/dummy/config/environments/test.rb | 8 ++++++-- test/ping_controller_test.rb | 14 ++++++-------- test/test_helper.rb | 1 + 11 files changed, 64 insertions(+), 27 deletions(-) delete mode 100644 .ruby-version diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f6ee3bc..472048a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,18 +1,24 @@ name: Ruby Test -on: push +on: [push] jobs: test: + name: Ruby Tests runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + ruby: ["3.0", "3.1", "3.2", "3.3"] + steps: - - name: Checkout code + - name: Checkout uses: actions/checkout@v3 - name: Setup Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: 3.3.0 bundler-cache: true + ruby-version: ${{ matrix.ruby }} - name: Run tests run: bin/test diff --git a/.ruby-version b/.ruby-version deleted file mode 100644 index 15a2799..0000000 --- a/.ruby-version +++ /dev/null @@ -1 +0,0 @@ -3.3.0 diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f9a923..c456f69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,11 @@ -### 0.16 +### 0.17 +* Adding support for rails 7.2. Mostly this was fixing the deprecation of `request.show_exceptions?` and support the various + values of `Rails.application.config.action_dispatch.show_exceptions`. +### 0.16 * Ensure `decode_rack_response` always calls `#close` on the Rack body in tests, as some middleware might be applying a BodyProxy ### 0.15 - * Exclude versions of Rails 7 which were incompatible with the pbbuilder ActionView handler, as pbbuilder cannot work there at all * Fix decode_rack_response to be compatible with Rack response body wrappers (and conform to the Rack SPEC) diff --git a/Gemfile b/Gemfile index 734075a..443bc88 100644 --- a/Gemfile +++ b/Gemfile @@ -8,7 +8,6 @@ gemspec gem "sqlite3" gem "pbbuilder" gem "standard" -gem "pry" # HACK(bouk): Overwrite Bundler's platform matcher to ignore universal CPU # The protobuf and gRPC 'universal' macOS gems break on M1 diff --git a/Rakefile b/Rakefile index bb704a1..a5ec7b7 100644 --- a/Rakefile +++ b/Rakefile @@ -6,8 +6,27 @@ require "rake/testtask" Rake::TestTask.new(:test) do |t| t.libs << "test" - t.pattern = "test/**/*_test.rb" - t.verbose = false + t.libs << "lib" + + # Running specific tests with line numbers, like with rails test, is not supported by default in rake. + # By setting the TESTOPS env var we can however specify the name of a single test with underscores instead of spaces. + # So run your single test by calling for ex: + # + # rake test /Users/sebastian/projects/cheddar/rails-twirp/test/ping_controller_test.rb "uncaught errors should bubble up to the test" + + file_name = ARGV[1] + test_name = ARGV[2]&.tr(" ", "_") + + ENV["TESTOPTS"] = "--verbose" + + t.test_files = if file_name + if test_name + ENV["TESTOPTS"] += " --name=test_#{test_name}" + end + [file_name] + else + FileList["test/**/*_test.rb"] + end end task default: :test diff --git a/lib/rails_twirp/exception_handling.rb b/lib/rails_twirp/exception_handling.rb index 4920c96..e1177c9 100644 --- a/lib/rails_twirp/exception_handling.rb +++ b/lib/rails_twirp/exception_handling.rb @@ -18,9 +18,19 @@ def process_action(*) # We adopt the same error handling logic as Rails' standard middlewares: # 1. When we 'show exceptions' we make the exception bubble up—this is useful for testing - # If the exception gets raised here error reporting will happen in the middleware of the APM package + # If the exception gets raised here, error reporting will happen in the middleware of the APM package # higher in the call stack. - raise e unless http_request.show_exceptions? + # + + # A backtrace cleaner acts like a filter that only shows us the backtrace + # with a selection of useful lines compared to all lines the code goes through. + backtrace_cleaner = http_request.get_header("action_dispatch.backtrace_cleaner") + + # Contains various exception related methods we can use and takes the backtrace_cleaner into consideration. + exception_wrapper = ActionDispatch::ExceptionWrapper.new(backtrace_cleaner, e) + # ExceptionWrapper.show? contains the logic that chooses to pass exceptions through or not based on the + # `Rails.application.config.action_dispatch.show_exceptions` setting of :none, :rescuable and :all + raise e unless exception_wrapper.show?(http_request) # 2. We report the error to the error tracking service, this needs to be configured. RailsTwirp.unhandled_exception_handler&.call(e) diff --git a/lib/rails_twirp/version.rb b/lib/rails_twirp/version.rb index 38df953..924bb85 100644 --- a/lib/rails_twirp/version.rb +++ b/lib/rails_twirp/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module RailsTwirp - VERSION = "0.16" + VERSION = "0.17" end diff --git a/rails_twirp.gemspec b/rails_twirp.gemspec index 557eea1..950f712 100644 --- a/rails_twirp.gemspec +++ b/rails_twirp.gemspec @@ -13,10 +13,9 @@ Gem::Specification.new do |spec| spec.files = `git ls-files`.split("\n") - # Rails has shipped an incompatible change in ActiveView, that was reverted in later versions - # but at this time has not been released as a 7.x version - # @see https://github.com/rails/rails/pull/51023 - spec.add_runtime_dependency "rails", ">= 6.1.3", " < 7.1" - spec.add_runtime_dependency "twirp", ">= 1.9", "< 1.11" + spec.add_runtime_dependency "rails", ">= 6.1.3", "!= 7.1" + spec.add_runtime_dependency "twirp", ">= 1.9", "!= 1.11" + spec.add_development_dependency "pry" + spec.required_ruby_version = ">= 3" end diff --git a/test/dummy/config/environments/test.rb b/test/dummy/config/environments/test.rb index e0b9cd4..192d83c 100644 --- a/test/dummy/config/environments/test.rb +++ b/test/dummy/config/environments/test.rb @@ -28,8 +28,12 @@ config.action_controller.perform_caching = false config.cache_store = :null_store - # Raise exceptions instead of rendering exception templates. - config.action_dispatch.show_exceptions = false + # Set to one of the following (default is :all): + # + # :all - render error pages for all exceptions + # :rescuable - render error pages for exceptions declared by config.action_dispatch.rescue_responses + # :none - raise all exceptions + config.action_dispatch.show_exceptions = :none # Disable request forgery protection in test environment. config.action_controller.allow_forgery_protection = false diff --git a/test/ping_controller_test.rb b/test/ping_controller_test.rb index 802c8bc..e3a1a4d 100644 --- a/test/ping_controller_test.rb +++ b/test/ping_controller_test.rb @@ -49,14 +49,16 @@ class PingControllerTest < RailsTwirp::IntegrationTest end test "uncaught errors should bubble up to the test" do + Rails.application.env_config["action_dispatch.show_exceptions"] = :none + req = RPC::DummyAPI::PingRequest.new assert_raises StandardError, "Uncaught" do rpc RPC::DummyAPI::DummyService, "UncaughtError", req end end - test "uncaught errors should return an internal error with details if show_exceptions is true" do - Rails.application.env_config["action_dispatch.show_exceptions"] = true + test "uncaught errors should return an internal error with details if show_exceptions is all" do + Rails.application.env_config["action_dispatch.show_exceptions"] = :all req = RPC::DummyAPI::PingRequest.new rpc RPC::DummyAPI::DummyService, "UncaughtError", req @@ -64,12 +66,10 @@ class PingControllerTest < RailsTwirp::IntegrationTest assert_equal :internal, response.code assert_equal "Uncaught", response.msg assert_equal "StandardError", response.meta["cause"] - ensure - Rails.application.env_config["action_dispatch.show_exceptions"] = false end test "uncaught errors should be fanned out to the exception handler proc if one is defined" do - Rails.application.env_config["action_dispatch.show_exceptions"] = true + Rails.application.env_config["action_dispatch.show_exceptions"] = :all captured_exception = nil RailsTwirp.unhandled_exception_handler = ->(e) { captured_exception = e } @@ -83,11 +83,10 @@ class PingControllerTest < RailsTwirp::IntegrationTest assert_kind_of StandardError, captured_exception ensure RailsTwirp.unhandled_exception_handler = nil - Rails.application.env_config["action_dispatch.show_exceptions"] = false end test "uncaught errors should return an internal error without if show_exceptions is true and show_detailed_exceptions is false" do - Rails.application.env_config["action_dispatch.show_exceptions"] = true + Rails.application.env_config["action_dispatch.show_exceptions"] = :all Rails.application.env_config["action_dispatch.show_detailed_exceptions"] = false req = RPC::DummyAPI::PingRequest.new @@ -97,7 +96,6 @@ class PingControllerTest < RailsTwirp::IntegrationTest assert_equal "Internal error", response.msg ensure Rails.application.env_config["action_dispatch.show_detailed_exceptions"] = true - Rails.application.env_config["action_dispatch.show_exceptions"] = false end test "before error" do diff --git a/test/test_helper.rb b/test/test_helper.rb index 27888ff..d5bdfb8 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -7,6 +7,7 @@ ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] require "rails/test_help" require "rails/test_unit/reporter" +require "pry" Rails::TestUnitReporter.executable = "bin/test" # Load fixtures from the engine