From 1cbe2ad70eddbf00ef2d3127a9ffcc9f220ee277 Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Tue, 12 Apr 2011 21:08:57 -0700 Subject: (7080) Adding json support to Indirector Request We'll be using this to do RPC over mcollective. Reviewed-by: Daniel Pittman Reviewed-by: Nick Lewis Signed-off-by: Luke Kanies --- lib/puppet/indirector/request.rb | 51 ++++++++++++++++++- spec/unit/indirector/request_spec.rb | 99 ++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb index fd8d654dd..1918a3fb5 100644 --- a/lib/puppet/indirector/request.rb +++ b/lib/puppet/indirector/request.rb @@ -14,6 +14,51 @@ class Puppet::Indirector::Request OPTION_ATTRIBUTES = [:ip, :node, :authenticated, :ignore_terminus, :ignore_cache, :instance, :environment] + def self.from_pson(json) + raise ArgumentError, "No indirection name provided in json data" unless indirection_name = json['type'] + raise ArgumentError, "No method name provided in json data" unless method = json['method'] + raise ArgumentError, "No key provided in json data" unless key = json['key'] + + request = new(indirection_name, method, key, json['attributes']) + + if instance = json['instance'] + klass = Puppet::Indirector::Indirection.instance(request.indirection_name).model + if instance.is_a?(klass) + request.instance = instance + else + request.instance = klass.from_pson(instance) + end + end + + request + end + + def to_pson(*args) + result = { + 'document_type' => 'Puppet::Indirector::Request', + 'data' => { + 'type' => indirection_name, + 'method' => method, + 'key' => key + } + } + data = result['data'] + attributes = {} + OPTION_ATTRIBUTES.each do |key| + next unless value = send(key) + attributes[key] = value + end + + options.each do |opt, value| + attributes[opt] = value + end + + data['attributes'] = attributes unless attributes.empty? + data['instance'] = instance if instance + + result.to_pson(*args) + end + # Is this an authenticated request? def authenticated? # Double negative, so we just get true or false @@ -61,9 +106,11 @@ class Puppet::Indirector::Request self.indirection_name = indirection_name self.method = method + options = options.inject({}) { |hash, ary| hash[ary[0].to_sym] = ary[1]; hash } + set_attributes(options) - @options = options.inject({}) { |hash, ary| hash[ary[0].to_sym] = ary[1]; hash } + @options = options if key_or_instance.is_a?(String) || key_or_instance.is_a?(Symbol) key = key_or_instance @@ -153,7 +200,7 @@ class Puppet::Indirector::Request def set_attributes(options) OPTION_ATTRIBUTES.each do |attribute| - if options.include?(attribute) + if options.include?(attribute.to_sym) send(attribute.to_s + "=", options[attribute]) options.delete(attribute) end diff --git a/spec/unit/indirector/request_spec.rb b/spec/unit/indirector/request_spec.rb index 87b9af438..152fe5871 100755 --- a/spec/unit/indirector/request_spec.rb +++ b/spec/unit/indirector/request_spec.rb @@ -301,4 +301,103 @@ describe Puppet::Indirector::Request do lambda { @request.query_string }.should raise_error(ArgumentError) end end + + describe "when converting to json" do + before do + @request = Puppet::Indirector::Request.new(:facts, :find, "foo") + end + + it "should produce a hash with the document_type set to 'request'" do + PSON.parse(@request.to_pson)["document_type"].should == "Puppet::Indirector::Request" + end + + it "should add its data under the 'data' attribute in the hash" do + PSON.parse(@request.to_pson)["data"].should be_instance_of(Hash) + end + + it "should set the 'key'" do + PSON.parse(@request.to_pson)["data"]['key'].should == "foo" + end + + it "should include an attribute for its indirection name" do + PSON.parse(@request.to_pson)["data"]['type'].should == "facts" + end + + it "should include a 'method' attribute set to its method" do + PSON.parse(@request.to_pson)["data"]['method'].should == "find" + end + + it "should add all attributes under the 'attributes' attribute" do + @request.ip = "127.0.0.1" + PSON.parse(@request.to_pson)["data"]['attributes']['ip'].should == "127.0.0.1" + end + + it "should add all options under the 'attributes' attribute" do + @request.options["opt"] = "value" + PSON.parse(@request.to_pson)["data"]['attributes']['opt'].should == "value" + end + + it "should include the instance if provided" do + facts = Puppet::Node::Facts.new("foo") + @request.instance = facts + PSON.parse(@request.to_pson)["data"]['instance'].should be_instance_of(Puppet::Node::Facts) + end + end + + describe "when converting from json" do + before do + @request = Puppet::Indirector::Request.new(:facts, :find, "foo") + @klass = Puppet::Indirector::Request + @format = Puppet::Network::FormatHandler.format('pson') + end + + def from_json(json) + @format.intern(Puppet::Indirector::Request, json) + end + + it "should set the 'key'" do + from_json(@request.to_pson).key.should == "foo" + end + + it "should fail if no key is provided" do + json = PSON.parse(@request.to_pson) + json['data'].delete("key") + lambda { from_json(json.to_pson) }.should raise_error(ArgumentError) + end + + it "should set its indirector name" do + from_json(@request.to_pson).indirection_name.should == :facts + end + + it "should fail if no type is provided" do + json = PSON.parse(@request.to_pson) + json['data'].delete("type") + lambda { from_json(json.to_pson) }.should raise_error(ArgumentError) + end + + it "should set its method" do + from_json(@request.to_pson).method.should == "find" + end + + it "should fail if no method is provided" do + json = PSON.parse(@request.to_pson) + json['data'].delete("method") + lambda { from_json(json.to_pson) }.should raise_error(ArgumentError) + end + + it "should initialize with all attributes and options" do + @request.ip = "127.0.0.1" + @request.options["opt"] = "value" + result = from_json(@request.to_pson) + result.options[:opt].should == "value" + result.ip.should == "127.0.0.1" + end + + it "should set its instance as an instance if one is provided" do + facts = Puppet::Node::Facts.new("foo") + @request.instance = facts + result = from_json(@request.to_pson) + result.instance.should be_instance_of(Puppet::Node::Facts) + end + end end -- cgit From f4acb025f3a125d4c3c359fb6896ac20b36e06ab Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Tue, 12 Apr 2011 21:41:06 -0700 Subject: Adding json support to Puppet::Node Reviewed-by: Daniel Pittman Reviewed-by: Nick Lewis Signed-off-by: Luke Kanies --- lib/puppet/node.rb | 23 ++++++++++++++++++ spec/unit/node_spec.rb | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/lib/puppet/node.rb b/lib/puppet/node.rb index 5b0a98615..4bd4d1de6 100644 --- a/lib/puppet/node.rb +++ b/lib/puppet/node.rb @@ -20,6 +20,29 @@ class Puppet::Node attr_accessor :name, :classes, :source, :ipaddress, :parameters attr_reader :time + def self.from_pson(pson) + raise ArgumentError, "No name provided in pson data" unless name = pson['name'] + + node = new(name) + node.classes = pson['classes'] + node.parameters = pson['parameters'] + node.environment = pson['environment'] + node + end + + def to_pson(*args) + result = { + 'document_type' => "Puppet::Node", + 'data' => {} + } + result['data']['name'] = name + result['data']['classes'] = classes unless classes.empty? + result['data']['parameters'] = parameters unless parameters.empty? + result['data']['environment'] = environment.name + + result.to_pson(*args) + end + def environment return super if @environment diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb index c15093d90..ae71046f0 100755 --- a/spec/unit/node_spec.rb +++ b/spec/unit/node_spec.rb @@ -36,6 +36,69 @@ describe Puppet::Node do node.environment.name.should == :bar end end + + describe "when converting to json" do + before do + @node = Puppet::Node.new("mynode") + end + + it "should provide its name" do + PSON.parse(@node.to_pson)['data']['name'].should == "mynode" + end + + it "should include the classes if set" do + @node.classes = %w{a b c} + PSON.parse(@node.to_pson)['data']['classes'].should == %w{a b c} + end + + it "should not include the classes if there are none" do + PSON.parse(@node.to_pson)['data'].should_not be_include('classes') + end + + it "should include parameters if set" do + @node.parameters = {"a" => "b", "c" => "d"} + PSON.parse(@node.to_pson)['data']['parameters'].should == {"a" => "b", "c" => "d"} + end + + it "should not include the parameters if there are none" do + PSON.parse(@node.to_pson)['data'].should_not be_include('parameters') + end + + it "should include the environment" do + @node.environment = "production" + PSON.parse(@node.to_pson)['data']['environment'].should == "production" + end + end + + describe "when converting from json" do + before do + @node = Puppet::Node.new("mynode") + @format = Puppet::Network::FormatHandler.format('pson') + end + + def from_json(json) + @format.intern(Puppet::Node, json) + end + + it "should set its name" do + from_json(@node.to_pson).name.should == "mynode" + end + + it "should include the classes if set" do + @node.classes = %w{a b c} + from_json(@node.to_pson).classes.should == %w{a b c} + end + + it "should include parameters if set" do + @node.parameters = {"a" => "b", "c" => "d"} + from_json(@node.to_pson).parameters.should == {"a" => "b", "c" => "d"} + end + + it "should include the environment" do + @node.environment = "production" + from_json(@node.to_pson).environment.name.should == :production + end + end end describe Puppet::Node, "when initializing" do -- cgit From 7e5ca648112a2703ba827f96f36fe2a5f7d1c751 Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Thu, 7 Jul 2011 00:03:51 -0700 Subject: Making Fact json handling more resilient We were failing if any values were nil, and values were often nil, at least in testing. We now only include non-nil values, and we handle nil values just fine. Signed-off-by: Luke Kanies Reviewed-by: Nick Lewis --- lib/puppet/node/facts.rb | 15 +++++++++------ spec/unit/node/facts_spec.rb | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/puppet/node/facts.rb b/lib/puppet/node/facts.rb index 577b62b62..8d0a03474 100755 --- a/lib/puppet/node/facts.rb +++ b/lib/puppet/node/facts.rb @@ -61,18 +61,21 @@ class Puppet::Node::Facts def self.from_pson(data) result = new(data['name'], data['values']) - result.timestamp = Time.parse(data['timestamp']) - result.expiration = Time.parse(data['expiration']) + result.timestamp = Time.parse(data['timestamp']) if data['timestamp'] + result.expiration = Time.parse(data['expiration']) if data['expiration'] result end def to_pson(*args) - { - 'expiration' => expiration, + result = { 'name' => name, - 'timestamp' => timestamp, 'values' => strip_internal, - }.to_pson(*args) + } + + result['timestamp'] = timestamp if timestamp + result['expiration'] = expiration if expiration + + result.to_pson(*args) end # Add internal data to the facts for storage. diff --git a/spec/unit/node/facts_spec.rb b/spec/unit/node/facts_spec.rb index efaa76e12..6d2b0a7ac 100755 --- a/spec/unit/node/facts_spec.rb +++ b/spec/unit/node/facts_spec.rb @@ -128,6 +128,20 @@ describe Puppet::Node::Facts, "when indirecting" do result['timestamp'].should == facts.timestamp.to_s result['expiration'].should == facts.expiration.to_s end + + it "should not include nil values" do + facts = Puppet::Node::Facts.new("foo", {'a' => 1, 'b' => 2, 'c' => 3}) + pson = PSON.parse(facts.to_pson) + pson.should_not be_include("expiration") + end + + it "should be able to handle nil values" do + pson = %Q({"name": "foo", "values": {"a": "1", "b": "2", "c": "3"}}) + format = Puppet::Network::FormatHandler.format('pson') + facts = format.intern(Puppet::Node::Facts,pson) + facts.name.should == 'foo' + facts.expiration.should be_nil + end end end end -- cgit From 8c4cc7cae65b43fc4ab2f56ec27ee05a9bfc0494 Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Thu, 7 Jul 2011 00:05:09 -0700 Subject: Switching to use of json matchers This is only in the Node and Indirection::Request code. It makes the tests much simpler. Signed-off-by: Luke Kanies Reviewed-by: Nick Lewis --- spec/unit/indirector/request_spec.rb | 16 ++++++---------- spec/unit/node_spec.rb | 20 ++++++++++---------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/spec/unit/indirector/request_spec.rb b/spec/unit/indirector/request_spec.rb index 152fe5871..450cad2ed 100755 --- a/spec/unit/indirector/request_spec.rb +++ b/spec/unit/indirector/request_spec.rb @@ -308,28 +308,24 @@ describe Puppet::Indirector::Request do end it "should produce a hash with the document_type set to 'request'" do - PSON.parse(@request.to_pson)["document_type"].should == "Puppet::Indirector::Request" - end - - it "should add its data under the 'data' attribute in the hash" do - PSON.parse(@request.to_pson)["data"].should be_instance_of(Hash) + @request.should set_json_document_type_to("Puppet::Indirector::Request") end it "should set the 'key'" do - PSON.parse(@request.to_pson)["data"]['key'].should == "foo" + @request.should set_json_attribute("key").to("foo") end it "should include an attribute for its indirection name" do - PSON.parse(@request.to_pson)["data"]['type'].should == "facts" + @request.should set_json_attribute("type").to("facts") end it "should include a 'method' attribute set to its method" do - PSON.parse(@request.to_pson)["data"]['method'].should == "find" + @request.should set_json_attribute("method").to("find") end it "should add all attributes under the 'attributes' attribute" do @request.ip = "127.0.0.1" - PSON.parse(@request.to_pson)["data"]['attributes']['ip'].should == "127.0.0.1" + @request.should set_json_attribute("attributes", "ip").to("127.0.0.1") end it "should add all options under the 'attributes' attribute" do @@ -340,7 +336,7 @@ describe Puppet::Indirector::Request do it "should include the instance if provided" do facts = Puppet::Node::Facts.new("foo") @request.instance = facts - PSON.parse(@request.to_pson)["data"]['instance'].should be_instance_of(Puppet::Node::Facts) + PSON.parse(@request.to_pson)["data"]['instance'].should be_instance_of(Hash) end end diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb index ae71046f0..a6ae67aeb 100755 --- a/spec/unit/node_spec.rb +++ b/spec/unit/node_spec.rb @@ -43,30 +43,30 @@ describe Puppet::Node do end it "should provide its name" do - PSON.parse(@node.to_pson)['data']['name'].should == "mynode" + @node.should set_json_attribute('name').to("mynode") end it "should include the classes if set" do @node.classes = %w{a b c} - PSON.parse(@node.to_pson)['data']['classes'].should == %w{a b c} + @node.should set_json_attribute("classes").to(%w{a b c}) end it "should not include the classes if there are none" do - PSON.parse(@node.to_pson)['data'].should_not be_include('classes') + @node.should_not set_json_attribute('classes') end it "should include parameters if set" do @node.parameters = {"a" => "b", "c" => "d"} - PSON.parse(@node.to_pson)['data']['parameters'].should == {"a" => "b", "c" => "d"} + @node.should set_json_attribute('parameters').to({"a" => "b", "c" => "d"}) end it "should not include the parameters if there are none" do - PSON.parse(@node.to_pson)['data'].should_not be_include('parameters') + @node.should_not set_json_attribute('parameters') end it "should include the environment" do @node.environment = "production" - PSON.parse(@node.to_pson)['data']['environment'].should == "production" + @node.should set_json_attribute('environment').to('production') end end @@ -81,22 +81,22 @@ describe Puppet::Node do end it "should set its name" do - from_json(@node.to_pson).name.should == "mynode" + Puppet::Node.should read_json_attribute('name').from(@node.to_pson).as("mynode") end it "should include the classes if set" do @node.classes = %w{a b c} - from_json(@node.to_pson).classes.should == %w{a b c} + Puppet::Node.should read_json_attribute('classes').from(@node.to_pson).as(%w{a b c}) end it "should include parameters if set" do @node.parameters = {"a" => "b", "c" => "d"} - from_json(@node.to_pson).parameters.should == {"a" => "b", "c" => "d"} + Puppet::Node.should read_json_attribute('parameters').from(@node.to_pson).as({"a" => "b", "c" => "d"}) end it "should include the environment" do @node.environment = "production" - from_json(@node.to_pson).environment.name.should == :production + Puppet::Node.should read_json_attribute('environment').from(@node.to_pson).as(Puppet::Node::Environment.new(:production)) end end end -- cgit From 361220166525762634dd1886f84c9a928b28766b Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Fri, 15 Jul 2011 11:05:50 -0700 Subject: (#7080) Registering PSON document types We were originally using the actual constant for both Indirector Requests and Nodes, and this registers their document types as symbolic names. Signed-off-by: Luke Kanies Reviewed-by: Nick Lewis --- lib/puppet/indirector/request.rb | 6 +++++- lib/puppet/node.rb | 5 ++++- spec/unit/indirector/request_spec.rb | 10 +++++++++- spec/unit/node_spec.rb | 8 ++++++++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb index 1918a3fb5..697d9df47 100644 --- a/lib/puppet/indirector/request.rb +++ b/lib/puppet/indirector/request.rb @@ -1,6 +1,7 @@ require 'cgi' require 'uri' require 'puppet/indirector' +require 'puppet/util/pson' # This class encapsulates all of the information you need to make an # Indirection call, and as a a result also handles REST calls. It's somewhat @@ -14,6 +15,9 @@ class Puppet::Indirector::Request OPTION_ATTRIBUTES = [:ip, :node, :authenticated, :ignore_terminus, :ignore_cache, :instance, :environment] + # Load json before trying to register. + Puppet.features.pson? and ::PSON.register_document_type('IndirectorRequest',self) + def self.from_pson(json) raise ArgumentError, "No indirection name provided in json data" unless indirection_name = json['type'] raise ArgumentError, "No method name provided in json data" unless method = json['method'] @@ -35,7 +39,7 @@ class Puppet::Indirector::Request def to_pson(*args) result = { - 'document_type' => 'Puppet::Indirector::Request', + 'document_type' => 'IndirectorRequest', 'data' => { 'type' => indirection_name, 'method' => method, diff --git a/lib/puppet/node.rb b/lib/puppet/node.rb index 4bd4d1de6..16a0e5c3d 100644 --- a/lib/puppet/node.rb +++ b/lib/puppet/node.rb @@ -19,6 +19,9 @@ class Puppet::Node attr_accessor :name, :classes, :source, :ipaddress, :parameters attr_reader :time + # + # Load json before trying to register. + Puppet.features.pson? and ::PSON.register_document_type('Node',self) def self.from_pson(pson) raise ArgumentError, "No name provided in pson data" unless name = pson['name'] @@ -32,7 +35,7 @@ class Puppet::Node def to_pson(*args) result = { - 'document_type' => "Puppet::Node", + 'document_type' => "Node", 'data' => {} } result['data']['name'] = name diff --git a/spec/unit/indirector/request_spec.rb b/spec/unit/indirector/request_spec.rb index 450cad2ed..059357869 100755 --- a/spec/unit/indirector/request_spec.rb +++ b/spec/unit/indirector/request_spec.rb @@ -2,8 +2,16 @@ require 'spec_helper' require 'matchers/json' require 'puppet/indirector/request' +require 'puppet/util/pson' describe Puppet::Indirector::Request do + + describe "when registering the document type" do + it "should register its document type with JSON" do + PSON.registered_document_types["IndirectorRequest"].should equal(Puppet::Indirector::Request) + end + end + describe "when initializing" do it "should require an indirection name, a key, and a method" do lambda { Puppet::Indirector::Request.new }.should raise_error(ArgumentError) @@ -308,7 +316,7 @@ describe Puppet::Indirector::Request do end it "should produce a hash with the document_type set to 'request'" do - @request.should set_json_document_type_to("Puppet::Indirector::Request") + @request.should set_json_document_type_to("IndirectorRequest") end it "should set the 'key'" do diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb index a6ae67aeb..476b0d6e5 100755 --- a/spec/unit/node_spec.rb +++ b/spec/unit/node_spec.rb @@ -3,6 +3,10 @@ require 'spec_helper' require 'matchers/json' describe Puppet::Node do + it "should register its document type as Node" do + PSON.registered_document_types["Node"].should equal(Puppet::Node) + end + describe "when managing its environment" do it "should use any set environment" do Puppet::Node.new("foo", :environment => "bar").environment.name.should == :bar @@ -46,6 +50,10 @@ describe Puppet::Node do @node.should set_json_attribute('name').to("mynode") end + it "should produce a hash with the document_type set to 'Node'" do + @node.should set_json_document_type_to("Node") + end + it "should include the classes if set" do @node.classes = %w{a b c} @node.should set_json_attribute("classes").to(%w{a b c}) -- cgit