[インデックス 13602] ファイルの概要

このコミットは、Goプロジェクトのコードレビューダッシュボードにおける、非レビューアユーザー向けの表示と機能に関する修正、およびテスト用のCGI引数名の変更を目的としています。

コミット

commit e803e1cfa61d6677025440b7336e20854f619755
Author: David Symonds <dsymonds@golang.org>
Date:   Wed Aug 8 13:16:21 2012 +1000

    misc/dashboard/codereview: fixes for non-reviewers.
    
    Also rename the testing CGI argument from "email" to "user".
    
    R=golang-dev, r
    CC=golang-dev
    https://golang.org/cl/6454117

GitHub上でのコミットページへのリンク

https://github.com/golang/go/commit/e803e1cfa61d6677025440b7336e20854f619755

元コミット内容

このコミットは、Goプロジェクトのコードレビューダッシュボード（misc/dashboard/codereview）に対する修正を含んでいます。主な内容は以下の2点です。

1. 非レビューアユーザー向けの修正: レビューアではないユーザーがダッシュボードを利用する際の表示やフィルタリングに関する問題を解決します。
2. テスト用CGI引数名の変更: テスト目的で使用されるCGI引数の名前を"email"から"user"にリネームします。

変更の背景

Goプロジェクトでは、Rietveldのようなコードレビューシステムを使用して変更（Change List, CL）を管理していました。このダッシュボードは、ユーザーが自身のCLやレビュー対象のCLを一覧で確認できるように設計されています。

このコミットが行われた背景には、以下のような課題があったと推測されます。

• 非レビューアユーザーの体験改善: ダッシュボードが主にレビューアの視点で作られていたため、レビュー権限を持たない一般の貢献者や開発者が自身のCLを効率的に追跡したり、関連するCLを適切にフィルタリングしたりする機能が不足していた可能性があります。特に、自身のCLが「CLs sent by you」（あなたが送ったCL）のセクションに正しく表示されない、または「Other CLs」（その他のCL）から自身のCLが除外されないといった問題があったと考えられます。
• テスト環境の明確化: emailというCGI引数は、おそらく特定のユーザーとしてダッシュボードをテストするために使用されていました。しかし、emailという名前は、実際のメールアドレスを指すのか、それとも単にユーザー識別子を指すのか曖昧であった可能性があります。userへの変更は、その引数がユーザー識別子として機能することをより明確にし、テストの意図を分かりやすくするためのリファクタリングです。
• データストアの効率的なクエリ: 非レビューア向けの機能追加に伴い、Google App Engine (GAE) のデータストアに対する新しいクエリパターンが必要になりました。特に、CLの「Owner」プロパティによるフィルタリングが導入されたため、これに対応するデータストアインデックスの追加が必須となりました。

前提知識の解説

このコミットを理解するためには、以下の技術的・概念的な知識が役立ちます。

• Go言語 (Golang): コミットの対象となっているコードはGo言語で書かれています。Goの基本的な構文、net/httpパッケージ（HTTPリクエストのハンドリング）、syncパッケージ（並行処理のための同期プリミティブ）、および構造体やインターフェースの利用方法に関する知識が必要です。
• Google App Engine (GAE): misc/dashboard/codereview/index.yamlファイルが存在すること、およびコード内でdatastoreのような用語やFilter, Limit, GetAllといったメソッドが使われていることから、このダッシュボードアプリケーションがGoogle App Engine上で動作していることがわかります。GAEのデータストアはNoSQLデータベースであり、効率的なクエリのためにはindex.yamlでカスタムインデックスを定義する必要があります。定義されていないインデックスを使ったクエリは、実行時エラーになるか、非常に非効率な全スキャンを引き起こします。
• コードレビューシステム: 「CL」（Change List）、「Reviewer」（レビューア）、「Author」（著者）、「Owner」（所有者）といった用語は、GerritやRietveldのようなコードレビューシステムで一般的に使用されます。
  • CL (Change List): 変更の単位。Gitのコミットに相当しますが、レビュープロセスを経る前の提案段階の変更を指すことが多いです。
  • Author: コードを実際に書いた人。
  • Owner: そのCLの責任者。多くの場合、Authorと同じですが、Authorが複数いる場合や、別の人がCLを提出した場合などに区別されることがあります。特に、テスト目的でユーザーを切り替える際に、Authorとは異なるOwnerを設定するシナリオが考えられます。
  • Reviewer: そのCLをレビューし、承認する責任を持つ人。
• CGI引数とHTTPフォーム値: Webアプリケーションにおいて、クライアントからサーバーにデータを渡す方法の一つです。HTTPリクエストのURLのクエリパラメータ（例: ?key=value）や、POSTリクエストのボディに含まれるフォームデータとして渡されます。Goのhttp.Request.FormValue()メソッドは、これらの値を取得するために使用されます。
• 並行処理 (Concurrency): sync.WaitGroupやchan errorが使用されていることから、ダッシュボードが複数のデータ取得処理を並行して実行していることがわかります。これは、ユーザー体験を向上させるために、複数のデータソースからの情報を同時にフェッチするためによく用いられるパターンです。

技術的詳細

このコミットの技術的詳細は、主に以下の点に集約されます。

1. ユーザー識別子の変更と柔軟なユーザー設定:

   • テスト目的でユーザーを切り替えるためのCGI引数が"email"から"user"に変更されました。これは、引数の意味をより明確にするためのリファクタリングです。
   • front.goのhandleFront関数内で、data.UserIsReviewerがfalse（非レビューア）の場合にcurrentPerson変数が直接u（フォーム値から取得したユーザー識別子）に設定されるようになりました。これにより、非レビューアユーザーもダッシュボード上で自身のCLを正しく識別できるようになります。以前は、レビューアでない場合はemailToPersonマップに存在しないため、currentPersonが空になる可能性がありました。

2. CLクエリのロジック改善（Ownerの導入）:

   • 「CLs sent by you」（あなたが送ったCL）を表示するtableFetch(1)のクエリロジックが変更されました。
     • レビューアの場合 (data.UserIsReviewerがtrue) は、引き続きAuthorプロパティでフィルタリングされます。これは、レビューアがコードの実際の著者に基づいてCLを追跡することを意図しているためと考えられます。
     • 非レビューアの場合 (data.UserIsReviewerがfalse) は、Ownerプロパティでフィルタリングされるようになりました。これは、非レビューアが自身のCLを追跡する際に、AuthorではなくOwnerとして登録されているCLを「自分が送ったCL」と見なす必要があるためです。この変更は、特にテストシナリオや、AuthorとOwnerが異なる場合の柔軟性を提供します。
   • このOwnerプロパティによるフィルタリングを効率的に行うため、misc/dashboard/codereview/index.yamlに新しいデータストアインデックスが追加されました。

3. 「Other CLs」のフィルタリングロジックの拡張:

   • 「Other CLs」（その他のCL）を表示するtableFetch(2)のフィルタリングロジックが変更されました。
   • 以前は、レビューアの場合にのみ、自身のAuthorまたはReviewerであるCLが「Other CLs」から除外されていました。
   • 変更後、この除外ロジックはdata.UserIsReviewerの条件が外され、すべてのユーザーに適用されるようになりました。
   • さらに、除外条件にcl.Owner == currentPersonが追加されました。これにより、現在のユーザーがOwnerであるCLも「Other CLs」から除外されるようになり、ダッシュボードの表示がより正確になりました。

4. データストアインデックスの追加:

   • index.yamlに以下の新しいインデックスが追加されました。

     - kind: CL
       properties:
       - name: Owner
       - name: Modified
         direction: desc
     

   • このインデックスは、front.goで導入されたactiveCLs.Filter("Owner =", currentPerson)のようなクエリをデータストアが効率的に処理するために不可欠です。GAEのデータストアは、複合クエリ（複数のプロパティでフィルタリングやソートを行うクエリ）に対して、対応するインデックスが定義されていないとエラーになるか、非常に遅くなる特性があります。この追加により、非レビューア向けの機能がパフォーマンス上の問題なく動作するようになります。

コアとなるコードの変更箇所

misc/dashboard/codereview/dashboard/front.go

--- a/misc/dashboard/codereview/dashboard/front.go
+++ b/misc/dashboard/codereview/dashboard/front.go
@@ -39,11 +39,14 @@ func handleFront(w http.ResponseWriter, r *http.Request) {
 	var currentPerson string
 	u := data.User
 	you := "you"
-	if e := r.FormValue("email"); e != "" {
+	if e := r.FormValue("user"); e != "" {
 		u = e
 		you = e
 	}
 	currentPerson, data.UserIsReviewer = emailToPerson[u]
+	if !data.UserIsReviewer {
+		currentPerson = u
+	}
 
 	var wg sync.WaitGroup
 	errc := make(chan error, 10)
@@ -63,10 +66,10 @@ func handleFront(w http.ResponseWriter, r *http.Request) {
 		}()
 	}
 
+\tdata.Tables[0].Title = "CLs assigned to " + you + " for review"
 	if data.UserIsReviewer {
 		tableFetch(0, func(tbl *clTable) error {
 			q := activeCLs.Filter("Reviewer =", currentPerson).Limit(maxCLs)
-\t\t\ttbl.Title = "CLs assigned to " + you + " for review"
 			tbl.Assignable = true
 			_, err := q.GetAll(c, &tbl.CLs)
 			return err
@@ -74,7 +77,13 @@ func handleFront(w http.ResponseWriter, r *http.Request) {
 	}
 
 	tableFetch(1, func(tbl *clTable) error {
-\t\tq := activeCLs.Filter("Author =", currentPerson).Limit(maxCLs)
+\t\tq := activeCLs
+\t\tif data.UserIsReviewer {
+\t\t\tq = q.Filter("Author =", currentPerson)
+\t\t} else {
+\t\t\tq = q.Filter("Owner =", currentPerson)
+\t\t}
+\t\tq = q.Limit(maxCLs)
 	\ttbl.Title = "CLs sent by " + you
 	\ttbl.Assignable = true
 	\t_, err := q.GetAll(c, &tbl.CLs)
@@ -89,14 +98,12 @@ func handleFront(w http.ResponseWriter, r *http.Request) {
 		\treturn err
 		}\n \t\t// filter
-\t\tif data.UserIsReviewer {\n-\t\t\tfor i := len(tbl.CLs) - 1; i >= 0; i-- {\n-\t\t\t\tcl := tbl.CLs[i]\n-\t\t\t\tif cl.Author == currentPerson || cl.Reviewer == currentPerson {\n-\t\t\t\t\t// Preserve order.\n-\t\t\t\t\tcopy(tbl.CLs[i:], tbl.CLs[i+1:])\n-\t\t\t\t\ttbl.CLs = tbl.CLs[:len(tbl.CLs)-1]\n-\t\t\t\t}\n+\t\tfor i := len(tbl.CLs) - 1; i >= 0; i-- {\n+\t\t\tcl := tbl.CLs[i]\n+\t\t\tif cl.Owner == currentPerson || cl.Author == currentPerson || cl.Reviewer == currentPerson {\n+\t\t\t\t// Preserve order.\n+\t\t\t\tcopy(tbl.CLs[i:], tbl.CLs[i+1:])\n+\t\t\t\ttbl.CLs = tbl.CLs[:len(tbl.CLs)-1]\n \t\t\t}\n \t\t}\n \t\treturn nil

misc/dashboard/codereview/index.yaml

--- a/misc/dashboard/codereview/index.yaml
+++ b/misc/dashboard/codereview/index.yaml
@@ -6,6 +6,12 @@ indexes:
   - name: Modified
     direction: desc
 
+- kind: CL
+  properties:
+  - name: Owner
+  - name: Modified
+    direction: desc
+
 - kind: CL
   properties:
   - name: Closed

コアとなるコードの解説

front.goの変更点

1. CGI引数名の変更とcurrentPersonの初期化:

   • r.FormValue("email")がr.FormValue("user")に変更されました。これにより、テスト時にユーザーを切り替えるための引数がuserになります。
   • if !data.UserIsReviewer { currentPerson = u }が追加されました。これは、フォーム値から取得したユーザー（u）がレビューアでない場合、currentPersonをそのユーザー識別子に直接設定することを意味します。emailToPersonマップはレビューアのメールアドレスと内部的な人物名をマッピングしているため、非レビューアの場合はこのマップに存在しない可能性があります。この行により、非レビューアでもcurrentPersonが正しく設定され、後続のクエリやフィルタリングで自身のCLを識別できるようになります。

2. テーブルタイトルの設定位置変更:

   • data.Tables[0].Title = "CLs assigned to " + you + " for review"の行が、if data.UserIsReviewerブロックの外に移動しました。これにより、レビューアであるかどうかにかかわらず、最初のテーブルのタイトルが常に「CLs assigned to [you/user] for review」と表示されるようになります。これは、非レビューアがダッシュボードを見た際にも、一貫したタイトルが表示されるようにするための調整です。

3. 「CLs sent by you」のクエリロジック変更:

   • tableFetch(1)（「CLs sent by you」）のクエリが大幅に変更されました。
   • 以前は常にactiveCLs.Filter("Author =", currentPerson)でフィルタリングされていました。
   • 変更後、data.UserIsReviewerの値に応じてフィルタリングするプロパティが切り替わります。
     • レビューアの場合: q.Filter("Author =", currentPerson)
     • 非レビューアの場合: q.Filter("Owner =", currentPerson)
   • この変更は、レビューアはCLの「Author」（著者）に基づいて自身のCLを追跡し、非レビューアはCLの「Owner」（所有者）に基づいて自身のCLを追跡するという、異なる視点に対応しています。これにより、非レビューアが自身のCLをより正確に把握できるようになります。

4. 「Other CLs」のフィルタリングロジック変更:

   • tableFetch(2)（「Other CLs」）のフィルタリングロジックが変更されました。
   • 以前はif data.UserIsReviewerブロック内にあったフィルタリング処理が、そのブロックの外に移動しました。これにより、このフィルタリング（自身のCLを除外する処理）がレビューアであるかどうかにかかわらず、すべてのユーザーに適用されるようになりました。
   • 除外条件にcl.Owner == currentPersonが追加されました。これにより、現在のユーザーが「Owner」であるCLも「Other CLs」のリストから除外されるようになり、ダッシュボードの表示がより正確で、ユーザーにとって関連性の低いCLが減りました。

index.yamlの変更点

• 新しいデータストアインデックスが追加されました。

  - kind: CL
    properties:
    - name: Owner
    - name: Modified
      direction: desc
  

• このインデックスは、front.goで導入されたactiveCLs.Filter("Owner =", currentPerson)のようなクエリをGoogle App Engineのデータストアが効率的に実行するために必要です。データストアは、特定のプロパティでフィルタリングし、別のプロパティでソートする複合クエリを実行する際に、対応するインデックスがindex.yamlに定義されていることを要求します。このインデックスがないと、Ownerによるフィルタリングを含むクエリは失敗するか、非常に非効率な処理になります。

関連リンク

• Go言語の公式ウェブサイト: https://golang.org/
• Google App Engine (GAE) の公式ドキュメント: https://cloud.google.com/appengine/docs (当時のGAEのデータストアに関するドキュメントが参考になります)
• Rietveld (コードレビューツール): https://code.google.com/p/rietveld/ (Goプロジェクトがかつて使用していたコードレビューシステム)

参考にした情報源リンク

• コミットハッシュ: e803e1cfa61d6677025440b7336e20854f619755
• GoプロジェクトのGitHubリポジトリ: https://github.com/golang/go
• Goのコードレビューシステムに関する情報 (当時のGoプロジェクトの貢献ガイドラインなど): https://golang.org/doc/contribute.html (当時の情報にアクセスできる場合)
• Google App Engine Datastore Indexes: https://cloud.google.com/appengine/docs/standard/go/datastore/indexes (当時のGo向けGAEデータストアインデックスのドキュメント)
• Goのnet/httpパッケージドキュメント: https://pkg.go.dev/net/http
• Goのsyncパッケージドキュメント: https://pkg.go.dev/sync
• Goのappengine/datastoreパッケージドキュメント (当時のGo向けGAEデータストアAPI): https://pkg.go.dev/google.golang.org/appengine/datastore (当時のバージョンにアクセスできる場合)