Browse Source

Add Ruby 3.0 support (#16046)

* Fix issues with POSIX::Spawn, Terrapin and Ruby 3.0

Also improve the Terrapin monkey-patch for the stderr/stdout issue.

* Fix keyword argument handling throughout the codebase

* Monkey-patch Paperclip to fix keyword arguments handling in validators

* Change validation_extensions to please CodeClimate

* Bump microformats from 4.2.1 to 4.3.1

* Allow Ruby 3.0

* Add Ruby 3.0 test target to CircleCI

* Add test for admin dashboard warnings

* Fix admin dashboard warnings on Ruby 3.0
closed-social-v3
Claire 3 years ago
committed by GitHub
parent
commit
566fc90913
No known key found for this signature in database GPG Key ID: 4AEE18F83AFDEB23
19 changed files with 177 additions and 74 deletions
  1. +27
    -0
      .circleci/config.yml
  2. +1
    -1
      Gemfile
  3. +3
    -3
      Gemfile.lock
  4. +1
    -1
      app/controllers/activitypub/outboxes_controller.rb
  5. +2
    -2
      app/controllers/api/v1/accounts_controller.rb
  6. +1
    -1
      app/controllers/api/v1/follow_requests_controller.rb
  7. +1
    -1
      app/models/session_activation.rb
  8. +12
    -7
      app/models/user.rb
  9. +1
    -1
      app/views/admin/dashboard/index.html.haml
  10. +3
    -3
      app/workers/import/relationship_worker.rb
  11. +1
    -0
      config/application.rb
  12. +2
    -3
      config/initializers/session_store.rb
  13. +58
    -0
      lib/paperclip/validation_extensions.rb
  14. +45
    -42
      lib/terrapin/multi_pipe_extensions.rb
  15. +11
    -1
      spec/controllers/admin/dashboard_controller_spec.rb
  16. +2
    -2
      spec/mailers/notification_mailer_spec.rb
  17. +2
    -2
      spec/mailers/user_mailer_spec.rb
  18. +3
    -3
      spec/models/session_activation_spec.rb
  19. +1
    -1
      spec/presenters/account_relationships_presenter_spec.rb

+ 27
- 0
.circleci/config.yml View File

@ -129,6 +129,13 @@ jobs:
environment: *ruby_environment environment: *ruby_environment
<<: *install_ruby_dependencies <<: *install_ruby_dependencies
install-ruby3.0:
<<: *defaults
docker:
- image: circleci/ruby:3.0-buster-node
environment: *ruby_environment
<<: *install_ruby_dependencies
build: build:
<<: *defaults <<: *defaults
steps: steps:
@ -187,6 +194,18 @@ jobs:
- image: circleci/redis:5-alpine - image: circleci/redis:5-alpine
<<: *test_steps <<: *test_steps
test-ruby3.0:
<<: *defaults
docker:
- image: circleci/ruby:3.0-buster-node
environment: *ruby_environment
- image: circleci/postgres:12.2
environment:
POSTGRES_USER: root
POSTGRES_HOST_AUTH_METHOD: trust
- image: circleci/redis:5-alpine
<<: *test_steps
test-webui: test-webui:
<<: *defaults <<: *defaults
docker: docker:
@ -227,6 +246,10 @@ workflows:
requires: requires:
- install - install
- install-ruby2.7 - install-ruby2.7
- install-ruby3.0:
requires:
- install
- install-ruby2.7
- build: - build:
requires: requires:
- install-ruby2.7 - install-ruby2.7
@ -241,6 +264,10 @@ workflows:
requires: requires:
- install-ruby2.6 - install-ruby2.6
- build - build
- test-ruby3.0:
requires:
- install-ruby3.0
- build
- test-webui: - test-webui:
requires: requires:
- install - install

+ 1
- 1
Gemfile View File

@ -1,7 +1,7 @@
# frozen_string_literal: true # frozen_string_literal: true
source 'https://rubygems.org' source 'https://rubygems.org'
ruby '>= 2.5.0', '< 3.0.0'
ruby '>= 2.5.0', '< 3.1.0'
gem 'pkg-config', '~> 1.4' gem 'pkg-config', '~> 1.4'

+ 3
- 3
Gemfile.lock View File

@ -292,7 +292,7 @@ GEM
ipaddress (0.8.3) ipaddress (0.8.3)
iso-639 (0.3.5) iso-639 (0.3.5)
jmespath (1.4.0) jmespath (1.4.0)
json (2.3.1)
json (2.5.1)
json-canonicalization (0.2.1) json-canonicalization (0.2.1)
json-ld (3.1.9) json-ld (3.1.9)
htmlentities (~> 4.3) htmlentities (~> 4.3)
@ -344,7 +344,7 @@ GEM
redis (>= 3.0.5) redis (>= 3.0.5)
memory_profiler (1.0.0) memory_profiler (1.0.0)
method_source (1.0.0) method_source (1.0.0)
microformats (4.2.1)
microformats (4.3.1)
json (~> 2.2) json (~> 2.2)
nokogiri (~> 1.10) nokogiri (~> 1.10)
mime-types (3.3.1) mime-types (3.3.1)
@ -354,7 +354,7 @@ GEM
nokogiri (~> 1) nokogiri (~> 1)
rake rake
mini_mime (1.0.3) mini_mime (1.0.3)
mini_portile2 (2.5.0)
mini_portile2 (2.5.1)
minitest (5.14.4) minitest (5.14.4)
msgpack (1.4.2) msgpack (1.4.2)
multi_json (1.15.0) multi_json (1.15.0)

+ 1
- 1
app/controllers/activitypub/outboxes_controller.rb View File

@ -20,7 +20,7 @@ class ActivityPub::OutboxesController < ActivityPub::BaseController
def outbox_presenter def outbox_presenter
if page_requested? if page_requested?
ActivityPub::CollectionPresenter.new( ActivityPub::CollectionPresenter.new(
id: outbox_url(page_params),
id: outbox_url(**page_params),
type: :ordered, type: :ordered,
part_of: outbox_url, part_of: outbox_url,
prev: prev_page, prev: prev_page,

+ 2
- 2
app/controllers/api/v1/accounts_controller.rb View File

@ -35,7 +35,7 @@ class Api::V1::AccountsController < Api::BaseController
follow = FollowService.new.call(current_user.account, @account, reblogs: params.key?(:reblogs) ? truthy_param?(:reblogs) : nil, notify: params.key?(:notify) ? truthy_param?(:notify) : nil, with_rate_limit: true) follow = FollowService.new.call(current_user.account, @account, reblogs: params.key?(:reblogs) ? truthy_param?(:reblogs) : nil, notify: params.key?(:notify) ? truthy_param?(:notify) : nil, with_rate_limit: true)
options = @account.locked? || current_user.account.silenced? ? {} : { following_map: { @account.id => { reblogs: follow.show_reblogs?, notify: follow.notify? } }, requested_map: { @account.id => false } } options = @account.locked? || current_user.account.silenced? ? {} : { following_map: { @account.id => { reblogs: follow.show_reblogs?, notify: follow.notify? } }, requested_map: { @account.id => false } }
render json: @account, serializer: REST::RelationshipSerializer, relationships: relationships(options)
render json: @account, serializer: REST::RelationshipSerializer, relationships: relationships(**options)
end end
def block def block
@ -70,7 +70,7 @@ class Api::V1::AccountsController < Api::BaseController
end end
def relationships(**options) def relationships(**options)
AccountRelationshipsPresenter.new([@account.id], current_user.account_id, options)
AccountRelationshipsPresenter.new([@account.id], current_user.account_id, **options)
end end
def account_params def account_params

+ 1
- 1
app/controllers/api/v1/follow_requests_controller.rb View File

@ -29,7 +29,7 @@ class Api::V1::FollowRequestsController < Api::BaseController
end end
def relationships(**options) def relationships(**options)
AccountRelationshipsPresenter.new([params[:id]], current_user.account_id, options)
AccountRelationshipsPresenter.new([params[:id]], current_user.account_id, **options)
end end
def load_accounts def load_accounts

+ 1
- 1
app/models/session_activation.rb View File

@ -44,7 +44,7 @@ class SessionActivation < ApplicationRecord
end end
def activate(**options) def activate(**options)
activation = create!(options)
activation = create!(**options)
purge_old purge_old
activation activation
end end

+ 12
- 7
app/models/user.rb View File

@ -370,15 +370,20 @@ class User < ApplicationRecord
protected protected
def send_devise_notification(notification, *args)
def send_devise_notification(notification, *args, **kwargs)
# This method can be called in `after_update` and `after_commit` hooks, # This method can be called in `after_update` and `after_commit` hooks,
# but we must make sure the mailer is actually called *after* commit, # but we must make sure the mailer is actually called *after* commit,
# otherwise it may work on stale data. To do this, figure out if we are # otherwise it may work on stale data. To do this, figure out if we are
# within a transaction. # within a transaction.
# It seems like devise sends keyword arguments as a hash in the last
# positional argument
kwargs = args.pop if args.last.is_a?(Hash) && kwargs.empty?
if ActiveRecord::Base.connection.current_transaction.try(:records)&.include?(self) if ActiveRecord::Base.connection.current_transaction.try(:records)&.include?(self)
pending_devise_notifications << [notification, args]
pending_devise_notifications << [notification, args, kwargs]
else else
render_and_send_devise_message(notification, *args)
render_and_send_devise_message(notification, *args, **kwargs)
end end
end end
@ -389,8 +394,8 @@ class User < ApplicationRecord
end end
def send_pending_devise_notifications def send_pending_devise_notifications
pending_devise_notifications.each do |notification, args|
render_and_send_devise_message(notification, *args)
pending_devise_notifications.each do |notification, args, kwargs|
render_and_send_devise_message(notification, *args, **kwargs)
end end
# Empty the pending notifications array because the # Empty the pending notifications array because the
@ -403,8 +408,8 @@ class User < ApplicationRecord
@pending_devise_notifications ||= [] @pending_devise_notifications ||= []
end end
def render_and_send_devise_message(notification, *args)
devise_mailer.send(notification, self, *args).deliver_later
def render_and_send_devise_message(notification, *args, **kwargs)
devise_mailer.send(notification, self, *args, **kwargs).deliver_later
end end
def set_approved def set_approved

+ 1
- 1
app/views/admin/dashboard/index.html.haml View File

@ -5,7 +5,7 @@
.flash-message-stack .flash-message-stack
- @system_checks.each do |message| - @system_checks.each do |message|
.flash-message.warning .flash-message.warning
= t("admin.system_checks.#{message.key}.message_html", message.value ? { value: content_tag(:strong, message.value) } : {})
= t("admin.system_checks.#{message.key}.message_html", value: message.value ? content_tag(:strong, message.value) : nil)
- if message.action - if message.action
= link_to t("admin.system_checks.#{message.key}.action"), message.action = link_to t("admin.system_checks.#{message.key}.action"), message.action

+ 3
- 3
app/workers/import/relationship_worker.rb View File

@ -5,7 +5,7 @@ class Import::RelationshipWorker
sidekiq_options queue: 'pull', retry: 8, dead: false sidekiq_options queue: 'pull', retry: 8, dead: false
def perform(account_id, target_account_uri, relationship, options = {})
def perform(account_id, target_account_uri, relationship, options)
from_account = Account.find(account_id) from_account = Account.find(account_id)
target_domain = domain(target_account_uri) target_domain = domain(target_account_uri)
target_account = stoplight_wrap_request(target_domain) { ResolveAccountService.new.call(target_account_uri, { check_delivery_availability: true }) } target_account = stoplight_wrap_request(target_domain) { ResolveAccountService.new.call(target_account_uri, { check_delivery_availability: true }) }
@ -16,7 +16,7 @@ class Import::RelationshipWorker
case relationship case relationship
when 'follow' when 'follow'
begin begin
FollowService.new.call(from_account, target_account, options)
FollowService.new.call(from_account, target_account, **options)
rescue ActiveRecord::RecordInvalid rescue ActiveRecord::RecordInvalid
raise if FollowLimitValidator.limit_for_account(from_account) < from_account.following_count raise if FollowLimitValidator.limit_for_account(from_account) < from_account.following_count
end end
@ -27,7 +27,7 @@ class Import::RelationshipWorker
when 'unblock' when 'unblock'
UnblockService.new.call(from_account, target_account) UnblockService.new.call(from_account, target_account)
when 'mute' when 'mute'
MuteService.new.call(from_account, target_account, options)
MuteService.new.call(from_account, target_account, **options)
when 'unmute' when 'unmute'
UnmuteService.new.call(from_account, target_account) UnmuteService.new.call(from_account, target_account)
end end

+ 1
- 0
config/application.rb View File

@ -10,6 +10,7 @@ require_relative '../lib/exceptions'
require_relative '../lib/enumerable' require_relative '../lib/enumerable'
require_relative '../lib/sanitize_ext/sanitize_config' require_relative '../lib/sanitize_ext/sanitize_config'
require_relative '../lib/redis/namespace_extensions' require_relative '../lib/redis/namespace_extensions'
require_relative '../lib/paperclip/validation_extensions'
require_relative '../lib/paperclip/url_generator_extensions' require_relative '../lib/paperclip/url_generator_extensions'
require_relative '../lib/paperclip/attachment_extensions' require_relative '../lib/paperclip/attachment_extensions'
require_relative '../lib/paperclip/media_type_spoof_detector_extensions' require_relative '../lib/paperclip/media_type_spoof_detector_extensions'

+ 2
- 3
config/initializers/session_store.rb View File

@ -1,7 +1,6 @@
# Be sure to restart your server when you modify this file. # Be sure to restart your server when you modify this file.
Rails.application.config.session_store :cookie_store, {
Rails.application.config.session_store :cookie_store,
key: '_mastodon_session', key: '_mastodon_session',
secure: (Rails.env.production? || ENV['LOCAL_HTTPS'] == 'true'), secure: (Rails.env.production? || ENV['LOCAL_HTTPS'] == 'true'),
same_site: :lax,
}
same_site: :lax

+ 58
- 0
lib/paperclip/validation_extensions.rb View File

@ -0,0 +1,58 @@
# frozen_string_literal: true
# Monkey-patch various Paperclip validators for Ruby 3.0 compatibility
module Paperclip
module Validators
module AttachmentSizeValidatorExtensions
def validate_each(record, attr_name, _value)
base_attr_name = attr_name
attr_name = "#{attr_name}_file_size".to_sym
value = record.send(:read_attribute_for_validation, attr_name)
if value.present?
options.slice(*Paperclip::Validators::AttachmentSizeValidator::AVAILABLE_CHECKS).each do |option, option_value|
option_value = option_value.call(record) if option_value.is_a?(Proc)
option_value = extract_option_value(option, option_value)
next if value.send(Paperclip::Validators::AttachmentSizeValidator::CHECKS[option], option_value)
error_message_key = options[:in] ? :in_between : option
[attr_name, base_attr_name].each do |error_attr_name|
record.errors.add(error_attr_name, error_message_key, **filtered_options(value).merge(
min: min_value_in_human_size(record),
max: max_value_in_human_size(record),
count: human_size(option_value)
))
end
end
end
end
end
module AttachmentContentTypeValidatorExtensions
def mark_invalid(record, attribute, types)
record.errors.add attribute, :invalid, **options.merge({ types: types.join(', ') })
end
end
module AttachmentPresenceValidatorExtensions
def validate_each(record, attribute, _value)
if record.send("#{attribute}_file_name").blank?
record.errors.add(attribute, :blank, **options)
end
end
end
module AttachmentFileNameValidatorExtensions
def mark_invalid(record, attribute, patterns)
record.errors.add attribute, :invalid, options.merge({ names: patterns.join(', ') })
end
end
end
end
Paperclip::Validators::AttachmentSizeValidator.prepend(Paperclip::Validators::AttachmentSizeValidatorExtensions)
Paperclip::Validators::AttachmentContentTypeValidator.prepend(Paperclip::Validators::AttachmentContentTypeValidatorExtensions)
Paperclip::Validators::AttachmentPresenceValidator.prepend(Paperclip::Validators::AttachmentPresenceValidatorExtensions)
Paperclip::Validators::AttachmentFileNameValidator.prepend(Paperclip::Validators::AttachmentFileNameValidatorExtensions)

+ 45
- 42
lib/terrapin/multi_pipe_extensions.rb View File

@ -1,61 +1,64 @@
# frozen_string_literal: false # frozen_string_literal: false
# Fix adapted from https://github.com/thoughtbot/terrapin/pull/5
require 'fcntl'
module Terrapin module Terrapin
module MultiPipeExtensions module MultiPipeExtensions
def read
read_streams(@stdout_in, @stderr_in)
end
def initialize
@stdout_in, @stdout_out = IO.pipe
@stderr_in, @stderr_out = IO.pipe
def close_read
begin
@stdout_in.close
rescue IOError
# Do nothing
end
begin
@stderr_in.close
rescue IOError
# Do nothing
end
clear_nonblocking_flags!
end end
def read_streams(output, error)
@stdout_output = ''
@stderr_output = ''
def pipe_options
# Add some flags to explicitly close the other end of the pipes
{ out: @stdout_out, err: @stderr_out, @stdout_in => :close, @stderr_in => :close }
end
read_fds = [output, error]
def read
# While we are patching Terrapin, fix child process potentially getting stuck on writing
# to stderr.
until read_fds.empty?
to_read, = IO.select(read_fds)
@stdout_output = +''
@stderr_output = +''
if to_read.include?(output)
@stdout_output << read_stream(output)
read_fds.delete(output) if output.closed?
end
fds_to_read = [@stdout_in, @stderr_in]
until fds_to_read.empty?
rs, = IO.select(fds_to_read)
if to_read.include?(error)
@stderr_output << read_stream(error)
read_fds.delete(error) if error.closed?
end
read_nonblocking!(@stdout_in, @stdout_output, fds_to_read) if rs.include?(@stdout_in)
read_nonblocking!(@stderr_in, @stderr_output, fds_to_read) if rs.include?(@stderr_in)
end end
end end
def read_stream(io)
result = ''
begin
while (partial_result = io.read_nonblock(8192))
result << partial_result
end
rescue EOFError, Errno::EPIPE
io.close
rescue Errno::EINTR, Errno::EWOULDBLOCK, Errno::EAGAIN
# Do nothing
private
# @param [IO] io IO Stream to read until there is nothing to read
# @param [String] result Mutable string to which read values will be appended to
# @param [Array<IO>] fds_to_read Mutable array from which `io` should be removed on EOF
def read_nonblocking!(io, result, fds_to_read)
while (partial_result = io.read_nonblock(8192))
result << partial_result
end end
rescue IO::WaitReadable
# Do nothing
rescue EOFError
fds_to_read.delete(io)
end
def clear_nonblocking_flags!
# Ruby 3.0 sets pipes to non-blocking mode, and resets the flags as
# needed when calling fork/exec-related syscalls, but posix-spawn does
# not currently do that, so we need to do it manually for the time being
# so that the child process do not error out when the buffers are full.
stdout_flags = @stdout_out.fcntl(Fcntl::F_GETFL)
@stdout_out.fcntl(Fcntl::F_SETFL, stdout_flags & ~Fcntl::O_NONBLOCK) if stdout_flags & Fcntl::O_NONBLOCK
result
stderr_flags = @stderr_out.fcntl(Fcntl::F_GETFL)
@stderr_out.fcntl(Fcntl::F_SETFL, stderr_flags & ~Fcntl::O_NONBLOCK) if stderr_flags & Fcntl::O_NONBLOCK
rescue NameError, NotImplementedError, Errno::EINVAL
# Probably on windows, where pipes are blocking by default
end end
end end
end end

+ 11
- 1
spec/controllers/admin/dashboard_controller_spec.rb View File

@ -3,9 +3,19 @@
require 'rails_helper' require 'rails_helper'
describe Admin::DashboardController, type: :controller do describe Admin::DashboardController, type: :controller do
render_views
describe 'GET #index' do describe 'GET #index' do
it 'returns 200' do
before do
allow(Admin::SystemCheck).to receive(:perform).and_return([
Admin::SystemCheck::Message.new(:database_schema_check),
Admin::SystemCheck::Message.new(:rules_check, nil, admin_rules_path),
Admin::SystemCheck::Message.new(:sidekiq_process_check, 'foo, bar'),
])
sign_in Fabricate(:user, admin: true) sign_in Fabricate(:user, admin: true)
end
it 'returns 200' do
get :index get :index
expect(response).to have_http_status(200) expect(response).to have_http_status(200)

+ 2
- 2
spec/mailers/notification_mailer_spec.rb View File

@ -10,12 +10,12 @@ RSpec.describe NotificationMailer, type: :mailer do
it 'renders subject localized for the locale of the receiver' do it 'renders subject localized for the locale of the receiver' do
locale = %i(de en).sample locale = %i(de en).sample
receiver.update!(locale: locale) receiver.update!(locale: locale)
expect(mail.subject).to eq I18n.t(*args, kwrest.merge(locale: locale))
expect(mail.subject).to eq I18n.t(*args, **kwrest.merge(locale: locale))
end end
it 'renders subject localized for the default locale if the locale of the receiver is unavailable' do it 'renders subject localized for the default locale if the locale of the receiver is unavailable' do
receiver.update!(locale: nil) receiver.update!(locale: nil)
expect(mail.subject).to eq I18n.t(*args, kwrest.merge(locale: I18n.default_locale))
expect(mail.subject).to eq I18n.t(*args, **kwrest.merge(locale: I18n.default_locale))
end end
end end

+ 2
- 2
spec/mailers/user_mailer_spec.rb View File

@ -9,12 +9,12 @@ describe UserMailer, type: :mailer do
it 'renders subject localized for the locale of the receiver' do it 'renders subject localized for the locale of the receiver' do
locale = I18n.available_locales.sample locale = I18n.available_locales.sample
receiver.update!(locale: locale) receiver.update!(locale: locale)
expect(mail.subject).to eq I18n.t(*args, kwrest.merge(locale: locale))
expect(mail.subject).to eq I18n.t(*args, **kwrest.merge(locale: locale))
end end
it 'renders subject localized for the default locale if the locale of the receiver is unavailable' do it 'renders subject localized for the default locale if the locale of the receiver is unavailable' do
receiver.update!(locale: nil) receiver.update!(locale: nil)
expect(mail.subject).to eq I18n.t(*args, kwrest.merge(locale: I18n.default_locale))
expect(mail.subject).to eq I18n.t(*args, **kwrest.merge(locale: I18n.default_locale))
end end
end end

+ 3
- 3
spec/models/session_activation_spec.rb View File

@ -74,13 +74,13 @@ RSpec.describe SessionActivation, type: :model do
let(:options) { { user: Fabricate(:user), session_id: '1' } } let(:options) { { user: Fabricate(:user), session_id: '1' } }
it 'calls create! and purge_old' do it 'calls create! and purge_old' do
expect(described_class).to receive(:create!).with(options)
expect(described_class).to receive(:create!).with(**options)
expect(described_class).to receive(:purge_old) expect(described_class).to receive(:purge_old)
described_class.activate(options)
described_class.activate(**options)
end end
it 'returns an instance of SessionActivation' do it 'returns an instance of SessionActivation' do
expect(described_class.activate(options)).to be_kind_of SessionActivation
expect(described_class.activate(**options)).to be_kind_of SessionActivation
end end
end end

+ 1
- 1
spec/presenters/account_relationships_presenter_spec.rb View File

@ -13,7 +13,7 @@ RSpec.describe AccountRelationshipsPresenter do
allow(Account).to receive(:domain_blocking_map).with(account_ids, current_account_id).and_return(default_map) allow(Account).to receive(:domain_blocking_map).with(account_ids, current_account_id).and_return(default_map)
end end
let(:presenter) { AccountRelationshipsPresenter.new(account_ids, current_account_id, options) }
let(:presenter) { AccountRelationshipsPresenter.new(account_ids, current_account_id, **options) }
let(:current_account_id) { Fabricate(:account).id } let(:current_account_id) { Fabricate(:account).id }
let(:account_ids) { [Fabricate(:account).id] } let(:account_ids) { [Fabricate(:account).id] }
let(:default_map) { { 1 => true } } let(:default_map) { { 1 => true } }

Loading…
Cancel
Save