はじめに
こんにちは! 皆様クソコードをお焚き上げしていますか?
今回は私が直接やらかしたわけではないものですが、某基幹システムでテーブルをばよえ～んしてぶっ飛ばした恐怖のコードをお焚き上げさせていただきます。
背景
- 私が前前前職くらいだったころのお話です(15年前くらい?)
- そのシステムはJava1.6, Struts, O/Rマッパーとして
MyBatisを使っていました - 前日の夜間にとある基幹システムの改修を本番化しました
- 翌日の業務開始時間に正常稼働を確認し、よかったよかったしていると、ユーザーの運用部門から設備の基本情報テーブルが空になったとクレームが入りました
- 幸い、本番化にあたってフルバックアップを取っていたので改修前の状態に切り戻すことはできました
原因
調査の結果、こんなコードがあることが判明しました。
(最低限状況を説明するためのコードで、正確なコードではありません。雰囲気だけ掴んでいただければ)
// 特定の状態に遷移したレコードを基幹テーブルから削除する処理（新規機能)
@service("badTableDeleteService")
public class BadTableDeleteService {
Map condition = null;
public void initCondition(...) {
condition = new HashMap();
}
public void makeCondition1(...) {
condition.put(key, value);
}
// makeCondition2, 3もある
}
public Map<String, Object> execute {
// MyBatisを使ってテーブルから delete する処理
}
}
MyBatis側では以下のようなXMLでクエリを定義していました
<delete id="deleteBaseTableWhenSpecifyCondition" parameterType="java.util.Map">
DELETE * FROM base_table
<where>
<if test="state != null">
state = #{state}
</if>
<if test="title != null">
AND title like #{title}
</if>
<if test="author != null and author.name != null">
AND author_name like #{author.name}
</if>
</where>
</delete>
どこがクソコードか?
このサービスクラスを実装したメンバーは、複数の複雑にセットする条件を1つのサービスクラス内に共通参照するため、条件を保存する変数 condition をインスタンス変数にしたようです。
これが大きな間違いでした。
Struts2のサービスコンテナはコントローラー側でDIして利用しますが、メモリ効率化のためにシングルトンで動作します。リクエストごとにサービスクラスが生成されるわけではなく、1つのインスタンスがすべてのリクエストで使い回されます。
もうお察しの通り、このサービスに同時にアクセスが発生するとレースコンディションが発生して execute する際に別のリクエストで中途半端に変更された condition が使われる可能性があります。
さらなる問題
次にMyBatis側のXMLに注目してください。 where タグで削除するレコードの条件を指定しています。
条件式に使うデータは HashMap で渡されます。 つまりこのサービスの condition です。
もし condition が空のHashMapの状態でこのXMLを実行したらどうなるか?
WHERE句ごと消えてなくなります
つまり DELETE FROM base_table; が実行されるわけです。ばよえ～ん!! 全消し!!
しくじりの原因
主に3つの原因が考えられます。
-
フレームワーク自体の理解不足
Struts2ではDIコンテナはシングルトンで動作する、という基本的なフレームワークの仕組みが実装者に不足していました。フレームワークの理解不足から発生したバグと言えるでしょう -
コードレビューをしていない
この頃所属していた会社ではコードレビューを綿密に行う文化がありませんでした。単体テスト、結合テストのみガッチリやっていましたが、実装されたコードのレビューは行われていませんでした。 -
システムテストの不備
当該システムはユニークユーザーが少ない業務システムだったので、負荷テスト、同時接続テストなど非機能要件に関するシステムテストは実施されていませんでした。当時の会社は二次請けベンダーで、契約上は結合テストまでが責務でシステムテストは一次ベンダーの責務だったのも見逃しの要因になったかもしれません。
まとめ
- 自身が開発に携わるフレームワークはしっかり学習しましょう
- コードレビュー、特に 0→1 フェーズのコードレビューは綿密に行いましょう。技術的負債を抱えないように
- 可能な限り自動テストを書きましょう。 予期しない
nullやemptyが処理データに入った際は適切に例外が出ることを確認しましょう
弊社の取り組み
弊社ネオジニアではフレームワークへの理解を深めること、コードレビューをタスクの進行に合わせて段階的に実施することをワークルールで定めております。この記事を書くにあたりこのようなルールが有用かつ効果的であることを再認識いたしました。
以上でお焚き上げ完了です。
南無大師遍照金剛