fix(rest): parse query string even when there is no rest query param

See #2088

Author: raymondfeng

packages/rest/package.json

@@ -30,7 +30,6 @@
     "@types/express": "^4.11.1",
     "@types/express-serve-static-core": "^4.16.0",
     "@types/http-errors": "^1.6.1",
-    "@types/parseurl": "^1.3.1",
     "@types/serve-static": "1.13.2",
     "@types/type-is": "^1.6.2",
     "ajv": "^6.5.1",
@@ -43,7 +42,6 @@
     "lodash": "^4.17.5",
     "openapi-schema-to-json-schema": "^2.1.0",
     "openapi3-ts": "^1.0.0",
-    "parseurl": "^1.3.2",
     "path-to-regexp": "^2.2.0",
     "qs": "^6.5.2",
     "strong-error-handler": "^3.2.0",

packages/rest/src/parser.ts

@@ -10,20 +10,14 @@ import {
   ParameterObject,
   SchemasObject,
 } from '@loopback/openapi-v3-types';
-import * as debugModule from 'debug';
-import * as parseUrl from 'parseurl';
-import {parse as parseQuery} from 'qs';
+import * as debugFactory from 'debug';
 import {RequestBody, RequestBodyParser} from './body-parsers';
 import {coerceParameter} from './coercion/coerce-parameter';
 import {RestHttpErrors} from './rest-http-error';
 import {ResolvedRoute} from './router';
 import {OperationArgs, PathParameterValues, Request} from './types';
 import {validateRequestBody} from './validation/request-body.validator';
-
-const debug = debugModule('loopback:rest:parser');
-
-export const QUERY_NOT_PARSED = {};
-Object.freeze(QUERY_NOT_PARSED);
+const debug = debugFactory('loopback:rest:parser');
 
 /**
  * Parses the request to derive arguments to be passed in for the Application
@@ -99,29 +93,15 @@ function getParamFromRequest(
 ) {
   switch (spec.in) {
     case 'query':
-      ensureRequestQueryWasParsed(request);
       return request.query[spec.name];
     case 'path':
       return pathParams[spec.name];
     case 'header':
       // @jannyhou TBD: check edge cases
       return request.headers[spec.name.toLowerCase()];
-      break;
     // TODO(jannyhou) to support `cookie`,
     // see issue https://github.com/strongloop/loopback-next/issues/997
     default:
       throw RestHttpErrors.invalidParamLocation(spec.in);
   }
 }
-
-function ensureRequestQueryWasParsed(request: Request) {
-  if (request.query && request.query !== QUERY_NOT_PARSED) return;
-
-  const input = parseUrl(request)!.query;
-  if (input && typeof input === 'string') {
-    request.query = parseQuery(input);
-  } else {
-    request.query = {};
-  }
-  debug('Parsed request query: ', request.query);
-}

packages/rest/src/rest.server.ts

@@ -31,7 +31,6 @@ import {ServeStaticOptions} from 'serve-static';
 import {BodyParser, REQUEST_BODY_PARSER_TAG} from './body-parsers';
 import {HttpHandler} from './http-handler';
 import {RestBindings} from './keys';
-import {QUERY_NOT_PARSED} from './parser';
 import {RequestContext} from './request-context';
 import {
   ControllerClass,
@@ -193,15 +192,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
 
   protected _setupRequestHandler() {
     this._expressApp = express();
-
-    // Disable express' built-in query parser, we parse queries ourselves
-    // Note that when disabled, express sets query to an empty object,
-    // which makes it difficult for us to detect whether the query
-    // has been parsed or not. At the same time, we want `request.query`
-    // to remain as an object, because everybody in express ecosystem expects
-    // that property to be defined. A static singleton object to the rescue!
-    this._expressApp.set('query parser fn', (str: string) => QUERY_NOT_PARSED);
-
+    this._expressApp.set('query parser', 'extended');
     this.requestHandler = this._expressApp;
 
     // Allow CORS support for all endpoints so that users

packages/rest/test/integration/rest.server.integration.ts

@@ -5,39 +5,39 @@
 
 import {Application} from '@loopback/core';
 import {
-  supertest,
-  expect,
   createClientForHandler,
-  itSkippedOnTravis,
-  httpsGetAsync,
+  expect,
   givenHttpServerConfig,
+  httpsGetAsync,
+  itSkippedOnTravis,
+  supertest,
 } from '@loopback/testlab';
+import * as fs from 'fs';
+import {IncomingMessage, ServerResponse} from 'http';
+import * as yaml from 'js-yaml';
+import * as path from 'path';
+import {is} from 'type-is';
+import * as util from 'util';
 import {
-  RestBindings,
-  RestServer,
-  RestComponent,
+  BodyParser,
   get,
+  post,
   Request,
+  requestBody,
+  RequestContext,
+  RestBindings,
+  RestComponent,
+  RestServer,
   RestServerConfig,
-  BodyParser,
 } from '../..';
-import {IncomingMessage, ServerResponse} from 'http';
-import * as yaml from 'js-yaml';
-import * as path from 'path';
-import * as fs from 'fs';
-import * as util from 'util';
 const readFileAsync = util.promisify(fs.readFile);
 
-import {is} from 'type-is';
-import {requestBody, post} from '../../src';
-
 const FIXTURES = path.resolve(__dirname, '../../../fixtures');
 const ASSETS = path.resolve(FIXTURES, 'assets');
 
 describe('RestServer (integration)', () => {
   it('exports url property', async () => {
-    // Explicitly setting host to IPv4 address so test runs on Travis
-    const server = await givenAServer({rest: {port: 0, host: '127.0.0.1'}});
+    const server = await givenAServer();
     server.handler(dummyRequestHandler);
     expect(server.url).to.be.undefined();
     await server.start();
@@ -52,8 +52,26 @@ describe('RestServer (integration)', () => {
     expect(server.url).to.be.undefined();
   });
 
+  it('parses query without decorated rest query params', async () => {
+    // This handler responds with the query object (which is expected to
+    // be parsed by Express)
+    function requestWithQueryHandler({request, response}: RequestContext) {
+      response.json(request.query);
+      response.end();
+    }
+
+    // See https://github.com/strongloop/loopback-next/issues/2088
+    const server = await givenAServer();
+    server.handler(requestWithQueryHandler);
+    await server.start();
+    await supertest(server.url)
+      .get('/?x=1&y[a]=2')
+      .expect(200, {x: '1', y: {a: '2'}});
+    await server.stop();
+  });
+
   it('updates rest.port binding when listening on ephemeral port', async () => {
-    const server = await givenAServer({rest: {port: 0}});
+    const server = await givenAServer();
     await server.start();
     expect(server.getSync(RestBindings.PORT)).to.be.above(0);
     await server.stop();
@@ -73,7 +91,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('responds with 500 when Sequence fails with unhandled error', async () => {
-    const server = await givenAServer({rest: {port: 0}});
+    const server = await givenAServer();
     server.handler((context, sequence) => {
       return Promise.reject(new Error('unhandled test error'));
     });
@@ -111,11 +129,7 @@ describe('RestServer (integration)', () => {
 
   it('allows static assets via api', async () => {
     const root = ASSETS;
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
 
     server.static('/html', root);
     const content = fs
@@ -129,11 +143,7 @@ describe('RestServer (integration)', () => {
 
   it('allows static assets to be mounted on multiple paths', async () => {
     const root = ASSETS;
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
 
     server.static('/html-0', root);
     server.static('/html-1', root);
@@ -154,11 +164,7 @@ describe('RestServer (integration)', () => {
   it('merges different static asset directories when mounted on the same path', async () => {
     const root = ASSETS;
     const otherAssets = path.join(FIXTURES, 'other-assets');
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
 
     server.static('/html', root);
     server.static('/html', otherAssets);
@@ -178,11 +184,7 @@ describe('RestServer (integration)', () => {
 
   it('allows static assets via api after start', async () => {
     const root = ASSETS;
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
     await createClientForHandler(server.requestHandler)
       .get('/html/index.html')
       .expect(404);
@@ -196,11 +198,7 @@ describe('RestServer (integration)', () => {
 
   it('allows non-static routes after assets', async () => {
     const root = ASSETS;
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
     server.static('/html', root);
     server.handler(dummyRequestHandler);
 
@@ -211,11 +209,7 @@ describe('RestServer (integration)', () => {
 
   it('gives precedence to API routes over static assets', async () => {
     const root = ASSETS;
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
     server.static('/html', root);
     server.handler(dummyRequestHandler);
 
@@ -226,11 +220,7 @@ describe('RestServer (integration)', () => {
 
   it('registers controllers defined later than static assets', async () => {
     const root = ASSETS;
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
     server.static('/html', root);
     server.controller(DummyController);
 
@@ -256,7 +246,7 @@ describe('RestServer (integration)', () => {
       }
     }
 
-    const server = await givenAServer({rest: {port: 0}});
+    const server = await givenAServer();
     // Register a request body parser for xml
     server.bodyParser(XmlBodyParser);
     server.controller(DummyXmlController);
@@ -269,7 +259,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('allows cors', async () => {
-    const server = await givenAServer({rest: {port: 0}});
+    const server = await givenAServer();
     server.handler(dummyRequestHandler);
 
     await createClientForHandler(server.requestHandler)
@@ -280,7 +270,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('allows cors preflight', async () => {
-    const server = await givenAServer({rest: {port: 0}});
+    const server = await givenAServer();
     server.handler(dummyRequestHandler);
 
     await createClientForHandler(server.requestHandler)
@@ -311,11 +301,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('exposes "GET /openapi.json" endpoint', async () => {
-    const server = await givenAServer({
-      rest: {
-        port: 0,
-      },
-    });
+    const server = await givenAServer();
     const greetSpec = {
       responses: {
         200: {
@@ -407,7 +393,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('exposes "GET /openapi.yaml" endpoint', async () => {
-    const server = await givenAServer({rest: {port: 0}});
+    const server = await givenAServer();
     const greetSpec = {
       responses: {
         200: {
@@ -701,7 +687,10 @@ paths:
     await server.stop();
   });
 
-  async function givenAServer(options?: {rest: RestServerConfig}) {
+  async function givenAServer(
+    options: {rest: RestServerConfig} = {rest: {port: 0}},
+  ) {
+    options.rest = givenHttpServerConfig(options.rest);
     const app = new Application(options);
     app.component(RestComponent);
     return await app.getServer(RestServer);

packages/rest/test/unit/coercion/utils.ts

@@ -54,8 +54,7 @@ export async function testCoercion<T>(config: TestArgs<T>) {
   /* istanbul ignore next */
   try {
     const pathParams: PathParameterValues = {};
-
-    const req = givenRequest();
+    let url = '/';
     const spec = givenOperationWithParameters([config.paramSpec]);
     const route = givenResolvedRoute(spec, pathParams);
 
@@ -68,7 +67,7 @@ export async function testCoercion<T>(config: TestArgs<T>) {
           {aparameter: config.rawValue},
           {encodeValuesOnly: true},
         );
-        req.url += `?${q}`;
+        url += `?${q}`;
         break;
       case 'header':
       case 'cookie':
@@ -81,6 +80,10 @@ export async function testCoercion<T>(config: TestArgs<T>) {
         break;
     }
 
+    // Create the request after url is fully populated so that request.query
+    // is parsed
+    const req = givenRequest({url});
+
     const requestBodyParser = new RequestBodyParser();
     if (config.expectError) {
       await expect(

packages/testlab/src/shot.ts

@@ -107,6 +107,7 @@ export function stubExpressContext(
   }
   request.app = app;
   request.originalUrl = request.url;
+  parseQuery(request);
 
   let response: express.Response | undefined;
   let result = new Promise<ObservedResponse>(resolve => {
@@ -131,6 +132,17 @@ export function stubExpressContext(
   return context;
 }
 
+/**
+ * Use `express.query` to parse the query string into `request.query` object
+ * @param request Express http request object
+ */
+function parseQuery(request: express.Request) {
+  // Use `express.query` to parse the query string
+  // See https://github.com/expressjs/express/blob/master/lib/express.js#L79
+  // See https://github.com/expressjs/express/blob/master/lib/middleware/query.js
+  (express as any).query()(request, {}, () => {});
+}
+
 function defineCustomContextInspect(
   context: HandlerContextStub,
   requestOptions: ShotRequestOptions,