fix: remove request which is handled by fetchAndHandleErrors (#2613)

darrenjaneczek · web-flow · commit a4928eb76e58 · 2023-11-02T13:27:01.000-04:00
fix: rename `requestWithOrgId` to `request`
diff --git a/public/app/services/apps.spec.ts b/public/app/services/apps.spec.ts
@@ -647,7 +647,7 @@ describe('appsService', () => {
     ])(
       `server returned='%s', should transform into %s`,
       async (response, expected) => {
-        const spy = jest.spyOn(base, 'requestWithOrgID');
+        const spy = jest.spyOn(base, 'request');
         spy.mockReturnValue(Promise.resolve(Result.ok(response)));
 
         const res = await fetchApps();
@@ -658,7 +658,7 @@ describe('appsService', () => {
   });
 
   it('works', async () => {
-    const spy = jest.spyOn(base, 'requestWithOrgID');
+    const spy = jest.spyOn(base, 'request');
     spy.mockReturnValue(Promise.resolve(Result.ok(mockData)));
 
     const res = await fetchApps();
@@ -750,7 +750,7 @@ describe('appsService', () => {
   });
 
   it('works with pyroscope_app or service_name', async () => {
-    const spy = jest.spyOn(base, 'requestWithOrgID');
+    const spy = jest.spyOn(base, 'request');
     const mockData = {
       labelsSet: [
         {
@@ -821,7 +821,7 @@ describe('appsService', () => {
   // The server will return with that level of granularity
   // Which is not required to build an "App"
   it('remove duplicates from same _profile_type__/name pair', async () => {
-    const spy = jest.spyOn(base, 'requestWithOrgID');
+    const spy = jest.spyOn(base, 'request');
     const mockData = {
       labelsSet: [
         {
@@ -889,7 +889,7 @@ describe('appsService', () => {
   });
 
   it('filters out flagSets without pyroscope_app nor service_name', async () => {
-    const spy = jest.spyOn(base, 'requestWithOrgID');
+    const spy = jest.spyOn(base, 'request');
     const mockData = {
       labelsSet: [
         {
diff --git a/public/app/services/apps.ts b/public/app/services/apps.ts
@@ -8,11 +8,7 @@ import {
 import { Result } from '@pyroscope/util/fp';
 import { z, ZodError } from 'zod';
 import type { RequestError } from '@pyroscope/services/base';
-import {
-  parseResponse,
-  request,
-  requestWithOrgID,
-} from '@pyroscope/services/base';
+import { parseResponse, request } from '@pyroscope/services/base';
 
 // SeriesResponse refers to the response from the server, without any manipulation
 const SeriesResponseSchema = z.preprocess(
@@ -86,7 +82,7 @@ export async function fetchApps(
   fromMs?: number,
   untilMs?: number
 ): Promise<Result<App[], RequestError | ZodError>> {
-  let response = await requestWithOrgID('/querier.v1.QuerierService/Series', {
+  let response = await request('/querier.v1.QuerierService/Series', {
     method: 'POST',
     body: JSON.stringify({
       matchers: [],
@@ -110,7 +106,7 @@ export async function fetchApps(
   }
 
   // try without labelNames in case of an error since this has been added in a later version
-  response = await requestWithOrgID('/querier.v1.QuerierService/Series', {
+  response = await request('/querier.v1.QuerierService/Series', {
     method: 'POST',
     body: JSON.stringify({
       matchers: [],
diff --git a/public/app/services/base.spec.ts b/public/app/services/base.spec.ts
@@ -1,4 +1,4 @@
-import { requestWithOrgID } from '@pyroscope/services/base';
+import { request } from '@pyroscope/services/base';
 import * as storageSvc from '@pyroscope/services/storage';
 
 jest.mock('@pyroscope/services/base', () => {
@@ -36,7 +36,7 @@ describe('base', () => {
   });
 
   it('uses X-Scope-OrgID if set manually', () => {
-    requestWithOrgID('/', {
+    request('/', {
       headers: {
         'X-Scope-OrgID': 'myID',
       },
@@ -54,7 +54,7 @@ describe('base', () => {
 
     tenantIdSpy.mockReturnValueOnce('');
 
-    requestWithOrgID('/');
+    request('/');
 
     expect(global.fetch).toHaveBeenCalledWith('http://localhost/', {
       headers: {},
@@ -66,7 +66,7 @@ describe('base', () => {
 
     tenantIdSpy.mockReturnValueOnce('myid');
 
-    requestWithOrgID('/');
+    request('/');
 
     expect(global.fetch).toHaveBeenCalledWith('http://localhost/', {
       headers: {
diff --git a/public/app/services/base.ts b/public/app/services/base.ts
@@ -20,7 +20,7 @@ export class RequestNotOkError extends CustomError {
  * request wraps around the original request
  * while sending the OrgID if available
  */
-export async function requestWithOrgID(
+export async function request(
   request: RequestInfo,
   config?: RequestInit
 ): Promise<Result<unknown, RequestError>> {
@@ -62,7 +62,7 @@ export async function downloadWithOrgID(
   });
 }
 
-export async function connectDownload(
+async function connectDownload(
   request: RequestInfo,
   config?: RequestInit
 ): Promise<Result<Response, RequestError>> {
@@ -73,7 +73,7 @@ export async function connectDownload(
   return Result.ok(await response.value);
 }
 
-export async function connectRequest(
+async function connectRequest(
   request: RequestInfo,
   config?: RequestInit
 ): Promise<Result<unknown, RequestError>> {
@@ -247,7 +247,7 @@ function join(base: string, path: string): string {
   return `${base}/${path}`;
 }
 
-export function mountURL(req: RequestInfo): string {
+function mountURL(req: RequestInfo): string {
   const baseName = basename();
 
   if (baseName) {
@@ -266,7 +266,7 @@ export function mountURL(req: RequestInfo): string {
   return new URL(`${req}`, window.location.href).href;
 }
 
-export function mountRequest(req: RequestInfo): RequestInfo {
+function mountRequest(req: RequestInfo): RequestInfo {
   const url = mountURL(req);
 
   if (typeof req === 'string') {
@@ -279,114 +279,6 @@ export function mountRequest(req: RequestInfo): RequestInfo {
   };
 }
 
-export async function request(
-  request: RequestInfo,
-  config?: RequestInit
-): Promise<Result<unknown, RequestError>> {
-  const req = mountRequest(request);
-  let response: Response;
-  try {
-    response = await fetch(req, config);
-  } catch (e) {
-    // 'e' is unknown, but most cases it should be an Error
-    let message = '';
-    if (e instanceof Error) {
-      message = e.message;
-    }
-
-    if (e instanceof Error && e.name === 'AbortError') {
-      return Result.err(new RequestAbortedError(message));
-    }
-
-    return Result.err(new RequestIncompleteError(message));
-  }
-
-  if (!response.ok) {
-    const textBody = await response.text();
-
-    // There's nothing in the body, so let's use a default message
-    if (!textBody || !textBody.length) {
-      return Result.err(
-        new RequestNotOkError(response.status, 'No description available')
-      );
-    }
-
-    // We know there's data, so let's check if it's in JSON format
-    try {
-      const data = JSON.parse(textBody);
-
-      // Check if it's 401 unauthorized error
-      if (response.status === 401) {
-        // TODO: Introduce some kind of interceptor (?)
-        // if (!/\/(login|signup)$/.test(window?.location?.pathname)) {
-        //   window.location.href = mountURL('/login');
-        // }
-        return Result.err(new RequestNotOkError(response.status, data.error));
-      }
-
-      // Usually it's a feedback on user's actions like form validation
-      if ('errors' in data && Array.isArray(data.errors)) {
-        return Result.err(
-          new RequestNotOkWithErrorsList(response.status, data.errors)
-        );
-      }
-
-      // Error message may come in an 'error' field
-      if ('error' in data && typeof data.error === 'string') {
-        return Result.err(new RequestNotOkError(response.status, data.error));
-      }
-
-      // Error message may come in an 'message' field
-      if ('message' in data && typeof data.message === 'string') {
-        return Result.err(new RequestNotOkError(response.status, data.message));
-      }
-
-      return Result.err(
-        new RequestNotOkError(
-          response.status,
-          `Could not identify an error message. Payload is ${JSON.stringify(
-            data
-          )}`
-        )
-      );
-    } catch (e) {
-      // We couldn't parse, but there's definitly some data
-      // We must handle this case since the go server sometimes responds with plain text
-
-      // It's HTML
-      // Which normally happens when hitting a broken URL, which makes the server return the SPA
-      // Poor heuristic for identifying it's a html file
-      if (/<\/?[a-z][\s\S]*>/i.test(textBody)) {
-        return Result.err(
-          new ResponseNotOkInHTMLFormat(response.status, textBody)
-        );
-      }
-      return Result.err(new RequestNotOkError(response.status, textBody));
-    }
-  }
-
-  // Server responded with 2xx
-  const textBody = await response.text();
-
-  // There's nothing in the body
-  if (!textBody || !textBody.length) {
-    return Result.ok({
-      statusCode: response.status,
-    });
-  }
-
-  // We know there's data, so let's check if it's in JSON format
-  try {
-    const data = JSON.parse(textBody);
-
-    // We could parse the response
-    return Result.ok(data);
-  } catch (e) {
-    // We couldn't parse, but there's definitly some data
-    return Result.err(new ResponseOkNotInJSONFormat(response.status, textBody));
-  }
-}
-
 // We have to call it something else otherwise it will conflict with the global "Response"
 type ResponseFromRequest = Awaited<ReturnType<typeof request>>;
 type Schema = Parameters<typeof modelToResult>[0];
diff --git a/public/app/services/render.ts b/public/app/services/render.ts
@@ -14,11 +14,7 @@ import {
 import { Timeline, TimelineSchema } from '@pyroscope/models/timeline';
 import { Annotation, AnnotationSchema } from '@pyroscope/models/annotation';
 import type { RequestError } from '@pyroscope/services/base';
-import {
-  request,
-  parseResponse,
-  requestWithOrgID,
-} from '@pyroscope/services/base';
+import { request, parseResponse } from '@pyroscope/services/base';
 
 export interface RenderOutput {
   profile: Profile;
@@ -50,7 +46,7 @@ export async function renderSingle(
 ): Promise<Result<RenderOutput, RequestError | ZodError>> {
   const url = buildRenderURL(props);
   // TODO
-  const response = await requestWithOrgID(`/pyroscope${url}&format=json`, {
+  const response = await request(`/pyroscope${url}&format=json`, {
     signal: controller?.signal,
   });
 
@@ -110,7 +106,7 @@ export async function renderDiff(
     rightUntil: props.rightUntil,
   });
 
-  const response = await requestWithOrgID(`/pyroscope/render-diff?${params}`, {
+  const response = await request(`/pyroscope/render-diff?${params}`, {
     signal: controller?.signal,
   });
 
@@ -156,7 +152,7 @@ export async function renderExplore(
   }
 ): Promise<Result<RenderExploreOutput, RequestError | ZodError>> {
   const url = buildRenderURL(props);
-  const response = await requestWithOrgID(`/pyroscope/${url}&format=json`, {
+  const response = await request(`/pyroscope/${url}&format=json`, {
     signal: controller?.signal,
   });
   return parseResponse<RenderExploreOutput>(response, RenderExploreSchema);
diff --git a/public/app/services/tags.ts b/public/app/services/tags.ts
@@ -1,4 +1,4 @@
-import { parseResponse, requestWithOrgID } from '@pyroscope/services/base';
+import { parseResponse, request } from '@pyroscope/services/base';
 import { z } from 'zod';
 
 const labelNamesSchema = z.preprocess(
@@ -33,20 +33,17 @@ export function queryToMatchers(query: string) {
 }
 
 export async function fetchTags(query: string, from: number, until: number) {
-  const response = await requestWithOrgID(
-    '/querier.v1.QuerierService/LabelNames',
-    {
-      method: 'POST',
-      body: JSON.stringify({
-        matchers: queryToMatchers(query),
-        start: from,
-        end: until,
-      }),
-      headers: {
-        'content-type': 'application/json',
-      },
-    }
-  );
+  const response = await request('/querier.v1.QuerierService/LabelNames', {
+    method: 'POST',
+    body: JSON.stringify({
+      matchers: queryToMatchers(query),
+      start: from,
+      end: until,
+    }),
+    headers: {
+      'content-type': 'application/json',
+    },
+  });
   const isMetaTag = (tag: string) => tag.startsWith('__') && tag.endsWith('__');
 
   return parseResponse<string[]>(
@@ -63,21 +60,18 @@ export async function fetchLabelValues(
   from: number,
   until: number
 ) {
-  const response = await requestWithOrgID(
-    '/querier.v1.QuerierService/LabelValues',
-    {
-      method: 'POST',
-      body: JSON.stringify({
-        matchers: queryToMatchers(query),
-        name: label,
-        start: from,
-        end: until,
-      }),
-      headers: {
-        'content-type': 'application/json',
-      },
-    }
-  );
+  const response = await request('/querier.v1.QuerierService/LabelValues', {
+    method: 'POST',
+    body: JSON.stringify({
+      matchers: queryToMatchers(query),
+      name: label,
+      start: from,
+      end: until,
+    }),
+    headers: {
+      'content-type': 'application/json',
+    },
+  });
 
   return parseResponse<string[]>(
     response,
diff --git a/public/app/services/tenant.ts b/public/app/services/tenant.ts
@@ -1,7 +1,7 @@
-import { RequestNotOkError, requestWithOrgID } from '@pyroscope/services/base';
+import { RequestNotOkError, request } from '@pyroscope/services/base';
 
 export async function isMultiTenancyEnabled() {
-  const res = await requestWithOrgID('/querier.v1.QuerierService/LabelNames', {
+  const res = await request('/querier.v1.QuerierService/LabelNames', {
     // Without this it would automatically add the OrgID
     // Which doesn't tell us whether multitenancy is enabled or not
     headers: {
@@ -22,7 +22,7 @@ export async function isMultiTenancyEnabled() {
   return isOrgRequiredError(res);
 }
 
-function isOrgRequiredError(res: Awaited<ReturnType<typeof requestWithOrgID>>) {
+function isOrgRequiredError(res: Awaited<ReturnType<typeof request>>) {
   // TODO: is 'no org id' a stable message?
   return (
     res.isErr &&
