Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PUP-11981) Syntactically incorrect types cause nil in Puppet::InfoService::ClassInformationService #9190

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

AriaXLi
Copy link
Contributor

@AriaXLi AriaXLi commented Dec 19, 2023

This commit adds logic to validate class parameters are literal and provides a descriptive warning if parameter(s) are not literal, adds QualifiedReference and AccessExpression as a valid literal values & updates spec tests to check for more cases of invalid class parameters and any that broke as a result of the changes.

The logic to validate class parameters is done in internal_check_parameter_type_literal method where the appropriate literal method is called on each parameter. There are different literal methods for each literal object/type [1]. For example if the parameter is String type, then literal_LiteralString is called. If a parameter is found to be not literal, then a :not_literal will be thrown & warning message with the invalid class parameter and its type will be printed.

QualifiedReference is considered literal since it is a literal reference, however, there is a possibility it could be a faulty dangling reference that doesn't reference anything. AccessExpresions are anything with the Access Operator ([ ]) such as Optional[String] or Variant[Boolean, Float] [2]. Since these are valid class parameters, AccessExpressions are also considered literal.

The logic to add QualifiedReference & AccessExpression as valid literal values is done in the literal_QualifiedReference & literal_AccessExpression methods. The literal_AccessExpression method works by validating each key in the expression is literal. The literal_QualifiedReference method looks at o.value, similar to other existing literal methods like literal_LiteralString.

[1] http://puppet-on-the-edge.blogspot.com/2014/02/puppet-internals-polymorphic-dispatch.html
[2] https://github.com/puppetlabs/puppet-specifications/blob/master/language/expressions.md#assignment-operator

For more information on this issue, see #9140

@AriaXLi AriaXLi requested a review from a team as a code owner December 19, 2023 18:26
@AriaXLi AriaXLi marked this pull request as draft December 20, 2023 19:28
@AriaXLi AriaXLi force-pushed the PUP-11981 branch 2 times, most recently from b46508d to 1a2c4b7 Compare January 11, 2024 21:12
@AriaXLi
Copy link
Contributor Author

AriaXLi commented Jan 11, 2024

jenkins please test this

@AriaXLi
Copy link
Contributor Author

AriaXLi commented Jan 11, 2024

@AriaXLi AriaXLi force-pushed the PUP-11981 branch 3 times, most recently from 97d848d to 00d0414 Compare January 11, 2024 22:30
@AriaXLi AriaXLi changed the title (PUP-11981) WIP (PUP-11981) Syntactically incorrect types cause nil in Puppet::InfoService::ClassInformationService Jan 11, 2024
@AriaXLi AriaXLi marked this pull request as ready for review January 11, 2024 22:37
This commit adds logic to validate class parameters are literal and provides a
descriptive warning if parameter(s) are not literal, adds QualifiedReference
and AccessExpression as a valid literal values & updates spec tests to check
for more cases of invalid class parameters and any that broke as a result of
the changes.

The logic to validate class parameters is done in
internal_check_parameter_type_literal method where the appropriate literal
method is called on each parameter. There are different literal methods for
each literal object/type [1]. For example if the parameter is String type, then
literal_LiteralString is called. If a parameter is found to be not literal,
then a :not_literal will be thrown & warning message with the invalid class
parameter and its type will be printed.

QualifiedReference is considered literal since it is a literal reference,
however, there is a possibility it could be a faulty dangling reference that
doesn't reference anything. AccessExpresions are anything with the Access
Operator ([ ]) such as Optional[String] or Variant[Boolean, Float] [2]. Since
these are valid class parameters, AccessExpressions are also considered
literal.

The logic to add QualifiedReference & AccessExpression as valid
literal values is done in the literal_QualifiedReference &
literal_AccessExpression methods. The literal_AccessExpression works by
validating each key in the expression is literal. The
literal_QualifiedReference method looks at o.value, similar to other existing
literal methods like literal_LiteralString.

[1] http://puppet-on-the-edge.blogspot.com/2014/02/puppet-internals-polymorphic-dispatch.html
[2] https://github.com/puppetlabs/puppet-specifications/blob/master/language/expressions.md#assignment-operator

For more information on this issue, see puppetlabs#9140
@joshcooper joshcooper merged commit 6fd9af4 into puppetlabs:main Jan 17, 2024
9 checks passed
@joshcooper
Copy link
Contributor

@AriaXLi since this issue was originally encountered in the ClassInfoService, could you add a test to somewhere in

it "produces expression string if a default value is not literal" do
files = ['fee.pp'].map {|f| File.join(code_dir, f) }
result = Puppet::InfoService.classes_per_environment({'production' => files })
expect(result).to eq({
"production"=>{
"#{code_dir}/fee.pp"=>{:classes => [
{:name=>"fee",
:params=>[
{:name=>"fee_a", :type=>"Integer", :default_source=>"1+1"}
]},
]}} # end production env
})
end
it "produces source string for literals that are not pure json" do
files = ['json_unsafe.pp'].map {|f| File.join(code_dir, f) }
result = Puppet::InfoService.classes_per_environment({'production' => files })
expect(result).to eq({
"production"=>{
"#{code_dir}/json_unsafe.pp" => {:classes => [
{:name=>"json_unsafe",
:params => [
{:name => "arg1",
:default_source => "/.*/" },
{:name => "arg2",
:default_source => "default" },
{:name => "arg3",
:default_source => "{1 => 1}" }
]}
]}} # end production env
})
end
it "produces no type entry if type is not given" do
files = ['fum.pp'].map {|f| File.join(code_dir, f) }
result = Puppet::InfoService.classes_per_environment({'production' => files })
expect(result).to eq({
"production"=>{
"#{code_dir}/fum.pp"=>{:classes => [
{:name=>"fum",
:params=>[
{:name=>"fum_a" }
]},
]}} # end production env
})
end
for non-literal class parameters?

@joshcooper
Copy link
Contributor

@AriaXLi could you backport this and 8992763 to 7.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntactically incorrect types cause nil types in Puppet::InfoService::ClassInformationService
2 participants