Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add type coercion #1370

Merged
merged 1 commit into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions examples/todo/src/controllers/todo.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ export class TodoController {
@param.path.number('id') id: number,
@requestBody() todo: Todo,
): Promise<boolean> {
// REST adapter does not coerce parameter values coming from string sources
// like path & query. As a workaround, we have to cast the value to a number
// ourselves.
// See https://github.com/strongloop/loopback-next/issues/750
id = +id;

return await this.todoRepo.replaceById(id, todo);
}

Expand All @@ -62,12 +56,6 @@ export class TodoController {
@param.path.number('id') id: number,
@requestBody() todo: Todo,
): Promise<boolean> {
// REST adapter does not coerce parameter values coming from string sources
// like path & query. As a workaround, we have to cast the value to a number
// ourselves.
// See https://github.com/strongloop/loopback-next/issues/750
id = +id;

return await this.todoRepo.updateById(id, todo);
}

Expand Down
115 changes: 115 additions & 0 deletions packages/rest/src/coercion/coerce-parameter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Copyright IBM Corp. 2018. All Rights Reserved.
// Node module: @loopback/rest
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {ParameterObject, isReferenceObject} from '@loopback/openapi-v3-types';
import {Validator} from './validator';
import * as debugModule from 'debug';
import {RestHttpErrors} from '../';

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

/**
* Coerce the http raw data to a JavaScript type data of a parameter
* according to its OpenAPI schema specification.
*
* @param data The raw data get from http request
* @param schema The parameter's schema defined in OpenAPI specification
*/
export function coerceParameter(data: string, spec: ParameterObject) {
const schema = spec.schema;
if (!schema || isReferenceObject(schema)) {
debug(
'The parameter with schema %s is not coerced since schema' +
'dereference is not supported yet.',
schema,
);
return data;
}
const OAIType = getOAIPrimitiveType(schema.type, schema.format);
const validator = new Validator({parameterSpec: spec});

validator.validateParamBeforeCoercion(data);

switch (OAIType) {
case 'byte':
return Buffer.from(data, 'base64');
case 'date':
return new Date(data);
case 'float':
case 'double':
return parseFloat(data);
case 'number':
const coercedData = data ? Number(data) : undefined;
if (coercedData === undefined) return;
if (isNaN(coercedData)) throw RestHttpErrors.invalidData(data, spec.name);
return coercedData;
case 'long':
return Number(data);
case 'integer':
return parseInt(data);
case 'boolean':
return isTrue(data) ? true : isFalse(data) ? false : undefined;
case 'string':
case 'password':
// serialize will be supported in next PR
case 'serialize':
default:
return data;
}
}

/**
* A set of truthy values. A data in this set will be coerced to `true`.
*
* @param data The raw data get from http request
* @returns The corresponding coerced boolean type
*/
function isTrue(data: string): boolean {
return ['true', '1'].includes(data);
}

/**
* A set of falsy values. A data in this set will be coerced to `false`.
* @param data The raw data get from http request
* @returns The corresponding coerced boolean type
*/
function isFalse(data: string): boolean {
return ['false', '0'].includes(data);
}

/**
* Return the corresponding OpenAPI data type given an OpenAPI schema
*
* @param type The type in an OpenAPI schema specification
* @param format The format in an OpenAPI schema specification
*/
function getOAIPrimitiveType(type?: string, format?: string) {
// serizlize will be supported in next PR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: serizlize -> serialize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@b-admike You may review a wrong commit? In the latest code it's already fixed, actually it was fixed long time(1-2 weeks) ago lol

if (type === 'object' || type === 'array') return 'serialize';
if (type === 'string') {
switch (format) {
case 'byte':
return 'byte';
case 'binary':
return 'binary';
case 'date':
return 'date';
case 'date-time':
return 'date-time';
case 'password':
return 'password';
default:
return 'string';
}
}
if (type === 'boolean') return 'boolean';
if (type === 'number')
return format === 'float'
? 'float'
: format === 'double'
? 'double'
: 'number';
if (type === 'integer') return format === 'int64' ? 'long' : 'integer';
}
16 changes: 16 additions & 0 deletions packages/rest/src/coercion/rest-http-error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import * as HttpErrors from 'http-errors';
export namespace RestHttpErrors {
export function invalidData<T>(data: T, name: string) {
const msg = `Invalid data ${JSON.stringify(data)} for parameter ${name}!`;
return new HttpErrors.BadRequest(msg);
}
export function missingRequired(name: string): HttpErrors.HttpError {
const msg = `Required parameter ${name} is missing!`;
return new HttpErrors.BadRequest(msg);
}
export function invalidParamLocation(location: string): HttpErrors.HttpError {
return new HttpErrors.NotImplemented(
'Parameters with "in: ' + location + '" are not supported yet.',
);
}
}
68 changes: 68 additions & 0 deletions packages/rest/src/coercion/validator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright IBM Corp. 2018. All Rights Reserved.
// Node module: @loopback/rest
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {ParameterObject} from '@loopback/openapi-v3-types';
import {RestHttpErrors} from '../';

/**
* A set of options to pass into the validator functions
*/
export type ValidationOptions = {
required?: boolean;
};

/**
* The context information that a validator needs
*/
export type ValidationContext = {
parameterSpec: ParameterObject;
};

/**
* Validator class provides a bunch of functions that perform
* validations on the request parameters and request body.
*/
export class Validator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were using AJV for the validation portion of the epic. Is this catered for interweaving coercion and validation together?

I think I may get a better understanding if I was to see how the code here would be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial thought:

Can we leave Validation out of scope of this initial pull request please?

As I understand our plan, the scope of #750 is only coercion. Validation will be covered by #118.

Even if we think that #750 should cover basic validation, I would prefer to keep this initial pull request as small as possible so that we can land it sooner.


On the second though, it's difficult to implement coercion without basic validation, because we need some way to handle string values that cannot be converted to the target type.

Would it make sense to (temporarily?) simplify this part and let coerceParameter throw an error when the string value cannot be converted to the target type? My idea is to reject only values that cannot be converted to target type. Any validation rules beyond that would be left for the validation framework to implement and handle.

As for required flag, I think that's an example of a validation rule that's out of scope of coercion. When the request does not provide any value for the argument (or provides an empty string), the coercion should return undefined and let the rest of the framework to take care of that.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A code snippet to illustrate what I meant.

Now:

   case 'number':
      validator.validateParamBeforeCoercion('number', data);
      coercedResult = data ? Number(data) : undefined;
      validator.validateParamAfterCoercion('number', coercedResult);

My proposal:

// handle empty string for all types at the top

   case 'number':
      // we can assume "data" is not an empty string here
      coercedResult = Number(data);
      if (isNaN(coercedResult)) {
        // report coercion error - data is not a number
     }

Copy link
Contributor Author

@jannyHou jannyHou Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos I think the problem is it's not possible to ignore the requirement check if I add all edge cases for a type.
AJV only validates the schema spec, but not the parameter spec, which means it's still our responsibility to do the check for required.

Would it make sense to (temporarily?) simplify this part and let coerceParameter throw an error when the string value cannot be converted to the target type? My idea is to reject only values that cannot be converted to target type.

That's the tricky part...required affects how we define "values that cannot be converted to target type", by saying that, I mean:

  • required: empty string cannot be converted
  • not required: empty string can be converted

IMO, requirement check should be part of the basic validation.

// handle empty string for all types at the top
   case 'number':
      // we can assume "data" is not an empty string here
      coercedResult = Number(data);
      if (isNaN(coercedResult)) {
        // report coercion error - data is not a number
     }

^ That looks good to me within this PR 👍While I still want to apply those checking functions through a validator class. The reason why I define the validator(does basic validation) as a class is

  • It's easy to extend
  • easy to share the context by a class constructor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is it's not possible to ignore the requirement check if I add all edge cases for a type.
AJV only validates the schema spec, but not the parameter spec, which means it's still our responsibility to do the check for required.

Good point about AJV capabilities. Even if we don't use AJV to validate parameter spec, my opinion is that validation is out of scope of #750.

That's the tricky part...required affects how we define "values that cannot be converted to target type", by saying that, I mean:

required: empty string cannot be converted
not required: empty string can be converted
IMO, requirement check should be part of the basic validation.

I have a different view. When the spec says that a parameter is of type number, I consider undefined as a valid value signaling there was no parameter value provided by the client. Whether a parameter can be undefined (is optional vs. required) is another matter that can be handled by different piece of code.

Having said that, I have re-read the Parameter Object from OpenAPI-Specification and the way how to spec is laid out, I agree with you it makes sense to handle required flag as part of parameter coercion.

constructor(public ctx: ValidationContext) {}

/**
* The validation executed before type coercion. Like
* checking absence.
*
* @param type A parameter's type.
* @param value A parameter's raw value from http request.
* @param opts options
*/
validateParamBeforeCoercion(
value: string | object | undefined,
opts?: ValidationOptions,
) {
if (this.isAbsent(value) && this.isRequired(opts)) {
const name = this.ctx.parameterSpec.name;
throw RestHttpErrors.missingRequired(name);
}
}

/**
* Check is a parameter required or not.
*
* @param opts
*/
isRequired(opts?: ValidationOptions) {
if (this.ctx.parameterSpec.required) return true;
if (opts && opts.required) return true;
return false;
}

/**
* Return `true` if the value is empty, return `false` otherwise.
*
* @param value
*/
// tslint:disable-next-line:no-any
isAbsent(value: any) {
return value === '' || value === undefined;
}
}
1 change: 1 addition & 0 deletions packages/rest/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,4 @@ export * from './rest.component';
export * from './rest.server';
export * from './sequence';
export * from '@loopback/openapi-v3';
export * from './coercion/rest-http-error';
47 changes: 30 additions & 17 deletions packages/rest/src/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {REQUEST_BODY_INDEX} from '@loopback/openapi-v3';
import {promisify} from 'util';
import {OperationArgs, Request, PathParameterValues} from './types';
import {ResolvedRoute} from './router/routing-table';
import {coerceParameter} from './coercion/coerce-parameter';
import {RestHttpErrors} from './index';
type HttpError = HttpErrors.HttpError;

// tslint:disable-next-line:no-any
Expand Down Expand Up @@ -101,24 +103,35 @@ function buildOperationArguments(
throw new Error('$ref parameters are not supported yet.');
}
const spec = paramSpec as ParameterObject;
switch (spec.in) {
case 'query':
paramArgs.push(request.query[spec.name]);
break;
case 'path':
paramArgs.push(pathParams[spec.name]);
break;
case 'header':
paramArgs.push(request.headers[spec.name.toLowerCase()]);
break;
// TODO(jannyhou) to support `cookie`,
// see issue https://github.com/strongloop/loopback-next/issues/997
default:
throw new HttpErrors.NotImplemented(
'Parameters with "in: ' + spec.in + '" are not supported yet.',
);
}
const rawValue = getParamFromRequest(spec, request, pathParams);
const coercedValue = coerceParameter(rawValue, spec);
paramArgs.push(coercedValue);
}
if (requestBodyIndex > -1) paramArgs.splice(requestBodyIndex, 0, body);
return paramArgs;
}

function getParamFromRequest(
spec: ParameterObject,
request: Request,
pathParams: PathParameterValues,
) {
let result;
switch (spec.in) {
case 'query':
result = request.query[spec.name];
break;
case 'path':
result = pathParams[spec.name];
break;
case 'header':
// @jannyhou TBD: check edge cases
result = 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);
}
return result;
}
58 changes: 58 additions & 0 deletions packages/rest/test/acceptance/coercion/coercion.acceptance.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import {supertest, createClientForHandler, sinon} from '@loopback/testlab';
import {RestApplication, get, param} from '../../..';

describe('Coercion', () => {
let app: RestApplication;
let client: supertest.SuperTest<supertest.Test>;

before(givenAClient);

after(async () => {
await app.stop();
});

class MyController {
@get('/create-number-from-path/{num}')
createNumberFromPath(@param.path.number('num') num: number) {
return num;
}

@get('/create-number-from-query')
createNumberFromQuery(@param.query.number('num') num: number) {
return num;
}

@get('/create-number-from-header')
createNumberFromHeader(@param.header.number('num') num: number) {
return num;
}
}

it('coerces parameter in path from string to number', async () => {
const spy = sinon.spy(MyController.prototype, 'createNumberFromPath');
await client.get('/create-number-from-path/100').expect(200);
sinon.assert.calledWithExactly(spy, 100);
});

it('coerces parameter in header from string to number', async () => {
const spy = sinon.spy(MyController.prototype, 'createNumberFromHeader');
await client.get('/create-number-from-header').set({num: 100});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter here should be in string if I understand how http headers work correctly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the HTTP client we are using (supertest/superagent) has to convert the value to string during transport, so it should not really matter whether we use a number or a string here.

Having said that, I don't mind either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shimks

I believe the HTTP client we are using (supertest/superagent) has to convert the value to string during transport,

Yep, see this edge case for type number, there is a conversion done by http client before the original query data reaches our paramParser, and that's why whatever data type you provide in the query/path/header, the paramParser always receive it as a string.

I use a number type in this test to mock a real use case. The unit tests are written in a way that the raw data is always a string.

sinon.assert.calledWithExactly(spy, 100);
});

it('coerces parameter in query from string to number', async () => {
const spy = sinon.spy(MyController.prototype, 'createNumberFromQuery');
await client
.get('/create-number-from-query')
.query({num: 100})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

.expect(200);
sinon.assert.calledWithExactly(spy, 100);
});

async function givenAClient() {
app = new RestApplication();
app.controller(MyController);
await app.start();
client = await createClientForHandler(app.requestHandler);
}
});
18 changes: 18 additions & 0 deletions packages/rest/test/unit/coercion/invalid-spec.unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright IBM Corp. 2018. All Rights Reserved.
// Node module: @loopback/rest
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {test} from './utils';
import {RestHttpErrors} from '../../../';
import {ParameterLocation} from '@loopback/openapi-v3-types';

const INVALID_PARAM = {
in: <ParameterLocation>'unknown',
name: 'aparameter',
schema: {type: 'unknown'},
};

describe('throws error for invalid parameter spec', () => {
test(INVALID_PARAM, '', RestHttpErrors.invalidParamLocation('unknown'));
});
19 changes: 19 additions & 0 deletions packages/rest/test/unit/coercion/paramStringToBoolean.unit.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright IBM Corp. 2018. All Rights Reserved.
// Node module: @loopback/rest
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {test} from './utils';
import {ParameterLocation} from '@loopback/openapi-v3-types';

const BOOLEAN_PARAM = {
in: <ParameterLocation>'path',
name: 'aparameter',
schema: {type: 'boolean'},
};

describe('coerce param from string to boolean', () => {
test(BOOLEAN_PARAM, 'false', false);
test(BOOLEAN_PARAM, 'true', true);
test(BOOLEAN_PARAM, undefined, undefined);
});
Loading