Go to Qiita Advent Calendar 2024 Top
LoginSignup
10
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 1 year has passed since last update.

Ruby開発Advent Calendar 2022

Day 14
@suketa

そのrubocopの警告スルーしていませんか？

Last updated at Posted at 2022-12-13

rubocop の警告が煩わしいと思うことがありませんか？
面倒だから警告を diable にして、警告を消してOKにしてませんか？
逆に深く考えずに警告を修正して、警告が消えたから、それでOKなんてことしてませんか？

ちょっと待ってください。警告は自分のスキルアップにつながるチャンスかも知れません。警告にどんな意味があるか、どう修正すべきか調べることで、自分の知識を深めることにつながるかも知れません。今日は、そんな自分の経験をいくつかご紹介したいと思います。

なお、ここでは、 rubocop と rubocop-rspec gem を使っています。

以下のコードと spec を例にして説明します。

web_download.rb
# frozen-string-literal: true

require 'open-uri'

# Web ページのクラス
class Web
  def initialize(url)
    @url = url
  end

  def read
    URI.open(@url, &:read)
  end
end

# Web ページをダウンロードするクラス
class WebDownloadService
  def run(web, io = $stdout)
    io.write web.read
  end
end
spec/web_download_service_spec.rb
# frozen_string_literal: true

require_relative '../web_download'

describe WebDownloadService do
  describe '#run' do
    let(:io) { StringIO.new }
    let(:service) { described_class.new }

    it 'run calls the web#read method' do
      web = double(Web)
      expect(web).to receive(:read)
      service.run(web, io)
    end
  end
end

Security/Open

まずは、 Web クラスの rubocop の警告から見ていきましょう。

class Web
  ...
  def read
    URI.open(@url, &:read)
  end
end

rubocop で以下のような警告が出ます。

web_download.rb:12:9: C: Security/Open: The use of URI.open is a serious
security risk.
    URI.open(@url, &:read)
        ^^^^

rubocop Security/Open で検索してみると https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Security/Open が見つかります。

ページには次のように書かれています。　

Kernel#open and URI.open enable not only file access but also process invocation by prefixing a pipe symbol (e.g., open(“| ls”)). So, it may lead to a serious security risk by using variable input to the argument of Kernel#open and URI.open. It would be better to use File.open, IO.popen or URI.parse#open explicitly.

URI.open ではなく、 URI.parse#open を使えということのようです。この理由をもう少し詳しく調べてみましょう。
例えば、Web クラスを使うときに | を使って

url = '| rm very_important_file'
Web.new(url).read

とすると、 rm コマンドが実行できてしまい、 very_important_file が削除できてしまいます。URI.open にはこういったリスクがあるということですね。

では、説明にあるように URI.parse を使うように書き換えます(ここでは、 open を使わずに書き換えることができます)。

class Web
  ...
  def read
    URI.parse(@url).read
  end
end

今度は、

url = '| rm very_important_file'
Web.new(url).read

としても、以下の例外が発生します。 very_important_file は削除されません。

bad URI(is not URI?): "| rm very_important_file" (URI::InvalidURIError)

URI.open より、 URI.parse#open を使った方が良いことが、理由も含めて理解できました。

RSpec/VerifiedDoubles

次に WebDownloadService クラスと、その spec を例にとります。

# Web ページをダウンロードするクラス
class WebDownloadService
  def run(web, io = $stdout)
    io.write web.read
  end
end

spec は、 WebDownloadService#run メソッドが Web#read を呼び出していることをテストしています。

spec/web_download_service_spec.rb
describe WebDownloadService do
  describe '#run' do
    let(:io) { StringIO.new }
    let(:service) { described_class.new }

    it 'run calls the web#read method' do
      web = double(Web)
      expect(web).to receive(:read)
      service.run(web, io)
    end
  end
end

rubocop で spec/web_download_service_spec.rb をチェックすると次の警告が出ます。

spec/web_download_service_spec.rb:10:13: C: RSpec/VerifiedDoubles: Prefer
using verifying doubles over normal doubles.
      web = double(Web)
            ^^^^^^^^^^^

RSpec/VerifiedDoubles rubocop で検索すると https://www.rubydoc.info/gems/rubocop-rspec/1.3.1/RuboCop/Cop/RSpec/VerifiedDoubles が見つかります。

double よりも、 instance_double の方を使えということのようですが、どうしてでしょうか？

https://relishapp.com/rspec/rspec-mocks/docs/verifying-doubles を読めばわかります。

instance_double は、引数で指定したクラスのメソッドが実際に存在するかどうかをチェックしてくれますが、 double はチェックしてくれません。

例えば、 Web#read を Web#body にメソッド名を変更したとしましょう。

class Web
  ...

  # read -> body にメソッド名を変更
  def body
    URI.open(@url, &:read)
  end
end

class WebDownloadService
  def run(web, io = $stdout)
    io.write web.read # 存在しない read メソッドを呼び出している
  end
end

このとき、実際にコードを実行すると WebDownloeadService#run メソッドは、存在しない Web#read を呼び出すため、エラーになります。
ところが、specを実行しても、エラーにはならないのです
spec は、Web#read を呼び出すことをテストしますが、 Web#read が実際に存在するかチェックしないからです。

spec/web_download_service_spec.rb
    it 'run calls the web#read method' do
      web = double(Web)
      # Web#read を呼ぶかどうか確認する。 Web#readが存在することをチェックをしない。
      expect(web).to receive(:read) 
      service.run(web, io)
    end

double を instance_double に変更してみます。

spec/web_download_service_spec.rb
    it 'run calls the web#read method' do
      web = instance_double(Web)
      expect(web).to receive(:read)
      service.run(web, io)
    end

instance_double を使うと、 spec は、呼び出すメソッドが実際に存在することをチェックします。
spec を実行すると、 Web#read が存在しないため spec が失敗します。
実際にコードを動かさなくても、spec を実行しただけで、コードに何か問題がありそうだとわかります。

$ bundle exec rspec
F

Failures:

  1) WebDownloadService#run run calls the web#read method
     Failure/Error: expect(web).to receive(:read)
       the Web class does not implement the instance method: read
     # ./spec/web_download_service_spec.rb:11:in `block (3 levels) in <top (required)>'

double よりも instance_double が推奨される理由がわかりました。

RSpec/MessageSpies

spec には、 rubocop の警告がまだあります。

spec/web_download_service_spec.rb:11:22: C: RSpec/MessageSpies: 
Prefer have_received for setting message expectations. 
Setup web as a spy using allow or instance_spy.
      expect(web).to receive(:read)
                     ^^^^^^^

have_received に変更してみます。

spec/web_download_service_spec.rb
    it 'run calls the web#read method' do
      web = instance_double(Web)
      expect(web).to have_received(:read)
      service.run(web, io)
    end

rubocop の警告は消えますが、今度は spec がエラーになります。

$ bundle exec rspec
F

Failures:

  1) WebDownloadService#run run calls the web#read method
     Failure/Error: expect(web).to have_received(:read)
       #<InstanceDouble(Web) (anonymous)> expected to have received read, but that object is not a spy or method has not been stubbed.
     # ./spec/web_download_service_spec.rb:11:in `block (3 levels) in <top (required)>'

では、 allow を使ってみましょう。

spec/web_download_service_spec.rb
    it 'run calls the web#read method' do
      web = instance_double(Web)
      allow(web).to have_received(:read)
      service.run(web, io)
    end

これも rubocop の警告は消えますが、 spec はエラーになります。

$ bundle exec rspec
F

Failures:

  1) WebDownloadService#run run calls the web#read method
     Failure/Error: allow(web).to have_received(:read)
       Using allow(...) with the `have_received` matcher is not supported as it would have no effect.
     # ./spec/web_download_service_spec.rb:11:in `block (3 levels) in <top (required)>'

では、 have_received を receive に変えてみましょう。

spec/web_download_service_spec.rb
    it 'run calls the web#read method' do
      web = instance_double(Web)
      allow(web).to receive(:read)
      service.run(web, io)
    end

spec も通るし、rubocop の警告も消えました。これですべてクリア...ではないんですねー。

このテストだと、 次のように Web#read を呼ばないコードに変更しても spec が通ってしまいます。

web_download.rb
class WebDownloadService
  def run(web, io = $stdout)
    io.write web # Web#read を呼ばないが、specは通る。
  end
end

allow は実際に Web#read が呼ばれたことを確認しないからです。

次のように書くのが正解です。

spec/web_download_service_spec.rb
    it 'run calls the web#read method' do
      web = instance_double(Web)
      allow(web).to receive(:read)
      service.run(web, io)
      expect(web).to have_received(:read)
    end

service.run(web, io) を実行した後で、 expect(web).to have_received(:read) で、実際に Web#read が呼ばれたことを確認するのが正解です。

修正前のコードに対して、 rubocop が警告を出す理由を少し考えてみます。
修正前のコードと修正後のコードを比較してみましょう。

修正前の spec は、Web#read が呼ばれることをテストするよ、と先に書いて、 service.run(web, io) を実行するよ(すると Web#read が呼ばれるはず)、と後で書いてます。
メソッドの呼び出し (service.run(web, io)) が後、メソッドを呼び出した結果どうなるかの確認 (Web#read が呼ばれること) が先に登場するのです。

spec/web_download_service_spec.rb
      ...
      # Web#read が呼ばれることをテストするよ
      expect(web).to receive(:read)
      ...
      # WebDownloadService#run を実行するよ(すると `Web#read` が呼ばれるはず)
      service.run(web, io)
      ...

修正後の spec は、 service.run(web, io) を実行するよ、その結果、 Web#read が呼ばれたことをテストするよ、と順番に上から下に素直に読むことができます。

spec/web_download_service_spec.rb
      ...
      # WebDownloadService#run を実行するよ
      service.run(web, io)

      # Web#read が呼ばれたことをテストするよ
      expect(web).to have_received(:read)
      ...

修正後の spec の方が順を追って流れを理解しやすいことから、 rubocop は警告を出すのだと思います。

修正後のコード

少し、spec のリファクタリングもして、最終的なコードは以下のようになりました。

web_download.rb
# frozen-string-literal: true

require 'open-uri'

# Web ページのクラス
class Web
  def initialize(url)
    @url = url
  end

  def read
    URI.parse(@url).read
  end
end

# Web ページをダウンロードするクラス
class WebDownloadService
  def run(web, io = $stdout)
    io.write web.read
  end
end
spec/web_download_service_spec.rb
# frozen_string_literal: true

require_relative '../web_download'

describe WebDownloadService do
  describe '#run' do
    let(:io) { StringIO.new }
    let(:web) { instance_double(Web) }
    let(:service) { described_class.new }

    before do
      allow(web).to receive(:read)
    end

    it 'calls the Web#read method' do
      service.run(web, io)
      expect(web).to have_received(:read)
    end
  end
end

まとめ

rubocop の警告が出た場合に安易に無効にしたり、言われた通りに機械的に修正するだけではなく、一度くらいは、自分の知識を更に深める良い機会だと考えて、調べてみてはいかがでしょうか？

修正後のコードと修正前のコードは以下で確認できます。

修正前:
https://github.com/suketa/rubocop_sample/tree/initial_code

修正後:
https://github.com/suketa/rubocop_sample/tree/final_code

10
1
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
10
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?