Skip to content

Commit

Permalink
Fix Ensure check methods can't modify tokens array
Browse files Browse the repository at this point in the history
The functionality originally released in rodjek#296
was probably broken at the time, possibly made worse by later rubocop
'fixes' and also incompatible with ruby 3.4.

We now look at the stack trace only from the `tokens` wrapper method in
`checkplugin`.

Fixes #225
  • Loading branch information
alexjfisher committed Feb 8, 2025
1 parent 002f275 commit ff0337e
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 28 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
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
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
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
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
end

# Add new token.
Expand Down
36 changes: 36 additions & 0 deletions spec/unit/puppet-lint/checkplugin_spec.rb
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
end

def fix
tokens << :fix_token
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

0 comments on commit ff0337e

Please sign in to comment.