From c9d26e5125b0b0c5e58b95612ded2ea1e6b4ce6c Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Sat, 23 Dec 2023 19:48:14 +0100 Subject: [PATCH] Rewrite loadjson to use the modern function API This also resolves a bug where JSON.parse returned a StringIO. To properly catch this, the tests are rewritten to avoid mocking where possible. Exceptions are with external URLs and where a failure is expected. --- lib/puppet/functions/loadjson.rb | 58 ++++++++++ lib/puppet/parser/functions/loadjson.rb | 75 ------------- spec/functions/loadjson_spec.rb | 141 ++++++------------------ 3 files changed, 89 insertions(+), 185 deletions(-) create mode 100644 lib/puppet/functions/loadjson.rb delete mode 100644 lib/puppet/parser/functions/loadjson.rb diff --git a/lib/puppet/functions/loadjson.rb b/lib/puppet/functions/loadjson.rb new file mode 100644 index 000000000..97a2a0d8f --- /dev/null +++ b/lib/puppet/functions/loadjson.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +# @summary +# Load a JSON file containing an array, string, or hash, and return the data +# in the corresponding native data type. +# +# @example Example Usage: +# $myhash = loadjson('/etc/puppet/data/myhash.json') +# $myhash = loadjson('https://example.local/my_hash.json') +# $myhash = loadjson('https://username:password@example.local/my_hash.json') +# $myhash = loadjson('no-file.json', {'default' => 'value'}) +Puppet::Functions.create_function(:loadjson) do + # @param path + # A file path or a URL. + # @param default + # The default value to be returned if the file was not found or could not + # be parsed. + # + # @return + # The data stored in the JSON file, the type depending on the type of data + # that was stored. + dispatch :loadjson do + param 'String[1]', :path + optional_param 'Data', :default + return_type 'Data' + end + + def loadjson(path, default = nil) + if path.start_with?('http://', 'https://') + require 'open-uri' + begin + content = URI.open(path) { |f| load_json_source(f) } + rescue OpenURI::HTTPError => e + Puppet.warn("Can't load '#{url}' HTTP Error Code: '#{e.io.status[0]}'") + return default + end + elsif File.exist?(path) + content = File.open(path) { |f| load_json_source(f) } + else + Puppet.warn("Can't load '#{path}' File does not exist!") + return default + end + + content || default + rescue StandardError => e + raise e unless default + + default + end + + def load_json_source(source) + if Puppet::Util::Package.versioncmp(Puppet.version, '8.0.0').negative? + PSON.load(source) + else + JSON.load(source) + end + end +end diff --git a/lib/puppet/parser/functions/loadjson.rb b/lib/puppet/parser/functions/loadjson.rb deleted file mode 100644 index a85cd18ee..000000000 --- a/lib/puppet/parser/functions/loadjson.rb +++ /dev/null @@ -1,75 +0,0 @@ -# frozen_string_literal: true - -# -# loadjson.rb -# - -module Puppet::Parser::Functions - newfunction(:loadjson, type: :rvalue, arity: -2, doc: <<-DOC) do |args| - @summary - Load a JSON file containing an array, string, or hash, and return the data - in the corresponding native data type. - - The first parameter can be a file path or a URL. - The second parameter is the default value. It will be returned if the file - was not found or could not be parsed. - - @return [Array|String|Hash] - The data stored in the JSON file, the type depending on the type of data that was stored. - - @example Example Usage: - $myhash = loadjson('/etc/puppet/data/myhash.json') - $myhash = loadjson('https://example.local/my_hash.json') - $myhash = loadjson('https://username:password@example.local/my_hash.json') - $myhash = loadjson('no-file.json', {'default' => 'value'}) - DOC - - raise ArgumentError, 'Wrong number of arguments. 1 or 2 arguments should be provided.' unless args.length >= 1 - - require 'open-uri' - begin - if args[0].start_with?('http://', 'https://') - http_options = {} - if (match = args[0].match(%r{(http://|https://)(.*):(.*)@(.*)})) - # If URL is in the format of https://username:password@example.local/my_hash.yaml - protocol, username, password, path = match.captures - url = "#{protocol}#{path}" - http_options[:http_basic_authentication] = [username, password] - elsif (match = args[0].match(%r{(http://|https://)(.*)@(.*)})) - # If URL is in the format of https://username@example.local/my_hash.yaml - protocol, username, path = match.captures - url = "#{protocol}#{path}" - http_options[:http_basic_authentication] = [username, ''] - else - url = args[0] - end - begin - contents = OpenURI.open_uri(url, http_options) - rescue OpenURI::HTTPError => e - res = e.io - warning("Can't load '#{url}' HTTP Error Code: '#{res.status[0]}'") - args[1] - end - if Puppet::Util::Package.versioncmp(Puppet.version, '8.0.0').negative? - PSON.load(contents) || args[1] - else - JSON.parse(contents) || args[1] - end - elsif File.exist?(args[0]) - content = File.read(args[0]) - if Puppet::Util::Package.versioncmp(Puppet.version, '8.0.0').negative? - PSON.load(content) || args[1] - else - JSON.parse(content) || args[1] - end - else - warning("Can't load '#{args[0]}' File does not exist!") - args[1] - end - rescue StandardError => e - raise e unless args[1] - - args[1] - end - end -end diff --git a/spec/functions/loadjson_spec.rb b/spec/functions/loadjson_spec.rb index 17d01e451..41c96f1ae 100644 --- a/spec/functions/loadjson_spec.rb +++ b/spec/functions/loadjson_spec.rb @@ -1,33 +1,22 @@ # frozen_string_literal: true require 'spec_helper' +require 'open-uri' describe 'loadjson' do it { is_expected.not_to be_nil } - it { is_expected.to run.with_params.and_raise_error(ArgumentError, %r{wrong number of arguments}i) } + it { is_expected.to run.with_params.and_raise_error(ArgumentError, "'loadjson' expects between 1 and 2 arguments, got none") } describe 'when calling with valid arguments' do - before :each do - # In Puppet 7, there are two prior calls to File.read prior to the responses we want to mock - allow(File).to receive(:read).with(anything, anything).and_call_original - allow(File).to receive(:read).with(%r{/(stdlib|test)/metadata.json}, encoding: 'utf-8').and_return('{"name": "puppetlabs-stdlib"}') - allow(File).to receive(:read).with(%r{/(stdlib|test)/metadata.json}).and_return('{"name": "puppetlabs-stdlib"}') - # Additional modules used by litmus which are identified while running these dues to being in fixtures - allow(File).to receive(:read).with(%r{/(provision|puppet_agent|facts)/metadata.json}, encoding: 'utf-8') - end - context 'when a non-existing file is specified' do let(:filename) do - if Puppet::Util::Platform.windows? - 'C:/tmp/doesnotexist' - else - '/tmp/doesnotexist' - end + file = Tempfile.create + file.close + File.unlink(file.path) + file.path end before(:each) do - allow(File).to receive(:exist?).and_call_original - allow(File).to receive(:exist?).with(filename).and_return(false).once if Puppet::PUPPETVERSION[0].to_i < 8 allow(PSON).to receive(:load).never # rubocop:disable RSpec/ReceiveNever Switching to not_to receive breaks testing in this case else @@ -35,144 +24,76 @@ end end - it { is_expected.to run.with_params(filename, 'default' => 'value').and_return('default' => 'value') } - it { is_expected.to run.with_params(filename, 'đẽƒằưļŧ' => '٧ẵłựέ').and_return('đẽƒằưļŧ' => '٧ẵłựέ') } - it { is_expected.to run.with_params(filename, 'デフォルト' => '値').and_return('デフォルト' => '値') } + it { is_expected.to run.with_params(filename, {'default' => 'value'}).and_return({'default' => 'value'}) } + it { is_expected.to run.with_params(filename, {'đẽƒằưļŧ' => '٧ẵłựέ'}).and_return({'đẽƒằưļŧ' => '٧ẵłựέ'}) } + it { is_expected.to run.with_params(filename, {'デフォルト' => '値'}).and_return({'デフォルト' => '値'}) } end context 'when an existing file is specified' do - let(:filename) do - if Puppet::Util::Platform.windows? - 'C:/tmp/doesexist' - else - '/tmp/doesexist' - end - end let(:data) { { 'key' => 'value', 'ķęŷ' => 'νậŀųề', 'キー' => '値' } } let(:json) { '{"key":"value", {"ķęŷ":"νậŀųề" }, {"キー":"値" }' } - before(:each) do - allow(File).to receive(:exist?).and_call_original - allow(File).to receive(:exist?).with(filename).and_return(true).once - allow(File).to receive(:read).with(filename).and_return(json).once - allow(File).to receive(:read).with(filename).and_return(json).once - if Puppet::PUPPETVERSION[0].to_i < 8 - allow(PSON).to receive(:load).with(json).and_return(data).once - else - allow(JSON).to receive(:parse).with(json).and_return(data).once + it do + Tempfile.new do |file| + file.write(json) + file.flush + + is_expected.to run.with_params(file.path).and_return(data) end end - - it { is_expected.to run.with_params(filename).and_return(data) } end context 'when the file could not be parsed' do - let(:filename) do - if Puppet::Util::Platform.windows? - 'C:/tmp/doesexist' - else - '/tmp/doesexist' - end - end let(:json) { '{"key":"value"}' } - before(:each) do - allow(File).to receive(:exist?).and_call_original - allow(File).to receive(:exist?).with(filename).and_return(true).once - allow(File).to receive(:read).with(filename).and_return(json).once - if Puppet::PUPPETVERSION[0].to_i < 8 - allow(PSON).to receive(:load).with(json).once.and_raise StandardError, 'Something terrible have happened!' - else - allow(JSON).to receive(:parse).with(json).once.and_raise StandardError, 'Something terrible have happened!' - end - end - - it { is_expected.to run.with_params(filename, 'default' => 'value').and_return('default' => 'value') } - end - - context 'when an existing URL is specified' do - let(:filename) do - 'https://example.local/myhash.json' - end - let(:data) { { 'key' => 'value', 'ķęŷ' => 'νậŀųề', 'キー' => '値' } } - let(:json) { '{"key":"value", {"ķęŷ":"νậŀųề" }, {"キー":"値" }' } + it do + Tempfile.new do |file| + file.write(json) + file.flush - it { - expect(OpenURI).to receive(:open_uri).with(filename, {}).and_return(json) - if Puppet::PUPPETVERSION[0].to_i < 8 - expect(PSON).to receive(:load).with(json).and_return(data).once - else - expect(JSON).to receive(:parse).with(json).and_return(data).once + is_expected.to run.with_params(filename, {'default' => 'value'}).and_return({'default' => 'value'}) end - expect(subject).to run.with_params(filename).and_return(data) - } - end - - context 'when an existing URL (with username and password) is specified' do - let(:filename) do - 'https://user1:pass1@example.local/myhash.json' end - let(:url_no_auth) { 'https://example.local/myhash.json' } - let(:basic_auth) { { http_basic_authentication: ['user1', 'pass1'] } } - let(:data) { { 'key' => 'value', 'ķęŷ' => 'νậŀųề', 'キー' => '値' } } - let(:json) { '{"key":"value", {"ķęŷ":"νậŀųề" }, {"キー":"値" }' } - - it { - expect(OpenURI).to receive(:open_uri).with(url_no_auth, basic_auth).and_return(json) - if Puppet::PUPPETVERSION[0].to_i < 8 - expect(PSON).to receive(:load).with(json).and_return(data).once - else - expect(JSON).to receive(:parse).with(json).and_return(data).once - end - expect(subject).to run.with_params(filename).and_return(data) - } end - context 'when an existing URL (with username) is specified' do + context 'when an existing URL is specified' do let(:filename) do - 'https://user1@example.local/myhash.json' + 'https://example.com/myhash.json' end - let(:url_no_auth) { 'https://example.local/myhash.json' } - let(:basic_auth) { { http_basic_authentication: ['user1', ''] } } let(:data) { { 'key' => 'value', 'ķęŷ' => 'νậŀųề', 'キー' => '値' } } - let(:json) { '{"key":"value", {"ķęŷ":"νậŀųề" }, {"キー":"値" }' } + let(:json) { '{"key":"value", "ķęŷ":"νậŀųề", "キー":"値" }' } it { - expect(OpenURI).to receive(:open_uri).with(url_no_auth, basic_auth).and_return(json) - if Puppet::PUPPETVERSION[0].to_i < 8 - expect(PSON).to receive(:load).with(json).and_return(data).once - else - expect(JSON).to receive(:parse).with(json).and_return(data).once - end + expect(URI).to receive(:open).with(filename).and_yield(json) expect(subject).to run.with_params(filename).and_return(data) } end context 'when the URL output could not be parsed, with default specified' do let(:filename) do - 'https://example.local/myhash.json' + 'https://example.com/myhash.json' end let(:json) { ',;{"key":"value"}' } it { - expect(OpenURI).to receive(:open_uri).with(filename, {}).and_return(json) + expect(URI).to receive(:open).with(filename).and_yield(json) if Puppet::PUPPETVERSION[0].to_i < 8 expect(PSON).to receive(:load).with(json).once.and_raise StandardError, 'Something terrible have happened!' else - expect(JSON).to receive(:parse).with(json).once.and_raise StandardError, 'Something terrible have happened!' + expect(JSON).to receive(:load).with(json).once.and_raise StandardError, 'Something terrible have happened!' end - expect(subject).to run.with_params(filename, 'default' => 'value').and_return('default' => 'value') + expect(subject).to run.with_params(filename, {'default' => 'value'}).and_return({'default' => 'value'}) } end context 'when the URL does not exist, with default specified' do let(:filename) do - 'https://example.local/myhash.json' + 'https://example.com/myhash.json' end it { - expect(OpenURI).to receive(:open_uri).with(filename, {}).and_raise OpenURI::HTTPError, '404 File not Found' - expect(subject).to run.with_params(filename, 'default' => 'value').and_return('default' => 'value') + expect(URI).to receive(:open).with(filename).and_raise OpenURI::HTTPError, '404 File not Found' + expect(subject).to run.with_params(filename, {'default' => 'value'}).and_return({'default' => 'value'}) } end end