Skip to content

Fix Ensure check methods can't modify tokens array #233

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Ensure check methods can't modify tokens array #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ jobs:
- '2.7'
- '3.2'
- '3.3'
- '3.4'
name: "spec (ruby ${{ matrix.ruby_version }})"
uses: "puppetlabs/cat-github-actions/.github/workflows/gem_ci.yml@main"
secrets: "inherit"
Expand All @@ -37,6 +38,7 @@ jobs:
- '2.7'
- '3.2'
- '3.3'
- '3.4'
name: "acceptance (ruby ${{ matrix.ruby_version }})"
needs: "spec"
uses: "puppetlabs/cat-github-actions/.github/workflows/gem_acceptance.yml@main"
Expand Down
3 changes: 2 additions & 1 deletion .rubocop_todo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,15 @@ RSpec/RepeatedExampleGroupDescription:
- 'spec/unit/puppet-lint/plugins/check_resources/unquoted_file_mode_spec.rb'
- 'spec/unit/puppet-lint/plugins/legacy_facts/legacy_facts_spec.rb'

# Offense count: 8
# Offense count: 9
# Configuration parameters: Include, CustomTransform, IgnoreMethods, IgnoreMetadata.
# Include: **/*_spec.rb
RSpec/SpecFilePathFormat:
Exclude:
- '**/spec/routing/**/*'
- 'spec/unit/puppet-lint/bin_spec.rb'
- 'spec/unit/puppet-lint/checks_spec.rb'
- 'spec/unit/puppet-lint/checkplugin_spec.rb'
- 'spec/unit/puppet-lint/configuration_spec.rb'
- 'spec/unit/puppet-lint/data_spec.rb'
- 'spec/unit/puppet-lint/lexer/string_slurper_spec.rb'
Expand Down
4 changes: 3 additions & 1 deletion lib/puppet-lint/checkplugin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ def fix_problems
#
# Returns an Array of PuppetLint::Lexer::Token objects.
def tokens
PuppetLint::Data.tokens
# When called from a plugins `check` method, the tokens array returned should be a (shallow) copy
called_from_check = (caller_locations(1..1).first.base_label == 'check')
PuppetLint::Data.tokens(duplicate: called_from_check)
end

def add_token(index, token)
Expand Down
29 changes: 3 additions & 26 deletions lib/puppet-lint/data.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,37 +38,14 @@ def tokens=(tokens)
@defaults_indexes = nil
end

# @api private
def ruby1?
@ruby1 = RbConfig::CONFIG['MAJOR'] == '1' if @ruby1.nil?
@ruby1
end

# Get the tokenised manifest.
#
# @param duplicate [Boolean] if true, returns a duplicate of the token array.
# @return [Array[PuppetLint::Lexer::Token]]
#
# @api public
def tokens
calling_method = if ruby1?
begin
caller[0][%r{`.*'}][1..-2] # rubocop:disable Performance/Caller
rescue NoMethodError
caller[1][%r{`.*'}][1..-2] # rubocop:disable Performance/Caller
end
else
begin
caller(0..0).first[%r{`.*'}][1..-2]
rescue NoMethodError
caller(1..1).first[%r{`.*'}][1..-2]
end
end

if calling_method == 'check'
@tokens.dup
else
@tokens
end
def tokens(duplicate: false)
duplicate ? @tokens.dup : @tokens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a crazy thought: rather than duplicating, would it make sense to freeze it? https://rubyapi.org/o/array#method-i-freeze says it will not do anything if already frozen, so should be cheap and you can't modify it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There wasn't much information I could find about to why this was originally introduced, but other methods (such as fix) definitely do need to be able to modify the array.

end

# Add new token.
Expand Down
36 changes: 36 additions & 0 deletions spec/unit/puppet-lint/checkplugin_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require 'spec_helper'

class DummyCheckPlugin < PuppetLint::CheckPlugin
def check
# Since we're calling `tokens` from a `check` method, we should get our own Array object.
# If we add an extra token to it, PuppetLint::Data.tokens should remain unchanged.
tokens << :extra_token
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When returning a frozen array it will raise a FrozenError. Perhaps we should catch that and raise a deprecation warning?

end

def fix
tokens << :fix_token
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is supposed to be a valid API because there's also the add_token to actually modify it.

end
end

describe PuppetLint::CheckPlugin do
before(:each) do
PuppetLint::Data.tokens = [:token1, :token2, :token3]
end

it 'returns a duplicate of the token array when called from check' do
plugin = DummyCheckPlugin.new

plugin.check

# Verify that the global token array remains unchanged.
expect(PuppetLint::Data.tokens).to eq([:token1, :token2, :token3])
end

it 'other methods can modify the tokens array' do
plugin = DummyCheckPlugin.new

plugin.fix

expect(PuppetLint::Data.tokens).to eq([:token1, :token2, :token3, :fix_token])
end
end
Loading