Skip to content

[PoC - WIP] very rough PoC on request parameter type coercion/validation #1256

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.

Sign up for GitHub

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

Closed
shimks wants to merge 5 commits into from
Closed

[PoC - WIP] very rough PoC on request parameter type coercion/validation #1256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5119da
feat: add @loopback/types to be the typing system
raymondfeng Feb 1, 2018
f3b2b8e
feature: do a very short PoC on type coercion
shimks Apr 13, 2018
94977f9
feat: add tier 1 validation support
shimks Apr 18, 2018
f41eebc
add 2nd tier validation and helpful error messages
shimks Apr 20, 2018
b5ec200
add valdation function support for @validator
shimks Apr 25, 2018
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
2 changes: 2 additions & 0 deletions packages/openapi-v3/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
"@loopback/context": "^0.8.1",
"@loopback/openapi-v3-types": "^0.5.0",
"@loopback/repository-json-schema": "^0.6.1",
"@loopback/validator": "^0.1.0",
"@types/json-schema": "^6.0.1",
"debug": "^3.1.0",
"lodash": "^4.17.5"
}
Expand Down
15 changes: 8 additions & 7 deletions packages/openapi-v3/src/json-to-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@

import {JsonDefinition} from '@loopback/repository-json-schema';
import {SchemaObject} from '@loopback/openapi-v3-types';
import {JSONSchema6} from 'json-schema';
import * as _ from 'lodash';

export function jsonToSchemaObject(json: JsonDefinition): SchemaObject {
export function jsonToSchemaObject(json: JSONSchema6): SchemaObject {
const result: SchemaObject = {};
const propsToIgnore = [
'anyOf',
Expand Down Expand Up @@ -36,27 +37,27 @@ export function jsonToSchemaObject(json: JsonDefinition): SchemaObject {
}
case 'definitions': {
result.definitions = _.mapValues(json.definitions, def =>
jsonToSchemaObject(def),
jsonToSchemaObject(def as JSONSchema6),
);
break;
}
case 'properties': {
result.properties = _.mapValues(json.properties, item =>
jsonToSchemaObject(item),
jsonToSchemaObject(item as JSONSchema6),
);
break;
}
case 'additionalProperties': {
if (typeof json.additionalProperties !== 'boolean') {
result.additionalProperties = jsonToSchemaObject(
json.additionalProperties as JsonDefinition,
json.additionalProperties as JSONSchema6,
);
}
break;
}
case 'items': {
const items = Array.isArray(json.items) ? json.items[0] : json.items;
result.items = jsonToSchemaObject(items as JsonDefinition);
result.items = jsonToSchemaObject(items as JSONSchema6);
break;
}
case 'enum': {
Expand All @@ -67,7 +68,7 @@ export function jsonToSchemaObject(json: JsonDefinition): SchemaObject {
newEnum.push(element);
} else {
// if element is JsonDefinition, convert to SchemaObject
newEnum.push(jsonToSchemaObject(element as JsonDefinition));
newEnum.push(jsonToSchemaObject(element as JSONSchema6));
}
}
result.enum = newEnum;
Expand All @@ -82,7 +83,7 @@ export function jsonToSchemaObject(json: JsonDefinition): SchemaObject {
break;
}
default: {
result[property] = json[property as keyof JsonDefinition];
result[property] = json[property as keyof JSONSchema6];
break;
}
}
Expand Down
29 changes: 15 additions & 14 deletions packages/openapi-v3/test/unit/json-to-schema.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,20 @@ import {JsonDefinition} from '@loopback/repository-json-schema';

import {SchemaObject} from '@loopback/openapi-v3-types';
import {jsonToSchemaObject} from '../..';
import {JSONSchema6} from 'json-schema';

describe('jsonToSchemaObject', () => {
it('does nothing when given an empty object', () => {
expect({}).to.eql({});
});
const typeDef: JsonDefinition = {type: ['string', 'number']};
const typeDef: JSONSchema6 = {type: ['string', 'number']};
const expectedType: SchemaObject = {type: 'string'};
it('converts type', () => {
propertyConversionTest(typeDef, expectedType);
});

it('ignores non-compatible JSON schema properties', () => {
const nonCompatibleDef: JsonDefinition = {
const nonCompatibleDef = {
anyOf: [],
oneOf: [],
additionalItems: {
Expand All @@ -34,7 +35,7 @@ describe('jsonToSchemaObject', () => {
});

it('converts allOf', () => {
const allOfDef: JsonDefinition = {
const allOfDef: JSONSchema6 = {
allOf: [typeDef, typeDef],
};
const expectedAllOf: SchemaObject = {
Expand All @@ -44,7 +45,7 @@ describe('jsonToSchemaObject', () => {
});

it('converts definitions', () => {
const definitionsDef: JsonDefinition = {
const definitionsDef: JSONSchema6 = {
definitions: {foo: typeDef, bar: typeDef},
};
const expectedDef: SchemaObject = {
Expand All @@ -54,7 +55,7 @@ describe('jsonToSchemaObject', () => {
});

it('converts properties', () => {
const propertyDef: JsonDefinition = {
const propertyDef: JSONSchema6 = {
properties: {
foo: typeDef,
},
Expand All @@ -69,7 +70,7 @@ describe('jsonToSchemaObject', () => {

context('additionalProperties', () => {
it('is converted properly when the type is JsonDefinition', () => {
const additionalDef: JsonDefinition = {
const additionalDef: JSONSchema6 = {
additionalProperties: typeDef,
};
const expectedAdditional: SchemaObject = {
Expand All @@ -79,7 +80,7 @@ describe('jsonToSchemaObject', () => {
});

it('is converted properly when it is "false"', () => {
const noAdditionalDef: JsonDefinition = {
const noAdditionalDef: JSONSchema6 = {
additionalProperties: false,
};
const expectedDef: SchemaObject = {};
Expand All @@ -88,7 +89,7 @@ describe('jsonToSchemaObject', () => {
});

it('converts items', () => {
const itemsDef: JsonDefinition = {
const itemsDef: JSONSchema6 = {
type: 'array',
items: typeDef,
};
Expand All @@ -101,7 +102,7 @@ describe('jsonToSchemaObject', () => {

context('enum', () => {
it('is converted properly when the type is primitive', () => {
const enumStringDef: JsonDefinition = {
const enumStringDef: JSONSchema6 = {
enum: ['foo', 'bar'],
};
const expectedStringDef: SchemaObject = {
Expand All @@ -111,17 +112,17 @@ describe('jsonToSchemaObject', () => {
});

it('is converted properly when it is null', () => {
const enumNullDef: JsonDefinition = {
const enumNullDef: JSONSchema6 = {
enum: [null, null],
};
const expectedNullDef: JsonDefinition = {
const expectedNullDef: JSONSchema6 = {
enum: [null, null],
};
propertyConversionTest(enumNullDef, expectedNullDef);
});

it('is converted properly when the type is complex', () => {
const enumCustomDef: JsonDefinition = {
const enumCustomDef: JSONSchema6 = {
enum: [typeDef, typeDef],
};
const expectedCustomDef: SchemaObject = {
Expand All @@ -132,7 +133,7 @@ describe('jsonToSchemaObject', () => {
});

it('retains given properties in the conversion', () => {
const inputDef: JsonDefinition = {
const inputDef: JSONSchema6 = {
title: 'foo',
type: 'object',
properties: {
Expand All @@ -142,7 +143,7 @@ describe('jsonToSchemaObject', () => {
},
default: 'Default string',
};
const expectedDef: SchemaObject = {
const expectedDef: JSONSchema6 = {
title: 'foo',
type: 'object',
properties: {
Expand Down
1 change: 1 addition & 0 deletions packages/repository-json-schema/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"dependencies": {
"@loopback/context": "^0.8.1",
"@loopback/repository": "^0.8.1",
"@types/json-schema": "^6.0.1",
"lodash": "^4.17.5",
"typescript-json-schema": "^0.22.0"
},
Expand Down
15 changes: 8 additions & 7 deletions packages/repository-json-schema/src/build-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ import {
import {includes} from 'lodash';
import {Definition, PrimitiveType} from 'typescript-json-schema';
import {MetadataInspector, MetadataAccessor} from '@loopback/context';
import {JSONSchema6} from 'json-schema';

export const JSON_SCHEMA_KEY = MetadataAccessor.create<JsonDefinition>(
export const JSON_SCHEMA_KEY = MetadataAccessor.create<JSONSchema6>(
'loopback:json-schema',
);

Expand All @@ -38,7 +39,7 @@ export interface JsonDefinition extends Definition {
* in a cache. If not, one is generated and then cached.
* @param ctor Contructor of class to get JSON Schema from
*/
export function getJsonSchema(ctor: Function): JsonDefinition {
export function getJsonSchema(ctor: Function): JSONSchema6 {
// NOTE(shimks) currently impossible to dynamically update
const jsonSchema = MetadataInspector.getClassMetadata(JSON_SCHEMA_KEY, ctor);
if (jsonSchema) {
Expand Down Expand Up @@ -89,9 +90,9 @@ export function isComplexType(ctor: Function) {
* Converts property metadata into a JSON property definition
* @param meta
*/
export function metaToJsonProperty(meta: PropertyDefinition): JsonDefinition {
export function metaToJsonProperty(meta: PropertyDefinition): JSONSchema6 {
let ctor = meta.type as string | Function;
let def: JsonDefinition = {};
let def: JSONSchema6 = {};

// errors out if @property.array() is not used on a property of array
if (ctor === Array) {
Expand All @@ -108,7 +109,7 @@ export function metaToJsonProperty(meta: PropertyDefinition): JsonDefinition {

if (meta.array) {
def.type = 'array';
def.items = propDef;
def.items = <JSONSchema6>propDef;
} else {
Object.assign(def, propDef);
}
Expand All @@ -124,9 +125,9 @@ export function metaToJsonProperty(meta: PropertyDefinition): JsonDefinition {
* reflection API
* @param ctor Constructor of class to convert from
*/
export function modelToJsonSchema(ctor: Function): JsonDefinition {
export function modelToJsonSchema(ctor: Function): JSONSchema6 {
const meta: ModelDefinition | {} = ModelMetadataHelper.getModelMetadata(ctor);
const result: JsonDefinition = {};
const result: JSONSchema6 = {};

// returns an empty object if metadata is an empty object
if (!(meta instanceof ModelDefinition)) {
Expand Down
3 changes: 2 additions & 1 deletion packages/repository-json-schema/test/integration/build-schema.integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {modelToJsonSchema} from '../../src/build-schema';
import {expect} from '@loopback/testlab';
import {MetadataInspector} from '@loopback/context';
import {JSON_SCHEMA_KEY, getJsonSchema} from '../../index';
import {JSONSchema6} from 'json-schema';

describe('build-schema', () => {
describe('modelToJsonSchema', () => {
Expand Down Expand Up @@ -339,7 +340,7 @@ describe('build-schema', () => {
class TestModel {
@property() foo: number;
}
const cachedSchema = {
const cachedSchema: JSONSchema6 = {
properties: {
cachedProperty: {
type: 'string',
Expand Down
1 change: 1 addition & 0 deletions packages/rest/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"@loopback/core": "^0.6.1",
"@loopback/openapi-v3": "^0.8.0",
"@loopback/openapi-v3-types": "^0.5.0",
"@loopback/types": "^0.1.0",
"@types/cors": "^2.8.3",
"@types/http-errors": "^1.6.1",
"body": "^5.1.0",
Expand Down
29 changes: 27 additions & 2 deletions packages/rest/src/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
OperationObject,
ParameterObject,
isReferenceObject,
isSchemaObject,
} from '@loopback/openapi-v3-types';
import {REQUEST_BODY_INDEX} from '@loopback/openapi-v3';
import {promisify} from 'util';
Expand All @@ -18,6 +19,8 @@ import {
PathParameterValues,
} from './internal-types';
import {ResolvedRoute} from './router/routing-table';
import {getSerializer} from '@loopback/types';
const debug = require('debug')('loopback:rest:parser');
type HttpError = HttpErrors.HttpError;

// tslint:disable-next-line:no-any
Expand Down Expand Up @@ -123,6 +126,28 @@ function buildOperationArguments(
);
}
}
if (requestBodyIndex > -1) paramArgs.splice(requestBodyIndex, 0, body);
return paramArgs;

debug('Coercing parameters', paramArgs);

const coercedParamArgs: OperationArgs = [];
const paramObjects = operationSpec.parameters;
// coercion is done IFF parameters are defined in the OpenAPI spec
if (paramObjects) {
for (let i = 0; i < paramArgs.length; i++) {
const paramObject = paramObjects[i];
if (!isReferenceObject(paramObject)) {
// is a ParameterObject
if (paramObject.schema) {
if (isSchemaObject(paramObject.schema)) {
// basic type
const serializer = getSerializer(paramObject.schema.type!);
coercedParamArgs.push(serializer.coerce(paramArgs[i]));
Copy link
Member

@bajtos bajtos Apr 27, 2018

Choose a reason for hiding this comment

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

I have mixed feelings about rolling out our own coercion implementation. At one hand, it allows us to use the same coercion algorithm in multiple places (e.g. REST, ORM/database, etc.) and have full control over the coercion rules. At the other hand, there are many edge cases that are tricky to get right and we can easy end up with a behavior that's surprising for users because it's inconsistent with other coercion implementations (LB 3.x, AJV coercion).

Copy link
Contributor Author

@shimks shimks Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, I don't think there is a popular coercion package that we can leverage to fit to our needs.

I feel coercion rules from string to whatever seem consistent for most coercion implementations. I like your suggestion of having a comprehensive testing suite in your main comment down below and I think it will make for a good starting point.

Copy link
Contributor

@raymondfeng raymondfeng Apr 27, 2018

Choose a reason for hiding this comment

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

In this context, it's probably better to talk about deserialization. The responsibility is to extract typed/formatted values from http requests (a set of strings from path/query/header/body/...).

}
}
}
}
}
if (requestBodyIndex > -1) coercedParamArgs.splice(requestBodyIndex, 0, body);

return coercedParamArgs;
}
1 change: 1 addition & 0 deletions packages/rest/src/router/routing-table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
invokeMethod,
instantiateClass,
ValueOrPromise,
MetadataInspector,
} from '@loopback/context';
import {ServerRequest} from 'http';
import * as HttpErrors from 'http-errors';
Expand Down
Loading