【雑記】実装・設計・リリース時に気をつけたいこと

マルチテナントのサービスの場合、テナントの分離が正しく行われているか

画面からuserのidをもらって検索するとき、他テナントのデータが取得できてしまわないよう、必ずテナントのスコープを通す。

# bad: 全ユーザーから検索してしまう
User.find(id)

# good: テナントに紐づくユーザーからのみ検索する
tenant.users.find(id)

参考: activerecord-multi-tenant などのgemの利用も検討する

SQLインジェクション対策

文字列補間でSQLを組み立てるとSQLインジェクションの脆弱性になる。

# bad
User.where("name = #{name}")

# good: プレースホルダを使う
User.where("name = ?", name)

# better: ハッシュ形式が最もシンプル
User.where(name: name)

LIKE句を使う場合は % _ といったワイルドカード文字をサニタイズする。

# good
User.where('name LIKE ?', "%#{sanitize_sql_like(name)}%")

時間がかかる処理は非同期化を検討

CSVインポートのような重い処理を同期で実行すると、フロント側でタイムアウトが発生することがある。あらかじめ非同期化（SidekiqなどのJobキュー）を検討しておく。

join系

まず各メソッドの用途を整理する。

関連の種類に応じた使い分けの目安は以下のとおり。

includes は基本的に使わない。includes は検索条件でjoin先を参照しているかどうかで eager_load になるか preload になるか変わるため、意図しない挙動になる可能性がある。

引数の数

引数が増えてきたらオブジェクトを定義してDTO(Data Transfer Object)的に扱う（ActiveModel を使うのもあり）。

RuboCopのデフォルトでは引数が5つを超えると警告が出る。5つ渡している時点でメソッドの責務が広がりすぎているサインであることが多いので、4〜5つを超えてきたら設計を見直す目安にするとよい。

基本的にキーワード引数を使う。bool や数値などのプリミティブな値を複数渡す場合、位置引数だとミスに気づきにくい。

# bad
def create(name, age, admin, active)
end

# good
def create(name:, age:, admin:, active:)
end

Serviceクラスのパブリックメソッドは1つのみ

外部から呼び出すメソッドは execute、call、perform のいずれか1つのみにする（名前は好み）。

同じServiceに search_xxxx、create_xxxx、update_xxxx、delete_xxxx のように複数のpublicメソッドを定義すると、Serviceがさまざまな責務を持つようになり、privateメソッドがどの処理のためのものかわからなくなる。また、search のみ修正したつもりが同じクラスにある他の処理にも影響を与えてしまうリスクがある。

リリース時のマイグレーションやindex追加

データ件数が多いテーブルに対するマイグレーションやindex追加は時間がかかる。

事前に本番相当のデータで実行時間を計測し、長時間かかる場合は夜間リリースやメンテナンスモードの導入を検討する。後述のメタデータロックにも注意。

テーブルロックを回避するために オンラインDDL の使用も検討する。操作によって使えるアルゴリズムが異なるため、事前に確認が必要。

-- セカンダリインデックスの追加（同時DML可）
ALTER TABLE tbl_name ADD INDEX idx_name (col_name), ALGORITHM=INPLACE, LOCK=NONE;

ただし、オンラインDDLを使ってもメタデータロックは発生する。公式ドキュメントに以下の記述がある。

オンライン DDL 操作では、テーブルのメタデータロックを保持する同時トランザクションがコミットまたはロールバックされるまで待機する必要がある場合があります。DDL 操作の前または実行中に開始されたトランザクションは、変更されるテーブルのメタデータロックを保持できます。長時間実行中または非アクティブなトランザクションの場合、オンライン DDL 操作は排他的メタデータロックの待機中にタイムアウトすることがあります。また、オンライン DDL 操作によってリクエストされた保留中の排他的メタデータロックによって、テーブルの後続のトランザクションがブロックされます。

参考: オンライン DDL のパフォーマンスと同時実行性 - オンライン DDL およびメタデータロック

テーブルコピー中のロックを減らせるだけで、後述のメタデータロック問題は別途対策が必要。

リリース時のメタデータロック

参考: MySQLのメタデータロックについて

トランザクション中に触ったテーブルに対してDDL（index追加・カラム追加・テーブル削除など）を実行すると、そのトランザクションが終了するまでメタデータロックで待機する。

DDLの待機中はそのテーブルへの他セッションからの後続クエリがすべて待機されてしまう。頻繁にアクセスされるテーブルでこの問題が起きると、サービス全体が停止する可能性がある。

いくつか考えられる対策を紹介する。

1. トランザクションを短く保つ

メタデータロックの待機時間を最小化する根本対策。長いトランザクションが張られていなければ影響は小さい。

# bad: 不要な処理をトランザクション内に含めている
ActiveRecord::Base.transaction do
  heavy_external_api_call()  # 時間のかかる処理
  user.update!(name: name)
end

# good: DB操作のみトランザクション内に収める
heavy_external_api_call()
ActiveRecord::Base.transaction do
  user.update!(name: name)
end

2. マイグレーション前に長時間トランザクションがないか確認する

-- 実行中のトランザクションを確認
SELECT * FROM information_schema.innodb_trx;

デプロイ直前にこのクエリで長時間トランザクションがないか確認してからDDLを実行する。

3. メンテナンスモードで実施する

根本的にはアクセスを止めてしまうのが一番安全。

バリデーションの追加

既存の処理に本来あるべきバリデーションが抜けていて後から追加するケースでは、今まで通っていた操作がエラーになるため、事前にユーザーへ周知する。

何も考慮せずリリースすると「昨日までできたのにいきなりエラーになる！」という問い合わせが発生する。

名前空間の定義

ネストが深い module 定義は、同名のモデルと衝突してエラーになることがある。

# 注意: BというModelが定義されていると、Cの中でB.find(1)が解決できずエラーになる
#       その場合 ::B.find(1) のように絶対パスで書く必要がある
module A
  module B
    class C
    end
  end
end

# bad: A、Bが長くなると横が長くなりrubocopに引っかかることがある
class A::B::C
end

# good: 階層を浅くする
module A
  class C
  end
end

一時的なファイル作成はTempfileを使う

ブロック形式を使うことでファイルのクローズ・削除が自動で行われる。

# bad: クローズし忘れるとファイルが残り続ける
f = Tempfile.open('xxxx')
# なんらかの処理
f.close

# good: ブロックを抜けると自動でclose・deleteされる
Tempfile.open('xxxx') do |f|
  # なんらかの処理
end