[gold] Update /json/v2/details RPC endpoint and details page to take a grouping as input.
I don't think there are any places that use this RPC besides the details page, so it's probably safe to modify it "in place" rather than creating a /json/v3/details RPC.
Bug: skia:13712
Change-Id: Ibc3b21b6f66fd729926b339e4736edb727259b47
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/578679
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
diff --git a/golden/cmd/gold_frontend/gold_frontend.go b/golden/cmd/gold_frontend/gold_frontend.go
index 1c6c022..7079a26 100644
--- a/golden/cmd/gold_frontend/gold_frontend.go
+++ b/golden/cmd/gold_frontend/gold_frontend.go
@@ -499,7 +499,7 @@
add("/json/v2/clusterdiff", handlers.ClusterDiffHandler, "GET")
add("/json/v2/commits", handlers.CommitsHandler, "GET")
add("/json/v1/positivedigestsbygrouping/{groupingID}", handlers.PositiveDigestsByGroupingIDHandler, "GET")
- add("/json/v2/details", handlers.DetailsHandler, "GET")
+ add("/json/v2/details", handlers.DetailsHandler, "POST")
add("/json/v2/diff", handlers.DiffHandler, "POST")
add("/json/v2/digests", handlers.DigestListHandler, "GET")
add("/json/v2/latestpositivedigest/{traceID}", handlers.LatestPositiveDigestHandler, "GET")
diff --git a/golden/go/web/frontend/generate_typescript_rpc_types/main.go b/golden/go/web/frontend/generate_typescript_rpc_types/main.go
index 2af302c..0de8ac6 100644
--- a/golden/go/web/frontend/generate_typescript_rpc_types/main.go
+++ b/golden/go/web/frontend/generate_typescript_rpc_types/main.go
@@ -89,7 +89,10 @@
// Response for the /json/v1/diff RPC endpoint.
generator.Add(frontend.DigestComparison{})
- // Response for the /json/v1/details RPC endpoint.
+ // Request for the /json/v2/details RPC endpoint.
+ generator.Add(frontend.DetailsRequest{})
+
+ // Response for the /json/v2/details RPC endpoint.
generator.Add(frontend.DigestDetails{})
// Response for the /json/v1/clusterdiff RPC endpoint.
diff --git a/golden/go/web/frontend/types.go b/golden/go/web/frontend/types.go
index 9168390..ac867dd 100644
--- a/golden/go/web/frontend/types.go
+++ b/golden/go/web/frontend/types.go
@@ -705,3 +705,11 @@
type GroupingForTestResponse struct {
Grouping paramtools.Params `json:"grouping"`
}
+
+// DetailsRequest is the request for the /json/v2/details RPC.
+type DetailsRequest struct {
+ Grouping paramtools.Params `json:"grouping"`
+ Digest types.Digest `json:"digest"`
+ ChangelistID string `json:"changelist_id,omitempty"`
+ CodeReviewSystem string `json:"crs,omitempty"`
+}
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 4a99e07..b4deadd 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -469,35 +469,29 @@
return
}
- // Extract: test, digest, issue
- if err := r.ParseForm(); err != nil {
- httputils.ReportError(w, err, "Failed to parse form values", http.StatusInternalServerError)
+ req := frontend.DetailsRequest{}
+ if err := parseJSON(r, &req); err != nil {
+ httputils.ReportError(w, err, "Failed to parse JSON request.", http.StatusBadRequest)
return
}
- // TODO(kjlubick) require corpus
- test := r.Form.Get("test")
- digest := r.Form.Get("digest")
- if test == "" || !validation.IsValidDigest(digest) {
- http.Error(w, "Some query parameters are wrong or missing", http.StatusBadRequest)
+ sklog.Infof("Details request: %#v", req)
+
+ if len(req.Grouping) == 0 {
+ http.Error(w, "Grouping cannot be empty.", http.StatusBadRequest)
return
}
- clID := r.Form.Get("changelist_id")
- crs := r.Form.Get("crs")
- if clID != "" {
- if _, ok := wh.getCodeReviewSystem(crs); !ok {
- http.Error(w, "Invalid Code Review System; did you include crs?", http.StatusBadRequest)
+ if !validation.IsValidDigest(string(req.Digest)) {
+ http.Error(w, "Invalid digest.", http.StatusBadRequest)
+ return
+ }
+ if req.CodeReviewSystem != "" && req.ChangelistID != "" {
+ if _, ok := wh.getCodeReviewSystem(req.CodeReviewSystem); !ok {
+ http.Error(w, "Invalid code review system.", http.StatusBadRequest)
return
}
- } else {
- crs = ""
}
- grouping, err := wh.getGroupingForTest(ctx, test)
- if err != nil {
- httputils.ReportError(w, err, "could not get grouping", http.StatusInternalServerError)
- return
- }
- ret, err := wh.Search2API.GetDigestDetails(ctx, grouping, types.Digest(digest), clID, crs)
+ ret, err := wh.Search2API.GetDigestDetails(ctx, req.Grouping, types.Digest(req.Digest), req.ChangelistID, req.CodeReviewSystem)
if err != nil {
httputils.ReportError(w, err, "Unable to get digest details.", http.StatusInternalServerError)
return
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index 400a2d5..ef0289c 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -3238,6 +3238,128 @@
assertJSONResponseWas(t, http.StatusOK, `{"grouping":{"name":"square","source_type":"corners"}}`, w)
}
+func TestDetailsHandler_InvalidRequest_Error(t *testing.T) {
+ wh := Handlers{
+ anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
+ HandlersConfig: HandlersConfig{
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: dks.GerritCRS,
+ },
+ },
+ },
+ }
+
+ test := func(name string, req frontend.DetailsRequest, expectedError string) {
+ t.Run(name, func(t *testing.T) {
+ reqBytes, err := json.Marshal(req)
+ require.NoError(t, err)
+
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, "/json/v2/details", bytes.NewReader(reqBytes))
+ wh.DetailsHandler(w, r)
+
+ res := w.Result()
+ resBytes, err := io.ReadAll(w.Body)
+ require.NoError(t, err)
+
+ assert.Equal(t, http.StatusBadRequest, res.StatusCode)
+ assert.Contains(t, string(resBytes), expectedError)
+ })
+ }
+
+ test("empty request", frontend.DetailsRequest{}, "Grouping cannot be empty.")
+ test(
+ "invalid digest",
+ frontend.DetailsRequest{
+ Grouping: paramtools.Params{
+ types.CorpusField: "fake_corpus",
+ types.PrimaryKeyField: "fake_test",
+ },
+ Digest: "invalid digest",
+ },
+ "Invalid digest.")
+ test(
+ "invalid code review system",
+ frontend.DetailsRequest{
+ Grouping: paramtools.Params{
+ types.CorpusField: "fake_corpus",
+ types.PrimaryKeyField: "fake_test",
+ },
+ Digest: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+ CodeReviewSystem: "invalid code review system",
+ ChangelistID: "123456",
+ },
+ "Invalid code review system.")
+}
+
+func TestDetailsHandler_ValidRequest_Success(t *testing.T) {
+ wh := Handlers{
+ anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
+ HandlersConfig: HandlersConfig{
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: dks.GerritCRS,
+ },
+ },
+ },
+ }
+
+ test := func(name string, req frontend.DetailsRequest, expectedResponse string) {
+ t.Run(name, func(t *testing.T) {
+ ms := &mock_search.API{}
+
+ ms.On("GetDigestDetails", testutils.AnyContext, req.Grouping, req.Digest, req.ChangelistID, req.CodeReviewSystem).
+ Return(frontend.DigestDetails{
+ Result: frontend.SearchResult{
+ Digest: req.Digest,
+ Test: types.TestName(req.Grouping[types.PrimaryKeyField]),
+ },
+ Commits: []frontend.Commit{},
+ }, nil)
+ wh.HandlersConfig.Search2API = ms
+
+ reqBytes, err := json.Marshal(req)
+ require.NoError(t, err)
+
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, "/json/v2/details", bytes.NewReader(reqBytes))
+ wh.DetailsHandler(w, r)
+
+ assertJSONResponseWas(t, http.StatusOK, expectedResponse, w)
+ })
+ }
+
+ test(
+ "without crs/cl",
+ frontend.DetailsRequest{
+ Grouping: paramtools.Params{
+ types.CorpusField: dks.CornersCorpus,
+ types.PrimaryKeyField: dks.SquareTest,
+ },
+ Digest: dks.DigestA01Pos,
+ },
+ `{"digest":{"digest":"a01a01a01a01a01a01a01a01a01a01a0","test":"square",`+
+ `"status":"","triage_history":null,"paramset":null,`+
+ `"traces":{"traces":null,"digests":null,"total_digests":0},`+
+ `"refDiffs":null,"closestRef":""},"commits":[]}`)
+ test(
+ "with crs/cl",
+ frontend.DetailsRequest{
+ Grouping: paramtools.Params{
+ types.CorpusField: dks.RoundCorpus,
+ types.PrimaryKeyField: dks.CircleTest,
+ },
+ Digest: dks.DigestC01Pos,
+ CodeReviewSystem: dks.GerritCRS,
+ ChangelistID: dks.ChangelistIDThatAttemptsToFixIOS,
+ },
+ `{"digest":{"digest":"c01c01c01c01c01c01c01c01c01c01c0","test":"circle",`+
+ `"status":"","triage_history":null,"paramset":null,`+
+ `"traces":{"traces":null,"digests":null,"total_digests":0},`+
+ `"refDiffs":null,"closestRef":""},"commits":[]}`)
+}
+
// 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"
diff --git a/golden/modules/details-page-sk/details-page-sk-demo.ts b/golden/modules/details-page-sk/details-page-sk-demo.ts
index 8a2bc7a..ba4db32 100644
--- a/golden/modules/details-page-sk/details-page-sk-demo.ts
+++ b/golden/modules/details-page-sk/details-page-sk-demo.ts
@@ -3,17 +3,15 @@
import { $$ } from 'common-sk/modules/dom';
import fetchMock from 'fetch-mock';
-import { toObject } from 'common-sk/modules/query';
-import { HintableObject } from 'common-sk/modules/hintable';
-import {
- fakeNow, twoHundredCommits, makeTypicalSearchResult,
-} from '../digest-details-sk/test_data';
+import { fakeNow, twoHundredCommits, makeTypicalSearchResult } from '../digest-details-sk/test_data';
import { delay } from '../demo_util';
import { testOnlySetSettings } from '../settings';
import { exampleStatusData } from '../last-commit-sk/demo_data';
import { GoldScaffoldSk } from '../gold-scaffold-sk/gold-scaffold-sk';
import { DetailsPageSk } from './details-page-sk';
-import { DigestDetails } from '../rpc_types';
+import {
+ DetailsRequest, DigestDetails, GroupingForTestRequest, GroupingForTestResponse,
+} from '../rpc_types';
import { setQueryString } from '../../../infra-sk/modules/test_util';
import { groupingsResponse } from '../search-page-sk/demo_data';
@@ -25,28 +23,33 @@
// Load the demo page with some params to load if there aren't any already.
if (window.location.search.length === 0) {
setQueryString(
- '?digest=6246b773851984c726cb2e1cb13510c2&test=This%20is%20a%20test%20with%20spaces&'
+ '?digest=6246b773851984c726cb2e1cb13510c2&'
+ + 'grouping=name%3DThis%2520is%2520a%2520test%2520with%2520spaces%26source_type%3Dinfra&'
+ 'changelist_id=12353&crs=gerrit-internal',
);
}
Date.now = () => fakeNow;
-interface UrlParams {
- digest: string;
- test: string;
-}
-
fetchMock.get('/json/v1/groupings', groupingsResponse);
-fetchMock.get('glob:/json/v2/details*', (url) => {
+fetchMock.post('/json/v1/groupingfortest', (url, opts) => {
+ const request: GroupingForTestRequest = JSON.parse(opts.body!.toString());
+ const response: GroupingForTestResponse = {
+ grouping: {
+ name: request.test_name,
+ source_type: 'infra',
+ },
+ };
+ return response;
+});
+
+fetchMock.post('/json/v2/details', (url, opts) => {
if ($$<HTMLInputElement>('#simulate-rpc-error')!.checked) {
return 500;
}
- // Make a response based on the URL parameters. This is needed by the Puppeteer test.
- const hint: UrlParams = { digest: '', test: '' };
- const urlParams = toObject(url.split('?')[1], hint as unknown as HintableObject) as unknown as UrlParams;
-
+ // Make a response based on the RPC request. This is needed by the Puppeteer test.
+ const request: DetailsRequest = JSON.parse(opts.body!.toString());
if ($$<HTMLInputElement>('#simulate-not-found-in-index')!.checked) {
const response: DigestDetails = {
digest: {
@@ -69,9 +72,9 @@
}
const knownDigest1 = '99c58c7002073346ff55f446d47d6311';
const knownDigest2 = '6246b773851984c726cb2e1cb13510c2';
- const closestDigest = urlParams.digest === knownDigest1 ? knownDigest2 : knownDigest1;
+ const closestDigest = request.digest === knownDigest1 ? knownDigest2 : knownDigest1;
const response: DigestDetails = {
- digest: makeTypicalSearchResult(urlParams.test, urlParams.digest, closestDigest),
+ digest: makeTypicalSearchResult(request.grouping.name, request.digest, closestDigest),
commits: twoHundredCommits,
};
return delay(response);
diff --git a/golden/modules/details-page-sk/details-page-sk.ts b/golden/modules/details-page-sk/details-page-sk.ts
index d0133d1..2e77ef8 100644
--- a/golden/modules/details-page-sk/details-page-sk.ts
+++ b/golden/modules/details-page-sk/details-page-sk.ts
@@ -13,7 +13,7 @@
import '../digest-details-sk';
import { sendBeginTask, sendEndTask, sendFetchError } from '../common';
import {
- Commit, DigestDetails, GroupingsResponse, SearchResult,
+ Commit, DetailsRequest, DigestDetails, GroupingForTestRequest, GroupingForTestResponse, GroupingsResponse, Params, SearchResult,
} from '../rpc_types';
export class DetailsPageSk extends ElementSk {
@@ -22,9 +22,12 @@
return html`<h1>Loading...</h1>`;
}
if (!ele.details?.digest) {
+ const testName = Object.keys(ele.grouping).length === 0
+ ? ele.testName
+ : ele.grouping.name;
return html`
<div>
- Could not load details for digest ${ele.digest} and test "${ele.grouping}".
+ Could not load details for digest ${ele.digest} and test "${testName}".
<br>
It might not exist or be too new so as not to be indexed yet.
</div>
@@ -42,7 +45,9 @@
private groupings: GroupingsResponse | null = null;
- private grouping = '';
+ private grouping: Params = {};
+
+ private testName = ''; // TODO(lovisolo): Delete once all inbound links include a grouping.
private digest = '';
@@ -67,7 +72,8 @@
this.stateChanged = stateReflector(
/* getState */() => ({
// provide empty values
- test: this.grouping, // TODO(kjlubick) rename test -> grouping
+ grouping: this.grouping,
+ test: this.testName, // TODO(lovisolo): Delete once all inbound links include a grouping.
digest: this.digest,
changelist_id: this.changeListID,
crs: this.crs,
@@ -76,7 +82,9 @@
return;
}
// default values if not specified.
- this.grouping = newState.test as string || '';
+ this.grouping = newState.grouping as Params || {};
+ // TODO(lovisolo): Delete once all inbound links include a grouping.
+ this.testName = newState.test as string || '';
this.digest = newState.digest as string || '';
this.changeListID = newState.changelist_id as string || '';
this.crs = newState.crs as string || '';
@@ -106,7 +114,7 @@
}
}
- private fetchDigestDetails() {
+ private async fetchDigestDetails() {
if (this.fetchController) {
// Kill any outstanding requests
this.fetchController.abort();
@@ -115,17 +123,46 @@
// Make a fresh abort controller for each set of fetches.
// They cannot be re-used once aborted.
this.fetchController = new AbortController();
- const extra = {
- signal: this.fetchController.signal,
- };
sendBeginTask(this);
- const urlBase = '/json/v2/details';
- const url = `${urlBase}?test=${encodeURIComponent(this.grouping)}`
- + `&digest=${encodeURIComponent(this.digest)}&changelist_id=${this.changeListID}`
- + `&crs=${this.crs}`;
- fetch(url, extra)
- .then(jsonOrThrow)
+ // Fetch grouping if not specified as an URL parameter.
+ if (Object.keys(this.grouping).length === 0) {
+ const request: GroupingForTestRequest = {
+ test_name: this.testName,
+ };
+ try {
+ const response: GroupingForTestResponse = await fetch('/json/v1/groupingfortest', {
+ method: 'POST',
+ body: JSON.stringify(request),
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ }).then(jsonOrThrow);
+ this.grouping = response.grouping;
+ } catch (e) {
+ this._render();
+ sendFetchError(this, e, 'fetching grouping for test');
+ return;
+ }
+ }
+
+ const request: DetailsRequest = {
+ digest: this.digest,
+ grouping: this.grouping,
+ };
+ if (this.changeListID && this.crs) {
+ request.changelist_id = this.changeListID;
+ request.crs = this.crs;
+ }
+
+ fetch('/json/v2/details', {
+ method: 'POST',
+ body: JSON.stringify(request),
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ signal: this.fetchController.signal,
+ }).then(jsonOrThrow)
.then((digestDetails: DigestDetails) => {
this.commits = digestDetails.commits || [];
this.details = digestDetails.digest;
diff --git a/golden/modules/details-page-sk/details-page-sk_test.ts b/golden/modules/details-page-sk/details-page-sk_test.ts
index 86e76cd..1027cb6 100644
--- a/golden/modules/details-page-sk/details-page-sk_test.ts
+++ b/golden/modules/details-page-sk/details-page-sk_test.ts
@@ -2,8 +2,12 @@
import fetchMock from 'fetch-mock';
import { expect } from 'chai';
import { deepCopy } from 'common-sk/modules/object';
-import { eventPromise, eventSequencePromise, setUpElementUnderTest } from '../../../infra-sk/modules/test_util';
-import { DigestDetails, TriageRequestV3, TriageResponse } from '../rpc_types';
+import {
+ eventPromise, eventSequencePromise, setQueryString, setUpElementUnderTest,
+} from '../../../infra-sk/modules/test_util';
+import {
+ DigestDetails, GroupingForTestRequest, GroupingForTestResponse, TriageRequestV3, TriageResponse,
+} from '../rpc_types';
import { DetailsPageSk } from './details-page-sk';
import { DetailsPageSkPO } from './details-page-sk_po';
import { groupingsResponse } from '../search-page-sk/demo_data';
@@ -15,50 +19,100 @@
let detailsPageSk: DetailsPageSk;
let detailsPageSkPO: DetailsPageSkPO;
- beforeEach(async () => {
+ const initialize = async (queryString: string, useGroupingForTestRPC: boolean) => {
+ setQueryString(queryString);
+
detailsPageSk = newInstance();
detailsPageSkPO = new DetailsPageSkPO(detailsPageSk);
fetchMock.getOnce('/json/v1/groupings', () => deepCopy(groupingsResponse));
+ if (useGroupingForTestRPC) {
+ fetchMock.post('/json/v1/groupingfortest', (url, opts) => {
+ const request: GroupingForTestRequest = JSON.parse(opts.body!.toString());
+ const response: GroupingForTestResponse = {
+ grouping: {
+ name: request.test_name,
+ source_type: 'infra',
+ },
+ };
+ return response;
+ });
+ }
const digestDetails: DigestDetails = {
digest: deepCopy(typicalDetails),
commits: deepCopy(twoHundredCommits),
};
- fetchMock.get('glob:/json/v2/details*', digestDetails);
+ fetchMock.post('/json/v2/details', digestDetails);
// Wait for the above RPCs to complete.
await eventSequencePromise(['end-task', 'end-task']);
- });
+ };
+
+ const addTests = () => {
+ it('renders', async () => {
+ expect(await detailsPageSkPO.digestDetailsSkPO.getTestName())
+ .to.equal('Test: dots-legend-sk_too-many-digests');
+ });
+
+ it('can triage', async () => {
+ // This tests the wiring that passes the groupings returned by the /json/v1/groupings RPC to
+ // the digest-details-sk element.
+ const triageRequest: TriageRequestV3 = {
+ deltas: [
+ {
+ grouping: {
+ source_type: 'infra',
+ name: 'dots-legend-sk_too-many-digests',
+ },
+ digest: '6246b773851984c726cb2e1cb13510c2',
+ label_before: 'positive',
+ label_after: 'negative',
+ },
+ ],
+ changelist_id: '12353',
+ crs: 'gerrit-internal',
+ };
+ const triageResponse: TriageResponse = { status: 'ok' };
+ fetchMock.post(
+ { url: '/json/v3/triage', body: triageRequest },
+ { status: 200, body: triageResponse },
+ );
+
+ const endTask = eventPromise('end-task');
+ await detailsPageSkPO.digestDetailsSkPO.triageSkPO.clickButton('negative');
+ await endTask;
+ });
+ };
afterEach(() => {
expect(fetchMock.done()).to.be.true; // All mock RPCs called at least once.
fetchMock.reset();
});
- it('can triage', async () => {
- // This tests the wiring that passes the groupings returned by the /json/v1/groupings RPC to
- // the digest-details-sk element.
- const triageRequest: TriageRequestV3 = {
- deltas: [
- {
- grouping: {
- source_type: 'infra',
- name: 'dots-legend-sk_too-many-digests',
- },
- digest: '6246b773851984c726cb2e1cb13510c2',
- label_before: 'positive',
- label_after: 'negative',
- },
- ],
- };
- const triageResponse: TriageResponse = { status: 'ok' };
- fetchMock.post(
- { url: '/json/v3/triage', body: triageRequest },
- { status: 200, body: triageResponse },
- );
+ describe('with grouping in URL', () => {
+ beforeEach(async () => {
+ await initialize(
+ '?digest=6246b773851984c726cb2e1cb13510c2&'
+ + 'grouping=name%3Ddots-legend-sk_too-many-digests%26source_type%3Dinfra&'
+ + 'changelist_id=12353&crs=gerrit-internal',
+ /* useGroupingForTestRPC= */ false,
+ );
+ });
- const endTask = eventPromise('end-task');
- await detailsPageSkPO.digestDetailsSkPO.triageSkPO.clickButton('negative');
- await endTask;
+ addTests();
+ });
+
+ // TODO(lovisolo): Delete once all inbound links include a grouping.
+ describe('legacy links with just test in URL', () => {
+ beforeEach(async () => {
+ await initialize(
+ '?digest=6246b773851984c726cb2e1cb13510c2&'
+ + 'test=dots-legend-sk_too-many-digests&'
+ + 'changelist_id=12353&crs=gerrit-internal',
+ /* useGroupingForTestRPC= */ true,
+ );
+ });
+
+ addTests();
});
});
diff --git a/golden/modules/rpc_types.ts b/golden/modules/rpc_types.ts
index ef4be74..457917f 100644
--- a/golden/modules/rpc_types.ts
+++ b/golden/modules/rpc_types.ts
@@ -234,6 +234,13 @@
right: SRDiffDigest;
}
+export interface DetailsRequest {
+ grouping: Params;
+ digest: Digest;
+ changelist_id?: string;
+ crs?: string;
+}
+
export interface DigestDetails {
digest: SearchResult;
commits: Commit[] | null;
