LoginSignup
46
11

More than 5 years have passed since last update.

Rails Polymorphicの間違えた使い方

Last updated at Posted at 2018-12-14

この記事はex-crowdworks Advent Calendar 2018の14日目です.

今まで割とみんなポジティブな話をしたので(偉い!!),思いっきりネガティブな話をします.

ただ,これは俺が居たときの辛さであり,かつ 俺が個人的に感じていただけのツラミです.
今でもこれが残っているかはわからないし,改善しているといいね.

あと,これは妄想の話で,実在する組織・企業とは一切関係ありません

コードに関する辛み

フロント方向の話は,
https://qiita.com/ayasuda/items/6332e8e72bfe8db48f75

こちらですでに書かれているので,特にここでは触れない.
ここではバックエンドのRailsサーバの話をメインでする.

決済系のコードとデータ構造

前職の決済系処理は,コードもデータ構造も割とクソで,もともと禁則事項です が頻繁に発生しており,その都度エンジニアが対応していた.
ていうか上場企業で 禁則事項です とかいいんだ,と当初思った.実装難しいのはわかるけど.

これがあまりにも数が多くなってきたので,我々で対応したのだけれど,元がかなりクソ実装だったので,少しはましになったんだけど依然として難易度はやたら高いまま.

「ホントごめん」と思いながらやばい実装がいっぱい残っている.

両端ポリモーフィックのテーブルが実在した

特に闇の中枢となっているのが,DepositBreakdown という両端Polymorhicなモデルがいて,これが象徴的すぎて我々のチームはDepositBreakdownと呼ばれていた(モデル名は架空の名称です).

deposit_breakdown.rb
class DepositBreakdown < ActiveRecord::Base
  belongs_to :depositable, polymorphic: true
  belongs_to :breakdownable, polymorphic: true
end
escrow.rb
class Escrow < ActiveRecord::Base
  has_many :deposit_breakdowns, as: :breakdownable
end
bank_transfer.rb
class BankTransfer < ActiveRecord::Base
  has_many :deposit_breakdowns, as: :depositable
end

で,このモデルを見ればわかると思うけど,breakdownable は決済する対象が増えるたびにどんどん増える.

そして,depositable も決済手段が増えるたびにどんどん増える.

Rails的にpolymorphicを使うだけなら全然問題ないんだけど,流石に決済系だとあとからこれをSQLで辿ったりすることが,まれによくある.

そうすると,これらはすべてLEFT OUTER JOIN していく必要が合って,モデルの個数分だけ LEFT OUTER JOIN するので,軽く10以上のテーブルをJOINしたSQLを書くことになる.

これがドキュメントに書かれていて,よくこれをコピペして使っていた.

流石にこんなにやばい両端ポリモーフィックなモデルはこれ以外なかったと思うなぁ.

おまけにポリモーフィックを無視してくる

モデルの構造を見ればわかると思うけど,depositable 側も breakdownablehas_many になっている.
これはつまり,1決済手段で複数の商品を購入する or 1商品で複数決済手段を使う ことを許容している.それどころか 複数商品を複数決済手段で購入する ことすら許容している.

で,これは本当に申し訳ないんだけど,「銀行振り込みで,このタイミングでAという商品が買われた」ときに「実は銀行振込だけじゃなくて割引決済も混ざっていたらしいのでそれを抽出したい」みたいな,要求がどうしても生まれてしまった.
この要求事態は決済系のバグを解消するためにどうしても必要で,しかたなく入ったのだが…….

また,「銀行振込のときだけはこれをやる!」みたいな処理もふんだんに入り込んでいる.

わかると思うけれど,こういう特別な処理を含むものをポリモーフィックで実現してはいけない.結果として breakdownable_typedepositable_type によって分岐するコードが,それなりの量ある.
というか元からそういうパターンに応じて分岐が発生しており,もうこれどうすんの…….

似た名前のモデルがいる

慣れると本質的ではないのであまり気にならないんだけど,Depositっていう全く別の役割を果たすモデルもいるんで注意な.
ついでにこのDepositbreakdownable の一種になり得る.

禁則事項です

仕組み的に仕方がないんだけど,前職には禁則事項ですという機能がある.
これも決済周りに含まれるのだが,かなりの闇で,こいつが禁則事項ですを生み出す.

ただ,この辺のデータの流れがいまいちで,「ある禁則事項ですとき,その決済に利用された禁則事項は何を禁則事項ですされたのか」というのが禁則事項です,というデータ構造になっている.
禁則事項ですはできるんだけどね…….

なのであとから検証しようとしたときに,一体これは何の禁則事項ですが引かれたのかは,想像するしかない.
他の情報で「この人はもともとこれだけ禁則事項ですがあったから」みたいな推測を行うしかない.

その結果として禁則事項ですには禁則事項です禁則事項です禁則事項ですされてしまい,エンジニア陣が禁則事項ですかけて延々禁則事項ですすることになった上に,禁則事項ですだと思うんだけど,仕方ないよね.

トランザクション内で決済処理を行う

みんな,こういうこと,絶対にやっちゃだめだぞ.

前職の決済処理は,その処理自体がめちゃくちゃ複雑で,割引とかが頻繁に入り込んでくるせいで,そもそも決済画面に遷移する際に「本当にその決済できるのか?」というのがわからんというのがあった.
で,これを厳密には解決できていない.というか決済時の決済手段確定処理が,裏側の割引等の処理に依存しすぎていて,事前に計算できない.

そのため トランザクションで囲んだブロック内で,一度決済処理を走らせてみて,どういう状態になるかを観測してから決済画面を表示させている

言い訳をさせてもらうと,もとのコードは eval 使った邪悪実装(しかもバグってる)で,もうその実装をまともに治すことができないレベルになっていた…….

このあたりは元からトランザクションを4重くらいにネストするのが普通のコード達ばっかりだったので,もうなにがなんだかわからない.
とりあえず「こいつの呼び出し元トランザクション張ってる?」というのをいつも気にしないといけない茨の道だった.

一括系処理

これは俺自身がそこまで作り込んだわけではないのだが,前職には一括〇〇系の処理が結構ある.
このくらいはどこの会社もあると思うんだけど,前職ではこれを管理画面から叩けるようにしていて,エンジニア以外も使えるようになっていた.

この中身,結構えげつない,というか処理的に,SQLにすごく重い処理が多い.
普通に2時間走り続けるジョブを生み出したりするので,邪悪.

軽くしようとは試みたことがあるんだけど,要望自体は変えられないので小手先の高速化しかできない.

これで内部的には応募を作ったりメールを送ったりしてしまうので,削れなかった.で,たまに限界が来てサービスの負荷を上げたりしている.
マジでこいつを早めになんとかしなきゃいけないなーと思い続けて,思ったまま辞めた.

DelayedJob

で,そういう重たいジョブの裏側にはだいたいこいつが控えている.
DelayedJob自体は用法・容量を守って使えば悪い子ではないんだけど,前職は完全に守っていなかった.

まず,トランザクション内でDelayedJobにエンキューするというコードが,おそらくまだ結構な量残っている.

トランザクション内でエンキューすると何が起こるかというと,

  1. 本来の処理をトランザクションにくるんで始める
  2. 処理の途中でDelayedJobにエンキューする
  3. キューはRDSに書き込まれるが,これもトランザクションに囲まれている!!!
  4. 本来の処理が失敗する
  5. トランザクションがロールバックすると,DelayedJobのキューもロールバックする!!!!!

まじかよ,と思う本当の話.
本来の処理が正常に終了して,トランザクションがコミットされたときに,初めて他から見えるDelayedJobのキューがエンキューされる.

これをトランザクション外に出そうとして,まさかのElasticsearchへの同期周りでこれが仕込まれていて,盛大にバグったこともあったね.

あと,DelayedJobは引数にRubyのオブジェクトを渡せて(!!!),これをやるとシリアライズしてDBに保存してくれる.
が,非同期処理でこれをやられると……リリース前のオブジェクトを保存したDelayedJobとかが普通に居残るので,マジで止めてほしい.

細かい話

コードベースがとにかくでかいので,もはやいろんな闇を抱えていた.
3年居て知らない闇もあったんじゃないかな.

他にも

  • hogehoge_flag というカラムでありながら,int型で0,1,2が入る
  • 税金の処理がIntegerクラスの拡張で実装されている
  • 禁則事項です 集計処理バッチが再実行不可能で一度失敗すると手動でデータ修正が必要
  • だれにもわからない 禁則事項です
  • 引き継ぐ人がみんな辞めた 禁則事項です

とか,大変なことは色々あるよね.

組織に関する辛み

ちなみに,エンジニアなので,コードはどんだけ汚くてもやりがいはある.
が,組織的な辛みは単に不毛なだけなので,個人的にはこっちのほうがよっぽど辛かった.

朝会

前職には毎週月曜日朝9:30から全社朝会があった.

ちなみに,勤務時間は10:00 - 19:00だった.
フレックスが入ってからは10:00 - 16:00がコアタイム.

あれ?9:30?
もともと9:30開始は 業務時間に含まれていなかったんだけど ,実質強制参加なのでヤバイということで,月曜だけは9:30 - 18:30になった.

発表

そもそも朝9:30が早いので行きたくないんだけど,その上朝会では「頑張ってるチームによる発表」があった.
たまに発表者やってくれというお願いも来て,「最近こんなことしてる」みたいな話を持っていくと,

  • それの何がすごいのか
  • どこを頑張ったのか

がないとダメと言われる.なんだそれは.
できるだけ頑張らずにシステムが動くようにしてるっていうのに.

そして朝会にはリハーサルがある.前の週からお題を出してチェックしてもらってリハーサルして,当日の朝も1時間早くきてリハーサルして…….

そして頑張った話をする.

不毛だ.
時間の無駄としか思えない.

結局そんなわけなので,禁則事項です したときも 禁則事項です のときも禁則事項です のときも,ポジティブな話だけを共有される.
実態はやばくて,仕事としては対応しなきゃならないのに,そういう話はひっそりと聞かされるばかりで,延々「うちは大丈夫」「こんなにすごい」「頑張ってる」ばかり聞かされるので,俺の周りではこれを大本営発表と呼ぶ

一応補足すると,一時期ちゃんと業績や会社の状態についての話をしていた時期もあった.が取りやめになった.
それ取りやめにしたんなら朝会もやめればいいのに……

これも我々がいたときはこうだっただけなので,今は違うのかも.

朝会から逃げる

普段より早い9:30開始なので,遅刻してくる人もいる.
そうすると,遅刻する人をチェックするようになった.

そもそもが不毛な話しか聞けない上に,遅刻チェックまでされるようになったので,最初から朝会に行かずに平然と10:00に出社するちょっとでも遅刻したらチェックされるので,10:00まで別の階で待機する という技を使う猛者もいた.
そりゃそうなるわ,合理的だ.

CW3

船を降りてもらって構わない

2017年にCW3というのを打ち出した.

  • 10倍ゴール、100倍ゴール
  • まず決めて、すぐ動く
  • 最高、最良

で,この価値観に納得できない人は 船を降りてもらって構わない,という話も一緒にされた.

2018年9月期に倍の成長をするという目標を立て、それに合意できる人に残ってほしいというメッセージを発信し

「あ,いいんだ,辞めるか」と思ったのは,俺だけじゃなかったらしい.

説明会もある!

このCW3,数人ずつ社長室に呼ばれて説明を受けるというイベントがあった.

なんで作ったのかとかを聞くんだけど,その後話を聞いていると,

  • 意見の対立により経営会議が進まない
  • CW3という価値観を提示して納得できない人に去ってもらった
  • 最近はスムーズにことが進む

という流れだったと記憶している.

いや,それって反対意見を言ってるやつを粛清しただけじゃん.

そりゃ異を唱える人を辞めさせればスムーズだろうよ.でもそれって禁則事項 というだけの話なのでは?

こうやって会社がビジョンとかバリューとかを布教しようとし始めると,一気に萎えるな…….

意思決定という言葉

CW3に合わせて,言葉の定義の話も出てくるようになった.
特に頻繁に出てきたのが「意思決定」という言葉.

ただ,我々が意見を言ったときに「それは意思決定だから」という返し方をされることがある.

前述の朝会が不毛だという話,一度社員側から「朝会辞めてみない?」という話が出たのだが,これに対して「意思決定だから続ける」という返答をされた.

これってつまり「文句を言うな,決めたことに従え」ということなんだよね(せめて「検討する」くらいの大人な返しをすればいいのにね).

決めるのは別にいいんだけど,フィードバックを受け付けないという姿勢は,なんというか愚かだ.
これでは改善するサイクルが発生しないし,単に自分の好きなように決めたいだけにしか見えない.
けど,それで「Be Agile」 と言い始めてるしきっと改善したんでしょう.

エンゲージメントスコア

最近はみんなモチベーションがあがってAAAになったらしい?

でも実態としては禁則事項ですとして禁則事項ですという話が出ていて,その結果禁則事項です禁則事項です禁則事項ですして回答するという事態になっていたので,そりゃスコア上がるわな…….

はてブで誰かが鋭いコメントしていたけど,上記のような理由により不満を持っている人が辞めたというのはかなり大きと俺は思っているよ

そこに賛同できないという人は、会社を去りました。

それもそれで会社のやり方だとは思うけどね.

ただそれによって,マネージャーやCTO,CIOを含むエンジニアが半分くらいゴソっと抜けたので,

こういう大変なことになってたんでしょうけど,新しい人も入ってそうだしきっとマトモになったんだろう.

まとめ

みたいな話を辞める前に会社の日報に書いたら,

  • 口だけのやつはいらない
  • 経営者目線を持て

(意訳)

と言われたので,ああ,辞めてよかった.

と,俺は思っているんだけど,また他の人から見たら違うのかも知れない.
最近は,フルフレックスも,フルリモートも始まったらしいしね.
https://www.wantedly.com/companies/crowdworks2/post_articles/130568
https://www.wantedly.com/companies/crowdworks2/post_articles/141208

俺は多分自分の知り合いにこの会社を薦めることはもうないと思うけど,今はきっと全部解消されて良い会社になったんだろうな!
居る人はいい ってみんな言ってるので頑張って.

46
11
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
46
11