fix: handle full URLs for GraphQL and Search request (#416)

octokit · Jun 26, 2021 · c9c3bf6 · c9c3bf6
1 parent 9002a51
commit c9c3bf6
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 24 deletions.
diff --git a/src/index.ts b/src/index.ts
@@ -109,8 +109,9 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
   // @ts-ignore
   state.retryLimiter.on("failed", async function (error, info) {
     const options = info.args[info.args.length - 1];
+    const { pathname } = new URL(options.url, "http://github.test");
     const shouldRetryGraphQL =
-      options.url.startsWith("/graphql") && error.status !== 401;
+      pathname.startsWith("/graphql") && error.status !== 401;
 
     if (!(shouldRetryGraphQL || error.status === 403)) {
       return;

diff --git a/src/wrap-request.ts b/src/wrap-request.ts
@@ -8,9 +8,9 @@ export function wrapRequest(state, request, options) {
 // @ts-ignore
 async function doRequest(state, request, options) {
   const isWrite = options.method !== "GET" && options.method !== "HEAD";
-  const isSearch =
-    options.method === "GET" && options.url.startsWith("/search/");
-  const isGraphQL = options.url.startsWith("/graphql");
+  const { pathname } = new URL(options.url, "http://github.test");
+  const isSearch = options.method === "GET" && pathname.startsWith("/search/");
+  const isGraphQL = pathname.startsWith("/graphql");
 
   const retryCount = ~~options.request.retryCount;
   const jobOptions = retryCount > 0 ? { priority: 0, weight: 0 } : {};
@@ -28,7 +28,7 @@ async function doRequest(state, request, options) {
   }
 
   // Guarantee at least 3000ms between requests that trigger notifications
-  if (isWrite && state.triggersNotification(options.url)) {
+  if (isWrite && state.triggersNotification(pathname)) {
     await state.notifications.key(state.id).schedule(jobOptions, noop);
   }
 

diff --git a/test/plugin.test.ts b/test/plugin.test.ts
@@ -232,34 +232,48 @@ describe("Github API best practices", function () {
       },
     });
 
-    const req1 = octokit.request("GET /search/route1", {
+    const req1 = octokit.request("GET /search/route", {
       request: {
-        responses: [{ status: 201, headers: {}, data: {} }],
-      },
-    });
-    const req2 = octokit.request("GET /route2", {
-      request: {
-        responses: [{ status: 202, headers: {}, data: {} }],
+        responses: [{ status: 200, headers: {}, data: {} }],
       },
     });
-    const req3 = octokit.request("GET /search/route3", {
+    const req2 = octokit.request("GET /other-route", {
       request: {
-        responses: [{ status: 203, headers: {}, data: {} }],
+        responses: [{ status: 200, headers: {}, data: {} }],
       },
     });
+    const req3 = octokit.request(
+      "GET https://api.github.com/search/route?page=2",
+      {
+        request: {
+          responses: [{ status: 200, headers: {}, data: {} }],
+        },
+      }
+    );
+    const req4 = octokit.request(
+      "GET https://api.github.com/search/route?page=3",
+      {
+        request: {
+          responses: [{ status: 200, headers: {}, data: {} }],
+        },
+      }
+    );
 
-    await Promise.all([req1, req2, req3]);
+    await Promise.all([req1, req2, req3, req4]);
     expect(octokit.__requestLog).toStrictEqual([
-      "START GET /route2",
-      "END GET /route2",
-      "START GET /search/route1",
-      "END GET /search/route1",
-      "START GET /search/route3",
-      "END GET /search/route3",
+      "START GET /other-route",
+      "END GET /other-route",
+      "START GET /search/route",
+      "END GET /search/route",
+      "START GET https://api.github.com/search/route?page=2",
+      "END GET https://api.github.com/search/route?page=2",
+      "START GET https://api.github.com/search/route?page=3",
+      "END GET https://api.github.com/search/route?page=3",
     ]);
-    expect(
-      octokit.__requestTimings[4] - octokit.__requestTimings[2]
-    ).toBeLessThan(70);
+
+    const ms = octokit.__requestTimings[4] - octokit.__requestTimings[2];
+    expect(ms).toBeLessThan(70);
+    expect(ms).toBeGreaterThan(30);
   });
 
   it("Should optimize throughput rather than maintain ordering", async function () {

diff --git a/test/retry.test.ts b/test/retry.test.ts
@@ -213,6 +213,56 @@ describe("Retry", function () {
       expect(ms).toBeGreaterThan(30);
     });
 
+    it("Should work with full URL", async function () {
+      let eventCount = 0;
+      const octokit = new TestOctokit({
+        throttle: {
+          write: new Bottleneck.Group({ minTime: 50 }),
+          onRateLimit: (retryAfter: number, options: object) => {
+            expect(options).toMatchObject({
+              method: "POST",
+              url: "https://api.github.com/graphql",
+              request: { retryCount: eventCount },
+            });
+            expect(retryAfter).toEqual(0);
+            eventCount++;
+            return true;
+          },
+          onAbuseLimit: () => 1,
+        },
+      });
+
+      const res = await octokit.request("POST https://api.github.com/graphql", {
+        request: {
+          responses: [
+            {
+              status: 200,
+              headers: {
+                "x-ratelimit-remaining": "0",
+                "x-ratelimit-reset": "123",
+              },
+              data: { errors: [{ type: "RATE_LIMITED" }] },
+            },
+            { status: 200, headers: {}, data: { message: "Yay!" } },
+          ],
+        },
+      });
+
+      expect(res.status).toEqual(200);
+      expect(res.data).toMatchObject({ message: "Yay!" });
+      expect(eventCount).toEqual(1);
+      expect(octokit.__requestLog).toStrictEqual([
+        "START POST https://api.github.com/graphql",
+        "END POST https://api.github.com/graphql",
+        "START POST https://api.github.com/graphql",
+        "END POST https://api.github.com/graphql",
+      ]);
+
+      const ms = octokit.__requestTimings[2] - octokit.__requestTimings[0];
+      expect(ms).toBeLessThan(70);
+      expect(ms).toBeGreaterThan(30);
+    });
+
     it("Should ignore other error types", async function () {
       let eventCount = 0;
       const octokit = new TestOctokit({