Skip to content

Support migration output messages containing multibyte characters #542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

louim
Copy link
Contributor

@louim louim commented Dec 10, 2024

The Content-Length header should be the number of bytes in the JSON. Since we're operating in binmode, the encoding of the JSON will be ASCII-8BIT, so we can use the bytesize method to get the number of bytes.

I noticed this when the LSP crashed while running a migration. Turns out that out migration output contained multibyte characters, and that was breaking the the length of the content that was read. The JSON that was read would be truncated because the content-length that was read was too short.

The easiest way to reproduce the problem would be to test with this migration:

# frozen_string_literal: true
class Multibytes < ActiveRecord::Migration[7.1]
  def change
    up_only do
      say_with_time 'This message is using curly quotes “” and em-dashes —' do
        # This is a no-op migration to test multibyte characters in migration files
      end
    end
  end
end

The error looked like that:

Error processing : /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/json-2.7.2/lib/json/common.rb:220:in `parse': unexpected token at '{"result":{"message":"Successfully installed foreman-0.88.1\n1 gem installed\n== 20241210202137 Multibytes: migrating =======================================\n-- This message is using curly quotes “” and em-dashes —\n   -> 0.0000s\n== 20241210202137 Multibytes: migrated (0.0000s) ==============================\n\n","statu' (JSON::ParserError)
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/json-2.7.2/lib/json/common.rb:220:in `parse'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/bundler/gems/ruby-lsp-rails-6cba8681e701/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb:300:in `read_response'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/bundler/gems/ruby-lsp-rails-6cba8681e701/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb:268:in `make_request'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/bundler/gems/ruby-lsp-rails-6cba8681e701/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb:204:in `run_migrations'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11690/lib/types/private/methods/_methods.rb:279:in `bind_call'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11690/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/bundler/gems/ruby-lsp-rails-6cba8681e701/lib/ruby_lsp/ruby_lsp_rails/addon.rb:146:in `handle_window_show_message_response'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11690/lib/types/private/methods/_methods.rb:279:in `bind_call'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11690/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/ruby-lsp-0.22.1/lib/ruby_lsp/server.rb:1225:in `window_show_message_request'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/ruby-lsp-0.22.1/lib/ruby_lsp/server.rb:148:in `process_response'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/ruby-lsp-0.22.1/lib/ruby_lsp/server.rb:110:in `process_message'
	from /Users/louim/.local/share/mise/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/ruby-lsp-0.22.1/lib/ruby_lsp/base_server.rb:163:in `block in new_worker'

I also fixed the runner-client for the same symptoms. However, I'm not sure when that implementation of send_message is used and didn't find an easy way to add a test. Let me know if this part of the change is required.

The Content-Length header should be the number of bytes in the JSON.
Since we're operating in binmode, the encoding of the JSON will be
ASCII-8BIT, so we can use the bytesize method to get the number of
bytes.
@louim louim requested a review from a team as a code owner December 10, 2024 20:03
@st0012 st0012 added the bugfix This PR fixes an existing bug label Dec 10, 2024
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix!

@st0012 st0012 merged commit 955dc27 into Shopify:main Dec 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants