From e912ef37a540ba16c89686507cfb08515f342963 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Mon, 18 Nov 2024 14:10:22 -0500 Subject: [PATCH] Unify all error handling for the server --- .vscode/settings.json | 7 ++ lib/ruby_lsp/ruby_lsp_rails/server.rb | 170 ++++++++++++++------------ test/ruby_lsp_rails/server_test.rb | 19 ++- 3 files changed, 111 insertions(+), 85 deletions(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 00000000..58babe76 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,7 @@ +{ + "rubyLsp.indexing": { + "includedPatterns": [ + "test/dummy/**/*.rb" + ] + } +} diff --git a/lib/ruby_lsp/ruby_lsp_rails/server.rb b/lib/ruby_lsp/ruby_lsp_rails/server.rb index a649b3cf..72862046 100644 --- a/lib/ruby_lsp/ruby_lsp_rails/server.rb +++ b/lib/ruby_lsp/ruby_lsp_rails/server.rb @@ -19,7 +19,43 @@ def send_message(message) # Log a message to the editor's output panel def log_message(message) $stderr.puts(message) - send_message({ result: nil }) + end + + # Sends an error result to a request, if the request failed. DO NOT INVOKE THIS METHOD FOR NOTIFICATIONS! Use + # `log_message` instead, otherwise the client/server communication will go out of sync + def send_error_response(message) + send_message({ error: message }) + end + + # Sends a result back to the client + def send_result(result) + send_message({ result: result }) + end + + # Handle possible errors for a request. This should only be used for requests, which means messages that return a + # response back to the client. Errors are returned as an error object back to the client + def with_request_error_handling(request_name, &block) + block.call + rescue ActiveRecord::ConnectionNotEstablished + # Since this is a common problem, we show a specific error message to the user, instead of the full stack trace. + send_error_response("Request #{request_name} failed because database connection was not established.") + rescue ActiveRecord::NoDatabaseError + send_error_response("Request #{request_name} failed because the database does not exist.") + rescue => e + send_error_response("Request #{request_name} failed:\n#{e.full_message(highlight: false)}") + end + + # Handle possible errors for a notification. This should only be used for notifications, which means messages that + # do not return a response back to the client. Errors are logged to the editor's output panel + def with_notification_error_handling(notification_name, &block) + block.call + rescue ActiveRecord::ConnectionNotEstablished + # Since this is a common problem, we show a specific error message to the user, instead of the full stack trace. + log_message("Request #{notification_name} failed because database connection was not established.") + rescue ActiveRecord::NoDatabaseError + log_message("Request #{notification_name} failed because the database does not exist.") + rescue => e + log_message("Request #{notification_name} failed:\n#{e.full_message(highlight: false)}") end end @@ -88,11 +124,8 @@ def initialize(stdout: $stdout, override_default_output_device: true) end def start - # Load routes if they haven't been loaded yet (see https://github.com/rails/rails/pull/51614). - routes_reloader = ::Rails.application.routes_reloader - routes_reloader.execute_unless_loaded if routes_reloader&.respond_to?(:execute_unless_loaded) - - send_message({ result: { message: "ok", root: ::Rails.root.to_s } }) + load_routes + send_result({ message: "ok", root: ::Rails.root.to_s }) while @running headers = @stdin.gets("\r\n\r\n") @@ -104,41 +137,50 @@ def start end def execute(request, params) - request_name = request - request_name = "#{params[:server_addon_name]}##{params[:request_name]}" if request == "server_addon/delegate" - case request when "shutdown" @running = false when "model" - send_message(resolve_database_info_from_model(params.fetch(:name))) + with_request_error_handling(request) do + send_result(resolve_database_info_from_model(params.fetch(:name))) + end when "association_target_location" - send_message(resolve_association_target(params)) + with_request_error_handling(request) do + send_result(resolve_association_target(params)) + end when "pending_migrations_message" - send_message({ result: { pending_migrations_message: pending_migrations_message } }) + with_request_error_handling(request) do + send_result({ pending_migrations_message: pending_migrations_message }) + end when "run_migrations" - send_message({ result: run_migrations }) + with_request_error_handling(request) do + send_result(run_migrations) + end when "reload" - ::Rails.application.reloader.reload! + with_notification_error_handling(request) do + ::Rails.application.reloader.reload! + end when "route_location" - send_message(route_location(params.fetch(:name))) + with_request_error_handling(request) do + send_result(route_location(params.fetch(:name))) + end when "route_info" - send_message(resolve_route_info(params)) + with_request_error_handling(request) do + send_result(resolve_route_info(params)) + end when "server_addon/register" - require params[:server_addon_path] - ServerAddon.finalize_registrations!(@stdout) + with_notification_error_handling(request) do + require params[:server_addon_path] + ServerAddon.finalize_registrations!(@stdout) + end when "server_addon/delegate" server_addon_name = params[:server_addon_name] request_name = params[:request_name] + + # Do not wrap this in error handlers. Server add-ons need to have the flexibility to choose if they want to + # include a response or not as part of error handling, so a blanket approach is not appropriate. ServerAddon.delegate(server_addon_name, request_name, params.except(:request_name, :server_addon_name)) end - # Since this is a common problem, we show a specific error message to the user, instead of the full stack trace. - rescue ActiveRecord::ConnectionNotEstablished - log_message("Request #{request_name} failed because database connection was not established.") - rescue ActiveRecord::NoDatabaseError - log_message("Request #{request_name} failed because the database does not exist.") - rescue => e - log_message("Request #{request_name} failed:\n" + e.full_message(highlight: false)) end private @@ -156,19 +198,15 @@ def resolve_route_info(requirements) end source_location = route&.respond_to?(:source_location) && route.source_location + return unless source_location - if source_location - file, _, line = source_location.rpartition(":") - body = { - source_location: [::Rails.root.join(file).to_s, line], - verb: route.verb, - path: route.path.spec.to_s, - } + file, _, line = source_location.rpartition(":") - { result: body } - else - { result: nil } - end + { + source_location: [::Rails.root.join(file).to_s, line], + verb: route.verb, + path: route.path.spec.to_s, + } end # Older versions of Rails don't support `route_source_locations`. @@ -182,74 +220,48 @@ def route_location(name) end match_data = name.match(/^(.+)(_path|_url)$/) - return { result: nil } unless match_data + return unless match_data key = match_data[1] # A token could match the _path or _url pattern, but not be an actual route. route = ::Rails.application.routes.named_routes.get(key) - return { result: nil } unless route&.source_location - - { - result: { - location: ::Rails.root.join(route.source_location).to_s, - }, - } - rescue => e - { error: e.full_message(highlight: false) } + return unless route&.source_location + + { location: ::Rails.root.join(route.source_location).to_s } end else def route_location(name) - { result: nil } + nil end end def resolve_database_info_from_model(model_name) const = ActiveSupport::Inflector.safe_constantize(model_name) - unless active_record_model?(const) - return { - result: nil, - } - end + return unless active_record_model?(const) info = { - result: { - columns: const.columns.map { |column| [column.name, column.type, column.default, column.null] }, - primary_keys: Array(const.primary_key), - }, + columns: const.columns.map { |column| [column.name, column.type, column.default, column.null] }, + primary_keys: Array(const.primary_key), } if ActiveRecord::Tasks::DatabaseTasks.respond_to?(:schema_dump_path) - info[:result][:schema_file] = - ActiveRecord::Tasks::DatabaseTasks.schema_dump_path(const.connection.pool.db_config) - + info[:schema_file] = ActiveRecord::Tasks::DatabaseTasks.schema_dump_path(const.connection.pool.db_config) end + info - rescue => e - { error: e.full_message(highlight: false) } end def resolve_association_target(params) const = ActiveSupport::Inflector.safe_constantize(params[:model_name]) - unless active_record_model?(const) - return { - result: nil, - } - end + return unless active_record_model?(const) association_klass = const.reflect_on_association(params[:association_name].intern).klass - source_location = Object.const_source_location(association_klass.to_s) - { - result: { - location: source_location.first + ":" + source_location.second.to_s, - }, - } + { location: source_location.first + ":" + source_location.second.to_s } rescue NameError - { - result: nil, - } + nil end def active_record_model?(const) @@ -282,6 +294,14 @@ def run_migrations { message: stdout, status: status.exitstatus } end + + def load_routes + with_notification_error_handling("initial_load_routes") do + # Load routes if they haven't been loaded yet (see https://github.com/rails/rails/pull/51614). + routes_reloader = ::Rails.application.routes_reloader + routes_reloader.execute_unless_loaded if routes_reloader&.respond_to?(:execute_unless_loaded) + end + end end end end diff --git a/test/ruby_lsp_rails/server_test.rb b/test/ruby_lsp_rails/server_test.rb index 2025e7cf..b6f15a99 100644 --- a/test/ruby_lsp_rails/server_test.rb +++ b/test/ruby_lsp_rails/server_test.rb @@ -200,22 +200,21 @@ def resolve_route_info(requirements) test "shows error if there is a database connection error" do @server.expects(:pending_migrations_message).raises(ActiveRecord::ConnectionNotEstablished) + @server.execute("pending_migrations_message", {}) - _, stderr = capture_subprocess_io do - @server.execute("pending_migrations_message", {}) - end - assert_equal(stderr, "Request pending_migrations_message failed because database connection was not established.\n") - assert_equal({ result: nil }, response) + assert_equal( + { error: "Request pending_migrations_message failed because database connection was not established." }, response + ) end test "shows error if database does not exist" do @server.expects(:pending_migrations_message).raises(ActiveRecord::NoDatabaseError) + @server.execute("pending_migrations_message", {}) - _, stderr = capture_subprocess_io do - @server.execute("pending_migrations_message", {}) - end - assert_equal(stderr, "Request pending_migrations_message failed because the database does not exist.\n") - assert_equal({ result: nil }, response) + assert_equal( + { error: "Request pending_migrations_message failed because the database does not exist." }, + response, + ) end private