初めに
こんにちは！ kintone開発チームでソフトウェアエンジニアをしている池田 (@motacapla) です。
今回が初投稿となります、よろしくお願いいたします！
kintoneではHTTPリクエストのURLをルールベースで処理して書き換えたり、特定の属性値を設定するため UrlRewriteFilter (以降urlrewriteと呼びます) という機構を利用していました。
しかしながらurlrewrite自体は既に10年以上リリースがなく、複数のルールに当てはまるリクエストも存在しており、メンテナンスや学習のコストが高まっている問題がありました。 このurlrewriteで実現している処理は Spring Frameworkの標準的な機能で代用することが出来るため、有志の方が集まって2022年夏頃から移行作業を進めていました。
移行時の課題と解決策
今回の移行では、主に3つのポイントがあります:
- 複雑化したルールの整理、依存の排除（書き換えの複数回適用など）
- Spring Frameworkの仕組みへ移行し、実装もシンプル化
- 意図しないエンドポイントの発生を防ぐための自動テスト拡充
複数のルールで処理されるURL
既存のurlrewriteはServlet Filter (以降Filterと呼びます) として定義されており、いずれかのルールに当てはまってリクエストがforwardされる度に30個以上あるルールをそれぞれ該当するか確認する処理が走っていました。
FilterRegistrationBeanのDispatcherTypesにFORWARDが設定されていたため、ルールに該当してforwardされた場合には繰り返しFilterのルールをもう1周確認していました。
例. 定義されていたルールのサンプル
<rule match-type="regex">
<name>ruleA</name>
<from>^/foo/(?:([1-9][0-9]*)/)?(.*)$</from> // このルールに該当する正規表現
<set name="id">$1</set> // 属性値idの設定, setタグでServletRequest#setAttribute
<to type="forward">/$2</to> // 書き換えてforward
</rule>
<rule match-type="regex">
<name>ruleB</name>
<from>^/bar/(.*)$</from> // このルールに該当する正規表現
<set name="attr">$1</set> // 属性値attrの設定
<to type="forward">/$1</to> // 書き換えてforward
</rule>
中には複数のルールに該当するURLも存在しており、ルール同士に依存関係がありました。
属性値を使った再書き換えの抑制
ルールに該当するURLであっても書き換えされてほしくないURLが存在しており、rewritedという属性値を設定してcondition句で比較させる制御をしていました。この制御によって、例えば1周目では該当していたルールが2周目で該当しなくなるといった事が発生していました。
このように単一リクエストの中でも複数回Filterの確認が走り、更に周回によって当てはまるルールが異なるので、ルール同士が前後で密に依存しておりました。
例. rewritedによる制御のサンプル
1週目ではruleC,ruleDに該当して、rewrited=1が設定されます。forwardされた2周目ではruleDにのみ該当します。
<rule match-type="regex">
<name>ruleC</name>
<condition type="attribute" name="rewrited" operator="notequal">1</condition> // rewrited != 1 なら適用
<from>^(.*)$</from>
<to type="forward">$1</to>
</rule>
<rule match-type="regex">
<name>ruleD</name>
<from>^(.*)$</from>
<set name="rewrited">1</set> // rewrited = 1を設定
<to type="forward">$1</to>
</rule>
解決するために
影響が大多数のAPIに及んだり他のFilterで利用する属性値を設定する必要がある等の理由で依存を除けないルールは、DispatcherTypes.REQUESTのみ持つFilterを新しく作成してその中で定義しました。
新しいFilterのサンプル
private static final Pattern RULE_B_PATTERN = Pattern.compile("^(?:/foo(?:/[0-9]+)?)?/bar/(.*)$");
private boolean forwardRequestIfRuleBMatcherFinds(HttpServletRequest req, ServletResponse servletResponse,
String path) throws ServletException, IOException {
Matcher matcher = RULE_B_PATTERN.matcher(path);
if (!matcher.find()) {
return false;
}
String uri = "/" + matcher.group(1);
req.getRequestDispatcher(uri).forward(req, servletResponse);
return true;
}
private static final Pattern RULE_A_PATTERN = Pattern.compile("^/foo/(?:([1-9][0-9]*)/)?(.*)$");
private boolean forwardRequestIfRuleAMatcherFinds(HttpServletRequest req,
ServletResponse servletResponse, String path) throws ServletException, IOException {
Matcher matcher = RULE_A_PATTERN.matcher(path);
if (!matcher.find()) {
return false;
}
String extractedPath = "/" + matcher.group(2);
if (forwardRequestIfRuleBMatcherFinds(req, servletResponse, extractedPath)) {
return true;
}
req.getRequestDispatcher(extractedPath).forward(req, servletResponse);
return true;
}
private void setAttributes(HttpServletRequest req, String path) {
...
Matcher matcherB = RULE_B_PATTERN.matcher(path); // 依存を消したので、RULE_A_PATTERNの前でも可
if (matcherB.find()) {
req.setAttribute("attr", matcherB.group(1));
}
Matcher matcherA = RULE_A_PATTERN.matcher(path);
if (matcherA.find()) {
req.setAttribute("id", matcherA.group(1));
}
...
}
public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException {
...
setAttributes(req, path);
...
if (
forwardRequestIfRuleAMatcherFinds(req, servletResponse, path)
|| forwardRequestIfRuleBMatcherFinds(req, servletResponse, path)
) {
return;
}
filterChain.doFilter(servletRequest, servletResponse);
}
ルール同士の依存関係を排除できる場合は、正規表現で両方の条件を受け付けることで対応しました。
例. ruleAとruleB間の依存の削除
private static final Pattern RULE_B_PATTERN = Pattern.compile("^(?:/foo(?:/[0-9]+)?)?/bar/(.*)$");
setAttributes()メソッドはルールの順序を気にすることなく属性値を設定できるようになりました。
例. @RequestMappingで受け付けるコントローラのURLを正規表現で複数受け付けることで、ルール自体を削除
<rule match-type="regex">
<name>ruleE</name>
<from>^/api/piyo$</from>
<to last="true">piyo.json</to>
</rule>
/bar/api/piyo というURLにruleBとruleEが適用され、以下のコントローラにマッピングされていました。
@RequestMapping("piyo.json")
こちらのルールを削除して、.json付き/無し両方で動作していた振る舞いを保つために、両方ともマッピングされるように修正しました。
@RequestMapping({ "piyo.json", "piyo" })
ルールの一部が@RequestMapping で直接コントローラにマッピングするようになったため、新しいFilterに含まれるルール数も削減されました。
依存関係を排除したことで、rewrited属性値による制御も不要になりました。
存在が知られていないAPI
URLの原形が分かりづらい影響か、原形が /bar/foo であるケースを、既存の単体テストでは変換後の /foo としてテストしていたりと不十分であることが分かったので、移行の際に単体テストを整備しました。
また、非同期ジョブを発行するAPI 仕様書に漏れがあることも見つかり、こちらもURLの原形が分かりづらい影響が少なからずあったのではないかと推測しています。
今回は安全に移行するため、新しく定義したFilterの中でruleBの書き換え処理を行うことにしました。
Hyrumの法則
元々は複数のルールが適用された場合にURLの原形が分かりづらく、本来意図していない形式のURLでも動作してしまう状態になっていました。
例. /foo/1/bar/piyo.json というURLがあった場合
ruleAによって /bar/piyo.json に書き換えてforwardし、さらにruleBによって piyo.json に書き換えられる。
仮に、意図していない/bar/piyo.jsonというURLがあって属性値idが不要であれば、マッピングされて動作してしまう。
また、ruleAでは多くのURLから/foo を削除していたため、/foo/foo/api も /foo/api も書き換え後のURLは /api になってしまい、両方のURLが混在していました。
恐らく意図したURLは /foo/api ですが、現状/foo/foo/api といった形式も受け付けることができており、振る舞いに依存していることが発覚しました。
必要十分な処理だけに留めることの大切さを実感したので、今後は不要な処理を極力減らすような改善を実施していきたいです。
安全なリリースのために
未だに適用されているのか不明なルールがいくつかあったため、production環境のログから実際にリクエストされているURLや件数を調査したり、commit logを読むことで、ルールの削除・移行の意思決定を行いました。 中には一つ移行するだけでも広範囲に影響するルールがあったため、数ヶ月に渡って少しずつ変更をリリースしていきました。 masterブランチにマージしてから他製品と連携している箇所が壊れていることが発覚し、開発環境が壊れてしまいご迷惑をおかけしたりしたこともありましたが、回帰試験だけでなく単体試験の整備や結合試験を実施することで、できるだけ既存の振る舞いと品質を保ちながらリリースを進めました。
終わりに
urlrewriteの機構に依らずにURLの書き換えや属性値の設定ができるようになりました。単体テストのテストケースからどういったURLがどう書き換わるかが明示的に分かりやすくなったことや、Filterでの書き換え処理自体が大幅に減ったことで、メンテナンス・学習コストは下がったと思います。加えてドキュメントを整備して新しいエンドポイントの定義指針を開発チームに共有しました。
kintone開発チームでは、絶賛メンバーを募集しています! 今回のようなレガシー化したソースコードの改善だけでなく新規機能の開発、改修に興味がある方は、ぜひ応募をご検討ください。お待ちしております！
参考: Hyrumの法則とレガシ―コード置き換えの実践
Hyrumの法則とレガシ―コード置き換えの実践 - Cybozu Inside Out | サイボウズエンジニアのブログ
