From 9168a19a2d627f33a39a10b5b3fb25d0da7e743f Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 11 Sep 2019 15:29:10 +0800
Subject: [PATCH 1/7] (GH-177) Track dynamic registrations

Previously capability registrations were assumed to only have one possible
reqeuest per method however this isn't true.  Also the request ID is needed in
order to unregister a capability.  This commit;
* Modifies the Language Client to track registrations that are in progress and
  have completed
* Now processes error messages so that registration tracking can fail tracked
  registrations
* Adds tests for these scenarios
---
 lib/puppet-languageserver/language_client.rb  |  48 +++++-
 lib/puppet-languageserver/message_router.rb   |  12 +-
 .../language_client_spec.rb                   | 153 +++++++++++++++++-
 3 files changed, 204 insertions(+), 9 deletions(-)

diff --git a/lib/puppet-languageserver/language_client.rb b/lib/puppet-languageserver/language_client.rb
index 57c87efb..53e5bb4b 100644
--- a/lib/puppet-languageserver/language_client.rb
+++ b/lib/puppet-languageserver/language_client.rb
@@ -4,6 +4,16 @@ module PuppetLanguageServer
   class LanguageClient
     def initialize
       @client_capabilites = {}
+
+      # Internal registry of dynamic registrations and their current state
+      # @registrations[ <[String] method_name>] = [
+      #  {
+      #    :id         => [String] Request ID. Used for de-registration
+      #    :registered => [Boolean] true | false
+      #    :state      => [Enum] :pending | :complete
+      #   }
+      # ]
+      @registrations = {}
     end
 
     def client_capability(*names)
@@ -30,22 +40,52 @@ def parse_lsp_configuration_settings!(settings = [{}])
       # end
     end
 
+    def capability_registrations(method)
+      return [{ :registered => false, :state => :complete }] if @registrations[method].nil?
+      @registrations[method].dup
+    end
+
     def register_capability(message_router, method, options = {})
-      id = SecureRandom.uuid
+      id = new_request_id
 
       PuppetLanguageServer.log_message(:info, "Attempting to dynamically register the #{method} method with id #{id}")
 
+      if @registrations[method] && @registrations[method].select { |i| i[:state] == :pending }.count > 0
+        # The protocol doesn't specify whether this is allowed and is probably per client specific. For the moment we will allow
+        # the registration to be sent but log a message that something may be wrong.
+        PuppetLanguageServer.log_message(:warn, "A dynamic registration for the #{method} method is already in progress")
+      end
+
       params = LSP::RegistrationParams.new.from_h!('registrations' => [])
       params.registrations << LSP::Registration.new.from_h!('id' => id, 'method' => method, 'registerOptions' => options)
+      # Note - Don't put more than one method per request even though you can.  It makes decoding errors much harder!
+
+      @registrations[method] = [] if @registrations[method].nil?
+      @registrations[method] << { :registered => false, :state => :pending, :id => id }
 
       message_router.json_rpc_handler.send_client_request('client/registerCapability', params)
       true
     end
 
-    def parse_register_capability_response!(message_router, _response, original_request)
+    def parse_register_capability_response!(message_router, response, original_request)
       raise 'Response is not from client/registerCapability request' unless original_request['method'] == 'client/registerCapability'
+
+      unless response.key?('result')
+        original_request['params'].registrations.each do |reg|
+          # Mark the registration as completed and failed
+          @registrations[reg.method__lsp] = [] if @registrations[reg.method__lsp].nil?
+          @registrations[reg.method__lsp].select { |i| i[:id] == reg.id }.each { |i| i[:registered] = false; i[:state] = :complete } # rubocop:disable Style/Semicolon This is fine
+        end
+        return true
+      end
+
       original_request['params'].registrations.each do |reg|
         PuppetLanguageServer.log_message(:info, "Succesfully dynamically registered the #{reg.method__lsp} method")
+
+        # Mark the registration as completed and succesful
+        @registrations[reg.method__lsp] = [] if @registrations[reg.method__lsp].nil?
+        @registrations[reg.method__lsp].select { |i| i[:id] == reg.id }.each { |i| i[:registered] = true; i[:state] = :complete } # rubocop:disable Style/Semicolon This is fine
+
         # If we just registered the workspace/didChangeConfiguration method then
         # also trigger a configuration request to get the initial state
         send_configuration_request(message_router) if reg.method__lsp == 'workspace/didChangeConfiguration'
@@ -56,6 +96,10 @@ def parse_register_capability_response!(message_router, _response, original_requ
 
     private
 
+    def new_request_id
+      SecureRandom.uuid
+    end
+
     def safe_hash_traverse(hash, *names)
       return nil if names.empty?
       item = nil
diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index d234c556..ce74a0d6 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -314,15 +314,15 @@ def receive_notification(method, params)
     end
 
     def receive_response(response, original_request)
-      unless response.key?('result')
+      unless receive_response_succesful?(response) # rubocop:disable Style/IfUnlessModifier Too long
         PuppetLanguageServer.log_message(:error, "Response for method '#{original_request['method']}' with id '#{original_request['id']}' failed with #{response['error']}")
-        return
       end
-
+      # Error responses still need to be processed so process messages even if it failed
       case original_request['method']
       when 'client/registerCapability'
         client.parse_register_capability_response!(self, response, original_request)
       when 'workspace/configuration'
+        return unless receive_response_succesful?(response)
         client.parse_lsp_configuration_settings!(response['result'])
       else
         super
@@ -331,6 +331,12 @@ def receive_response(response, original_request)
       PuppetLanguageServer::CrashDump.write_crash_file(e, nil, 'response' => response, 'original_request' => original_request)
       raise
     end
+
+    private
+
+    def receive_response_succesful?(response)
+      response.key?('result')
+    end
   end
 
   class DisabledMessageRouter < BaseMessageRouter
diff --git a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
index 6ab62fa1..9d7bd016 100644
--- a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
@@ -142,6 +142,10 @@
   let(:json_rpc_handler) { MockJSONRPCHandler.new }
   let(:message_router) { MockMessageRouter.new.tap { |i| i.json_rpc_handler = json_rpc_handler } }
 
+  before(:each) do
+    allow(PuppetLanguageServer).to receive(:log_message)
+  end
+
   describe '#client_capability' do
     before(:each) do
       subject.parse_lsp_initialize!(initialize_params)
@@ -180,6 +184,106 @@
     # TODO: Future use.
   end
 
+  describe '#capability_registrations' do
+    let(:method_name) { 'mockMethod' }
+    let(:method_options) { {} }
+    let(:request_id) { 'id001' }
+
+    it 'defaults to false' do
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
+    end
+
+    it 'should track the registration process as it completes succesfully' do
+      req_method_name = nil
+      req_method_params = nil
+      # Remember the registration so we can fake a response later
+      allow(json_rpc_handler).to receive(:send_client_request) do |n, p|
+        req_method_name = n
+        req_method_params = p
+      end
+      # Fake the request id
+      allow(subject).to receive(:new_request_id).and_return(request_id)
+
+      # Should start out not registered
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
+      # Send as registration request
+      subject.register_capability(message_router, method_name, method_options)
+      # Should show as in progress
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :pending, :id => request_id}])
+      # Mock a valid response
+      response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
+      original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
+      subject.parse_register_capability_response!(message_router, response, original_request)
+      # Should show registered
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => true, :state => :complete, :id => request_id}])
+    end
+
+    it 'should track the registration process as it fails' do
+      req_method_name = nil
+      req_method_params = nil
+      # Remember the registration so we can fake a response later
+      allow(json_rpc_handler).to receive(:send_client_request) do |n, p|
+        req_method_name = n
+        req_method_params = p
+      end
+      # Fake the request id
+      allow(subject).to receive(:new_request_id).and_return(request_id)
+
+      # Should start out not registered
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
+      # Send as registration request
+      subject.register_capability(message_router, method_name, method_options)
+      # Should show as in progress
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :pending, :id => request_id}])
+      # Mock an error response
+      response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'error' => { 'code' => -1, 'message' => 'mock message' } }
+      original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
+      subject.parse_register_capability_response!(message_router, response, original_request)
+      # Should show registered
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete, :id => request_id}])
+    end
+
+    it 'should preserve the registration state until it is completed' do
+      req_method_name = nil
+      req_method_params = nil
+      # Remember the registration so we can fake a response later
+      allow(json_rpc_handler).to receive(:send_client_request) do |n, p|
+        req_method_name = n
+        req_method_params = p
+      end
+      # Fake the request id
+      request_id2 = 'id002'
+      allow(subject).to receive(:new_request_id).and_return(request_id, request_id2)
+
+      # Should start out not registered
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
+      # Send as registration request
+      subject.register_capability(message_router, method_name, method_options)
+      # Mock a valid response
+      response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
+      original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
+      subject.parse_register_capability_response!(message_router, response, original_request)
+      # Should show registered
+      expect(subject.capability_registrations(method_name)).to eq([{:registered => true, :state => :complete, :id => request_id}])
+      # Send another registration request
+      subject.register_capability(message_router, method_name, method_options)
+      # Should show as in progress
+      expect(subject.capability_registrations(method_name)).to eq([
+        {:registered => true,  :state => :complete, :id => request_id},
+        {:registered => false, :state => :pending,  :id => request_id2}
+      ])
+      # Mock an error response
+      response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'error' => { 'code' => -1, 'message' => 'mock message' } }
+      original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
+      subject.parse_register_capability_response!(message_router, response, original_request)
+      # Should still show registered
+      expect(subject.capability_registrations(method_name)).to eq([
+        {:registered => true,  :state => :complete, :id => request_id},
+        {:registered => false, :state => :complete,  :id => request_id2}
+      ])
+    end
+  end
+
   describe '#register_capability' do
     let(:method_name) { 'mockMethod' }
     let(:method_options) { {} }
@@ -198,6 +302,33 @@
       subject.register_capability(message_router, method_name, method_options)
       expect(json_rpc_handler.connection.buffer).to include('"registerOptions":{}')
     end
+
+    it 'should log a message if a registration is already in progress' do
+      allow(json_rpc_handler).to receive(:send_client_request)
+      expect(PuppetLanguageServer).to receive(:log_message).with(:warn, /#{method_name}/)
+
+      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(message_router, method_name, method_options)
+    end
+
+    it 'should not log a message if a previous registration completed' do
+      method_name = nil
+      method_params = nil
+      # Remember the registration so we can fake a response later
+      allow(json_rpc_handler).to receive(:send_client_request) do |n, p|
+        method_name = n
+        method_params = p
+      end
+      expect(PuppetLanguageServer).to_not receive(:log_message).with(:warn, /#{method_name}/)
+      # Send as registration request
+      subject.register_capability(message_router, method_name, method_options)
+      # Mock a valid response
+      response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
+      original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => method_name, 'params' => method_params }
+      subject.parse_register_capability_response!(message_router, response, original_request)
+
+      subject.register_capability(message_router, method_name, method_options)
+    end
   end
 
   describe '#parse_register_capability_response!' do
@@ -237,11 +368,25 @@
         params
       end
 
-      it 'should log the registration' do
-        allow(PuppetLanguageServer).to receive(:log_message)
-        expect(PuppetLanguageServer).to receive(:log_message).with(:info, /validMethod/)
+      context 'that failed' do
+        before(:each) do
+          response.delete('result') if response.key?('result')
+          response['error'] = { 'code' => -1, 'message' => 'mock message' }
+        end
 
-        subject.parse_register_capability_response!(message_router, response, original_request)
+        it 'should not log the registration' do
+          expect(PuppetLanguageServer).to_not receive(:log_message).with(:info, /validMethod/)
+
+          subject.parse_register_capability_response!(message_router, response, original_request)
+        end
+      end
+
+      context 'that succeeded' do
+        it 'should log the registration' do
+          expect(PuppetLanguageServer).to receive(:log_message).with(:info, /validMethod/)
+
+          subject.parse_register_capability_response!(message_router, response, original_request)
+        end
       end
     end
   end

From 9362435993e277fd498c0c966dbd5f678e1b7a33 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 16 Sep 2019 20:36:13 +0800
Subject: [PATCH 2/7] (GH-177) Refactor message_router as property of Language
 Client

Previously the message router was passed in as a method parameter however as
almost all methods require the message router, this commit changes the
initializer to pass in the message router at object create time.
---
 lib/puppet-languageserver/language_client.rb  | 13 +++--
 lib/puppet-languageserver/message_router.rb   | 10 ++--
 .../language_client_spec.rb                   | 52 +++++++++----------
 .../message_router_spec.rb                    |  6 +--
 4 files changed, 42 insertions(+), 39 deletions(-)

diff --git a/lib/puppet-languageserver/language_client.rb b/lib/puppet-languageserver/language_client.rb
index 53e5bb4b..84cfa95e 100644
--- a/lib/puppet-languageserver/language_client.rb
+++ b/lib/puppet-languageserver/language_client.rb
@@ -2,7 +2,10 @@
 
 module PuppetLanguageServer
   class LanguageClient
-    def initialize
+    attr_reader :message_router
+
+    def initialize(message_router)
+      @message_router = message_router
       @client_capabilites = {}
 
       # Internal registry of dynamic registrations and their current state
@@ -20,7 +23,7 @@ def client_capability(*names)
       safe_hash_traverse(@client_capabilites, *names)
     end
 
-    def send_configuration_request(message_router)
+    def send_configuration_request
       params = LSP::ConfigurationParams.new.from_h!('items' => [])
       params.items << LSP::ConfigurationItem.new.from_h!('section' => 'puppet')
 
@@ -45,7 +48,7 @@ def capability_registrations(method)
       @registrations[method].dup
     end
 
-    def register_capability(message_router, method, options = {})
+    def register_capability(method, options = {})
       id = new_request_id
 
       PuppetLanguageServer.log_message(:info, "Attempting to dynamically register the #{method} method with id #{id}")
@@ -67,7 +70,7 @@ def register_capability(message_router, method, options = {})
       true
     end
 
-    def parse_register_capability_response!(message_router, response, original_request)
+    def parse_register_capability_response!(response, original_request)
       raise 'Response is not from client/registerCapability request' unless original_request['method'] == 'client/registerCapability'
 
       unless response.key?('result')
@@ -88,7 +91,7 @@ def parse_register_capability_response!(message_router, response, original_reque
 
         # If we just registered the workspace/didChangeConfiguration method then
         # also trigger a configuration request to get the initial state
-        send_configuration_request(message_router) if reg.method__lsp == 'workspace/didChangeConfiguration'
+        send_configuration_request if reg.method__lsp == 'workspace/didChangeConfiguration'
       end
 
       true
diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index ce74a0d6..2f7fff40 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -34,7 +34,7 @@ class MessageRouter < BaseMessageRouter
     def initialize(options = {})
       super
       @server_options = options.nil? ? {} : options
-      @client = LanguageClient.new
+      @client = LanguageClient.new(self)
     end
 
     def documents
@@ -254,7 +254,7 @@ def receive_notification(method, params)
       when 'initialized'
         PuppetLanguageServer.log_message(:info, 'Client has received initialization')
         if client.client_capability('workspace', 'didChangeConfiguration', 'dynamicRegistration') == true
-          client.register_capability(self, 'workspace/didChangeConfiguration')
+          client.register_capability('workspace/didChangeConfiguration')
         else
           PuppetLanguageServer.log_message(:debug, 'Client does not support didChangeConfiguration dynamic registration. Using push method for configuration change detection.')
         end
@@ -300,7 +300,7 @@ def receive_notification(method, params)
         if params.key?('settings') && params['settings'].nil?
           # This is a notification from a dynamic registration.  Need to send a workspace/configuration
           # request to get the actual configuration
-          client.send_configuration_request(self)
+          client.send_configuration_request
         else
           client.parse_lsp_configuration_settings!(params['settings'])
         end
@@ -314,13 +314,13 @@ def receive_notification(method, params)
     end
 
     def receive_response(response, original_request)
-      unless receive_response_succesful?(response) # rubocop:disable Style/IfUnlessModifier Too long
+      unless receive_response_succesful?(response) # rubocop:disable Style/IfUnlessModifier Line is too long otherwise
         PuppetLanguageServer.log_message(:error, "Response for method '#{original_request['method']}' with id '#{original_request['id']}' failed with #{response['error']}")
       end
       # Error responses still need to be processed so process messages even if it failed
       case original_request['method']
       when 'client/registerCapability'
-        client.parse_register_capability_response!(self, response, original_request)
+        client.parse_register_capability_response!(response, original_request)
       when 'workspace/configuration'
         return unless receive_response_succesful?(response)
         client.parse_lsp_configuration_settings!(response['result'])
diff --git a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
index 9d7bd016..51c8affb 100644
--- a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
@@ -1,8 +1,10 @@
 require 'spec_helper'
 
 describe 'PuppetLanguageServer::LanguageClient' do
+  let(:json_rpc_handler) { MockJSONRPCHandler.new }
+  let(:message_router) { MockMessageRouter.new.tap { |i| i.json_rpc_handler = json_rpc_handler } }
   let(:subject_options) {}
-  let(:subject) { PuppetLanguageServer::LanguageClient.new }
+  let(:subject) { PuppetLanguageServer::LanguageClient.new(message_router) }
   let(:initialize_params) do
     # Example capabilities from VS Code
     {
@@ -139,8 +141,6 @@
       }
     }
   end
-  let(:json_rpc_handler) { MockJSONRPCHandler.new }
-  let(:message_router) { MockMessageRouter.new.tap { |i| i.json_rpc_handler = json_rpc_handler } }
 
   before(:each) do
     allow(PuppetLanguageServer).to receive(:log_message)
@@ -167,11 +167,11 @@
   describe '#send_configuration_request' do
     it 'should send a client request and return true' do
       expect(json_rpc_handler).to receive(:send_client_request).with('workspace/configuration', Object)
-      expect(subject.send_configuration_request(message_router)).to eq(true)
+      expect(subject.send_configuration_request).to eq(true)
     end
 
     it 'should include the puppet settings' do
-      subject.send_configuration_request(message_router)
+      subject.send_configuration_request
       expect(json_rpc_handler.connection.buffer).to include('{"section":"puppet"}')
     end
   end
@@ -207,13 +207,13 @@
       # Should start out not registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Should show as in progress
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :pending, :id => request_id}])
       # Mock a valid response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should show registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => true, :state => :complete, :id => request_id}])
     end
@@ -232,13 +232,13 @@
       # Should start out not registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Should show as in progress
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :pending, :id => request_id}])
       # Mock an error response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'error' => { 'code' => -1, 'message' => 'mock message' } }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should show registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete, :id => request_id}])
     end
@@ -258,15 +258,15 @@
       # Should start out not registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Mock a valid response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should show registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => true, :state => :complete, :id => request_id}])
       # Send another registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Should show as in progress
       expect(subject.capability_registrations(method_name)).to eq([
         {:registered => true,  :state => :complete, :id => request_id},
@@ -275,7 +275,7 @@
       # Mock an error response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'error' => { 'code' => -1, 'message' => 'mock message' } }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should still show registered
       expect(subject.capability_registrations(method_name)).to eq([
         {:registered => true,  :state => :complete, :id => request_id},
@@ -290,16 +290,16 @@
 
     it 'should send a client request and return true' do
       expect(json_rpc_handler).to receive(:send_client_request).with('client/registerCapability', Object)
-      expect(subject.register_capability(message_router, method_name, method_options)).to eq(true)
+      expect(subject.register_capability(method_name, method_options)).to eq(true)
     end
 
     it 'should include the method to register' do
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       expect(json_rpc_handler.connection.buffer).to include("\"method\":\"#{method_name}\"")
     end
 
     it 'should include the parameters to register' do
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       expect(json_rpc_handler.connection.buffer).to include('"registerOptions":{}')
     end
 
@@ -307,8 +307,8 @@
       allow(json_rpc_handler).to receive(:send_client_request)
       expect(PuppetLanguageServer).to receive(:log_message).with(:warn, /#{method_name}/)
 
-      subject.register_capability(message_router, method_name, method_options)
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
+      subject.register_capability(method_name, method_options)
     end
 
     it 'should not log a message if a previous registration completed' do
@@ -321,13 +321,13 @@
       end
       expect(PuppetLanguageServer).to_not receive(:log_message).with(:warn, /#{method_name}/)
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Mock a valid response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => method_name, 'params' => method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
 
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
     end
   end
 
@@ -342,7 +342,7 @@
       let(:request_params) { {} }
 
       it 'should raise an error if the original request was not a registration' do
-        expect{ subject.parse_register_capability_response!(message_router, response, original_request) }.to raise_error(/client\/registerCapability/)
+        expect{ subject.parse_register_capability_response!(response, original_request) }.to raise_error(/client\/registerCapability/)
       end
     end
 
@@ -355,8 +355,8 @@
       end
 
       it 'should send a configuration request' do
-        expect(subject).to receive(:send_configuration_request).with(message_router)
-        subject.parse_register_capability_response!(message_router, response, original_request)
+        expect(subject).to receive(:send_configuration_request)
+        subject.parse_register_capability_response!(response, original_request)
       end
     end
 
@@ -377,7 +377,7 @@
         it 'should not log the registration' do
           expect(PuppetLanguageServer).to_not receive(:log_message).with(:info, /validMethod/)
 
-          subject.parse_register_capability_response!(message_router, response, original_request)
+          subject.parse_register_capability_response!(response, original_request)
         end
       end
 
@@ -385,7 +385,7 @@
         it 'should log the registration' do
           expect(PuppetLanguageServer).to receive(:log_message).with(:info, /validMethod/)
 
-          subject.parse_register_capability_response!(message_router, response, original_request)
+          subject.parse_register_capability_response!(response, original_request)
         end
       end
     end
diff --git a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
index 34bbf79d..e6aba19f 100644
--- a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
@@ -858,7 +858,7 @@
         end
 
         it 'should attempt to register workspace/didChangeConfiguration' do
-          expect(subject.client).to receive(:register_capability).with(Object, 'workspace/didChangeConfiguration')
+          expect(subject.client).to receive(:register_capability).with('workspace/didChangeConfiguration')
 
           subject.receive_notification(notification_method, notification_params)
         end
@@ -1037,7 +1037,7 @@
         let(:config_settings) { nil }
 
         it 'should send a configuration request' do
-          expect(subject.client).to receive(:send_configuration_request).with(Object)
+          expect(subject.client).to receive(:send_configuration_request).with(no_args)
 
           subject.receive_notification(notification_method, notification_params)
         end
@@ -1116,7 +1116,7 @@
       let(:request_params) { {} }
 
       it 'should call client.parse_register_capability_response!' do
-        expect(subject.client).to receive(:parse_register_capability_response!).with(Object, response, original_request)
+        expect(subject.client).to receive(:parse_register_capability_response!).with(response, original_request)
 
         subject.receive_response(response, original_request)
       end

From b0e3a6730588fd32b3e36288d312543daa668b68 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 11 Sep 2019 17:16:46 +0800
Subject: [PATCH 3/7] (GH-177) Add unregistering of capabilities

Previously client capabilities could be dynamically registered.  This commit
adds the logic and track to dynamically unregister a capability;
* Adds the unregister_capability and parse_unregister_capability_response!
  methods
* Adds the client/unregisterCapability notification handler
* Adds tests for unregistering
---
 lib/puppet-languageserver/language_client.rb  |  53 ++++++-
 lib/puppet-languageserver/message_router.rb   |   2 +
 .../language_client_spec.rb                   | 146 ++++++++++++++++++
 3 files changed, 199 insertions(+), 2 deletions(-)

diff --git a/lib/puppet-languageserver/language_client.rb b/lib/puppet-languageserver/language_client.rb
index 84cfa95e..22351292 100644
--- a/lib/puppet-languageserver/language_client.rb
+++ b/lib/puppet-languageserver/language_client.rb
@@ -44,7 +44,7 @@ def parse_lsp_configuration_settings!(settings = [{}])
     end
 
     def capability_registrations(method)
-      return [{ :registered => false, :state => :complete }] if @registrations[method].nil?
+      return [{ :registered => false, :state => :complete }] if @registrations[method].nil? || @registrations[method].empty?
       @registrations[method].dup
     end
 
@@ -56,7 +56,7 @@ def register_capability(method, options = {})
       if @registrations[method] && @registrations[method].select { |i| i[:state] == :pending }.count > 0
         # The protocol doesn't specify whether this is allowed and is probably per client specific. For the moment we will allow
         # the registration to be sent but log a message that something may be wrong.
-        PuppetLanguageServer.log_message(:warn, "A dynamic registration for the #{method} method is already in progress")
+        PuppetLanguageServer.log_message(:warn, "A dynamic registration/deregistration for the #{method} method is already in progress")
       end
 
       params = LSP::RegistrationParams.new.from_h!('registrations' => [])
@@ -70,6 +70,31 @@ def register_capability(method, options = {})
       true
     end
 
+    def unregister_capability(method)
+      if @registrations[method].nil?
+        PuppetLanguageServer.log_message(:debug, "No registrations to deregister for the #{method}")
+        return true
+      end
+
+      params = LSP::UnregistrationParams.new.from_h!('unregisterations' => [])
+      @registrations[method].each do |reg|
+        next if reg[:id].nil?
+        PuppetLanguageServer.log_message(:warn, "A dynamic registration/deregistration for the #{method} method, with id #{reg[:id]} is already in progress") if reg[:state] == :pending
+        # Ignore registrations that don't need to be unregistered
+        next if reg[:state] == :complete && !reg[:registered]
+        params.unregisterations << LSP::Unregistration.new.from_h!('id' => reg[:id], 'method' => method)
+        reg[:state] = :pending
+      end
+
+      if params.unregisterations.count.zero?
+        PuppetLanguageServer.log_message(:debug, "Nothing to deregister for the #{method} method")
+        return true
+      end
+
+      message_router.json_rpc_handler.send_client_request('client/unregisterCapability', params)
+      true
+    end
+
     def parse_register_capability_response!(response, original_request)
       raise 'Response is not from client/registerCapability request' unless original_request['method'] == 'client/registerCapability'
 
@@ -97,6 +122,30 @@ def parse_register_capability_response!(response, original_request)
       true
     end
 
+    def parse_unregister_capability_response!(response, original_request)
+      raise 'Response is not from client/unregisterCapability request' unless original_request['method'] == 'client/unregisterCapability'
+
+      unless response.key?('result')
+        original_request['params'].unregisterations.each do |reg|
+          # Mark the registration as completed and failed
+          @registrations[reg.method__lsp] = [] if @registrations[reg.method__lsp].nil?
+          @registrations[reg.method__lsp].select { |i| i[:id] == reg.id && i[:registered] }.each { |i| i[:state] = :complete }
+          @registrations[reg.method__lsp].delete_if { |i| i[:id] == reg.id && !i[:registered] }
+        end
+        return true
+      end
+
+      original_request['params'].unregisterations.each do |reg|
+        PuppetLanguageServer.log_message(:info, "Succesfully dynamically unregistered the #{reg.method__lsp} method")
+
+        # Remove registrations
+        @registrations[reg.method__lsp] = [] if @registrations[reg.method__lsp].nil?
+        @registrations[reg.method__lsp].delete_if { |i| i[:id] == reg.id }
+      end
+
+      true
+    end
+
     private
 
     def new_request_id
diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index 2f7fff40..f6cd6b9c 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -321,6 +321,8 @@ def receive_response(response, original_request)
       case original_request['method']
       when 'client/registerCapability'
         client.parse_register_capability_response!(response, original_request)
+      when 'client/unregisterCapability'
+        client.parse_unregister_capability_response!(response, original_request)
       when 'workspace/configuration'
         return unless receive_response_succesful?(response)
         client.parse_lsp_configuration_settings!(response['result'])
diff --git a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
index 51c8affb..22a48eca 100644
--- a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
@@ -331,6 +331,72 @@
     end
   end
 
+  describe '#unregister_capability' do
+    let(:method_name) { 'mockMethod' }
+
+    before(:each) do
+      # Mock an already succesful registration
+      subject.instance_variable_set(:@registrations, {
+        method_name => [{ :id => 'id001', :state => :complete, :registered => true }]
+      })
+    end
+
+    it 'should send a client request and return true' do
+      expect(json_rpc_handler).to receive(:send_client_request).with('client/unregisterCapability', Object)
+      expect(subject.unregister_capability(method_name)).to eq(true)
+    end
+
+    it 'should include the method to register' do
+      subject.unregister_capability(method_name)
+      expect(json_rpc_handler.connection.buffer).to include("\"method\":\"#{method_name}\"")
+    end
+
+    it 'should log a message if a registration is already in progress' do
+      allow(json_rpc_handler).to receive(:send_client_request)
+      expect(PuppetLanguageServer).to receive(:log_message).with(:warn, /#{method_name}/)
+
+      subject.unregister_capability(method_name)
+      subject.unregister_capability(method_name)
+    end
+
+    it 'should not log a message if a previous registration completed' do
+      req_method_name = nil
+      req_method_params = nil
+      # Remember the registration so we can fake a response later
+      allow(json_rpc_handler).to receive(:send_client_request) do |n, p|
+        req_method_name = n
+        req_method_params = p
+      end
+
+      expect(PuppetLanguageServer).to_not receive(:log_message).with(:warn, /#{method_name}/)
+      # Send as registration request
+      subject.unregister_capability(method_name)
+      # Mock a valid response
+      response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
+      original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
+
+      subject.parse_unregister_capability_response!(response, original_request)
+
+      subject.unregister_capability(method_name)
+    end
+
+    it 'should not deregister methods that have not been registerd' do
+      expect(json_rpc_handler).to_not receive(:send_client_request)
+
+      subject.unregister_capability('unknown')
+    end
+
+    it 'should not deregister methods that are no longer registerd' do
+      expect(json_rpc_handler).to_not receive(:send_client_request)
+
+      subject.instance_variable_set(:@registrations, {
+        method_name => [{ :id => 'id001', :state => :complete, :registered => false }]
+      })
+
+      subject.unregister_capability(method_name)
+    end
+  end
+
   describe '#parse_register_capability_response!' do
     let(:request_id) { 0 }
     let(:response_result) { nil }
@@ -390,4 +456,84 @@
       end
     end
   end
+
+  describe '#parse_unregister_capability_response!' do
+    let(:request_id) { 0 }
+    let(:response_result) { nil }
+    let(:response) { {'jsonrpc'=>'2.0', 'id'=> request_id, 'result' => response_result } }
+    let(:original_request) { {'jsonrpc'=>'2.0', 'id'=> request_id, 'method' => request_method, 'params' => request_params} }
+    let(:method_name) { 'validMethod' }
+    let(:initial_registration) { true }
+
+    before(:each) do
+      # Mock an already succesful registration
+      subject.instance_variable_set(:@registrations, {
+        method_name => [{ :id => 'id001', :state => :complete, :registered => initial_registration }]
+      })
+    end
+
+    context 'Given an original request that is not an unregistration' do
+      let(:request_method) { 'mockMethod' }
+      let(:request_params) { {} }
+
+      it 'should raise an error if the original request was not a registration' do
+        expect{ subject.parse_unregister_capability_response!(response, original_request) }.to raise_error(/client\/unregisterCapability/)
+      end
+    end
+
+    context 'Given a valid original request' do
+      let(:request_method) { 'client/unregisterCapability' }
+      let(:request_params) do
+        params = LSP::UnregistrationParams.new.from_h!('unregisterations' => [])
+        params.unregisterations << LSP::Unregistration.new.from_h!('id' => 'id001', 'method' => method_name)
+        params
+      end
+
+      before(:each) do
+        # Mimic an unregistration that is in progress
+        subject.instance_variable_set(:@registrations, {
+          method_name => [{ :id => 'id001', :state => :pending, :registered => initial_registration }]
+        })
+      end
+
+      context 'that failed' do
+        before(:each) do
+          response.delete('result') if response.key?('result')
+          response['error'] = { 'code' => -1, 'message' => 'mock message' }
+        end
+
+        context 'and was previously registered' do
+          it 'should retain that it is registered' do
+            subject.parse_unregister_capability_response!(response, original_request)
+
+            expect(subject.capability_registrations(method_name)).to eq([{:id=>"id001", :registered=>true, :state=>:complete}])
+          end
+        end
+
+        context 'and was not previously registered' do
+          let(:initial_registration) { false }
+
+          it 'should no longer be in the registration list' do
+            subject.parse_unregister_capability_response!(response, original_request)
+
+            expect(subject.capability_registrations(method_name)).to eq([{ :registered => false, :state => :complete }])
+          end
+        end
+      end
+
+      context 'that succeeded' do
+        it 'should log the registration' do
+          expect(PuppetLanguageServer).to receive(:log_message).with(:info, /validMethod/)
+
+          subject.parse_unregister_capability_response!(response, original_request)
+        end
+
+        it 'should no longer be in the registration list' do
+          subject.parse_unregister_capability_response!(response, original_request)
+
+          expect(subject.capability_registrations(method_name)).to eq([{ :registered => false, :state => :complete }])
+        end
+      end
+    end
+  end
 end

From 00f75487a0b73e09a8687735e117ce04299bcde8 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Tue, 17 Sep 2019 14:43:48 +0800
Subject: [PATCH 4/7] (GH-177) Fix workspace/configuration settings hash

Previously the settings were passed directly from the response into the
client for processing however this does not emulate the legacy settings hash.

This commit adds the original scope onto the settings hash so it appears like
the legacy hash e.g. puppet.editorservices.abc instead of editorserivces.abc
---
 lib/puppet-languageserver/message_router.rb            |  5 ++++-
 .../unit/puppet-languageserver/message_router_spec.rb  | 10 +++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index f6cd6b9c..432c0a5b 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -325,7 +325,10 @@ def receive_response(response, original_request)
         client.parse_unregister_capability_response!(response, original_request)
       when 'workspace/configuration'
         return unless receive_response_succesful?(response)
-        client.parse_lsp_configuration_settings!(response['result'])
+        original_request['params'].items.each_with_index do |item, index|
+          # The response from the client strips the section name so we need to re-add it
+          client.parse_lsp_configuration_settings!(item.section => response['result'][index])
+        end
       else
         super
       end
diff --git a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
index e6aba19f..406303f9 100644
--- a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
@@ -1123,12 +1123,16 @@
     end
 
     context 'given an original workspace/configuration request' do
-      let(:response_result) { { 'setting1' => 'value1' } }
+      let(:response_result) { [{ 'setting1' => 'value1' }] }
       let(:request_method) { 'workspace/configuration'}
-      let(:request_params) { {} }
+      let(:request_params) do
+        params = LSP::ConfigurationParams.new.from_h!('items' => [])
+        params.items << LSP::ConfigurationItem.new.from_h!('section' => 'mock')
+        params
+      end
 
       it 'should call client.parse_lsp_configuration_settings!' do
-        expect(subject.client).to receive(:parse_lsp_configuration_settings!).with(response_result)
+        expect(subject.client).to receive(:parse_lsp_configuration_settings!).with({ 'mock' => response_result[0] })
 
         subject.receive_response(response, original_request)
       end

From 9d13ba8ae27a3f52d213e7746fd85a6d1d7c2145 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Tue, 17 Sep 2019 14:57:51 +0800
Subject: [PATCH 5/7] (GH-177) Move warning due to initialized notification

While technically the protocol does allow the server to send window/showMessage
notifcations during initialization, it's better to send the warning at the
end of initialization i.e. at the initialized notification.
---
 lib/puppet-languageserver/message_router.rb | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index 432c0a5b..a53ec309 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -47,14 +47,6 @@ def receive_request(request)
         PuppetLanguageServer.log_message(:debug, 'Received initialize method')
         client.parse_lsp_initialize!(request.params)
         request.reply_result('capabilities' => PuppetLanguageServer::ServerCapabilites.capabilities)
-        unless server_options[:puppet_version].nil? || server_options[:puppet_version] == Puppet.version
-          # Add a minor delay before sending the notification to give the client some processing time
-          sleep(0.5)
-          json_rpc_handler.send_show_message_notification(
-            LSP::MessageType::WARNING,
-            "Unable to use Puppet version '#{server_options[:puppet_version]}' as it is not available. Using version '#{Puppet.version}' instead."
-          )
-        end
 
       when 'shutdown'
         PuppetLanguageServer.log_message(:debug, 'Received shutdown method')
@@ -253,6 +245,14 @@ def receive_notification(method, params)
       case method
       when 'initialized'
         PuppetLanguageServer.log_message(:info, 'Client has received initialization')
+        # Raise a warning if the Puppet version is mismatched
+        unless server_options[:puppet_version].nil? || server_options[:puppet_version] == Puppet.version
+          json_rpc_handler.send_show_message_notification(
+            LSP::MessageType::WARNING,
+            "Unable to use Puppet version '#{server_options[:puppet_version]}' as it is not available. Using version '#{Puppet.version}' instead."
+          )
+        end
+        # Register for workspace setting changes if it's supported
         if client.client_capability('workspace', 'didChangeConfiguration', 'dynamicRegistration') == true
           client.register_capability('workspace/didChangeConfiguration')
         else

From db97c1aab2cb07b747bfa926c4b4ef4789ee3575 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Tue, 17 Sep 2019 17:53:10 +0800
Subject: [PATCH 6/7] (GH-177) Add registrations and settings for on type
 formatting

Now that the Language Client can process client settings, and dynamic
registrations it is now possible to setup the server to register to receive the
onTypeFormatting request.  This commit:

* Responds to the 'puppet.editorService.formatOnType.enable' client setting
* Dynamically registers the provider if dynamic registration is possible
  otherwise uses static registration
* Adds a format_on_type method on the Language Client to track whether this
  feature is enabled
* Adds dummy response to the `textDocument/onTypeFormatting` request
* Adds tests for the client interacting with the settings
---
 lib/puppet-languageserver/language_client.rb  |  34 ++++-
 lib/puppet-languageserver/message_router.rb   |   9 +-
 .../server_capabilities.rb                    |  12 +-
 .../language_client_spec.rb                   | 122 +++++++++++++++++-
 .../message_router_spec.rb                    |  84 +++++++++++-
 5 files changed, 249 insertions(+), 12 deletions(-)

diff --git a/lib/puppet-languageserver/language_client.rb b/lib/puppet-languageserver/language_client.rb
index 22351292..8d8329bb 100644
--- a/lib/puppet-languageserver/language_client.rb
+++ b/lib/puppet-languageserver/language_client.rb
@@ -4,6 +4,9 @@ module PuppetLanguageServer
   class LanguageClient
     attr_reader :message_router
 
+    # Client settings
+    attr_reader :format_on_type
+
     def initialize(message_router)
       @message_router = message_router
       @client_capabilites = {}
@@ -17,6 +20,9 @@ def initialize(message_router)
       #   }
       # ]
       @registrations = {}
+
+      # Default settings
+      @format_on_type = false
     end
 
     def client_capability(*names)
@@ -35,12 +41,20 @@ def parse_lsp_initialize!(initialize_params = {})
       @client_capabilites = initialize_params['capabilities']
     end
 
-    # Settings could be a hash or an array of hash
-    def parse_lsp_configuration_settings!(settings = [{}])
-      # TODO: Future use. Actually do something with the settings
-      # settings = [settings] unless settings.is_a?(Hash)
-      # settings.each do |hash|
-      # end
+    def parse_lsp_configuration_settings!(settings = {})
+      # format on type
+      value = safe_hash_traverse(settings, 'puppet', 'editorService', 'formatOnType', 'enable')
+      unless value.nil? || to_boolean(value) == @format_on_type # rubocop:disable Style/GuardClause  Ummm no.
+        # Is dynamic registration available?
+        if client_capability('textDocument', 'onTypeFormatting', 'dynamicRegistration') == true
+          if value
+            register_capability('textDocument/onTypeFormatting', PuppetLanguageServer::ServerCapabilites.document_on_type_formatting_options)
+          else
+            unregister_capability('textDocument/onTypeFormatting')
+          end
+        end
+        @format_on_type = value
+      end
     end
 
     def capability_registrations(method)
@@ -148,12 +162,18 @@ def parse_unregister_capability_response!(response, original_request)
 
     private
 
+    def to_boolean(value)
+      return false if value.nil? || value == false
+      return true if value == true
+      value.to_s =~ %r{^(true|t|yes|y|1)$/i}
+    end
+
     def new_request_id
       SecureRandom.uuid
     end
 
     def safe_hash_traverse(hash, *names)
-      return nil if names.empty?
+      return nil if names.empty? || hash.nil? || hash.empty?
       item = nil
       loop do
         name = names.shift
diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index a53ec309..d72e5dc2 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -46,7 +46,11 @@ def receive_request(request)
       when 'initialize'
         PuppetLanguageServer.log_message(:debug, 'Received initialize method')
         client.parse_lsp_initialize!(request.params)
-        request.reply_result('capabilities' => PuppetLanguageServer::ServerCapabilites.capabilities)
+        # Setup static registrations if dynamic registration is not available
+        info = {
+          :documentOnTypeFormattingProvider => !client.client_capability('textDocument', 'onTypeFormatting', 'dynamicRegistration')
+        }
+        request.reply_result('capabilities' => PuppetLanguageServer::ServerCapabilites.capabilities(info))
 
       when 'shutdown'
         PuppetLanguageServer.log_message(:debug, 'Received shutdown method')
@@ -201,6 +205,9 @@ def receive_request(request)
           request.reply_result(nil)
         end
 
+      when 'textDocument/onTypeFormatting'
+        request.reply_result(nil)
+
       when 'textDocument/signatureHelp'
         file_uri = request.params['textDocument']['uri']
         line_num = request.params['position']['line']
diff --git a/lib/puppet-languageserver/server_capabilities.rb b/lib/puppet-languageserver/server_capabilities.rb
index 3ada953d..f42e937e 100644
--- a/lib/puppet-languageserver/server_capabilities.rb
+++ b/lib/puppet-languageserver/server_capabilities.rb
@@ -2,10 +2,10 @@
 
 module PuppetLanguageServer
   module ServerCapabilites
-    def self.capabilities
+    def self.capabilities(options = {})
       # https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#initialize-request
 
-      {
+      value = {
         'textDocumentSync'        => LSP::TextDocumentSyncKind::FULL,
         'hoverProvider'           => true,
         'completionProvider'      => {
@@ -19,6 +19,14 @@ def self.capabilities
           'triggerCharacters' => ['(', ',']
         }
       }
+      value['documentOnTypeFormattingProvider'] = document_on_type_formatting_options if options[:documentOnTypeFormattingProvider]
+      value
+    end
+
+    def self.document_on_type_formatting_options
+      {
+        'firstTriggerCharacter' => '>'
+      }
     end
 
     def self.no_capabilities
diff --git a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
index 22a48eca..0057a2ba 100644
--- a/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/language_client_spec.rb
@@ -1,5 +1,101 @@
 require 'spec_helper'
 
+def pretty_value(value)
+  value.nil? ? 'nil' : value.to_s
+end
+
+# Requires
+#   :settings      : A hashtable of the inbound settings
+#   :setting_value : The value that will be set to
+RSpec.shared_examples "a client setting" do |method_name|
+  [
+    { :from => false, :setting => nil,   :expected_setting => false },
+    { :from => false, :setting => false, :expected_setting => false },
+    { :from => false, :setting => true,  :expected_setting => true },
+    { :from => true,  :setting => nil,   :expected_setting => true },
+    { :from => true,  :setting => false, :expected_setting => false },
+    { :from => true,  :setting => true,  :expected_setting => true },
+  ].each do |testcase|
+    context "When it transitions from #{pretty_value(testcase[:from])} with a setting value of #{pretty_value(testcase[:setting])}" do
+      let(:setting_value) { testcase[:setting] }
+
+      before(:each) do
+        subject.instance_variable_set("@#{method_name}".intern, testcase[:from])
+      end
+
+      it "should have a cached value to #{testcase[:expected_setting]}" do
+        expect(subject.send(method_name)).to eq(testcase[:from])
+
+        subject.parse_lsp_configuration_settings!(settings)
+        expect(subject.send(method_name)).to eq(testcase[:expected_setting])
+      end
+    end
+  end
+end
+
+# Requires
+#   :settings      : A hashtable of the inbound settings
+#   :setting_value : The value that will be set to
+RSpec.shared_examples "a setting with dynamic registrations" do |method_name, dynamic_reg, registration_method|
+  [
+    { :from => false, :setting => nil,   :noop       => true },
+    { :from => false, :setting => false, :noop       => true },
+    { :from => false, :setting => true,  :register   => true },
+    { :from => true,  :setting => nil,   :noop       => true },
+    { :from => true,  :setting => false, :unregister => true },
+    { :from => true,  :setting => true,  :noop       => true },
+  ].each do |testcase|
+    context "When it transitions from #{pretty_value(testcase[:from])} with a setting value of #{pretty_value(testcase[:setting])}" do
+      let(:setting_value) { testcase[:setting] }
+
+      before(:each) do
+        subject.instance_variable_set("@#{method_name}".intern, testcase[:from])
+      end
+
+      it 'should not call any capabilities', :if => testcase[:noop] do
+        expect(subject).to receive(:client_capability).exactly(0).times
+        expect(subject).to receive(:register_capability).exactly(0).times
+        expect(subject).to receive(:unregister_capability).exactly(0).times
+
+        subject.parse_lsp_configuration_settings!(settings)
+      end
+
+      context "when dynamic registration is not supported", :unless => testcase[:noop] do
+        before(:each) do
+          expect(subject).to receive(:client_capability).with(*dynamic_reg).and_return(false)
+        end
+
+        it 'should not call any registration or unregistrations' do
+          expect(subject).to receive(:register_capability).exactly(0).times
+          expect(subject).to receive(:unregister_capability).exactly(0).times
+
+          subject.parse_lsp_configuration_settings!(settings)
+        end
+      end
+
+      context "when dynamic registration is supported", :unless => testcase[:noop] do
+        before(:each) do
+          expect(subject).to receive(:client_capability).with(*dynamic_reg).and_return(true)
+        end
+
+        it "should register #{registration_method}", :if => testcase[:register] do
+          expect(subject).to receive(:register_capability).with(registration_method, Object)
+          expect(subject).to receive(:unregister_capability).exactly(0).times
+
+          subject.parse_lsp_configuration_settings!(settings)
+        end
+
+        it "should unregister #{registration_method}", :if => testcase[:unregister] do
+          expect(subject).to receive(:unregister_capability).with(registration_method)
+          expect(subject).to receive(:register_capability).exactly(0).times
+
+          subject.parse_lsp_configuration_settings!(settings)
+        end
+      end
+    end
+  end
+end
+
 describe 'PuppetLanguageServer::LanguageClient' do
   let(:json_rpc_handler) { MockJSONRPCHandler.new }
   let(:message_router) { MockMessageRouter.new.tap { |i| i.json_rpc_handler = json_rpc_handler } }
@@ -146,6 +242,12 @@
     allow(PuppetLanguageServer).to receive(:log_message)
   end
 
+  describe '#format_on_type' do
+    it 'should be false by default' do
+      expect(subject.format_on_type).to eq(false)
+    end
+  end
+
   describe '#client_capability' do
     before(:each) do
       subject.parse_lsp_initialize!(initialize_params)
@@ -181,7 +283,25 @@
   # end
 
   describe '#parse_lsp_configuration_settings!' do
-    # TODO: Future use.
+    describe 'puppet.editorService.formatOnType.enable' do
+      let(:settings) do
+        { 'puppet' => {
+            'editorService' => {
+              'formatOnType' => {
+                'enable' => setting_value
+              }
+            }
+          }
+        }
+      end
+
+      it_behaves_like 'a client setting', :format_on_type
+
+      it_behaves_like 'a setting with dynamic registrations',
+        :format_on_type,
+        ['textDocument', 'onTypeFormatting', 'dynamicRegistration'],
+        'textDocument/onTypeFormatting'
+    end
   end
 
   describe '#capability_registrations' do
diff --git a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
index 406303f9..090039b1 100644
--- a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
@@ -18,6 +18,12 @@
     match { |actual| !actual.send(method_name).nil? }
   end
 
+  RSpec::Matchers.define :server_capability do |name|
+    match do |actual|
+      actual['capabilities'] && actual['capabilities'][name]
+    end
+  end
+
   describe '#documents' do
     it 'should respond to documents method' do
       expect(subject).to respond_to(:documents)
@@ -67,7 +73,7 @@
     # initialize - https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#initialize
     context 'given an initialize request' do
       let(:request_rpc_method) { 'initialize' }
-      let(:request_params) { { 'cap1' => 'value1' } }
+      let(:request_params) { { 'capabilities' => { 'cap1' => 'value1' } } }
       it 'should reply with capabilites' do
         expect(request).to receive(:reply_result).with(hash_including('capabilities'))
 
@@ -79,6 +85,55 @@
 
         subject.receive_request(request)
       end
+
+      context 'when onTypeFormatting does support dynamic registration' do
+        let(:request_params) do
+          { 'capabilities' => {
+              'textDocument' => {
+                'onTypeFormatting' => {
+                  'dynamicRegistration' => true
+                }
+              }
+            }
+          }
+        end
+
+        it 'should statically register a documentOnTypeFormattingProvider' do
+          expect(request).to_not receive(:reply_result).with(server_capability('documentOnTypeFormattingProvider'))
+          allow(request).to receive(:reply_result)
+
+          subject.receive_request(request)
+        end
+      end
+
+      context 'when onTypeFormatting does not support dynamic registration' do
+        let(:request_params) do
+          { 'capabilities' => {
+              'textDocument' => {
+                'onTypeFormatting' => {
+                  'dynamicRegistration' => false
+                }
+              }
+            }
+          }
+        end
+
+        it 'should statically register a documentOnTypeFormattingProvider' do
+          expect(request).to receive(:reply_result).with(server_capability('documentOnTypeFormattingProvider'))
+
+          subject.receive_request(request)
+        end
+      end
+
+      context 'when onTypeFormatting does not specify dynamic registration' do
+        let(:request_params) { {} }
+
+        it 'should statically register a documentOnTypeFormattingProvider' do
+          expect(request).to receive(:reply_result).with(server_capability('documentOnTypeFormattingProvider'))
+
+          subject.receive_request(request)
+        end
+      end
     end
 
     # shutdown - https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#shutdown
@@ -800,6 +855,33 @@
       end
     end
 
+    # textDocument/onTypeFormatting - https://microsoft.github.io/language-server-protocol/specification#textDocument_onTypeFormatting
+    context 'given a textDocument/onTypeFormatting request' do
+      let(:request_rpc_method) { 'textDocument/onTypeFormatting' }
+      let(:file_uri) { MANIFEST_FILENAME }
+      let(:line_num) { 1 }
+      let(:char_num) { 6 }
+      let(:trigger_char) { '>' }
+      let(:formatting_options) { { 'tabSize' => 2, 'insertSpaces' => true} }
+      let(:request_params) { {
+        'textDocument' => {
+          'uri' => file_uri
+        },
+        'position' => {
+          'line' => line_num,
+          'character' => char_num,
+        },
+        'ch' => trigger_char,
+        'options' => formatting_options
+      } }
+
+      it 'should not log an error message' do
+        expect(PuppetLanguageServer).to_not receive(:log_message).with(:error,"Unknown RPC method #{request_rpc_method}")
+
+        subject.receive_request(request)
+      end
+    end
+
     context 'given an unknown request' do
       let(:request_rpc_method) { 'unknown_request_method' }
 

From 87fa677e4e6622bc80d1f4b05edf63b4313e21c6 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Tue, 17 Sep 2019 20:21:26 +0800
Subject: [PATCH 7/7] (GH-177) Add auto-align hash rocket feature

Previously the onTypeFormatting provider was created and responds to client
settings, turning it on and off appropriately.  This commit actually adds the
on type formatter:

* Only for puppet manifests (.pp)
* Uses puppet-lint lexer to tokenise the document and uses the tokens to
  determine indenting
* Will not operate if the document is over 4KB in size or the client is using
  tabs instead of spaces
* Adds tests for the formatting
---
 .../manifest/format_on_type_provider.rb       | 137 ++++++++++++++++++
 lib/puppet-languageserver/message_router.rb   |  26 +++-
 lib/puppet-languageserver/providers.rb        |   1 +
 .../manifest/format_on_type_provider_spec.rb  | 123 ++++++++++++++++
 .../message_router_spec.rb                    |  73 +++++++++-
 5 files changed, 356 insertions(+), 4 deletions(-)
 create mode 100644 lib/puppet-languageserver/manifest/format_on_type_provider.rb
 create mode 100644 spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb

diff --git a/lib/puppet-languageserver/manifest/format_on_type_provider.rb b/lib/puppet-languageserver/manifest/format_on_type_provider.rb
new file mode 100644
index 00000000..0092e24d
--- /dev/null
+++ b/lib/puppet-languageserver/manifest/format_on_type_provider.rb
@@ -0,0 +1,137 @@
+# frozen_string_literal: true
+
+require 'puppet-lint'
+
+module PuppetLanguageServer
+  module Manifest
+    class FormatOnTypeProvider
+      class << self
+        def instance
+          @instance ||= new
+        end
+      end
+
+      def format(content, line, char, trigger_character, formatting_options)
+        result = []
+        # Abort if the user has pressed something other than `>`
+        return result unless trigger_character == '>'
+        # Abort if the formatting is tab based. Can't do that yet
+        return result unless formatting_options['insertSpaces'] == true
+        # Abort if content is too big
+        return result if content.length > 4096
+
+        lexer = PuppetLint::Lexer.new
+        tokens = lexer.tokenise(content)
+
+        # Find where in the manifest the cursor is
+        cursor_token = find_token_by_location(tokens, line, char)
+        return result if cursor_token.nil?
+        # The cursor should be at the end of a hashrocket, otherwise exit
+        return result unless cursor_token.type == :FARROW
+
+        # Find the start of the hash with respect to the cursor
+        start_brace = cursor_token.prev_token_of(:LBRACE, skip_blocks: true)
+        # Find the end  of the hash with respect to the cursor
+        end_brace = cursor_token.next_token_of(:RBRACE, skip_blocks: true)
+
+        # The line count between the start and end brace needs to be at least 2 lines. Otherwise there's nothing to align to
+        return result if end_brace.nil? || start_brace.nil? || end_brace.line - start_brace.line <= 2
+
+        # Find all hashrockets '=>' between the hash braces, ignoring nested hashes
+        farrows = []
+        farrow_token = start_brace
+        lines = []
+        loop do
+          farrow_token = farrow_token.next_token_of(:FARROW, skip_blocks: true)
+          # if there are no more hashrockets, or we've gone past the end_brace, we can exit the loop
+          break if farrow_token.nil? || farrow_token.line > end_brace.line
+          # if there's a hashrocket AFTER the closing brace (why?) then we can also exit the loop
+          break if farrow_token.line == end_brace.line && farrow_token.character > end_brace.character
+          # Check for multiple hashrockets on the same line. If we find some, then we can't do any automated indentation
+          return result if lines.include?(farrow_token.line)
+          lines << farrow_token.line
+          farrows << { token: farrow_token }
+        end
+
+        # Now we have a list of farrows, time for figure out the indentation marks
+        farrows.each do |item|
+          item.merge!(calculate_indentation_info(item[:token]))
+        end
+
+        # Now we have the list of indentations we can find the biggest
+        max_indent = -1
+        farrows.each do |info|
+          max_indent = info[:indent] if info[:indent] > max_indent
+        end
+        # No valid indentations found
+        return result if max_indent == -1
+
+        # Now we have the indent size, generate all of the required TextEdits
+        farrows.each do |info|
+          # Ignore invalid hashrockets
+          next if info[:indent] == -1
+          end_name_token = info[:name_token].column + info[:name_token].to_manifest.length
+          begin_farrow_token = info[:token].column
+          new_whitespace = max_indent - end_name_token
+          # If the whitespace is already what we want, then ignore it.
+          next if begin_farrow_token - end_name_token == new_whitespace
+
+          # Create the TextEdit
+          result << LSP::TextEdit.new.from_h!(
+            'newText' => ' ' * new_whitespace,
+            'range'   => LSP.create_range(info[:token].line - 1, end_name_token - 1, info[:token].line - 1, begin_farrow_token - 1)
+          )
+        end
+        result
+      end
+
+      private
+
+      VALID_TOKEN_TYPES = %i[NAME STRING SSTRING].freeze
+
+      def find_token_by_location(tokens, line, character)
+        return nil if tokens.empty?
+        # Puppet Lint uses base 1, but LSP is base 0, so adjust accordingly
+        cursor_line = line + 1
+        cursor_column = character + 1
+        idx = -1
+        while idx < tokens.count
+          idx += 1
+          # if the token is on previous lines keep looking...
+          next if tokens[idx].line < cursor_line
+          # return nil if we skipped over the line we need
+          return nil if tokens[idx].line > cursor_line
+          # return nil if we skipped over the character position we need
+          return nil if tokens[idx].column > cursor_column
+          # return the token if it starts on the cursor column we are interested in
+          return tokens[idx] if tokens[idx].column == cursor_column
+          end_column = tokens[idx].column + tokens[idx].to_manifest.length
+          # return the token it the cursor column is within the token string
+          return tokens[idx] if cursor_column <= end_column
+          # otherwise, keep on searching
+        end
+        nil
+      end
+
+      def calculate_indentation_info(farrow_token)
+        result = { indent: -1 }
+        # This is not a valid hashrocket if there's no previous tokens
+        return result if farrow_token.prev_token.nil?
+        if VALID_TOKEN_TYPES.include?(farrow_token.prev_token.type)
+          # Someone forgot the whitespace! e.g. ensure=>
+          result[:indent] = farrow_token.column + 1
+          result[:name_token] = farrow_token.prev_token
+          return result
+        end
+        if farrow_token.prev_token.type == :WHITESPACE
+          # If the whitespace has no previous token (which shouldn't happen) or the thing before the whitespace is not a property name this it not a valid hashrocket
+          return result if farrow_token.prev_token.prev_token.nil?
+          return result unless VALID_TOKEN_TYPES.include?(farrow_token.prev_token.prev_token.type)
+          result[:name_token] = farrow_token.prev_token.prev_token
+          result[:indent] = farrow_token.prev_token.column + 1 # The indent is the whitespace column + 1
+        end
+        result
+      end
+    end
+  end
+end
diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb
index d72e5dc2..b0574cbf 100644
--- a/lib/puppet-languageserver/message_router.rb
+++ b/lib/puppet-languageserver/message_router.rb
@@ -206,7 +206,31 @@ def receive_request(request)
         end
 
       when 'textDocument/onTypeFormatting'
-        request.reply_result(nil)
+        unless client.format_on_type
+          request.reply_result(nil)
+          return
+        end
+        file_uri = request.params['textDocument']['uri']
+        line_num = request.params['position']['line']
+        char_num = request.params['position']['character']
+        content  = documents.document(file_uri)
+        begin
+          case documents.document_type(file_uri)
+          when :manifest
+            request.reply_result(PuppetLanguageServer::Manifest::FormatOnTypeProvider.instance.format(
+                                   content,
+                                   line_num,
+                                   char_num,
+                                   request.params['ch'],
+                                   request.params['options']
+                                 ))
+          else
+            raise "Unable to format on type on #{file_uri}"
+          end
+        rescue StandardError => e
+          PuppetLanguageServer.log_message(:error, "(textDocument/onTypeFormatting) #{e}")
+          request.reply_result(nil)
+        end
 
       when 'textDocument/signatureHelp'
         file_uri = request.params['textDocument']['uri']
diff --git a/lib/puppet-languageserver/providers.rb b/lib/puppet-languageserver/providers.rb
index 874b08d9..01c38eb0 100644
--- a/lib/puppet-languageserver/providers.rb
+++ b/lib/puppet-languageserver/providers.rb
@@ -5,6 +5,7 @@
   manifest/completion_provider
   manifest/definition_provider
   manifest/document_symbol_provider
+  manifest/format_on_type_provider
   manifest/signature_provider
   manifest/validation_provider
   manifest/hover_provider
diff --git a/spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb b/spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb
new file mode 100644
index 00000000..65320d2f
--- /dev/null
+++ b/spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb
@@ -0,0 +1,123 @@
+require 'spec_helper'
+
+describe 'PuppetLanguageServer::Manifest::FormatOnTypeProvider' do
+  let(:subject) { PuppetLanguageServer::Manifest::FormatOnTypeProvider.new }
+
+  describe '::instance' do
+    it 'should exist' do
+      expect(PuppetLanguageServer::Manifest::FormatOnTypeProvider).to respond_to(:instance)
+    end
+
+    it 'should return the same object' do
+      object1 = PuppetLanguageServer::Manifest::FormatOnTypeProvider.instance
+      object2 = PuppetLanguageServer::Manifest::FormatOnTypeProvider.instance
+      expect(object1).to eq(object2)
+    end
+  end
+
+  describe '#format' do
+    let(:formatting_options) do
+      LSP::FormattingOptions.new.tap do |item|
+        item.tabSize = 2
+        item.insertSpaces = true
+      end.to_h
+    end
+
+    [' ', '=', ','].each do |trigger|
+      context "given a trigger character of '#{trigger}'" do
+        it 'should return an empty array' do
+          result = subject.format("{\n  oneline    =>\n}\n", 1, 1, trigger, formatting_options)
+          expect(result).to eq([])
+        end
+      end
+    end
+
+    context "given a trigger character of greater-than '>'" do
+      let(:trigger_character) { '>' }
+      let(:content) do <<-MANIFEST
+user {
+  ensure=> 'something',
+  password   =>
+  name => {
+    'abc' => '123',
+    'def'    => '789',
+  },
+  name2    => 'correct',
+}
+MANIFEST
+      end
+      let(:valid_cursor) { { line: 2, char: 15 } }
+      let(:inside_cursor) { { line: 5, char: 15 } }
+
+      it 'should return an empty array if the cursor is not on a hashrocket' do
+        result = subject.format(content, 1, 1, trigger_character, formatting_options)
+        expect(result).to eq([])
+      end
+
+      it 'should return an empty array if the formatting options uses tabs' do
+        result = subject.format(content, valid_cursor[:line], valid_cursor[:char], trigger_character, formatting_options.tap { |i| i['insertSpaces'] = false} )
+        expect(result).to eq([])
+      end
+
+      it 'should return an empty array if the document is large' do
+        large_content = content + ' ' * 4096
+        result = subject.format(large_content, valid_cursor[:line], valid_cursor[:char], trigger_character, formatting_options)
+        expect(result).to eq([])
+      end
+
+      # Valid hashrocket key tests
+      [
+        { name: 'bare name',            text: 'barename' },
+        { name: 'single quoted string', text: '\'name\'' },
+        { name: 'double quoted string', text: '"name"' },
+      ].each do |testcase|
+        context "and given a manifest with #{testcase[:name]}" do
+          let(:content) { "{\n  a =>\n  ##TESTCASE## => 'value'\n}\n"}
+
+          it 'should return an empty' do
+            result = subject.format(content.gsub('##TESTCASE##', testcase[:text]), 1, 6, trigger_character, formatting_options)
+            # The expected TextEdit should edit the `a =>`
+            expect(result.count).to eq(1)
+            expect(result[0].range.start.line).to eq(1)
+            expect(result[0].range.start.character).to eq(3)
+            expect(result[0].range.end.line).to eq(1)
+            expect(result[0].range.end.character).to eq(4)
+          end
+        end
+      end
+
+      it 'should have valid text edits in the outer hash' do
+        result = subject.format(content, valid_cursor[:line], valid_cursor[:char], trigger_character, formatting_options)
+
+        expect(result.count).to eq(3)
+        expect(result[0].to_h).to eq({"range"=>{"start"=>{"character"=>8,  "line"=>1}, "end"=>{"character"=>8,  "line"=>1}}, "newText"=>"   "})
+        expect(result[1].to_h).to eq({"range"=>{"start"=>{"character"=>10, "line"=>2}, "end"=>{"character"=>13, "line"=>2}}, "newText"=>" "})
+        expect(result[2].to_h).to eq({"range"=>{"start"=>{"character"=>6,  "line"=>3}, "end"=>{"character"=>7,  "line"=>3}}, "newText"=>"     "})
+      end
+
+      it 'should have valid text edits in the inner hash' do
+        result = subject.format(content, inside_cursor[:line], inside_cursor[:char], trigger_character, formatting_options)
+
+        expect(result.count).to eq(1)
+        expect(result[0].to_h).to eq({"range"=>{"start"=>{"character"=>9, "line"=>5}, "end"=>{"character"=>13, "line"=>5}}, "newText"=>" "})
+      end
+
+      # Invalid scenarios
+      [
+        { name: 'only one line',                      content: "{\n  oneline    =>\n}\n" },
+        { name: 'there is nothing to indent',         content: "{\n  oneline    =>\n  nextline12 => 'value',\n}\n" },
+        { name: 'no starting Left Brace',             content: "\n  oneline    =>\n  nextline12 => 'value',\n}\n" },
+        { name: 'no ending Right Brace',              content: "{\n  oneline    =>\n  nextline12 => 'value',\n\n" },
+        { name: 'hashrockets on the same line',       content: "{\n  oneline    => ,  nextline12 => 'value',\n\n"},
+        { name: 'invalid text before the hashrocket', content: "{\n  String[]   =>\n  nextline => 'value',\n}\n" },
+      ].each do |testcase|
+        context "and given a manifest with #{testcase[:name]}" do
+          it 'should return an empty' do
+            result = subject.format(testcase[:content], 1, 15, trigger_character, formatting_options)
+            expect(result).to eq([])
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
index 090039b1..829c389f 100644
--- a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
+++ b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb
@@ -859,6 +859,7 @@
     context 'given a textDocument/onTypeFormatting request' do
       let(:request_rpc_method) { 'textDocument/onTypeFormatting' }
       let(:file_uri) { MANIFEST_FILENAME }
+      let(:file_content) { "{\n  a =>\n  name => 'value'\n}\n" }
       let(:line_num) { 1 }
       let(:char_num) { 6 }
       let(:trigger_char) { '>' }
@@ -874,11 +875,77 @@
         'ch' => trigger_char,
         'options' => formatting_options
       } }
+      let(:provider) { PuppetLanguageServer::Manifest::FormatOnTypeProvider.new }
 
-      it 'should not log an error message' do
-        expect(PuppetLanguageServer).to_not receive(:log_message).with(:error,"Unknown RPC method #{request_rpc_method}")
+      before(:each) do
+        subject.documents.clear
+        subject.documents.set_document(file_uri,file_content, 0)
+      end
 
-        subject.receive_request(request)
+      context 'with client.format_on_type set to false' do
+        before(:each) do
+          allow(subject.client).to receive(:format_on_type).and_return(false)
+        end
+
+        it 'should reply with nil' do
+          expect(request).to receive(:reply_result).with(nil)
+          subject.receive_request(request)
+        end
+      end
+
+      context 'with client.format_on_type set to true' do
+        before(:each) do
+          allow(subject.client).to receive(:format_on_type).and_return(true)
+        end
+
+        context 'for a file the server does not understand' do
+          let(:file_uri) { UNKNOWN_FILENAME }
+
+          it 'should log an error message' do
+            expect(PuppetLanguageServer).to receive(:log_message).with(:error,/Unable to format on type on/)
+
+            subject.receive_request(request)
+          end
+
+          it 'should reply with nil' do
+            expect(request).to receive(:reply_result).with(nil)
+
+            subject.receive_request(request)
+          end
+        end
+
+        context 'for a puppet manifest file' do
+          let(:file_uri) { MANIFEST_FILENAME }
+
+          before(:each) do
+            allow(PuppetLanguageServer::Manifest::FormatOnTypeProvider).to receive(:instance).and_return(provider)
+          end
+
+          it 'should call format method on the Format On Type provider' do
+            expect(provider).to receive(:format)
+              .with(file_content, line_num, char_num, trigger_char, formatting_options).and_return('something')
+
+            result = subject.receive_request(request)
+          end
+
+          context 'and an error occurs during formatting' do
+            before(:each) do
+              expect(provider).to receive(:format).and_raise('MockError')
+            end
+
+            it 'should log an error message' do
+              expect(PuppetLanguageServer).to receive(:log_message).with(:error,/MockError/)
+
+              subject.receive_request(request)
+            end
+
+            it 'should reply with nil' do
+              expect(request).to receive(:reply_result).with(nil)
+
+              subject.receive_request(request)
+            end
+          end
+        end
       end
     end