Public: anyone can read this; the crew can change it.

A repository's forks list names its Server-Timing phases: session, forks, render #4996

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

`GET /api/v1/repos/:owner/:name/forks` now answers `Server-Timing` with `session`, `forks`, `render`, in that order. The forks read sits inside the `forks` phase and `json(…)` inside `render`. Bodies, status codes and access rules are unchanged. A refused read still names `session`, because that is noted before the repo lookup, as `…/tags` does. The change is in `src/http/api.ts`.

shipped production is serving it deploy 9b150b4

opened this

GET /api/v1/repos/:owner/:name/forks now answers Server-Timing with session, forks, render, in that order. The forks read sits inside the forks phase and json(…) inside render. Bodies, status codes and access rules are unchanged. A refused read still names session, because that is noted before the repo lookup, as …/tags does. The change is in src/http/api.ts.

The issue says "each route", but only this one route needed the change.

The tests are in a new file, test/forks-api-timing.test.ts. One checks the three phase names in order on a 200. The other checks that a 404 still names session. The order test fails without the change. The 404 test passes either way, since the old route emitted no session phase on the 404 either. I only ran the 404 against a repo that doesn't exist. I did not run it against a 403, so that path is unchecked.

The new tests and the existing test/forks-timing.test.ts pass, and tsc --noEmit is clean. I did not run the whole suite.

The server adds its own app phase to every response, so the test filters it out, as the existing forks-page test does.

I first overwrote the existing test/forks-timing.test.ts (the forks page's test) by mistake and restored it from git before committing. That file is unchanged in this pull.

Closes #4985

commented

GET /api/v1/repos/:owner/:name/forks now answers Server-Timing with session, forks, render, in that order. The forks read sits inside the forks phase and json(…) inside render. Bodies, status codes and access rules are unchanged. A refused read still names session, because that is noted before the repo lookup, as …/tags does. The change is in src/http/api.ts.

The issue says "each route", but only this one route needed the change.

The tests are in a new file, test/forks-api-timing.test.ts. One checks the three phase names in order on a 200. The other checks that a 404 still names session. The order test fails without the change. The 404 test passes either way, since the old route emitted no session phase on the 404 either. I only ran the 404 against a repo that doesn't exist. I did not run it against a 403, so that path is unchecked.

The new tests and the existing test/forks-timing.test.ts pass, and tsc --noEmit is clean. I did not run the whole suite.

The server adds its own app phase to every response, so the test filters it out, as the existing forks-page test does.

I first overwrote the existing test/forks-timing.test.ts (the forks page's test) by mistake and restored it from git before committing. That file is unchanged in this pull.

Approved

Approved. The change does what #4985 asks and copies the …/tags route exactly. session is noted before loadRepo. The visibleForks read runs inside forks, and json(…) runs inside render. Access, the response body and the status codes don't change.

On the preview, GET /api/v1/repos/nobody/nope/forks answers 404 with server-timing: app;dur=…, session;dur=…. The preview has no repositories, so I couldn't see a 200 there. The new test covers the 200 case and checks the order: session, forks, render. That test fails without the change.

The 403 path the author left unchecked doesn't exist here. loadRepo hides a private repo as a 404, so it takes the same path the second test covers.

Security: this pull adds no inputs, writes or new output. Server-Timing gives only durations and the names of phases, as other routes already do.

One minor note, not blocking: the 404 test passes without the change too. The 200 test is what carries the change.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs