スタートアップにクリーンアーキテクチャを適用したが、技術的負債が塵積った件 〜開発合宿で技術的負債を粉砕します〜

こんにちは。こんばんは。おはようございます。
アンドパッドで現在はバックエンドの方のエンジニアをやっている原田です。

アンドパッドには2021年6月にJOINしまして、現在までANDPADボードの開発に携わっています。
ANDPAD施工管理が比較的長期間の工事をターゲットにしているのに対して
ANDPADボードは1日〜数日の間に短期間の工事や施工を行う際のスケジュール管理を行えるサービスです。

lp.andpad.jp

今回は入社3ヶ月目というきりの良いタイミングで今まで行ってきたことを振り返りつつ、直近行った技術的負債を軽減するための「開発合宿」について書いていきます。
一応最初に書いておきますが、リファクタリングに関するチートスキルはないのでバーンとやってドーンと解決みたいなド派手な解決ではなく地道な改修作業をちまちま行いましたという内容です。

入社してからやってきたこと

入社してから現在までにやってきた事としては

  • ANDPADボードの新規機能要件定義と設計、実装 & ローンチ🎉
  • ANDPADボードのリファクタリング
  • 他のプロダクト覗いて必要そうな機能のissue書いたり修正PRを書く
  • 切り出したいサービスについてのDesignDocを書く

など。

ベンチャーならではだと思いますが、現状では自分の担当プロダクト以外のプロダクトに関しても気になった事に対して口を出せる自由度の高さはあります。
もちろん会社が成長するにつれ必然的にルールが増えるので自由にやれるのは今だけの特権なのかもしれませんね。

ANDPADボード開発合宿

ここからが本題

ANDPADボードは比較的新しいプロダクトでクリーンアーキテクチャの設計を取り入れているのですが、作り始めた時の要件から顧客要望をスピード重視で取り入れていった経緯があるため細かい実装部分は割とごちゃごちゃしていました。

ここら辺はシステムを使用するターゲットがある程度定まっている受託開発と違い、システムの要件やターゲット層自体を状況により切り替えていく必要のあるベンチャーならではの課題な気がします。
要するにプロダクト初期段階で想定していたターゲットや使い方とはまるで違う使い方を要求されることが多々あり、UIやら何やらゴソッと変わる可能性があるのでドメイン設計も何もあったもんじゃないということですね。

ANDPADボードが8月に無事ローンチし売り込みに行くターゲット層もある程度定まってきたというタイミングで今後の機能拡張をスピード感もって行うために今までの技術的負債を解消する

「ANDPADボード開発合宿」

と称して新規機能開発を1スプリント*1だけ停止して負債解消に充てる期間を用意しました。

「合宿」と言っても同じ場所に寝泊まりして徹夜でコードをイジるとかではなく、通常業務の中でエンジニアが自由に動ける時間を作ったという概念的なものですのでそこは誤解なきよう。

ANDPADボードAPIで使用しているLibrary&監視Service

まずANDPADボードのLibrary構成は以下の通り

よくある感じの構成ですね。
個人的にはgorm v1という部分が気になりはします*2が、誤爆で全件Deleteしないようgorm.DBの薄いWrapperを実装しています。とはいえ今回の話題とは関係ないので話としては省略。
onsi/ginkgoについては元々アンドパッド自体Rubyに強い会社なのでRSpec的な書き方をしたくて導入したんだと思います。3ヶ月触った今の所ginkgoにそこまでメリットを感じていない(CIだと勝手にlogにcolor codeが付くのでむしろ邪魔)ので、これから導入されようとしている方はそれ本当にtestingじゃなくてginkgoじゃないとダメなの?という点をよく考えた方が良いかもしれません。

話が逸れました。

現状課題の把握

まず合宿で技術的負債の解消を行うにあたり、実際に技術的負債となっていて今後の機能追加に支障がありそうな部分を洗い出していきました。

Fat Repository

  • 特定のアプリバージョンにのみ必要なparameterの値の修正がRepositoryに入っている
  • parameterのValidation checkがRepositoryに入っている
  • 複数ドメインにまたがる処理が同一ドメインのRepositoryに記載されている

テスト関連

  • e2eの不足
  • Repository層のテスト以外のUsecase層、Service層はたまたHandler層のテストに至るまでDBに依存する過剰なテストとなっている

OpenTracing関連

  • requestはTrace出来ているがsqlがtrace出来ていない

CIパフォーマンス関連

  • mockgenが遅い
  • testが遅い

などなど

洗い出した課題を元に優先順位付けを行いメンバーと分担しつつ技術的負債の解消を行っていきました。
優先順位としては

1. e2eを拡充して既存の動作を保証しリファクタリングによるデグレードを防ぐ
2. Service層、Handler層のテストのmock化
3. Fat Repositoryの解消
4. OpenTracingの対応
5. CIパフォーマンスの改善

としました。
リファクタリングをするにあたって一番怖いのはデグレードにより既存の動作が変わってしまうことです。
そのため、まずはe2eをしっかり書いて行こうという話になりました。

e2eの拡充

APIの全エンドポイントとそれぞれe2eの有無を洗い出し、メンバーと分担しつつ1つずつe2eのケースを追加していきます。
e2eの拡充自体は地道で非常に地味な作業なのですがe2eの拡充をするにあたって面白い出来事が起きました。

e2eを書くためにはまずはそのエンドポイント毎の正しい動作を認識する必要があり、仕様の確認をプロダクトオーナー/フロントエンドと詰める過程で実は使用していない不要なエンドポイントや仕様の明確でないエンドポイントが洗い出されてきたのです。
e2eをリファクタリングするだけのつもりが自然とAPIの仕様の理解が進み、仕様に関するドキュメントが充実していきました。
更には、リファクタリングの課題の洗い出しや既存バグの発見にも繋がりe2eのリファクタリングをするだけでもかなりの技術的負債の解消や解消への道筋作りに繋がりました。

Service層、Handler層のテストのmock化

Service層、Handler層のテストでDBにアクセスしていたというのは前述の通り。
何故そうなったのかというと後述するFat Repositoryになっていたからでしょう。
それはともかく各レイヤーでDBにアクセスしに行くと何が困るかというと、Repository層のロジックを修正しただけでService層やHandler層のテストも修正する必要が出てくるというメンテナンスコストの増大がまず1つ。
そして何より各レイヤーがDBに依存しているとレイヤー(golang的にはpackage)毎に並列でテストを走らせることが難しいためgo test -p 1で逐次的に実行する必要があり、テストの実行に非常に時間がかかるという事が問題点としてあります。

go testではテスト高速化のためにpackageが異なるテストはデフォルトでそれぞれ異なるバイナリが生成され別プロセスで並列に動作するようになっています。
-p 1を指定してgo testを実行するということはそのテスト高速化の恩恵が全く受けられないということです。

クリーンアーキテクチャ等のレイヤードアーキテクチャの良い点としては具象がレイヤー内に閉じ、他のレイヤーの呼び出しは全てinterface経由で行うこととなるため別レイヤーをmock化出来テストが書きやすいという点があります。
今回は本来クリーンアーキテクチャでのテストとしてあるべき姿に戻すべく各レイヤーの呼び出しをちゃんとmock化するように修正を行いました。

Fat Repositoryの処理を適切なLayerに分離

ANDPADボードは設計段階ではクリーンアーキテクチャで行こうと決めていたためしっかりとレイヤーの分離はされており、各レイヤーもクリーンアーキテクチャに沿ったネーミング、interface経由での呼び出しと依存方向の解決などの基本的な実装はクリーンアーキテクチャのよくあるテンプレート通りに行われていました。
クリーンアーキテクチャを採用したAPIでは例えば1つのエンドポイントを追加する毎に複数のレイヤーのインターフェースを設計する必要が出てきます。
プロダクト開発初期のスピード重視で開発しなければいけないタイミングでは、単純に新規機能追加に対する修正量が多くなりがちなので、よほどクリーンアーキテクチャを意識している人が確実にレビューを行う体制でない限りは結局どこかにロジックが集中してしまう結果になります。
ANDPADボードはローンチ前の小規模なプロジェクトだったこともあり、少人数でスピード重視で開発した結果ロジックがだいぶRepository層に寄ってしまいFatなRepositoryが出来上がっていました。

本来複雑怪奇なFat Logicにメスをいれることは比較的大きな決心が必要になると思いますが、今回は事前にe2eを拡充したことによりデグレードによるバグを検出しやすい環境となっていたこと、そして何より「開発合宿」と言う名の「技術的負債をなくすために新規開発止めて何でもやっていいよ」というリファクタリング期間が設けられていたことがAPI開発チームがその重大な手術を決心するきっかけになりました。

具体的に行った手術としては……

ドメインの異なる処理を分離

「とあるリクエストAを行うと同じRepository層内でαドメインモデルとβドメインモデルが処理される」というようなモデルベースの設計ではなく要件のユースケースベースの設計より作られたと思われる実装が幾つか見受けられました。

https://plantuml-server.kkeisuke.dev/png/TL9BJiCm5Dpx56utW4IIiaK8jUe2s24gn08XvCOlxHEE7TbEgmfnA3W8P-8rAVcqpihFCvCPU-AfyyBwCbUCZIHag87FNXQF1F2rzuMxJX8hjl4A_RfSbWviQhG7xUFKv8dOEYMMRhz6I8IBCcEZGWczTqmsLvDuaaXqderGfwgKK5hrxv_GEZTvRdHSxFC790lC2uKPWgnbGTqTQB129SzqG7VGdId8FFU6MrojFPTe8VLeD6ELwZnLIFTmbtmyhwSnZPcbwqMW9Q_fyGLZXKW_n0hulT1IWUMVZ7DjP0EypzXN8vhJzzQeMdSAFb1vDXCfL2xOx0AAolQGrDAzi0NO3LPUHyXGp37TpJXmlO0dA8n3R-okaz62Gp41HJUffIkKQaQ_ux_VpVJ3sE7lyEYsKtCpyPleLHhlGRv50zq5KHwHNw_CxgRd6gZg5AhyskAQgUSKRYE8LdwzUheRSYMQUWg0LDcKdYsz_3y0.png


ドメイン駆動設計の基本として複雑なユースケースの設計をする際はまずモデルベースで設計するとドメインロジックも自然とシンプルにまとまります。
今回の場合は「αドメインの処理を受け持つService層+Repository層」「βドメインの処理を受け持つService層+Repository層」に分離し「リクエストAが行われた場合、α Service と β Serviceの呼び出す」ように修正を行いました。

https://plantuml-server.kkeisuke.dev/png/XLH1JiCm4Bpx5QONGC99kIA4MlK0t10Lue1GvSQRriAn8zjfLI4UHHx2ct2IDd5QHpt5dfsT7NkIJ8obsXQPGEY2GSebuEdx_CK1U6rjRkxYMDDjjE9sNImB0pfHqeAqKQAoc6wDe4jJFGbt1KDIfKakgAmKuytjE1h7Z1kBBhXCHC6WqEAW7xldJ6MPab5UpH-Wp7cM2vvoO0qjo7i42PgA-CqPD7lUfO3KOgkmvgkrnOnhI2nN4g6QTPK8xivXRl1ergn6RfbgTpm2ak6cFBf6AAV95rq1VgAI2T3u6s4i5Mk1nHJzjAIPwzTADBnJyDbTdavf0hKARtS1HYsxdzILUyFce3UyrZf2XiOSbnT_lsVgPMTYNsnw-aZfcNg5N1bkbTvvlZfmo5g0SUmkIF4CTdNFemzMN1Sgjpli28TLYT3-NS3amGkrNU5xyi6KjS0-oK1IGHGJfzUBpj_qL4F1TFTzXbgYG7izt8iiqDQdb4C31vBf6i-pInGMgHtqOmltdn1znSQm8_JQ6sCLwWAUk4_Q_HN-0G00.png


Controller層で分岐した方が綺麗じゃないの?
という意見も自分の頭の中に浮かんできましたが、実装のわかり易さを優先してService層からService層のInterfaceを呼び出すことを許容しました。

https://plantuml-server.kkeisuke.dev/png/bLHBJiCm5Dpx56utW4IIiaK8jUe2s24gn0AXoerVMWl7ZcodLOKu53a8PyBvD75QHnKhs_FcpHjdaaor8ShaAKTeH25X0l3iRV5i0Rmn9jEtKQJ8BbmpiybNkGOLIs50c32MQKHscfELBjU0sO8E4gc2Z1DHAaRRcqauYIZJ1esOY7bE8LVye1_PTIxJL8emA-SFK1OipJXB6D2QvkKzW015UFHg3Ug6Ty4XCTX8hDXwOp1b2cB3f42eObt6dDbxw6iydjUxiTqcobul045nNHvV8PIH-9Ei0JyIGJae_8Km5fAsm7A6lblItFOhoMlUAVXa7vvAI0oL2cjF7aObsqpgohrX2r1RLcaT8KDZZilBtvypzQ8pqHJhdZvIEAPU89EQ6QdsZgyE7BBcOHoxyr8SWrtTyUY29TU6oimUMy9XLo3Kl0iuk7T2RNVuBdYdqXYW7y50KbuK1rQl5vrxwAdwWla7kK0UWMgy4vgdhKzfLSfcT-10DbtZUNQ1_2ALWtvm_luJegjO6xQ4Ndjjh4Hjn5FxKTl_mXy0.png

ここら辺はクリーンアーキテクチャの実装でいつも悩む部分ですね。クリーンアーキテクチャ実装に長けた人の意見を聞きたい所。

ここでドメインモデルの設計も修正する必要があるかと思ったのですが、幸いドメインモデル自体は適切に設計されていたためドメインロジックのみの修正で済みました。

ビジネスロジックをRepository層から分離

「ドメインの異なる処理を分離」とセットでの対応になりましたが、やることとしてはRepository層に書かれた「DB操作が必須なロジック」以外のビジネスロジックを分離することです。
パラメータ毎の分岐処理等をController層に入れるかService層に入れるか悩みはしましたが、「Fat Layerになりそうならまたリファクタリングすればいいや」の精神で一旦Service層に一括で分離しました。
ロジックを分離すること自体はそこまで大変ではないのですがテストの修正がだいぶ大変だったので、一部テストは無理に直さずリファクタリング後に新規で書くこととして一旦サクッと削除する判断を行っています。ちなみに消した分のテストは前述のe2eで担保してあるので安心感はありました。

requestのValidationをHandler層に分離

Repository層で行っていたValidationをもっと上位で判定させるため、gin frameworkのbinding時のvalidation機能を利用することにしました。
gin frameworkにも標準Validatorは存在するのですが

  • Validation errorの際の文章はユーザーに分かりやすいように日本語で出したい
  • requestされてくる時刻がUTCであるかチェックしたい
  • 時刻from - 時刻to の間隔が○ヶ月以内であることをチェックしたい

など標準のValidationでは難しい部分に関しては go-playground/validator を用いてカスタマイズした上で git frameworkのvalidatorをoverrideしました。
以下のinterfaceに沿ってさえいれば中身のValidationは好きにカスタマイズできます。

type CustomValidator interface {
	ValidateStruct(s interface{}) error
	Engine() interface{}
}

カスタムバリデーションの実装は大体こんな感じ。

package validator

import (
	"fmt"
	"reflect"
	"sync"

	"github.com/gin-gonic/gin/binding"

	"github.com/go-playground/locales/ja"
	ut "github.com/go-playground/universal-translator"
	"github.com/go-playground/validator/v10"
	trja "github.com/go-playground/validator/v10/translations/ja"
)

var (
	_ binding.StructValidator = &customValidator{}
)

type customValidator struct {
	once          sync.Once
	validate      *validator.Validate
	trans         ut.Translator
}

type CustomValidator interface {
	ValidateStruct(s interface{}) error
	Engine() interface{}
}

func NewCustomValidator() CustomValidator {
	return new(customValidator)
}

// ValidateStruct bindされたstructに対するValidationを実行
func (v *customValidator) ValidateStruct(s interface{}) (err error) {
	defer func() {
		v.checkStructValue = nil
		e := recover()
		if e != nil {
			err = fmt.Errorf("ValidateStruct error %v", e)
		}
	}()

	if kindOfData(s) == reflect.Struct {
		v.lazyinit()
		if e := v.validate.Struct(s); e != nil {
			err = v.getErrorMessages(e)
			return
		}
	}
	return
}

// Engine Validator Engineを処理化して返却
func (v *customValidator) Engine() interface{} {
	v.lazyinit()
	return v.validate
}

// lazyinit 初期処理の遅延読み込みを行う
func (v *customValidator) lazyinit() {
	v.once.Do(func() {
		v.validate = validator.New()
		// gin frameworkのValidation tag nameはbindingなのでそれに合わせる
		v.validate.SetTagName("binding")

		// Validation errorメッセージの日本語化
		jaJp := ja.New()
		uni := ut.New(jaJp, jaJp)
		v.trans, _ = uni.GetTranslator("ja")
		if err := trja.RegisterDefaultTranslations(v.validate, v.trans); err != nil {
			panic(err)
		}

		// 日本語フィールド用のカスタムタグを追加
		v.registerCustomFieldTags()
		// カスタムバリデーションを追加 実装は省略
		if err := v.registerCustomValidators(); err != nil {
			panic(err)
		}
	})
}

// ValidationのField名の代わりに表示する表示名を指定するタグをValidatorに設定する
func (v *customValidator) registerCustomFieldTags() {
	v.validate.RegisterTagNameFunc(func(fld reflect.StructField) string {
		transFieldName := fld.Tag.Get("ja")
		if transFieldName == "-" {
			return fld.Name
		}
		return transFieldName
	})
}

// getErrorMessages validatorで指定したメッセージを拾う
func (v *customValidator) getErrorMessages(err error) []string {
	if err == nil {
		return []string{}
	}
	messages := make([]string, 0, 10)
	for _, m := range err.(validator.ValidationErrors).Translate(v.trans) {
		messages = append(messages, m)
	}
	return messages
}

func kindOfData(data interface{}) reflect.Kind {
	value := reflect.ValueOf(data)
	valueType := value.Kind()

	if valueType == reflect.Ptr {
		valueType = value.Elem().Kind()
	}
	return valueType
}

registerCustomFieldTags() に関して、Translator実装のデフォルトのままではメッセージは日本語化されてもstructのフィールドの名前がそのまま出力されてしまうので、フィールド名をちゃんと日本語で出すための処理を追加してあげています。実際のコードでは来たるべき(来るのか?)グローバル対応考えてもう少し変更していたり。

type HogeParams struct {
	Name string `binding:"required,min=1,max=20" ja:"名前"`
	Date   *time.Time  `binding:"omitempty,utc" ja:"日付"`
}

jatagを定義してあげるとreflectでFieldの名前を拾ってきてエラーメッセージにも日本語名が表示されるようにしています。
使用するためににはgin.New()する前にcustom validatorをoverrideしてあげるだけでOKです。

binding.Validator = validator.NewCustomValidator()
r := gin.New()

OpenTracingの対応

OpenTracingというよりはDatadogを使用しているのでDatadog APMが正しいです。
ANDPADボードでは当初Datadog APMのトレーシング処理自体は埋まっていましたが、gorm実行時のSQLログにDatadog APMのtrace_id, span_idを紐付けていませんでした。
なので対応としてはgormでSQLを実行している処理全部に対して WithContext()context.Contextを渡してあげるというのが今回行った対応になります。
それ以上でもそれ以下でもないのであまり書くことはないのですが、とにかく箇所が多かったので結構大変でした。

CIパフォーマンスの改善

  • テスト実行速度の改善!

repository層以外のテストを全てmock化することでテストを並列に実行でき、CIも爆速に!!……なるはずだったのですが、如何せん量が多くて今回の開発合宿では間に合いませんでした。
徐々にmockに移行していく所存です。

  • mockgenの速度の改善!

今までMakefileで逐一mockgenを叩いていたのをgo-task/taskに切り替え task -p でpackage毎のmock作成を並列実行するように修正を行いました。
これによりlocalの実行でも40%くらいの高速化を実現しました。

またgo-task/taskのymlにBuildファイルを切り替えたおかげでMakefileよりもだいぶメンテしやすくなったという副次的な効果がありました。

開発合宿の感想など

開発合宿をやってみて結構な技術的負債を粉砕できたと思います。
今回のような安心してリファクタリングを行える時間を作るという事がコードをきれいに保つために重要だと実感しました。
それに加えて、今回の修正内容をメンバーと共有しつつソフトウェアの実装においてどのような設計を志すべきかレビュアーの観点を揃えるという事が重要だと感じています。

プロダクト開発を行っている会社の場合、新規開発を止めることはそれ自体競合他社に対して開発スピードで追いつかれるというリスクではありますが技術的負債を抱えたまま走り続けていつか爆発させるより、ある程度の期間足を止めリファクタリングに力を入れる事でその後の新規開発をやりやすくするという判断が行えるとより強いプロダクトになるのではないかと思っています。

そういえば書き忘れましたが、今後は緩めに入っていたLinterを徐々に強くしていく予定です。
機械的に弾ける場所に関してはやはり機械的に弾いてあげる方がレビュアーの負荷が下がって良いです。
後はやっぱりgormはどうにかしたいかなと思っていたりします。しかし、1スプリントだとどうにかなる気がしない…もう少し人が増えてから考えることにします。

クリーンアーキテクチャについて語りたい人!
そしてgolang好きな人、一緒にワイワイ開発しましょう!




アンドパッドでは一緒に働く仲間を大募集しています!ご興味が沸きましたらぜひ採用サイトもご覧ください。

engineer.andpad.co.jp

*1:ANDPADボードの1スプリントは2週間

*2:Gorm v1の過去のバージョンではstructのkeyの値にゼロ値が入った場合、Where条件がないとみなして全件削除を行っていた qiita.com