From a3572eaf9d57cd2a8f9b51b62b01fe4a6b78bb67 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Thu, 13 Oct 2016 17:59:32 -0400 Subject: [PATCH 1/2] Adding options to typeConverters functions options are now available in functions - fromTypedValue - fromSloppyValue - validate --- lib/type-registry.js | 12 ++++++------ lib/types/any.js | 10 +++++----- lib/types/array.js | 28 ++++++++++++++-------------- lib/types/boolean.js | 8 ++++---- lib/types/date.js | 8 ++++---- lib/types/integer.js | 10 +++++----- lib/types/number.js | 10 +++++----- lib/types/object.js | 12 ++++++------ lib/types/string.js | 10 +++++----- 9 files changed, 54 insertions(+), 54 deletions(-) diff --git a/lib/type-registry.js b/lib/type-registry.js index 1d855355..401b38c3 100644 --- a/lib/type-registry.js +++ b/lib/type-registry.js @@ -48,9 +48,9 @@ TypeRegistry.prototype.registerObjectType = function(typeName, factoryFn) { assert(typeof factoryFn === 'function', 'factoryFn must be a function'); var converter = { - fromTypedValue: function(ctx, data) { + fromTypedValue: function(ctx, data, options) { var objectConverter = ctx.typeRegistry.getConverter('object'); - var result = objectConverter.fromTypedValue(ctx, data); + var result = objectConverter.fromTypedValue(ctx, data, options); if (result.error || result.value === undefined || result.value === null) return result; @@ -61,14 +61,14 @@ TypeRegistry.prototype.registerObjectType = function(typeName, factoryFn) { } }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { var objectConverter = ctx.typeRegistry.getConverter('object'); - var result = objectConverter.fromSloppyValue(ctx, value); + var result = objectConverter.fromSloppyValue(ctx, value, options); return result.error ? result : this.fromTypedValue(ctx, result.value); }, - validate: function(ctx, value) { - return ctx.typeRegistry.getConverter('object').validate(ctx, value); + validate: function(ctx, value, options) { + return ctx.typeRegistry.getConverter('object').validate(ctx, value, options); }, }; diff --git a/lib/types/any.js b/lib/types/any.js index 3c406e00..bf34d60b 100644 --- a/lib/types/any.js +++ b/lib/types/any.js @@ -17,11 +17,11 @@ var IS_INT_REGEX = /^\-?(?:[0-9]|[1-9][0-9]*)$/; var IS_FLOAT_REGEX = /^\-?([0-9]+)?\.[0-9]+$/; module.exports = { - fromTypedValue: function(ctx, value) { + fromTypedValue: function(ctx, value, options) { return { value: value }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { if (value === 'null' || value === null) return { value: null }; @@ -51,17 +51,17 @@ module.exports = { try { var result = JSON.parse(value); debug('parsed %j as JSON: %j', value, result); - return this.fromTypedValue(ctx, result); + return this.fromTypedValue(ctx, result, options); } catch (ex) { debug('Cannot parse "any" value %j, assuming string. %s', value, ex); // no-op, use the original string value } } - return this.fromTypedValue(ctx, value); + return this.fromTypedValue(ctx, value, options); }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { // no-op, all values are valid }, }; diff --git a/lib/types/array.js b/lib/types/array.js index 06d50faf..a6cb114a 100644 --- a/lib/types/array.js +++ b/lib/types/array.js @@ -17,7 +17,7 @@ function ArrayConverter(itemType) { this._itemType = itemType; } -ArrayConverter.prototype.fromTypedValue = function(ctx, value) { +ArrayConverter.prototype.fromTypedValue = function(ctx, value, options) { if (value === undefined || value === null) return { value: value }; @@ -29,7 +29,7 @@ ArrayConverter.prototype.fromTypedValue = function(ctx, value) { var itemConverter = ctx.typeRegistry.getConverter(this._itemType); for (var ix in value) { - itemResult = itemConverter.fromTypedValue(ctx, value[ix]); + itemResult = itemConverter.fromTypedValue(ctx, value[ix], options); itemResult = validateConverterResult(itemResult); debug('typed item result: %j -> %j as %s', value[ix], itemResult, this._itemType); @@ -41,7 +41,7 @@ ArrayConverter.prototype.fromTypedValue = function(ctx, value) { return { value: items }; }; -ArrayConverter.prototype.fromSloppyValue = function(ctx, value) { +ArrayConverter.prototype.fromSloppyValue = function(ctx, value, options) { if (value === undefined || value === '') { // undefined was chosen so that it plays well with ES6 default parameters. return { value: undefined }; @@ -51,12 +51,12 @@ ArrayConverter.prototype.fromSloppyValue = function(ctx, value) { return { value: null }; } - return this._fromTypedValueString(ctx, value) || - this._fromDelimitedString(ctx, value) || - this._fromSloppyData(ctx, value); + return this._fromTypedValueString(ctx, value, options) || + this._fromDelimitedString(ctx, value, options) || + this._fromSloppyData(ctx, value, options); }; -ArrayConverter.prototype._fromTypedValueString = function(ctx, value) { +ArrayConverter.prototype._fromTypedValueString = function(ctx, value, options) { if (!looksLikeJsonArray(value)) return null; @@ -64,7 +64,7 @@ ArrayConverter.prototype._fromTypedValueString = function(ctx, value) { try { var result = JSON.parse(value); debug('parsed %j as JSON: %j', value, result); - return this.fromTypedValue(ctx, result); + return this.fromTypedValue(ctx, result, options); } catch (ex) { debug('Cannot parse array value %j. %s', value, ex); var err = new Error(g.f('Cannot parse JSON-encoded array value.')); @@ -73,7 +73,7 @@ ArrayConverter.prototype._fromTypedValueString = function(ctx, value) { } }; -ArrayConverter.prototype._fromDelimitedString = function(ctx, value) { +ArrayConverter.prototype._fromDelimitedString = function(ctx, value, options) { if (typeof value !== 'string') return null; @@ -93,10 +93,10 @@ ArrayConverter.prototype._fromDelimitedString = function(ctx, value) { var items = value.split(delims); // perform sloppy-string coercion - return this.fromSloppyValue(ctx, items); + return this.fromSloppyValue(ctx, items, options); }; -ArrayConverter.prototype._fromSloppyData = function(ctx, value) { +ArrayConverter.prototype._fromSloppyData = function(ctx, value, options) { if (!Array.isArray(value)) { // Alright, not array-like, just wrap it in an array on the way out. value = [value]; @@ -109,7 +109,7 @@ ArrayConverter.prototype._fromSloppyData = function(ctx, value) { var itemConverter = ctx.typeRegistry.getConverter(this._itemType); for (var ix in value) { - itemResult = itemConverter.fromSloppyValue(ctx, value[ix]); + itemResult = itemConverter.fromSloppyValue(ctx, value[ix], options); itemResult = validateConverterResult(itemResult); debug('item %d: sloppy converted %j to %j', ix, value[ix], itemResult); if (itemResult.error) @@ -119,7 +119,7 @@ ArrayConverter.prototype._fromSloppyData = function(ctx, value) { return { value: items }; }; -ArrayConverter.prototype.validate = function(ctx, value) { +ArrayConverter.prototype.validate = function(ctx, value, options) { if (value === undefined || value === null) return null; @@ -129,7 +129,7 @@ ArrayConverter.prototype.validate = function(ctx, value) { var itemConverter = ctx.typeRegistry.getConverter(this._itemType); var itemError; for (var ix in value) { - itemError = itemConverter.validate(ctx, value[ix]); + itemError = itemConverter.validate(ctx, value[ix], options); if (itemError) return itemError; } }; diff --git a/lib/types/boolean.js b/lib/types/boolean.js index 4b6859ef..d4d8e8b1 100644 --- a/lib/types/boolean.js +++ b/lib/types/boolean.js @@ -9,12 +9,12 @@ var debug = require('debug')('strong-remoting:http-coercion'); var g = require('strong-globalize')(); module.exports = { - fromTypedValue: function(ctx, value) { - var error = this.validate(ctx, value); + fromTypedValue: function(ctx, value, options) { + var error = this.validate(ctx, value, options); return error ? { error: error } : { value: value }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { if (value === '' || value === undefined) return { value: undefined }; @@ -31,7 +31,7 @@ module.exports = { return { error: invalidBooleanError() }; }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { if (value === undefined || typeof value === 'boolean') return null; diff --git a/lib/types/date.js b/lib/types/date.js index 37676e4d..f264d501 100644 --- a/lib/types/date.js +++ b/lib/types/date.js @@ -9,7 +9,7 @@ var debug = require('debug')('strong-remoting:http-coercion'); var g = require('strong-globalize')(); module.exports = { - fromTypedValue: function(ctx, value) { + fromTypedValue: function(ctx, value, options) { if (value === undefined) return { value: value }; @@ -24,7 +24,7 @@ module.exports = { return error ? { error: error } : { value: result }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { if (value === '') return { value: undefined }; @@ -34,10 +34,10 @@ module.exports = { value = +value; } - return this.fromTypedValue(ctx, value); + return this.fromTypedValue(ctx, value, options); }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { if (value === undefined) return null; diff --git a/lib/types/integer.js b/lib/types/integer.js index 962a9ce2..d1d049c3 100644 --- a/lib/types/integer.js +++ b/lib/types/integer.js @@ -11,23 +11,23 @@ var isSafeInteger = require('../number-checks').isSafeInteger; var numberConverter = require('./number'); module.exports = { - fromTypedValue: function(ctx, value) { - var error = this.validate(ctx, value); + fromTypedValue: function(ctx, value, options) { + var error = this.validate(ctx, value, options); return error ? { error: error } : { value: value }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { var result = numberConverter.fromSloppyValue(ctx, value); if (result.error) return result; return this.fromTypedValue(ctx, result.value); }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { if (value === undefined) return null; - var err = numberConverter.validate(ctx, value); + var err = numberConverter.validate(ctx, value, options); if (err) return err; diff --git a/lib/types/number.js b/lib/types/number.js index e56c1573..f0572038 100644 --- a/lib/types/number.js +++ b/lib/types/number.js @@ -9,20 +9,20 @@ var debug = require('debug')('strong-remoting:http-coercion'); var g = require('strong-globalize')(); module.exports = { - fromTypedValue: function(ctx, value) { - var error = this.validate(ctx, value); + fromTypedValue: function(ctx, value, options) { + var error = this.validate(ctx, value, options); return error ? { error: error } : { value: value }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { if (value === undefined || value === '') return { value: undefined }; var result = +value; - return this.fromTypedValue(ctx, result); + return this.fromTypedValue(ctx, result, options); }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { if (value === undefined) return null; diff --git a/lib/types/object.js b/lib/types/object.js index ad493bf1..7e82db3b 100644 --- a/lib/types/object.js +++ b/lib/types/object.js @@ -10,12 +10,12 @@ var g = require('strong-globalize')(); var looksLikeJsonObject = require('../looks-like-json').looksLikeJsonObject; module.exports = { - fromTypedValue: function(ctx, value) { - var error = this.validate(ctx, value); + fromTypedValue: function(ctx, value, options) { + var error = this.validate(ctx, value, options); return error ? { error: error } : { value: value }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { if (value === undefined || value === '') { // undefined was chosen so that it plays well with ES6 default parameters. return { value: undefined }; @@ -28,7 +28,7 @@ module.exports = { try { var result = JSON.parse(value); debug('parsed %j as JSON: %j', value, result); - return this.fromTypedValue(ctx, result); + return this.fromTypedValue(ctx, result, options); } catch (ex) { debug('Cannot parse object value %j. %s', value, ex); var err = new Error(g.f('Cannot parse JSON-encoded object value.')); @@ -38,10 +38,10 @@ module.exports = { } // NOTE: nested values in objects are intentionally not coerced - return this.fromTypedValue(ctx, value); + return this.fromTypedValue(ctx, value, options); }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { if (value === undefined || value === null) return null; diff --git a/lib/types/string.js b/lib/types/string.js index f3ab3d4b..d9298293 100644 --- a/lib/types/string.js +++ b/lib/types/string.js @@ -9,12 +9,12 @@ var debug = require('debug')('strong-remoting:http-coercion'); var g = require('strong-globalize')(); module.exports = { - fromTypedValue: function(ctx, value) { - var error = this.validate(ctx, value); + fromTypedValue: function(ctx, value, options) { + var error = this.validate(ctx, value, options); return error ? { error: error } : { value: value }; }, - fromSloppyValue: function(ctx, value) { + fromSloppyValue: function(ctx, value, options) { if (value === '') { // Pass on empty string as undefined. // undefined was chosen so that it plays well with ES6 default parameters. @@ -26,10 +26,10 @@ module.exports = { if (value !== undefined && value !== null) value = '' + value; - return this.fromTypedValue(ctx, value); + return this.fromTypedValue(ctx, value, options); }, - validate: function(ctx, value) { + validate: function(ctx, value, options) { if (value === undefined || typeof value === 'string') return null; From e9b7977cfb72de32e89ecc03b68a644caec37b7b Mon Sep 17 00:00:00 2001 From: David Cheung Date: Fri, 14 Oct 2016 14:23:15 -0400 Subject: [PATCH 2/2] allowArray flag for remoteObject `accept` In loopback 2.x endpoints supported batch create which passes an Array of Objects into the endpoint that expects Objects, this is nolonger allowed in 3.x's stricter coercion In order to maintain backwards compatiability, this is a short term fix to allow endpoints to accept Array of Objects with the allowArray --- lib/http-context.js | 5 ++-- lib/shared-method.js | 23 +++++++++++++-- lib/types/object.js | 26 +++++++++++++++-- .../jsonbody-object-type.suite.js | 21 ++++++++++++++ test/rest-coercion/jsonbody-object.suite.js | 21 ++++++++++++++ test/rest-coercion/jsonform-object.suite.js | 28 +++++++++++++++++++ 6 files changed, 117 insertions(+), 7 deletions(-) diff --git a/lib/http-context.js b/lib/http-context.js index d752cfc2..73a3f2cd 100644 --- a/lib/http-context.js +++ b/lib/http-context.js @@ -122,6 +122,7 @@ HttpContext.prototype.buildArgs = function(method) { var val; var typeConverter = ctx.typeRegistry.getConverter(o.type); + var conversionOptions = SharedMethod.getConversionOptionsForArg(o); // Turn off sloppy coercion for values coming from JSON payloads. // This is because JSON, unlike other methods, properly retains types @@ -187,8 +188,8 @@ HttpContext.prototype.buildArgs = function(method) { // // Use some sloppy typing semantics to try to guess what the user meant to send. var result = doSloppyCoerce ? - typeConverter.fromSloppyValue(ctx, val) : - typeConverter.fromTypedValue(ctx, val); + typeConverter.fromSloppyValue(ctx, val, conversionOptions) : + typeConverter.fromTypedValue(ctx, val, conversionOptions); debug('arg %j: %s converted %j to %j', name, doSloppyCoerce ? 'sloppy' : 'typed', val, result); diff --git a/lib/shared-method.js b/lib/shared-method.js index 097f74a9..f8df64db 100644 --- a/lib/shared-method.js +++ b/lib/shared-method.js @@ -225,9 +225,10 @@ SharedMethod.prototype.invoke = function(scope, args, remotingOptions, ctx, cb) var desc = accepts[i]; var name = desc.name || desc.arg; var uarg = SharedMethod.convertArg(desc, args[name]); + var conversionOptions = SharedMethod.getConversionOptionsForArg(desc); try { - uarg = validateInputArgument(uarg, desc, ctx); + uarg = validateInputArgument(uarg, desc, ctx, conversionOptions); } catch (e) { debug('- %s - ' + e.message, sharedMethod.name); return cb(e); @@ -306,7 +307,7 @@ function escapeRegex(d) { * @param {Context} ctx Remoting request context. * @return {*} Coerced argument. */ -function validateInputArgument(uarg, desc, ctx) { +function validateInputArgument(uarg, desc, ctx, conversionOptions) { var name = desc.name || desc.arg; // Verify that a required argument has a value @@ -318,7 +319,7 @@ function validateInputArgument(uarg, desc, ctx) { } var converter = ctx.typeRegistry.getConverter(desc.type); - var err = converter.validate(ctx, uarg); + var err = converter.validate(ctx, uarg, conversionOptions); if (err) { err.message = g.f('Invalid argument %j. ', name) + err.message; throw err; @@ -636,3 +637,19 @@ SharedMethod.prototype.addAlias = function(alias) { this.aliases.push(alias); } }; + +/** + * build conversion options from remote's' args + * + * @param {Object} arg Definition of accepts/returns argument. + * @returns {Object} Options object to pass to type-converter methods, e.g `validate` or `fromTypedValue`. + */ +SharedMethod.getConversionOptionsForArg = function(arg) { + var options = {}; + + // option for object coercion to allow Array of objects as well as objects + if (arg.allowArray) { + options.allowArray = arg.allowArray; + } + return options; +}; diff --git a/lib/types/object.js b/lib/types/object.js index 7e82db3b..ceca987c 100644 --- a/lib/types/object.js +++ b/lib/types/object.js @@ -42,6 +42,8 @@ module.exports = { }, validate: function(ctx, value, options) { + var self = this; + var options = options || {}; if (value === undefined || value === null) return null; @@ -50,8 +52,22 @@ module.exports = { // reject object-like values that have their own strong-remoting type - if (Array.isArray(value)) - return errorNotAnObject(); + if (Array.isArray(value)) { + // TODO: @davidcheung, remove this flag and support [array or Object] + // see strong-remoting/issues/360 for details + // allowArray flag is to handle persistedModels uses + // array of Objects to batch create, which was supported in 2.x + if (!options.allowArray) { + return errorNotAnObject(); + } else { + var hasInvalidItems = value.some(function(item) { + // option is not passed here so it should always reject array `item(s)` + return self.validate(ctx, item); + }); + + return hasInvalidItems ? errorArrayItemsNotAnObject() : null; + } + } if (value instanceof Date) return errorNotAnObject(); @@ -65,3 +81,9 @@ function errorNotAnObject() { err.statusCode = 400; return err; } + +function errorArrayItemsNotAnObject() { + var err = new Error(g.f('Some of array items are not an object.')); + err.statusCode = 400; + return err; +} diff --git a/test/rest-coercion/jsonbody-object-type.suite.js b/test/rest-coercion/jsonbody-object-type.suite.js index f99cac5b..adfd8928 100644 --- a/test/rest-coercion/jsonbody-object-type.suite.js +++ b/test/rest-coercion/jsonbody-object-type.suite.js @@ -69,4 +69,25 @@ module.exports = function(ctx) { [{ invalid: true }, ERROR_BAD_REQUEST], ]); }); + + describe('json body - CustomClass - allowArray: true', function() { + verifyTestCases({ arg: 'anyname', type: 'CustomClass', allowArray: true }, [ + // normal objects is valid + [{ x: '' }], + [{ x: null }], + [{ x: {}}], + [{ x: { key: null }}], + + // array of objects also valid + [[{}]], + [[{ x: '' }]], + [[{ x: null }]], + [[{ x: 1 }, { y: 'string' }]], + + // array of non-objects are invalid + [[{}, [{}]], ERROR_BAD_REQUEST], + [[{}, 3.1415], ERROR_BAD_REQUEST], + [[{}, 'non-object'], ERROR_BAD_REQUEST], + ]); + }); }; diff --git a/test/rest-coercion/jsonbody-object.suite.js b/test/rest-coercion/jsonbody-object.suite.js index 298243b3..201fb9ee 100644 --- a/test/rest-coercion/jsonbody-object.suite.js +++ b/test/rest-coercion/jsonbody-object.suite.js @@ -75,4 +75,25 @@ module.exports = function(ctx) { [[1, 2], ERROR_BAD_REQUEST], ]); }); + + describe('json body - object - allowArray: true', function() { + verifyTestCases({ arg: 'data', type: 'object', allowArray: true }, [ + // normal objects is valid + [{ x: '' }], + [{ x: null }], + [{ x: {}}], + [{ x: { key: null }}], + + // array of objects also valid + [[{}]], + [[{ x: '' }]], + [[{ x: null }]], + [[{ x: 1 }, { y: 'string' }]], + + // array of non-objects are invalid + [[{}, [{}]], ERROR_BAD_REQUEST], + [[{}, 3.1415], ERROR_BAD_REQUEST], + [[{}, 'non-object'], ERROR_BAD_REQUEST], + ]); + }); }; diff --git a/test/rest-coercion/jsonform-object.suite.js b/test/rest-coercion/jsonform-object.suite.js index b12c5920..430aed03 100644 --- a/test/rest-coercion/jsonform-object.suite.js +++ b/test/rest-coercion/jsonform-object.suite.js @@ -70,4 +70,32 @@ module.exports = function(ctx) { [{ arg: [1, 2] }, ERROR_BAD_REQUEST], ]); }); + + describe('json form - object - allowArray: true', function() { + verifyTestCases({ arg: 'arg', type: 'object', allowArray: true }, [ + // normal objects is valid + [{ arg: { x: null }}, { x: null }], + [{ arg: {}}, {}], + [{ arg: { x: 'value' }}, { x: 'value' }], + [{ arg: { x: 1 }}, { x: 1 }], + + // array of objects also valid + [{ arg: [{}] }, [{}]], + [{ arg: [{ x: 1 }, {}] }, [{ x: 1 }, {}]], + [{ arg: [{ x: null }] }, [{ x: null }]], + + // Invalid values should trigger ERROR_BAD_REQUEST + [{ arg: '' }, ERROR_BAD_REQUEST], + [{ arg: false }, ERROR_BAD_REQUEST], + [{ arg: true }, ERROR_BAD_REQUEST], + [{ arg: 0 }, ERROR_BAD_REQUEST], + [{ arg: 1 }, ERROR_BAD_REQUEST], + [{ arg: -1 }, ERROR_BAD_REQUEST], + + // array of non-objects are invalid + [{ arg: [{}, [{}]] }, ERROR_BAD_REQUEST], + [{ arg: [{}, 3.1415] }, ERROR_BAD_REQUEST], + [{ arg: [{}, 'non-object'] }, ERROR_BAD_REQUEST], + ]); + }); };