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

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

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

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

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

```ruby: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
```

```ruby: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 の警告から見ていきましょう。

```ruby
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 クラスを使うときに `|` を使って

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

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

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

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

今度は、


```ruby
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 を例にとります。

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

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

```ruby: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` をチェックすると次の警告が出ます。

```cons
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` にメソッド名を変更したとしましょう。

```ruby
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` が実際に存在するかチェックしないからです。

```ruby: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` に変更してみます。

```ruby: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` に変更してみます。


```ruby: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` を使ってみましょう。

```ruby: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` に変えてみましょう。


```ruby: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 が通ってしまいます。

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

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

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

```ruby: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` が呼ばれること) が先に登場するのです。

```ruby: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` が呼ばれたことをテストするよ、と順番に上から下に素直に読むことができます。

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

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

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

## 修正後のコード

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

```ruby: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
```

```ruby: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