PR feedback: Update custom params arg to take an object instead of a …

…query string
elastic · Aug 27, 2020 · f6a709d · f6a709d
1 parent 74a4149
commit f6a709d
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 13 deletions.
diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts
@@ -86,12 +86,12 @@ describe('EnterpriseSearchRequestHandler', () => {
     it('passes custom params set by the handler, which override request params', async () => {
       const requestHandler = enterpriseSearchRequestHandler.createRequest({
         path: '/api/example',
-        params: '?some=custom&params=true',
+        params: { someQuery: true },
       });
-      await makeAPICall(requestHandler, { query: { overriden: true } });
+      await makeAPICall(requestHandler, { query: { someQuery: false } });
 
       EnterpriseSearchAPI.shouldHaveBeenCalledWith(
-        'http://localhost:3002/api/example?some=custom&params=true'
+        'http://localhost:3002/api/example?someQuery=true'
       );
     });
   });
@@ -165,6 +165,11 @@ describe('EnterpriseSearchRequestHandler', () => {
       });
     });
   });
+
+  it('has a helper for checking empty objects', async () => {
+    expect(enterpriseSearchRequestHandler.isEmptyObj({})).toEqual(true);
+    expect(enterpriseSearchRequestHandler.isEmptyObj({ empty: false })).toEqual(false);
+  });
 });
 
 const makeAPICall = (handler: Function, params = {}) => {

diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts
@@ -21,7 +21,7 @@ interface IConstructorDependencies {
 }
 interface IRequestParams<ResponseBody> {
   path: string;
-  params?: string;
+  params?: object;
   hasValidData?: (body?: ResponseBody) => boolean;
 }
 export interface IEnterpriseSearchRequestHandler {
@@ -47,7 +47,7 @@ export class EnterpriseSearchRequestHandler {
 
   createRequest<ResponseBody>({
     path,
-    params,
+    params = {},
     hasValidData = () => true,
   }: IRequestParams<ResponseBody>) {
     return async (
@@ -57,13 +57,16 @@ export class EnterpriseSearchRequestHandler {
     ) => {
       try {
         // Set up API URL
-        params = params ?? (request.query ? `?${querystring.stringify(request.query)}` : '');
-        const url = encodeURI(this.enterpriseSearchUrl + path + params);
+        const queryParams = { ...request.query, ...params };
+        const queryString = !this.isEmptyObj(queryParams)
+          ? `?${querystring.stringify(queryParams)}`
+          : '';
+        const url = encodeURI(this.enterpriseSearchUrl + path + queryString);
 
         // Set up API options
         const { method } = request.route;
         const headers = { Authorization: request.headers.authorization as string };
-        const body = Object.keys(request.body as object).length
+        const body = !this.isEmptyObj(request.body as object)
           ? JSON.stringify(request.body)
           : undefined;
 
@@ -93,4 +96,9 @@ export class EnterpriseSearchRequestHandler {
       }
     };
   }
+
+  // Small helper
+  isEmptyObj(obj: object) {
+    return Object.keys(obj).length === 0;
+  }
 }
diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
@@ -38,7 +38,7 @@ describe('engine routes', () => {
 
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
         path: '/as/engines/collection',
-        params: encodeURI('?type=indexed&page[current]=1&page[size]=10'),
+        params: { type: 'indexed', 'page[current]': 1, 'page[size]': 10 },
         hasValidData: expect.any(Function),
       });
     });
@@ -48,7 +48,7 @@ describe('engine routes', () => {
 
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
         path: '/as/engines/collection',
-        params: encodeURI('?type=meta&page[current]=99&page[size]=10'),
+        params: { type: 'meta', 'page[current]': 99, 'page[size]': 10 },
         hasValidData: expect.any(Function),
       });
     });

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts
@@ -4,7 +4,6 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import querystring from 'querystring';
 import { schema } from '@kbn/config-schema';
 
 import { IRouteDependencies } from '../../plugin';
@@ -34,11 +33,11 @@ export function registerEnginesRoute({
 
       return enterpriseSearchRequestHandler.createRequest({
         path: '/as/engines/collection',
-        params: `?${querystring.stringify({
+        params: {
           type,
           'page[current]': pageIndex,
           'page[size]': ENGINES_PAGE_SIZE,
-        })}`,
+        },
         hasValidData: (body?: IEnginesResponse) =>
           Array.isArray(body?.results) && typeof body?.meta?.page?.total_results === 'number',
       })(context, request, response);