Skip to content

Commit

Permalink
Merge 0b01574 into cb57a42
Browse files Browse the repository at this point in the history
  • Loading branch information
@raymondfeng
raymondfeng committed Dec 3, 2018
2 parents cb57a42 + 0b01574 commit ee210e4
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 100 deletions.
2 changes: 0 additions & 2 deletions packages/rest/package.json
View file
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
25 changes: 4 additions & 21 deletions packages/rest/src/parser.ts
View file
Expand Up @@ -10,20 +10,17 @@ 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 = debugFactory('loopback:rest:parser');

const debug = debugModule('loopback:rest:parser');

export const QUERY_NOT_PARSED = {};
Object.freeze(QUERY_NOT_PARSED);
// See https://github.com/expressjs/express/blob/master/lib/express.js#L79
const expressQuery = require('express').query;

/**
* Parses the request to derive arguments to be passed in for the Application
Expand Down Expand Up @@ -99,29 +96,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);
}
11 changes: 1 addition & 10 deletions packages/rest/src/rest.server.ts
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
116 changes: 52 additions & 64 deletions packages/rest/test/integration/rest.server.integration.ts
View file
Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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'));
});
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -702,6 +688,8 @@ paths:
});

async function givenAServer(options?: {rest: RestServerConfig}) {
options = options || {rest: {port: 0}};
options.rest = givenHttpServerConfig(options.rest);
const app = new Application(options);
app.component(RestComponent);
return await app.getServer(RestServer);
Expand Down
9 changes: 6 additions & 3 deletions packages/rest/test/unit/coercion/utils.ts
View file
Expand Up @@ -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);

Expand All @@ -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':
Expand All @@ -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(
Expand Down
12 changes: 12 additions & 0 deletions packages/testlab/src/shot.ts
View file
Expand Up @@ -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 => {
Expand All @@ -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,
Expand Down

0 comments on commit ee210e4

Please sign in to comment.