diff options
14 files changed, 61 insertions, 41 deletions
diff --git a/actioncable/CHANGELOG.md b/actioncable/CHANGELOG.md index 946fdfb3fc..5c505d5dde 100644 --- a/actioncable/CHANGELOG.md +++ b/actioncable/CHANGELOG.md @@ -1,3 +1,8 @@ +* Allow channel identifiers with no backslahes/escaping to be accepted + by the subscription storer. + + *Jon Moss* + * Safely support autoloading and class unloading, by preventing concurrent loads, and disconnecting all cables during reload. diff --git a/actioncable/app/assets/javascripts/action_cable/connection.coffee b/actioncable/app/assets/javascripts/action_cable/connection.coffee index 4244322a1e..e8c9ae6bd0 100644 --- a/actioncable/app/assets/javascripts/action_cable/connection.coffee +++ b/actioncable/app/assets/javascripts/action_cable/connection.coffee @@ -73,8 +73,11 @@ class ActionCable.Connection events: message: (event) -> {identifier, message, type} = JSON.parse(event.data) - switch type + when message_types.welcome + @consumer.connectionMonitor.connected() + when message_types.ping + @consumer.connectionMonitor.ping() when message_types.confirmation @consumer.subscriptions.notify(identifier, "connected") when message_types.rejection diff --git a/actioncable/app/assets/javascripts/action_cable/connection_monitor.coffee b/actioncable/app/assets/javascripts/action_cable/connection_monitor.coffee index 75a6f1fb07..740e86643e 100644 --- a/actioncable/app/assets/javascripts/action_cable/connection_monitor.coffee +++ b/actioncable/app/assets/javascripts/action_cable/connection_monitor.coffee @@ -7,10 +7,7 @@ class ActionCable.ConnectionMonitor @staleThreshold: 6 # Server::Connections::BEAT_INTERVAL * 2 (missed two pings) - identifier: ActionCable.INTERNAL.identifiers.ping - constructor: (@consumer) -> - @consumer.subscriptions.add(this) @start() connected: -> @@ -22,11 +19,12 @@ class ActionCable.ConnectionMonitor disconnected: -> @disconnectedAt = now() - received: -> + ping: -> @pingedAt = now() reset: -> @reconnectAttempts = 0 + @consumer.connection.isOpen() start: -> @reset() diff --git a/actioncable/app/assets/javascripts/action_cable/subscriptions.coffee b/actioncable/app/assets/javascripts/action_cable/subscriptions.coffee index ae041ffa2b..2443bca14a 100644 --- a/actioncable/app/assets/javascripts/action_cable/subscriptions.coffee +++ b/actioncable/app/assets/javascripts/action_cable/subscriptions.coffee @@ -58,7 +58,5 @@ class ActionCable.Subscriptions sendCommand: (subscription, command) -> {identifier} = subscription - if identifier is ActionCable.INTERNAL.identifiers.ping - @consumer.connection.isOpen() - else - @consumer.send({command, identifier}) + @consumer.send({command, identifier}) + diff --git a/actioncable/lib/action_cable.rb b/actioncable/lib/action_cable.rb index 1dc66ef3ad..a8e4d1cb25 100644 --- a/actioncable/lib/action_cable.rb +++ b/actioncable/lib/action_cable.rb @@ -29,10 +29,9 @@ module ActionCable extend ActiveSupport::Autoload INTERNAL = { - identifiers: { - ping: '_ping'.freeze - }, message_types: { + welcome: 'welcome'.freeze, + ping: 'ping'.freeze, confirmation: 'confirm_subscription'.freeze, rejection: 'reject_subscription'.freeze } diff --git a/actioncable/lib/action_cable/connection/base.rb b/actioncable/lib/action_cable/connection/base.rb index afe0d958d7..f34f5eb109 100644 --- a/actioncable/lib/action_cable/connection/base.rb +++ b/actioncable/lib/action_cable/connection/base.rb @@ -115,7 +115,7 @@ module ActionCable end def beat - transmit ActiveSupport::JSON.encode(identifier: ActionCable::INTERNAL[:identifiers][:ping], message: Time.now.to_i) + transmit ActiveSupport::JSON.encode(type: ActionCable::INTERNAL[:message_types][:ping], message: Time.now.to_i) end def on_open # :nodoc: @@ -155,7 +155,7 @@ module ActionCable def handle_open connect if respond_to?(:connect) subscribe_to_internal_channel - confirm_connection_monitor_subscription + send_welcome_message message_buffer.process! server.add_connection(self) @@ -174,11 +174,11 @@ module ActionCable disconnect if respond_to?(:disconnect) end - def confirm_connection_monitor_subscription - # Send confirmation message to the internal connection monitor channel. + def send_welcome_message + # Send welcome message to the internal connection monitor channel. # This ensures the connection monitor state is reset after a successful # websocket connection. - transmit ActiveSupport::JSON.encode(identifier: ActionCable::INTERNAL[:identifiers][:ping], type: ActionCable::INTERNAL[:message_types][:confirmation]) + transmit ActiveSupport::JSON.encode(type: ActionCable::INTERNAL[:message_types][:welcome]) end def allow_request_origin? diff --git a/actioncable/lib/action_cable/connection/subscriptions.rb b/actioncable/lib/action_cable/connection/subscriptions.rb index 3742f248d1..5aa907c2d3 100644 --- a/actioncable/lib/action_cable/connection/subscriptions.rb +++ b/actioncable/lib/action_cable/connection/subscriptions.rb @@ -23,13 +23,13 @@ module ActionCable end def add(data) - id_key = data['identifier'] - id_options = ActiveSupport::JSON.decode(id_key).with_indifferent_access + id_options = decode_hash(data['identifier']) + identifier = normalize_identifier(id_options) subscription_klass = connection.server.channel_classes[id_options[:channel]] if subscription_klass - subscriptions[id_key] ||= subscription_klass.new(connection, id_key, id_options) + subscriptions[identifier] ||= subscription_klass.new(connection, identifier, id_options) else logger.error "Subscription class not found (#{data.inspect})" end @@ -37,7 +37,7 @@ module ActionCable def remove(data) logger.info "Unsubscribing from channel: #{data['identifier']}" - remove_subscription subscriptions[data['identifier']] + remove_subscription subscriptions[normalize_identifier(data['identifier'])] end def remove_subscription(subscription) @@ -46,7 +46,7 @@ module ActionCable end def perform_action(data) - find(data).perform_action ActiveSupport::JSON.decode(data['data']) + find(data).perform_action(decode_hash(data['data'])) end def identifiers @@ -63,8 +63,21 @@ module ActionCable private delegate :logger, to: :connection + def normalize_identifier(identifier) + identifier = ActiveSupport::JSON.encode(identifier) if identifier.is_a?(Hash) + identifier + end + + # If `data` is a Hash, this means that the original JSON + # sent by the client had no backslashes in it, and does + # not need to be decoded again. + def decode_hash(data) + data = ActiveSupport::JSON.decode(data) unless data.is_a?(Hash) + data.with_indifferent_access + end + def find(data) - if subscription = subscriptions[data['identifier']] + if subscription = subscriptions[normalize_identifier(data['identifier'])] subscription else raise "Unable to find subscription with identifier: #{data['identifier']}" diff --git a/actioncable/test/client_test.rb b/actioncable/test/client_test.rb index a6619d3bd2..75545993da 100644 --- a/actioncable/test/client_test.rb +++ b/actioncable/test/client_test.rb @@ -75,7 +75,7 @@ class ClientTest < ActionCable::TestCase @ws.on(:message) do |event| hash = JSON.parse(event.data) - if hash['identifier'] == '_ping' + if hash['type'] == 'ping' @pings += 1 else @messages << hash @@ -146,6 +146,7 @@ class ClientTest < ActionCable::TestCase def test_single_client with_puma_server do |port| c = faye_client(port) + assert_equal({"type" => "welcome"}, c.read_message) # pop the first welcome message off the stack c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel') assert_equal({"identifier"=>"{\"channel\":\"EchoChannel\"}", "type"=>"confirm_subscription"}, c.read_message) c.send_message command: 'message', identifier: JSON.dump(channel: 'EchoChannel'), data: JSON.dump(action: 'ding', message: 'hello') @@ -162,6 +163,7 @@ class ClientTest < ActionCable::TestCase barrier_2 = Concurrent::CyclicBarrier.new(clients.size) clients.map {|c| Concurrent::Future.execute { + assert_equal({"type" => "welcome"}, c.read_message) # pop the first welcome message off the stack c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel') assert_equal({"identifier"=>'{"channel":"EchoChannel"}', "type"=>"confirm_subscription"}, c.read_message) c.send_message command: 'message', identifier: JSON.dump(channel: 'EchoChannel'), data: JSON.dump(action: 'ding', message: 'hello') @@ -181,6 +183,7 @@ class ClientTest < ActionCable::TestCase clients = 100.times.map { faye_client(port) } clients.map {|c| Concurrent::Future.execute { + assert_equal({"type" => "welcome"}, c.read_message) # pop the first welcome message off the stack c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel') assert_equal({"identifier"=>'{"channel":"EchoChannel"}', "type"=>"confirm_subscription"}, c.read_message) c.send_message command: 'message', identifier: JSON.dump(channel: 'EchoChannel'), data: JSON.dump(action: 'ding', message: 'hello') @@ -194,12 +197,14 @@ class ClientTest < ActionCable::TestCase def test_disappearing_client with_puma_server do |port| c = faye_client(port) + assert_equal({"type" => "welcome"}, c.read_message) # pop the first welcome message off the stack c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel') assert_equal({"identifier"=>"{\"channel\":\"EchoChannel\"}", "type"=>"confirm_subscription"}, c.read_message) c.send_message command: 'message', identifier: JSON.dump(channel: 'EchoChannel'), data: JSON.dump(action: 'delay', message: 'hello') c.close # disappear before write c = faye_client(port) + assert_equal({"type" => "welcome"}, c.read_message) # pop the first welcome message off the stack c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel') assert_equal({"identifier"=>"{\"channel\":\"EchoChannel\"}", "type"=>"confirm_subscription"}, c.read_message) c.send_message command: 'message', identifier: JSON.dump(channel: 'EchoChannel'), data: JSON.dump(action: 'ding', message: 'hello') @@ -214,6 +219,7 @@ class ClientTest < ActionCable::TestCase identifier = JSON.dump(channel: 'EchoChannel') c = faye_client(port) + assert_equal({"type" => "welcome"}, c.read_message) c.send_message command: 'subscribe', identifier: identifier assert_equal({"identifier"=>"{\"channel\":\"EchoChannel\"}", "type"=>"confirm_subscription"}, c.read_message) assert_equal(1, app.connections.count) @@ -232,6 +238,7 @@ class ClientTest < ActionCable::TestCase def test_server_restart with_puma_server do |port| c = faye_client(port) + assert_equal({"type" => "welcome"}, c.read_message) c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel') assert_equal({"identifier"=>"{\"channel\":\"EchoChannel\"}", "type"=>"confirm_subscription"}, c.read_message) diff --git a/actioncable/test/connection/base_test.rb b/actioncable/test/connection/base_test.rb index fb11f9be64..fb83bd7d77 100644 --- a/actioncable/test/connection/base_test.rb +++ b/actioncable/test/connection/base_test.rb @@ -56,7 +56,7 @@ class ActionCable::Connection::BaseTest < ActionCable::TestCase run_in_eventmachine do connection = open_connection - connection.websocket.expects(:transmit).with({ identifier: "_ping", type: "confirm_subscription" }.to_json) + connection.websocket.expects(:transmit).with({ type: "welcome" }.to_json) connection.message_buffer.expects(:process!) connection.process diff --git a/actioncable/test/connection/subscriptions_test.rb b/actioncable/test/connection/subscriptions_test.rb index 62e41484fe..68a79c6e4c 100644 --- a/actioncable/test/connection/subscriptions_test.rb +++ b/actioncable/test/connection/subscriptions_test.rb @@ -82,13 +82,13 @@ class ActionCable::Connection::SubscriptionsTest < ActionCable::TestCase end end - test "unsubscrib from all" do + test "unsubscribe from all" do run_in_eventmachine do setup_connection channel1 = subscribe_to_chat_channel - channel2_id = ActiveSupport::JSON.encode(id: 2, channel: 'ActionCable::Connection::SubscriptionsTest::ChatChannel') + channel2_id = ActiveSupport::JSON.encode({ id: 2, channel: 'ActionCable::Connection::SubscriptionsTest::ChatChannel' }) channel2 = subscribe_to_chat_channel(channel2_id) channel1.expects(:unsubscribe_from_channel) diff --git a/actioncable/test/test_helper.rb b/actioncable/test/test_helper.rb index 8b157c9b46..797e7786d1 100644 --- a/actioncable/test/test_helper.rb +++ b/actioncable/test/test_helper.rb @@ -6,6 +6,7 @@ require 'puma' require 'mocha/setup' require 'rack/mock' +require 'active_support/core_ext/hash/indifferent_access' # Require all the stubs and models Dir[File.dirname(__FILE__) + '/stubs/*.rb'].each {|file| require file } diff --git a/actionpack/lib/action_controller/metal/implicit_render.rb b/actionpack/lib/action_controller/metal/implicit_render.rb index 6b540d42c7..3a6f784507 100644 --- a/actionpack/lib/action_controller/metal/implicit_render.rb +++ b/actionpack/lib/action_controller/metal/implicit_render.rb @@ -42,7 +42,7 @@ module ActionController "action but none of them were suitable for this request.\n\n" \ "This usually happens when the client requested an unsupported format " \ "(e.g. requesting HTML content from a JSON endpoint or vice versa), but " \ - "it might also be failing due to other constraints, such as locales or" \ + "it might also be failing due to other constraints, such as locales or " \ "variants.\n" if request.formats.any? diff --git a/actionpack/test/dispatch/routing/inspector_test.rb b/actionpack/test/dispatch/routing/inspector_test.rb index d8cffa425f..9d0d23d6de 100644 --- a/actionpack/test/dispatch/routing/inspector_test.rb +++ b/actionpack/test/dispatch/routing/inspector_test.rb @@ -170,7 +170,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_route_with_defaults + def test_rails_routes_shows_route_with_defaults output = draw do get 'photos/:id' => 'photos#show', :defaults => {:format => 'jpg'} end @@ -181,7 +181,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_route_with_constraints + def test_rails_routes_shows_route_with_constraints output = draw do get 'photos/:id' => 'photos#show', :id => /[A-Z]\d{5}/ end @@ -192,7 +192,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_routes_with_dashes + def test_rails_routes_shows_routes_with_dashes output = draw do get 'about-us' => 'pages#about_us' get 'our-work/latest' @@ -215,7 +215,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_route_with_rack_app + def test_rails_routes_shows_route_with_rack_app output = draw do get 'foo/:id' => MountedRackApp, :id => /[A-Z]\d{5}/ end @@ -226,7 +226,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_named_route_with_mounted_rack_app + def test_rails_routes_shows_named_route_with_mounted_rack_app output = draw do mount MountedRackApp => '/foo' end @@ -237,7 +237,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_overridden_named_route_with_mounted_rack_app_with_name + def test_rails_routes_shows_overridden_named_route_with_mounted_rack_app_with_name output = draw do mount MountedRackApp => '/foo', as: 'blog' end @@ -248,7 +248,7 @@ module ActionDispatch ], output end - def test_rake_routes_shows_route_with_rack_app_nested_with_dynamic_constraints + def test_rails_routes_shows_route_with_rack_app_nested_with_dynamic_constraints constraint = Class.new do def inspect "( my custom constraint )" @@ -267,7 +267,7 @@ module ActionDispatch ], output end - def test_rake_routes_dont_show_app_mounted_in_assets_prefix + def test_rails_routes_dont_show_app_mounted_in_assets_prefix output = draw do get '/sprockets' => MountedRackApp end @@ -275,7 +275,7 @@ module ActionDispatch assert_no_match(/\/sprockets/, output.first) end - def test_rake_routes_shows_route_defined_in_under_assets_prefix + def test_rails_routes_shows_route_defined_in_under_assets_prefix output = draw do scope '/sprockets' do get '/foo' => 'foo#bar' diff --git a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb index ed8cfae657..df0f3dc77f 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb @@ -134,10 +134,6 @@ module ActiveRecord ActiveRecord::Result.new(result.fields, result.to_a) if result end - def exec_insert(sql, name, binds, pk = nil, sequence_name = nil) - execute to_sql(sql, binds), name - end - def exec_delete(sql, name, binds) execute to_sql(sql, binds), name @connection.affected_rows |