From 99d5d8c336c440b7cbf3866aa535ef0860495b76 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 13 Mar 2024 14:45:15 -0700 Subject: [PATCH] (PUP-12026) Change non-literal type check to deprecation warning Previously, the ILLEGAL_NONLITERAL_PARAMETER_TYPE issue was under control of the `strict` setting. Since `strict` defaults to `error`, `puppet parser validate` logged an error message and exited non-zero for non-literal types such as `Integer[0, 60*24]` In order to gradually phase out non-literal parameter types, this commit changes the issue to be a deprecation. When parsing and validating puppet code a warning is now logged, regardless of `strict`, and it exits with 0: Warning: The parameter '$i' must be a literal type, not a ... (file: /etc/puppetlabs/code/environments/production/manifests/site.pp, line: 2, column: 19) As a result of this change, the ClassInfoService error does not include locator information in the error hash: {"error":"The parmeter '$i' is invalid: The expression <60*24> is not a valid type specification"} But the location information is included in the deprecation warning logged on the server. All language deprecation warnings can already be disabled via the `disable_warnings=deprecations` setting. In a future major release, the non-literal type issue will be converted to an error. This pattern follows how we handled the most recent language checks: ILLEGAL_DEFINITION_LOCATION: see 6e7835b0ebea, 44c4ea06f5c5 and 81e61de735d9 ILLEGAL_TOP_CONSTRUCT_LOCATION: see 56615099b0d8 and 81e61de735d9 (cherry picked from commit 86a8c1473bebb4b76546b546f8565838cde4f8eb) --- .../info_service/class_information_service.rb | 9 +++--- .../pops/validation/validator_factory_4_0.rb | 2 +- spec/lib/puppet_spec/matchers.rb | 6 ++++ spec/unit/info_service_spec.rb | 11 ++++--- .../pops/evaluator/literal_evaluator_spec.rb | 4 +-- spec/unit/pops/validator/validator_spec.rb | 29 +++++-------------- 6 files changed, 28 insertions(+), 33 deletions(-) diff --git a/lib/puppet/info_service/class_information_service.rb b/lib/puppet/info_service/class_information_service.rb index dd3b33df21e..97f3087840c 100644 --- a/lib/puppet/info_service/class_information_service.rb +++ b/lib/puppet/info_service/class_information_service.rb @@ -74,7 +74,7 @@ def extract_param(p) def extract_type(structure, p) return structure if p.type_expr.nil? - structure[:type] = typeexpr_to_string(p.type_expr) + structure[:type] = typeexpr_to_string(p.name, p.type_expr) structure end @@ -87,12 +87,11 @@ def extract_default(structure, p) structure end - def typeexpr_to_string(type_expr) + def typeexpr_to_string(name, type_expr) begin type_parser.interpret_any(type_expr, nil).to_s - rescue Puppet::ParseError - # type is to complex - contains expressions that are not literal - nil + rescue Puppet::ParseError => e + raise Puppet::Error, "The parameter '$#{name}' is invalid: #{e.message}", e.backtrace end end diff --git a/lib/puppet/pops/validation/validator_factory_4_0.rb b/lib/puppet/pops/validation/validator_factory_4_0.rb index 2516dc723a9..14e060ed093 100644 --- a/lib/puppet/pops/validation/validator_factory_4_0.rb +++ b/lib/puppet/pops/validation/validator_factory_4_0.rb @@ -37,7 +37,7 @@ def severity_producer p[Issues::EMPTY_RESOURCE_SPECIALIZATION] = :ignore p[Issues::CLASS_NOT_VIRTUALIZABLE] = :error - p[Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE] = Puppet[:strict] == :off ? :ignore : Puppet[:strict] + p[Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE] = :deprecation p end end diff --git a/spec/lib/puppet_spec/matchers.rb b/spec/lib/puppet_spec/matchers.rb index eaa7da8e9af..6a01a746ca4 100644 --- a/spec/lib/puppet_spec/matchers.rb +++ b/spec/lib/puppet_spec/matchers.rb @@ -14,6 +14,12 @@ end end +RSpec::Matchers.define :have_matching_log_with_source do |expected, file, line, pos| + match do |actual| + actual.any? { |item| item.message =~ expected && item.file == file && item.line == line && item.pos == pos } + end +end + RSpec::Matchers.define :exit_with do |expected| actual = nil match do |block| diff --git a/spec/unit/info_service_spec.rb b/spec/unit/info_service_spec.rb index 0c1d27618e6..6858d6e74f5 100644 --- a/spec/unit/info_service_spec.rb +++ b/spec/unit/info_service_spec.rb @@ -518,20 +518,23 @@ class oops_2(Optional[[String]] $double_brackets) { } it "warns with a descriptive message if non-literal class parameter is given" do files = ['non_literal.pp', 'non_literal_2.pp'].map {|f| File.join(code_dir, f) } Puppet::InfoService.classes_per_environment({'production' => files }) - expect(@logs).to include(an_object_having_attributes(message: "The parameter '$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression")) - expect(@logs).to include(an_object_having_attributes(message: "The parameter '$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression")) + expect(@logs).to have_matching_log_with_source(/The parameter '\$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal.pp", 1, 37) + expect(@logs).to have_matching_log_with_source(/The parameter '\$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal_2.pp", 1, 44) end it "errors in strict mode if non-literal class parameter is given" do Puppet[:strict] = "error" files = ['non_literal.pp', 'non_literal_2.pp'].map {|f| File.join(code_dir, f) } result = Puppet::InfoService.classes_per_environment({'production' => files }) + expect(@logs).to have_matching_log_with_source(/The parameter '\$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal.pp", 1, 37) + expect(@logs).to have_matching_log_with_source(/The parameter '\$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal_2.pp", 1, 44) + expect(result).to eq({ "production"=>{ "#{code_dir}/non_literal.pp" => - {:error=> "The parameter '$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: #{code_dir}/non_literal.pp, line: 1, column: 37)"}, + {:error=> "The parameter '\$bad_int' is invalid: The expression <1-3> is not a valid type specification."}, "#{code_dir}/non_literal_2.pp" => - {:error=> "The parameter '$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: #{code_dir}/non_literal_2.pp, line: 1, column: 44)"} + {:error=> "The parameter '\$double_brackets' is invalid: The expression is not a valid type specification."} } # end production env }) end diff --git a/spec/unit/pops/evaluator/literal_evaluator_spec.rb b/spec/unit/pops/evaluator/literal_evaluator_spec.rb index 7bb77922c80..7a1fdb218f5 100644 --- a/spec/unit/pops/evaluator/literal_evaluator_spec.rb +++ b/spec/unit/pops/evaluator/literal_evaluator_spec.rb @@ -21,9 +21,9 @@ 'Integer[-1]' => [-1], 'Integer[-5, -1]' => [-5, -1], 'Integer[-5, 5]' => [-5, 5], - # we can't actually represent MIN_INTEGER because it's glexed as + # we can't actually represent MIN_INTEGER below, because it's lexed as # UnaryMinusExpression containing a positive LiteralInteger and the integer - # must be <= MAX_INTEGER + # must be <= MAX_INTEGER. Therefore, the effective minimum is one greater. "Integer[#{Puppet::Pops::MIN_INTEGER + 1}]" => [-0x7FFFFFFFFFFFFFFF], "Integer[0, #{Puppet::Pops::MAX_INTEGER}]" => [0, 0x7FFFFFFFFFFFFFFF], 'Integer[0, default]' => [0, :default], diff --git a/spec/unit/pops/validator/validator_spec.rb b/spec/unit/pops/validator/validator_spec.rb index cc7eb531523..38a14761c7f 100644 --- a/spec/unit/pops/validator/validator_spec.rb +++ b/spec/unit/pops/validator/validator_spec.rb @@ -212,13 +212,6 @@ def with_environment(environment, env_params = {}) expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION) end end - - it 'produces a warning for non-literal class parameters' do - acceptor = validate(parse('class test(Integer[2-1] $port) {}')) - expect(acceptor.warning_count).to eql(1) - expect(acceptor.error_count).to eql(0) - expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) - end end context 'with --strict set to error' do @@ -270,13 +263,6 @@ def with_environment(environment, env_params = {}) expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION) end end - - it 'produces an error for non-literal class parameters' do - acceptor = validate(parse('class test(Integer[2-1] $port) {}')) - expect(acceptor.warning_count).to eql(0) - expect(acceptor.error_count).to eql(1) - expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) - end end context 'with --strict set to off' do @@ -307,13 +293,6 @@ def with_environment(environment, env_params = {}) expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION) end end - - it 'does not produce an error or warning for non-literal class parameters' do - acceptor = validate(parse('class test(Integer[2-1] $port) {}')) - expect(acceptor.warning_count).to eql(0) - expect(acceptor.error_count).to eql(0) - expect(acceptor).to_not have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) - end end context 'irrespective of --strict' do @@ -344,6 +323,14 @@ def with_environment(environment, env_params = {}) expect(acceptor.error_count).to eql(1) expect(acceptor).to have_issue(Puppet::Pops::Issues::CLASS_NOT_VIRTUALIZABLE) end + + it 'produces a deprecation warning for non-literal class parameters' do + acceptor = validate(parse('class test(Integer[2-1] $port) {}')) + expect(deprecation_count(acceptor)).to eql(1) + expect(acceptor.warning_count).to eql(1) + expect(acceptor.error_count).to eql(0) + expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) + end end context 'with --tasks set' do