Stop accepting encoded JSON string

... to avoid redundant parsing of json content from a given string.

This also allows us to add more strict validation later using Struct
data type.

Change-Id: I2a8bbd266d5e97a06b89c131bbd86e3710a68923

lib/puppet/functions/to_array_of_json_strings.rb

@@ -10,20 +10,10 @@ Puppet::Functions.create_function(:to_array_of_json_strings) do
   end
 
   def to_array_of_json_strings(*args)
-    require 'json'
-
     if (args.size != 1) then
       raise Puppet::ParseError, 'to_array_of_json_strings(): Wrong number of arguments'
     end
     list = args[0]
-    if list.class == String
-      begin
-        list = JSON.load(list)
-      rescue JSON::ParserError
-        raise Puppet::ParseError, "Syntax error: #{args[0]} is not valid"
-      end
-      list = [list] unless list.class == Array
-    end
     unless _array_of_hash?(list)
       raise Puppet::ParseError, "Syntax error: #{args[0]} is not an Array or JSON encoded String"
     end

manifests/compute/pci.pp

@@ -6,7 +6,7 @@
 #
 #  [*device_specs*]
 #   (optional) Specify the PCI devices available to VMs.
-#   Defaults to $facts['os_service_default']
+#   Defaults to []
 #   Example of format:
 #   [ { "vendor_id" => "1234","product_id" => "5678" },
 #     { "vendor_id" => "4321","product_id" => "8765", "physical_network" => "default" } ]
@@ -22,22 +22,22 @@
 #   Defaults to undef
 #
 class nova::compute::pci(
-  $device_specs         = $facts['os_service_default'],
+  Array[Hash] $device_specs          = [],
-  $report_in_placement  = $facts['os_service_default'],
+  $report_in_placement               = $facts['os_service_default'],
   # DEPRECATED PARAMETERS
-  $passthrough          = undef,
+  Optional[Array[Hash]] $passthrough = undef,
 ) {
   include nova::deps
 
   if $passthrough != undef {
     warning('The passthrough parameter is deprecated. Use the device_specs parameter.')
-    if empty($passthrough) or is_service_default($passthrough) {
+    if empty($passthrough) {
       $device_specs_real = $facts['os_service_default']
     } else {
       $device_specs_real = to_array_of_json_strings($passthrough)
     }
   } else {
-    if empty($device_specs) or is_service_default($device_specs) {
+    if empty($device_specs) {
       $device_specs_real = $facts['os_service_default']
     } else {
       $device_specs_real = to_array_of_json_strings($device_specs)

manifests/pci.pp

@@ -6,20 +6,20 @@
 #
 #  [*aliases*]
 #   (optional) A list of pci alias hashes
-#   Defaults to $facts['os_service_default']
+#   Defaults to []
 #   Example:
 #   [{"vendor_id" => "1234", "product_id" => "5678", "name" => "default"},
 #    {"vendor_id" => "1234", "product_id" => "6789", "name" => "other"}]
 
 class nova::pci(
-  $aliases = $facts['os_service_default']
+  Array[Hash] $aliases = []
 ) {
   include nova::deps
 
-  if !is_service_default($aliases) and !empty($aliases) {
+  if empty($aliases) {
-    $aliases_real = to_array_of_json_strings($aliases)
-  } else {
     $aliases_real = $facts['os_service_default']
+  } else {
+    $aliases_real = to_array_of_json_strings($aliases)
   }
   nova_config {
     'pci/alias': value => $aliases_real;

releasenotes/notes/require-hash-instead-of-encoded-json-string-3c016bb1968c2aca.yaml

@@ -0,0 +1,8 @@
+---
+upgrade:
+  - |
+    The following parameters no longer accept string values. Use arrays of
+    hashes instead.
+
+    - ``nova::compute::pci::passthrough``
+    - ``nova::pci::aliases``

spec/classes/nova_compute_pci_spec.rb

@@ -45,20 +45,6 @@ describe 'nova::compute::pci' do
       end
     end
 
-    context 'with device_specs JSON encoded string' do
-      let :params do
-        {
-          :device_specs => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"physical_network\":\"physnet1\"}]",
-        }
-      end
-
-      it 'configures nova pci device_spec entries' do
-        is_expected.to contain_nova_config('pci/device_spec').with(
-          'value' => ['{"vendor_id":"8086","product_id":"0126"}','{"vendor_id":"9096","product_id":"1520","physical_network":"physnet1"}']
-        )
-      end
-    end
-
     context 'when device_specs is empty' do
       let :params do
         {
@@ -70,18 +56,6 @@ describe 'nova::compute::pci' do
         is_expected.to contain_nova_config('pci/device_spec').with(:value => '<SERVICE DEFAULT>')
       end
     end
-
-    context 'when device_specs is empty string' do
-      let :params do
-        {
-          :device_specs => ""
-        }
-      end
-
-      it 'clears pci device_spec configuration' do
-        is_expected.to contain_nova_config('pci/device_spec').with(:value => '<SERVICE DEFAULT>')
-      end
-    end
   end
 
   on_supported_os({

spec/classes/nova_pci_spec.rb

@@ -32,19 +32,6 @@ describe 'nova::pci' do
       end
     end
 
-    context 'with aliases JSON encoded string' do
-      let :params do
-        {
-          :aliases => "[{\"vendor_id\":\"8086\",\"product_id\":\"0126\",\"name\":\"graphic_card\"},{\"vendor_id\":\"9096\",\"product_id\":\"1520\",\"name\":\"network_card\"}]",
-        }
-      end
-      it 'configures nova pci_alias entries' do
-        is_expected.to contain_nova_config('pci/alias').with(
-          'value' => ['{"vendor_id":"8086","product_id":"0126","name":"graphic_card"}','{"vendor_id":"9096","product_id":"1520","name":"network_card"}']
-        )
-      end
-    end
-
     context 'when aliases is empty' do
       let :params do
         {
@@ -56,18 +43,6 @@ describe 'nova::pci' do
         is_expected.to contain_nova_config('pci/alias').with(:value => '<SERVICE DEFAULT>')
       end
     end
-
-    context 'when aliases is empty string' do
-      let :params do
-        {
-          :aliases => ""
-        }
-      end
-
-      it 'clears pci_alias configuration' do
-        is_expected.to contain_nova_config('pci/alias').with(:value => '<SERVICE DEFAULT>')
-      end
-    end
   end
 
   on_supported_os({

spec/functions/to_array_of_json_strings_spec.rb

@@ -13,33 +13,23 @@ describe 'to_array_of_json_strings' do
     is_expected.to run.with_params('arg1', 'arg2').and_raise_error(Puppet::ParseError)
   end
 
-  it 'fails with invalid json string' do
+  it 'fails with a formatted json string' do
-    data = 'invalid json'
-    is_expected.to run.with_params(data).and_raise_error(Puppet::ParseError)
-  end
-
-  it 'fails with array of json string' do
-    data = ['{"valid": "json", "syntax": "here"}', '{"some": "data"}']
-    is_expected.to run.with_params(data).and_raise_error(Puppet::ParseError)
-  end
-
-  it 'works with valid json string' do
     data = '{"valid": "json", "syntax": "here"}'
-    retval = ['{"valid":"json","syntax":"here"}']
+    is_expected.to run.with_params(data).and_raise_error(Puppet::ParseError)
-    is_expected.to run.with_params(data).and_return(retval)
   end
 
-  it 'fails unless its an array or string' do
+  it 'fails with a hash' do
-    is_expected.to run.with_params(1234).and_raise_error(Puppet::ParseError)
+    data = {:some => "entry"}
+    is_expected.to run.with_params(data).and_raise_error(Puppet::ParseError)
   end
 
-  it 'fails unless array doesnt have hashes' do
+  it 'fails unless array does not have hashes' do
-    data = [12, 23]
+    data = ['{"valid": "json", "syntax": "here"}']
     is_expected.to run.with_params(data).and_raise_error(Puppet::ParseError)
   end
 
   it 'fails if array but only some entries are valid' do
-    data = [{:some => "entry"}, 23]
+    data = [{:some => "entry"}, '{"valid": "json", "syntax": "here"}']
     is_expected.to run.with_params(data).and_raise_error(Puppet::ParseError)
   end