[SEMVER-MAJOR] Fix object type conversion

[bajtos]

• Reject array/date values for object arguments
• Add rest-coercion tests for custom object types

This patch builds on top of recently landed #343

Connect to #312
Connect to strongloop-internal/scrum-loopback#974
Connect to strongloop-internal/scrum-loopback#885

@Amir-61 or @richardpringle please review
cc @ritch @STRML

[STRML]

This seems like it could be a nice utility function to pull out.

[bajtos]

Agreed, will extract.

[STRML]

LGTM, I agree that while JS says new Date() instanceof Object and Array() instanceof Object, that's nonsensical and clearly not what any developer would actually want.

[richardpringle]

I completely agree for dates, but some might be using any JSON notation as type-object, which of course includes an array.

I personally don't think that we should be so strict, if a developer wants to assure that an object strictly starts with {, they should implement the type check themselves.

[bajtos]

I completely agree for dates, but some might be using any JSON notation as type-object, which of course includes an array.

One can use type: 'any' for arguments that support multiple types.

I personally don't think that we should be so strict, if a developer wants to assure that an object strictly starts with {, they should implement the type check themselves.

My issue with this is that most developers won't realise their function can receive an array in an object argument.

IMO, the case where an array is not wanted will be much more common than the case where a method expects either an object or an array.

I am proposing to implement a new flag that will signal that an argument can accept both types and defer the implementation until there is a LoopBack user asking for this feature.

{ arg: 'data', type: 'object', allowArrays: true }

@richardpringle Thoughts?
cc @STRML @ritch

[bajtos]

My issue with this is that most developers won't realise their function can receive an array in an object argument.

For example, let's take a LoopBack model called Product:

$ node
> new Product([{ name: 'Pen' }, { name: 'Pencil' }])
{ '0': { name: 'Pen' }, '1': { name: 'Pencil' } }

Because the model implementation assumes the input is an object, it treats the array indices as property names and array items as property values. I don't think that's a result the caller was expecting!

[richardpringle]

I like the flag, because that allows anything in JSON format

[STRML]

Why not unions? Seems better to allow ['object', 'array'] than a special
flag.

On Sep 13, 2016 8:44 AM, "Richard Pringle" [email protected] wrote:

I like the flag, because that allows anything in JSON format

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#347 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABJFPxFZWIF6_3LNXZ397ctaKfrTcMKyks5qpqi4gaJpZM4J6rCX
.

[bajtos]

Why not unions? Seems better to allow ['object', 'array'] than a special flag.

Even better 👍

(Note that Swagger spec does not support union types yet, AFAIK.)

I still think this should be kept out of scope of this pull request though.

[STRML]

Yeah I agree.

In the future I'd like to see the ability to provide functions as types - could add the following extensions mirroring Model validators:

// First arg must match a provided arg in the remoting function
User.update.validate('firstname', function(err) {...})

And possibly even something to override coercion?

User.update.coerce('firstname', function(value) {...})

This way we could maintain swagger compat by still requiring a string type name.

[Amir-61]

Why not unions? Seems better to allow ['object', 'array'] than a special
flag.

Thats a great idea 👍 I agree we should keep it out of the scope of this PR.

LGTM

[richardpringle]

Might be a good idea to accept any valid JSON with type-JSON too (also beyond the scope of the PR).

[bajtos]

Thank you all for your input.