From b38dfb4253da488add4d5fe387120815f59803f4 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Thu, 1 Aug 2019 20:03:53 +0800 Subject: [PATCH 1/3] (maint) Add whitespace for rubocop This commit adds some whitespace to fix a rubocop warning. --- lib/puppet-languageserver-sidecar/puppet_strings_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet-languageserver-sidecar/puppet_strings_helper.rb b/lib/puppet-languageserver-sidecar/puppet_strings_helper.rb index 815457b4..02f847bc 100644 --- a/lib/puppet-languageserver-sidecar/puppet_strings_helper.rb +++ b/lib/puppet-languageserver-sidecar/puppet_strings_helper.rb @@ -282,7 +282,7 @@ class FileDocumentation attr_accessor :types def initialize(path = nil) - @path = path + @path = path @classes = PuppetLanguageServer::Sidecar::Protocol::PuppetClassList.new @functions = PuppetLanguageServer::Sidecar::Protocol::PuppetFunctionList.new @types = PuppetLanguageServer::Sidecar::Protocol::PuppetTypeList.new From 3b2bf01644033b0cb0274def04cd536d72f7b637 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Thu, 1 Aug 2019 20:05:15 +0800 Subject: [PATCH 2/3] (GH-163) Add AggregateMetadata sidecar protocol object Previously, to get all metadata information, the sidecar would need to be called multiple times, however, with the puppetstrings featureflag, we can now evaluate the metadata all at once. This commit adds a new object called AggregateMetadata which can hold all of the metadata objects (classes etc.) This commit also adds tests to ensure that it can be serialised and deserialised correctly. And finally this commit changes the puppetstrings helper to use the new aggregate instead of a standard hashtable. --- .../puppet_helper_puppetstrings.rb | 18 ++-- lib/puppet-languageserver/sidecar_protocol.rb | 83 ++++++++++++++++++- lib/puppet_languageserver_sidecar.rb | 12 +-- .../sidecar_protocol_spec.rb | 22 +++++ 4 files changed, 114 insertions(+), 21 deletions(-) diff --git a/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb b/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb index 62881139..aad12ae8 100644 --- a/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb +++ b/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb @@ -47,13 +47,9 @@ def self.retrieve_via_puppet_strings(cache, options = {}) object_types = options[:object_types].nil? ? available_documentation_types : options[:object_types] object_types.select! { |i| available_documentation_types.include?(i) } - result = {} + result = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new return result if object_types.empty? - result[:classes] = PuppetLanguageServer::Sidecar::Protocol::PuppetClassList.new if object_types.include?(:class) - result[:functions] = PuppetLanguageServer::Sidecar::Protocol::PuppetFunctionList.new if object_types.include?(:function) - result[:types] = PuppetLanguageServer::Sidecar::Protocol::PuppetTypeList.new if object_types.include?(:type) - current_env = current_environment for_agent = options[:for_agent].nil? ? true : options[:for_agent] loaders = Puppet::Pops::Loaders.new(current_env, for_agent) @@ -74,25 +70,25 @@ def self.retrieve_via_puppet_strings(cache, options = {}) next if file_doc.nil? if object_types.include?(:class) # rubocop:disable Style/IfUnlessModifier This reads better - file_doc.classes.each { |item| result[:classes] << item } + file_doc.classes.each { |item| result.append!(item) } end if object_types.include?(:function) # rubocop:disable Style/IfUnlessModifier This reads better - file_doc.functions.each { |item| result[:functions] << item } + file_doc.functions.each { |item| result.append!(item) } end if object_types.include?(:type) file_doc.types.each do |item| - result[:types] << item unless name == 'whit' || name == 'component' # rubocop:disable Style/MultipleComparison + result.append!(item) unless name == 'whit' || name == 'component' # rubocop:disable Style/MultipleComparison end end end # Remove Puppet3 functions which have a Puppet4 function already loaded if object_types.include?(:function) - pup4_functions = result[:functions].select { |i| i.function_version == 4 }.map { |i| i.key } - result[:functions].reject! { |i| i.function_version == 3 && pup4_functions.include?(i.key) } + pup4_functions = result.functions.select { |i| i.function_version == 4 }.map { |i| i.key } + result.functions.reject! { |i| i.function_version == 3 && pup4_functions.include?(i.key) } end - result.each { |key, item| PuppetLanguageServerSidecar.log_message(:debug, "[PuppetHelper::retrieve_via_puppet_strings] Finished loading #{item.count} #{key}") } + result.each_list { |key, item| PuppetLanguageServerSidecar.log_message(:debug, "[PuppetHelper::retrieve_via_puppet_strings] Finished loading #{item.count} #{key}") } result end diff --git a/lib/puppet-languageserver/sidecar_protocol.rb b/lib/puppet-languageserver/sidecar_protocol.rb index 9b716b73..68123a8f 100644 --- a/lib/puppet-languageserver/sidecar_protocol.rb +++ b/lib/puppet-languageserver/sidecar_protocol.rb @@ -141,9 +141,7 @@ def child_type end end - class NodeGraph - include Base - + class NodeGraph < BaseClass attr_accessor :dot_content attr_accessor :error_content @@ -337,7 +335,7 @@ def child_type end end - class Resource + class Resource < BaseClass attr_accessor :manifest def to_h @@ -365,6 +363,83 @@ def child_type Resource end end + + # This class is a little special as it contains a lot logic. + # In essence this is just wrapping a hash with specific methods for the + # the different types of metadata (classes, functions etc.) + class AggregateMetadata < BaseClass + def initialize + super + @aggregate = {} + end + + def classes + @aggregate[:classes] + end + + def functions + @aggregate[:functions] + end + + def types + @aggregate[:types] + end + + def append!(obj) + list_for_object_class(obj.class) << obj + end + + def to_json(*options) + @aggregate.to_json(options) + end + + def from_json!(json_string) + obj = JSON.parse(json_string) + obj.each do |key, value| + info = METADATA_LIST[key.intern] + next if info.nil? + list = list_for_object_class(info[:item_class]) + value.each { |i| list << info[:item_class].new.from_h!(i) } + end + self + end + + def each_list + return unless block_given? + @aggregate.each { |k, v| yield k, v } + end + + private + + # When adding a new metadata item: + # - Add to the information to this hash + # - Add a method to access the aggregate + METADATA_LIST = { + :classes => { + :item_class => PuppetClass, + :list_class => PuppetClassList + }, + :functions => { + :item_class => PuppetFunction, + :list_class => PuppetFunctionList + }, + :types => { + :item_class => PuppetType, + :list_class => PuppetTypeList + } + }.freeze + + def list_for_object_class(klass) + METADATA_LIST.each do |name, info| + if klass == info[:item_class] + @aggregate[name] = info[:list_class].new if @aggregate[name].nil? + return @aggregate[name] + end + end + + raise "Unknown object class #{klass.name}" + end + end end end end diff --git a/lib/puppet_languageserver_sidecar.rb b/lib/puppet_languageserver_sidecar.rb index 227faf17..73fb25ba 100644 --- a/lib/puppet_languageserver_sidecar.rb +++ b/lib/puppet_languageserver_sidecar.rb @@ -261,7 +261,7 @@ def self.execute(options) when 'default_classes' cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new if use_puppet_strings - PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:class])[:classes] + PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:class]).classes else PuppetLanguageServerSidecar::PuppetHelper.retrieve_classes(cache) end @@ -269,7 +269,7 @@ def self.execute(options) when 'default_functions' cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new if use_puppet_strings - PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:function])[:functions] + PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:function]).functions else PuppetLanguageServerSidecar::PuppetHelper.retrieve_functions(cache) end @@ -277,7 +277,7 @@ def self.execute(options) when 'default_types' cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new if use_puppet_strings - PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:type])[:types] + PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:type]).types else PuppetLanguageServerSidecar::PuppetHelper.retrieve_types(cache) end @@ -314,7 +314,7 @@ def self.execute(options) cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:class], - :root_path => PuppetLanguageServerSidecar::Workspace.root_path)[:classes] + :root_path => PuppetLanguageServerSidecar::Workspace.root_path).classes else PuppetLanguageServerSidecar::PuppetHelper.retrieve_classes(null_cache, :root_path => PuppetLanguageServerSidecar::Workspace.root_path) @@ -328,7 +328,7 @@ def self.execute(options) cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:function], - :root_path => PuppetLanguageServerSidecar::Workspace.root_path)[:functions] + :root_path => PuppetLanguageServerSidecar::Workspace.root_path).functions else PuppetLanguageServerSidecar::PuppetHelper.retrieve_functions(null_cache, :root_path => PuppetLanguageServerSidecar::Workspace.root_path) @@ -341,7 +341,7 @@ def self.execute(options) cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => [:type], - :root_path => PuppetLanguageServerSidecar::Workspace.root_path)[:types] + :root_path => PuppetLanguageServerSidecar::Workspace.root_path).types else PuppetLanguageServerSidecar::PuppetHelper.retrieve_types(null_cache) end diff --git a/spec/languageserver/unit/puppet-languageserver/sidecar_protocol_spec.rb b/spec/languageserver/unit/puppet-languageserver/sidecar_protocol_spec.rb index ddb0b0fc..df7a0a0a 100644 --- a/spec/languageserver/unit/puppet-languageserver/sidecar_protocol_spec.rb +++ b/spec/languageserver/unit/puppet-languageserver/sidecar_protocol_spec.rb @@ -14,6 +14,13 @@ expect(serial).to be_a(String) end + + it 'should roundtrip to_json to from_json!' do + subject_as_json = subject.to_json + copy = subject_klass.new.from_json!(subject_as_json) + expect(copy.to_json).to eq(subject_as_json) + expect(copy.hash).to eq(subject.hash) + end end shared_examples_for 'a base Sidecar Protocol Puppet object' do @@ -436,4 +443,19 @@ expect(subject.child_type).to eq(PuppetLanguageServer::Sidecar::Protocol::Resource) end end + + describe 'AggregateMetadata' do + let(:subject_klass) { PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata } + let(:subject) { + value = subject_klass.new + (1..3).each do |_| + value.append!(random_sidecar_puppet_class) + value.append!(random_sidecar_puppet_function) + value.append!(random_sidecar_puppet_type) + end + value + } + + it_should_behave_like 'a base Sidecar Protocol object' + end end From 7e6ac3be6484ac55454a57b741e077ae104220a8 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Thu, 1 Aug 2019 20:42:10 +0800 Subject: [PATCH 3/3] (GH-163) Add aggregate sidecar tasks Previously the AggregateMetadata sidecar protocol object was added. This commit adds a default_aggregate and workspace_aggregate sidecar action which will gather all of the metadata in one call. These actions will only be valid with the puppetstrings featureflag. This commt also adds integration tests for these new actions, with and without the featureflag set. --- lib/puppet_languageserver_sidecar.rb | 23 +++++ .../featureflag_puppetstrings_spec.rb | 93 +++++++++++++++++++ .../puppet-languageserver-sidecar_spec.rb | 30 ++++++ 3 files changed, 146 insertions(+) diff --git a/lib/puppet_languageserver_sidecar.rb b/lib/puppet_languageserver_sidecar.rb index 73fb25ba..0e686654 100644 --- a/lib/puppet_languageserver_sidecar.rb +++ b/lib/puppet_languageserver_sidecar.rb @@ -105,11 +105,13 @@ def self.require_gems(options) ACTION_LIST = %w[ noop + default_aggregate default_classes default_functions default_types node_graph resource_list + workspace_aggregate workspace_classes workspace_functions workspace_types @@ -258,6 +260,15 @@ def self.execute(options) when 'noop' [] + when 'default_aggregate' + cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new + if use_puppet_strings + PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => PuppetLanguageServerSidecar::PuppetHelper.available_documentation_types) + else + log_message(:warn, 'The default_aggregate action is only supported with the puppetstrings feature flag') + {} + end + when 'default_classes' cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new if use_puppet_strings @@ -307,6 +318,18 @@ def self.execute(options) end PuppetLanguageServerSidecar::PuppetHelper.get_puppet_resource(typename, title) + when 'workspace_aggregate' + return nil unless inject_workspace_as_module || inject_workspace_as_environment + cache = options[:disable_cache] ? PuppetLanguageServerSidecar::Cache::Null.new : PuppetLanguageServerSidecar::Cache::FileSystem.new + if use_puppet_strings + PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, + :object_types => PuppetLanguageServerSidecar::PuppetHelper.available_documentation_types, + :root_path => PuppetLanguageServerSidecar::Workspace.root_path) + else + log_message(:warn, 'The workspace_aggregate action is only supported with the puppetstrings feature flag') + {} + end + when 'workspace_classes' null_cache = PuppetLanguageServerSidecar::Cache::Null.new return nil unless inject_workspace_as_module || inject_workspace_as_environment diff --git a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb index 19f7fe6d..093b42ea 100644 --- a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb +++ b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb @@ -92,6 +92,37 @@ def expect_same_array_content(a, b) end end + describe 'when running default_aggregate action' do + let (:cmd_options) { ['--action', 'default_aggregate'] } + + it 'should return a cachable deserializable aggregate object with all default metadata' do + expect_empty_cache + + result = run_sidecar(cmd_options) + deserial = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + expect { deserial.from_json!(result) }.to_not raise_error + + # The contents of the result are tested later + + # Now run using cached information + expect_populated_cache + + result2 = run_sidecar(cmd_options) + deserial2 = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new() + expect { deserial2.from_json!(result2) }.to_not raise_error + + deserial.class + .instance_methods(false) + .reject { |name| %i[to_json from_json! each_list append!].include?(name) } + .each do |method_name| + # There should be at least one item + expect(deserial.send(method_name).count).to be > 0 + # Before and after should be the same + expect_same_array_content(deserial.send(method_name), deserial2.send(method_name)) + end + end + end + describe 'when running default_classes action' do let (:cmd_options) { ['--action', 'default_classes'] } @@ -236,6 +267,37 @@ def expect_same_array_content(a, b) end end + describe 'when running workspace_aggregate action' do + let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } + + it 'should return a cachable deserializable aggregate object with all default metadata' do + expect_empty_cache + + result = run_sidecar(cmd_options) + deserial = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + expect { deserial.from_json!(result) }.to_not raise_error + + # The contents of the result are tested later + + # Now run using cached information + expect_populated_cache + + result2 = run_sidecar(cmd_options) + deserial2 = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new() + expect { deserial2.from_json!(result2) }.to_not raise_error + + deserial.class + .instance_methods(false) + .reject { |name| %i[to_json from_json! each_list append!].include?(name) } + .each do |method_name| + # There should be at least one item + expect(deserial.send(method_name).count).to be > 0 + # Before and after should be the same + expect_same_array_content(deserial.send(method_name), deserial2.send(method_name)) + end + end + end + describe 'when running workspace_classes action' do let (:cmd_options) { ['--action', 'workspace_classes', '--local-workspace', workspace] } @@ -348,6 +410,37 @@ def expect_same_array_content(a, b) end end + describe 'when running workspace_aggregate action' do + let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } + + it 'should return a cachable deserializable aggregate object with all default metadata' do + expect_empty_cache + + result = run_sidecar(cmd_options) + deserial = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + expect { deserial.from_json!(result) }.to_not raise_error + + # The contents of the result are tested later + + # Now run using cached information + expect_populated_cache + + result2 = run_sidecar(cmd_options) + deserial2 = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new() + expect { deserial2.from_json!(result2) }.to_not raise_error + + deserial.class + .instance_methods(false) + .reject { |name| %i[to_json from_json! each_list append!].include?(name) } + .each do |method_name| + # There should be at least one item + expect(deserial.send(method_name).count).to be > 0 + # Before and after should be the same + expect_same_array_content(deserial.send(method_name), deserial2.send(method_name)) + end + end + end + describe 'when running workspace_classes action' do let (:cmd_options) { ['--action', 'workspace_classes', '--local-workspace', workspace] } diff --git a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb index 3bbebe4a..e25e8844 100644 --- a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb +++ b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb @@ -45,6 +45,16 @@ def with_temporary_file(content) end end + describe 'when running default_aggregate action' do + let (:cmd_options) { ['--action', 'default_aggregate'] } + + it 'should return an empty hash' do + result = run_sidecar(cmd_options) + + expect(result).to eq('{}') + end + end + describe 'when running default_classes action' do let (:cmd_options) { ['--action', 'default_classes'] } @@ -126,6 +136,16 @@ def with_temporary_file(content) end end + describe 'when running workspace_aggregate action' do + let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } + + it 'should return an empty hash' do + result = run_sidecar(cmd_options) + + expect(result).to eq('{}') + end + end + describe 'when running workspace_classes action' do let (:cmd_options) { ['--action', 'workspace_classes', '--local-workspace', workspace] } @@ -212,6 +232,16 @@ def with_temporary_file(content) end end + describe 'when running workspace_aggregate action' do + let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } + + it 'should return an empty hash' do + result = run_sidecar(cmd_options) + + expect(result).to eq('{}') + end + end + describe 'when running workspace_classes action' do let (:cmd_options) { ['--action', 'workspace_classes', '--local-workspace', workspace] }