allowArray flag for remoteObject `accept` by davidcheung · Pull Request #369 · strongloop/strong-remoting

davidcheung · 2016-10-11T19:15:35Z

fixes #360

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

davidcheung · 2016-10-12T15:34:44Z

@bajtos PTAL, this is so hacky 😭
any thoughts on how to improve the implementation?

bajtos · 2016-10-13T13:16:45Z

PTAL, this is so hacky

Yeah, I agree.

How about adding an options object to all three converter methods (fromSloppyValue, fromTypedValue, validate)? It will require one-time change in all converters to ensure options are passed down to other methods (please send it in a standalone commit), but then the implementation of allowArray should be very simple.

Thoughts?

davidcheung · 2016-10-14T19:37:07Z

@slnode test please

davidcheung · 2016-10-14T19:57:24Z

@bajtos PTAL again, i think this is better this time

bajtos

Agreed, this second version looks much better!

bajtos · 2016-10-17T09:55:00Z

    var httpFormat = o.http;
    var name = o.name || o.arg;
    var val;
+    var coercionOptions = {};


I would personally prefer conversionOptions over coercionOptions, because we already use "conversion" in TypeRegistry-related code, see e.g.

strong-remoting/lib/type-registry.js

Lines 22 to 31 in 97507d2

assert(typeof typeName === 'string' && typeName,

'typeName must be a non-empty string');

assert(typeof converter === 'object' && converter,

'converter must be an object');

assert(typeof converter.fromTypedValue === 'function',

'converter.fromTypedValue must be a function');

assert(typeof converter.fromSloppyValue === 'function',

'converter.fromSloppyValue must be a function');

assert(typeof converter.validate === 'function',

'converter.validate must be a function');

bajtos · 2016-10-17T09:55:07Z


    var typeConverter = ctx.typeRegistry.getConverter(o.type);
+    if (o.allowArray)
+      coercionOptions.allowArray = o.allowArray;


I am proposing to extract these two lines into a shared method, so that there is only one place to change if we need to add more options in the future. E.g. a static method SharedMethod.getConversionOptionsForArg(o).

bajtos · 2016-10-17T10:00:36Z

+            err = errorArrayItemsNotAnObject();
+            return;
+          }
+        });


I think this may be a bit cleaner:

var hasInvalidItems = value.some(function(item) { return typeof item !== 'object' || Array.isArray(item) || item === null; }); return hasInvalidItems ? errorArrayItemsNotAnObject() : null;

How about Dates as the array items, I think we should reject them too. Can we perhaps reuse validate inside the forEach/some callback?

var self = this; var hasInvalidItems = value.some(function(item) { return !self.validate(item/* no options (?) */); }); return hasInvalidItems ? errorArrayItemsNotAnObject() : null;

bajtos · 2016-10-17T10:05:13Z

 }
+
+function errorArrayItemsNotAnObject() {
+  var err = new Error(g.f('Array items are not an object.'));


How about Some of array items are not an object ?

bajtos · 2016-10-17T10:06:08Z

+
+  describe('json body - object - allowArray: true', function() {
+    verifyTestCases({ arg: 'data', type: 'object', allowArray: true }, [
+      // should accept regular objects, plus array of objects


could you please add few cases for "regular objects"?

davidcheung · 2016-10-17T20:03:05Z

oh nice, changed now auto collapse again, @bajtos PTAL
applied the feedback and also added test case in json-form and customClass

bajtos

Few more comments, PTAL

bajtos · 2016-10-18T12:54:44Z

+ * @param optionsObj (optional parameter that gets operated on if you provide it)
+ * @returns optionsObj(options to pass to typeConverter methods, eg: validate, fromTypedValue)
+ */
+SharedMethod.getConversionOptionsForArg = function(arg, options) {


Modifying input parameters is a bad practice that's better to be avoided.

SharedMethod.getConversionOptionsForArg = function(arg) { var options = {}; // ... return options; };

bajtos · 2016-10-18T12:55:21Z

      var name = desc.name || desc.arg;
      var uarg = SharedMethod.convertArg(desc, args[name]);
-
+      var coercionOptions = {};


Use SharedMethod.getConversionOptionsForArg, rename coercionOptions to conversionOptions.

bajtos · 2016-10-18T12:55:38Z

 * @return {*}                 Coerced argument.
 */
-function validateInputArgument(uarg, desc, ctx) {
+function validateInputArgument(uarg, desc, ctx, options) {


rename options to conversionOptions?

bajtos · 2016-10-18T12:56:45Z


-  validate: function(ctx, value) {
+  validate: function(ctx, value, options) {
+    var _self = this;


Why the leading underscore? AFAIK, we use self everywhere else in LoopBack.

bajtos

The jsdoc comment is not correct, the patch LGTM otherwise. No more review is needed.

bajtos · 2016-10-18T14:14:31Z

+ * build conversion options from remote's' args
+ *
+ * @param arg (remote args such as `accepts`)
+ * @param optionsObj (optional parameter that gets operated on if you provide it)


please remove

bajtos · 2016-10-18T14:15:33Z

+/**
+ * build conversion options from remote's' args
+ *
+ * @param arg (remote args such as `accepts`)


@param {Object} arg Definition of accepts/returns argument.

bajtos · 2016-10-18T14:16:15Z

+ *
+ * @param arg (remote args such as `accepts`)
+ * @param optionsObj (optional parameter that gets operated on if you provide it)
+ * @returns optionsObj(options to pass to typeConverter methods, eg: validate, fromTypedValue)


@returns {Object} Options object to pass to type-converter methods, e.g `validate` or `fromTypedValue`.

davidcheung · 2016-10-18T15:39:29Z

ci failing due to http-auth dropping node 10 / 12 support 😭 gevorg/http-auth@0097ed6

davidcheung · 2016-10-18T16:26:04Z

@slnode test please

davidcheung · 2016-10-18T17:42:23Z

@slnode test please

davidcheung · 2016-10-18T18:59:39Z

@slnode test please

options are now available in functions - fromTypedValue - fromSloppyValue - validate

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

davidcheung · 2016-10-19T15:30:38Z

@slnode test please

davidcheung added the #wip label Oct 11, 2016

davidcheung self-assigned this Oct 11, 2016

davidcheung added #review and removed #wip labels Oct 11, 2016

bajtos self-assigned this Oct 13, 2016

davidcheung force-pushed the allow-array branch 2 times, most recently from a94f021 to 073b003 Compare October 14, 2016 19:01

bajtos suggested changes Oct 17, 2016

View reviewed changes

davidcheung force-pushed the allow-array branch from 4d03f76 to 89a388c Compare October 17, 2016 20:00

bajtos suggested changes Oct 18, 2016

View reviewed changes

davidcheung force-pushed the allow-array branch 2 times, most recently from ebebf61 to e53a511 Compare October 18, 2016 14:01

bajtos approved these changes Oct 18, 2016

View reviewed changes

davidcheung force-pushed the allow-array branch from e53a511 to 08f6994 Compare October 18, 2016 14:58

David Cheung added 2 commits October 19, 2016 10:37

Adding options to typeConverters functions

a3572ea

options are now available in functions - fromTypedValue - fromSloppyValue - validate

davidcheung force-pushed the allow-array branch from 08f6994 to e9b7977 Compare October 19, 2016 14:37

davidcheung merged commit 6c7e23a into master Oct 19, 2016

davidcheung deleted the allow-array branch October 19, 2016 15:59

davidcheung removed the #review label Oct 19, 2016

davidcheung mentioned this pull request Oct 25, 2016

allow batch create for persisted models strongloop/loopback#2879

Merged

2 tasks

	assert(typeof typeName === 'string' && typeName,
	'typeName must be a non-empty string');
	assert(typeof converter === 'object' && converter,
	'converter must be an object');
	assert(typeof converter.fromTypedValue === 'function',
	'converter.fromTypedValue must be a function');
	assert(typeof converter.fromSloppyValue === 'function',
	'converter.fromSloppyValue must be a function');
	assert(typeof converter.validate === 'function',
	'converter.validate must be a function');

Uh oh!

Conversation

davidcheung commented Oct 11, 2016

Uh oh!

davidcheung commented Oct 12, 2016

Uh oh!

bajtos commented Oct 13, 2016

Uh oh!

davidcheung commented Oct 14, 2016

Uh oh!

davidcheung commented Oct 14, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidcheung commented Oct 17, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidcheung commented Oct 18, 2016

Uh oh!

davidcheung commented Oct 18, 2016

Uh oh!

davidcheung commented Oct 18, 2016

Uh oh!

davidcheung commented Oct 18, 2016

Uh oh!

davidcheung commented Oct 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants