[gold] Add RPC to smartly redirect changelist searches.
This should greatly increase the chance of redirecting the user
to a page for which they see untriaged digests (i.e. the reason
they clicked on the link in the logs, code review comment, or email).
Bug: skia:9879
Change-Id: Ie08b7826c02085dea785e67812f8c76c4921daa7
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/290025
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/golden/cmd/skiacorrectness/main.go b/golden/cmd/skiacorrectness/main.go
index a6b7456..b3e6712 100644
--- a/golden/cmd/skiacorrectness/main.go
+++ b/golden/cmd/skiacorrectness/main.go
@@ -575,6 +575,7 @@
loggedRouter.HandleFunc("/diff", templateHandler("diff.html"))
loggedRouter.HandleFunc("/detail", templateHandler("details.html"))
loggedRouter.HandleFunc("/details", templateHandler("details.html"))
+ loggedRouter.HandleFunc("/cl/{system}/{id}", handlers.ChangeListSearchRedirect)
// This route handles the legacy polymer "single page" app model
loggedRouter.PathPrefix("/").Handler(templateHandler("index.html"))
diff --git a/golden/go/code_review/commenter/commenter_test.go b/golden/go/code_review/commenter/commenter_test.go
index 6b5067b..1bba1b5 100644
--- a/golden/go/code_review/commenter/commenter_test.go
+++ b/golden/go/code_review/commenter/commenter_test.go
@@ -242,7 +242,7 @@
mcr.On("System").Return("github")
// Pretend all CLs queried have 2 untriaged digests.
- msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.DigestList{
+ msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
Digests: []types.Digest{"doesn't", "matter"},
TS: indexTime,
}, nil)
@@ -289,7 +289,7 @@
mcr.On("System").Return("github")
// Pretend all CLs queried have 2 untriaged digests.
- msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.DigestList{
+ msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
Digests: nil,
TS: indexTime,
}, nil)
@@ -372,7 +372,7 @@
mcr.On("System").Return("github")
// Pretend all CLs queried have 2 untriaged digests.
- msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.DigestList{
+ msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
Digests: []types.Digest{"doesn't", "matter"},
}, nil)
@@ -458,7 +458,7 @@
mcr.On("System").Return("gerritHub")
// Pretend all CLs queried have 2 untriaged digests.
- msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.DigestList{
+ msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
Digests: []types.Digest{"doesn't", "matter"},
}, nil)
diff --git a/golden/go/search/frontend/types.go b/golden/go/search/frontend/types.go
index 57bf83e..75caa90 100644
--- a/golden/go/search/frontend/types.go
+++ b/golden/go/search/frontend/types.go
@@ -136,9 +136,14 @@
Right *SRDiffDigest `json:"right"` // The right hand digest, its params and the diff result.
}
-// DigestList represents multiple returned digests.
-type DigestList struct {
+// UntriagedDigestList represents multiple digests that are untriaged for a given query.
+type UntriagedDigestList struct {
Digests []types.Digest `json:"digests"`
+
+ // Corpora is filed with the strings representing a corpus that has one or more Digests belong
+ // to it. In other words, it summarizes where the Digests come from.
+ Corpora []string `json:"corpora"`
+
// TS is the time that this data was created. It might be served from a cache, so this time will
// not necessarily be "now".
TS time.Time `json:"ts"`
diff --git a/golden/go/search/mocks/SearchAPI.go b/golden/go/search/mocks/SearchAPI.go
index 85f17b8..63f5ca0 100644
--- a/golden/go/search/mocks/SearchAPI.go
+++ b/golden/go/search/mocks/SearchAPI.go
@@ -90,15 +90,15 @@
}
// UntriagedUnignoredTryJobExclusiveDigests provides a mock function with given fields: _a0, _a1
-func (_m *SearchAPI) UntriagedUnignoredTryJobExclusiveDigests(_a0 context.Context, _a1 tjstore.CombinedPSID) (*frontend.DigestList, error) {
+func (_m *SearchAPI) UntriagedUnignoredTryJobExclusiveDigests(_a0 context.Context, _a1 tjstore.CombinedPSID) (*frontend.UntriagedDigestList, error) {
ret := _m.Called(_a0, _a1)
- var r0 *frontend.DigestList
- if rf, ok := ret.Get(0).(func(context.Context, tjstore.CombinedPSID) *frontend.DigestList); ok {
+ var r0 *frontend.UntriagedDigestList
+ if rf, ok := ret.Get(0).(func(context.Context, tjstore.CombinedPSID) *frontend.UntriagedDigestList); ok {
r0 = rf(_a0, _a1)
} else {
if ret.Get(0) != nil {
- r0 = ret.Get(0).(*frontend.DigestList)
+ r0 = ret.Get(0).(*frontend.UntriagedDigestList)
}
}
diff --git a/golden/go/search/search.go b/golden/go/search/search.go
index 20a43315..a699d1d 100644
--- a/golden/go/search/search.go
+++ b/golden/go/search/search.go
@@ -819,7 +819,7 @@
// UntriagedUnignoredTryJobExclusiveDigests implements the SearchAPI interface. It uses the cached
// TryJobResults, so as to improve performance.
-func (s *SearchImpl) UntriagedUnignoredTryJobExclusiveDigests(ctx context.Context, psID tjstore.CombinedPSID) (*frontend.DigestList, error) {
+func (s *SearchImpl) UntriagedUnignoredTryJobExclusiveDigests(ctx context.Context, psID tjstore.CombinedPSID) (*frontend.UntriagedDigestList, error) {
var resultsForThisPS []tjstore.TryJobResult
listTS := time.Now()
clIdx := s.indexSource.GetIndexForCL(psID.CRS, psID.CL)
@@ -845,6 +845,7 @@
knownDigestsForTest := idx.DigestCountsByTest(types.IncludeIgnoredTraces)
var returnDigests []types.Digest
+ var returnCorpora []string
for _, tr := range resultsForThisPS {
if err := ctx.Err(); err != nil {
@@ -867,14 +868,18 @@
// This trace matches an ignore
continue
}
+ if corpus := p[types.CorpusField]; !util.In(corpus, returnCorpora) {
+ returnCorpora = append(returnCorpora, corpus)
+ }
returnDigests = append(returnDigests, tr.Digest)
}
// Sort digests alphabetically for determinism.
sort.Slice(returnDigests, func(i, j int) bool {
return returnDigests[i] < returnDigests[j]
})
- return &frontend.DigestList{
+ return &frontend.UntriagedDigestList{
Digests: returnDigests,
+ Corpora: returnCorpora,
TS: listTS,
}, nil
}
diff --git a/golden/go/search/search_test.go b/golden/go/search/search_test.go
index 0000b8c..3f8bd31 100644
--- a/golden/go/search/search_test.go
+++ b/golden/go/search/search_test.go
@@ -1476,6 +1476,7 @@
dl, err := s.UntriagedUnignoredTryJobExclusiveDigests(context.Background(), expectedID)
require.NoError(t, err)
+ assert.Equal(t, []string{"gm"}, dl.Corpora)
assert.Equal(t, []types.Digest{alphaUntriagedTryJobDigest, betaUntriagedTryJobDigest}, dl.Digests)
// TS should be very recent, since the results were freshly computed.
assert.True(t, dl.TS.After(time.Now().Add(-time.Minute)))
@@ -1594,8 +1595,11 @@
dl, err := s.UntriagedUnignoredTryJobExclusiveDigests(context.Background(), expectedID)
require.NoError(t, err)
- assert.Equal(t, []types.Digest{alphaUntriagedTryJobDigest, betaUntriagedTryJobDigest}, dl.Digests)
- assert.Equal(t, indexTS, dl.TS)
+ assert.Equal(t, &frontend.UntriagedDigestList{
+ Digests: []types.Digest{alphaUntriagedTryJobDigest, betaUntriagedTryJobDigest},
+ Corpora: []string{"gm"},
+ TS: indexTS,
+ }, dl)
}
// TestGetDrawableTraces_DigestIndicesAreCorrect tests that we generate the output required to draw
diff --git a/golden/go/search/types.go b/golden/go/search/types.go
index bcf5219..9a3dee2 100644
--- a/golden/go/search/types.go
+++ b/golden/go/search/types.go
@@ -30,5 +30,5 @@
// UntriagedUnignoredTryJobExclusiveDigests returns a list of untriaged, non-ignored,
// digests that are exclusive to a given CL (i.e. they don't also show up on the master branch
// in the last N commits).
- UntriagedUnignoredTryJobExclusiveDigests(context.Context, tjstore.CombinedPSID) (*frontend.DigestList, error)
+ UntriagedUnignoredTryJobExclusiveDigests(context.Context, tjstore.CombinedPSID) (*frontend.UntriagedDigestList, error)
}
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index e5d41d0..465ac50 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -3,6 +3,7 @@
import (
"context"
"encoding/json"
+ "fmt"
"net/http"
"net/url"
"sort"
@@ -471,17 +472,18 @@
return
}
- clID, ok := mux.Vars(r)["id"]
+ requestVars := mux.Vars(r)
+ clID, ok := requestVars["id"]
if !ok {
http.Error(w, "Must specify 'id' of ChangeList.", http.StatusBadRequest)
return
}
- psID, ok := mux.Vars(r)["patchset"]
+ psID, ok := requestVars["patchset"]
if !ok {
http.Error(w, "Must specify 'patchset' of ChangeList.", http.StatusBadRequest)
return
}
- crs, ok := mux.Vars(r)["system"]
+ crs, ok := requestVars["system"]
if !ok {
http.Error(w, "Must specify 'system' of ChangeList.", http.StatusBadRequest)
return
@@ -1559,6 +1561,57 @@
sendJSONResponse(w, digestsByTraceId)
}
+// ChangeListSearchRedirect redirects the user to a search page showing the search results
+// for a given CL. It will do a quick scan of the untriaged digests - if it finds some, it will
+// include the corpus containing some of those untriaged digests in the search query so the user
+// will see results (instead of getting directed to a corpus with no results).
+func (wh *Handlers) ChangeListSearchRedirect(w http.ResponseWriter, r *http.Request) {
+ defer metrics2.FuncTimer().Stop()
+ if err := wh.cheapLimitForAnonUsers(r); err != nil {
+ httputils.ReportError(w, err, "Try again later", http.StatusInternalServerError)
+ }
+
+ requestVars := mux.Vars(r)
+ crs, ok := requestVars["system"]
+ if !ok {
+ http.Error(w, "Must specify 'system' of ChangeList.", http.StatusBadRequest)
+ return
+ }
+ clID, ok := requestVars["id"]
+ if !ok {
+ http.Error(w, "Must specify 'id' of ChangeList.", http.StatusBadRequest)
+ return
+ }
+
+ baseURL := fmt.Sprintf("/search?issue=%s", clID)
+
+ clIdx := wh.Indexer.GetIndexForCL(crs, clID)
+ if clIdx == nil {
+ // Not cached, so we can't cheaply determine the corpus to include
+ if _, err := wh.ChangeListStore.GetChangeList(r.Context(), clID); err != nil {
+ http.NotFound(w, r)
+ return
+ }
+ http.Redirect(w, r, baseURL, http.StatusTemporaryRedirect)
+ return
+ }
+
+ digestList, err := wh.SearchAPI.UntriagedUnignoredTryJobExclusiveDigests(r.Context(), clIdx.LatestPatchSet)
+ if err != nil {
+ sklog.Errorf("Could not find corpus to redirect to for CL %s: %s", clID, err)
+ http.Redirect(w, r, baseURL, http.StatusTemporaryRedirect)
+ return
+ }
+ if len(digestList.Corpora) == 0 {
+ http.Redirect(w, r, baseURL, http.StatusTemporaryRedirect)
+ return
+ }
+
+ withCorpus := baseURL + "&query=source_type%3D" + digestList.Corpora[0]
+ sklog.Debugf("Redirecting to %s", withCorpus)
+ http.Redirect(w, r, withCorpus, http.StatusTemporaryRedirect)
+}
+
func (wh *Handlers) now() time.Time {
if !wh.testingNow.IsZero() {
return wh.testingNow
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index 200b719..9364d29 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -38,6 +38,8 @@
mock_indexer "go.skia.org/infra/golden/go/indexer/mocks"
"go.skia.org/infra/golden/go/mocks"
"go.skia.org/infra/golden/go/paramsets"
+ search_fe "go.skia.org/infra/golden/go/search/frontend"
+ mock_search "go.skia.org/infra/golden/go/search/mocks"
bug_revert "go.skia.org/infra/golden/go/testutils/data_bug_revert"
"go.skia.org/infra/golden/go/tiling"
"go.skia.org/infra/golden/go/tjstore"
@@ -1949,6 +1951,155 @@
assertJSONResponseWas(t, http.StatusOK, expectedResponse, w)
}
+func TestChangeListSearchRedirect_CLHasUntriagedDigests_Success(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mockSearchAPI := &mock_search.SearchAPI{}
+ mockIndexSource := &mock_indexer.IndexSource{}
+
+ const crs = "gerrit"
+ const clID = "1234"
+
+ combinedID := tjstore.CombinedPSID{
+ CRS: crs,
+ CL: clID,
+ PS: "some patchset",
+ }
+
+ mockIndexSource.On("GetIndexForCL", crs, clID).Return(&indexer.ChangeListIndex{
+ LatestPatchSet: combinedID,
+ // Other fields should be ignored
+ })
+
+ mockSearchAPI.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, combinedID).Return(&search_fe.UntriagedDigestList{
+ Corpora: []string{"one_corpus", "two_corpus"},
+ }, nil)
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ Indexer: mockIndexSource,
+ SearchAPI: mockSearchAPI,
+ },
+ anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
+ r = mux.SetURLVars(r, map[string]string{
+ "system": crs,
+ "id": clID,
+ })
+ wh.ChangeListSearchRedirect(w, r)
+ assert.Equal(t, http.StatusTemporaryRedirect, w.Code)
+ headers := w.Header()
+ assert.Equal(t, []string{"/search?issue=1234&query=source_type%3Done_corpus"}, headers["Location"])
+}
+
+func TestChangeListSearchRedirect_ErrorGettingUntriagedDigests_RedirectsWithoutCorpus(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mockSearchAPI := &mock_search.SearchAPI{}
+ mockIndexSource := &mock_indexer.IndexSource{}
+ defer mockSearchAPI.AssertExpectations(t) // make sure the error was returned
+
+ const crs = "gerrit"
+ const clID = "1234"
+
+ combinedID := tjstore.CombinedPSID{
+ CRS: crs,
+ CL: clID,
+ PS: "some patchset",
+ }
+
+ mockIndexSource.On("GetIndexForCL", crs, clID).Return(&indexer.ChangeListIndex{
+ LatestPatchSet: combinedID,
+ // Other fields should be ignored
+ })
+
+ mockSearchAPI.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, combinedID).Return(nil, errors.New("this shouldn't happen"))
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ Indexer: mockIndexSource,
+ SearchAPI: mockSearchAPI,
+ },
+ anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
+ r = mux.SetURLVars(r, map[string]string{
+ "system": crs,
+ "id": clID,
+ })
+ wh.ChangeListSearchRedirect(w, r)
+ assert.Equal(t, http.StatusTemporaryRedirect, w.Code)
+ headers := w.Header()
+ assert.Equal(t, []string{"/search?issue=1234"}, headers["Location"])
+}
+
+func TestChangeListSearchRedirect_CLExistsNotIndexed_RedirectsWithoutCorpus(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mockIndexSource := &mock_indexer.IndexSource{}
+ mockCLStore := &mock_clstore.Store{}
+
+ const crs = "gerrit"
+ const clID = "1234"
+
+ mockIndexSource.On("GetIndexForCL", crs, clID).Return(nil)
+
+ // all this cares about is a non-error return code
+ mockCLStore.On("GetChangeList", testutils.AnyContext, clID).Return(code_review.ChangeList{}, nil)
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ Indexer: mockIndexSource,
+ ChangeListStore: mockCLStore,
+ },
+ anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
+ r = mux.SetURLVars(r, map[string]string{
+ "system": crs,
+ "id": clID,
+ })
+ wh.ChangeListSearchRedirect(w, r)
+ assert.Equal(t, http.StatusTemporaryRedirect, w.Code)
+ headers := w.Header()
+ assert.Equal(t, []string{"/search?issue=1234"}, headers["Location"])
+}
+
+func TestChangeListSearchRedirect_CLDoesNotExist_404Error(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mockIndexSource := &mock_indexer.IndexSource{}
+ mockCLStore := &mock_clstore.Store{}
+
+ const crs = "gerrit"
+ const clID = "1234"
+
+ mockIndexSource.On("GetIndexForCL", crs, clID).Return(nil)
+
+ // all this cares about is a non-error return code
+ mockCLStore.On("GetChangeList", testutils.AnyContext, clID).Return(code_review.ChangeList{}, code_review.ErrNotFound)
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ Indexer: mockIndexSource,
+ ChangeListStore: mockCLStore,
+ },
+ anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
+ r = mux.SetURLVars(r, map[string]string{
+ "system": crs,
+ "id": clID,
+ })
+ wh.ChangeListSearchRedirect(w, r)
+ assert.Equal(t, http.StatusNotFound, w.Code)
+}
+
// Because we are calling our handlers directly, the target URL doesn't matter. The target URL
// would only matter if we were calling into the router, so it knew which handler to call.
const requestURL = "/does/not/matter"
