Missing required argument doesn't cause an error when that argument type is boolean

[CarlosCuevas]

Person.remoteMethod('createPerson', {
        accepts: [
            { arg: 'name', type: 'string', required: true },
            { arg: 'age', type: 'number', required: true },
            { arg: 'is_alive', type: 'boolean', required: true }
        ],
        http: {
            path: '/',
            verb: 'post'
        }
    });

If I send a request to this endpoint that is missing 'name' or 'age', it will return an error. If I send a request missing 'is_alive', it will proceed with 'is_alive' set to false.

[Neil-UWA]

I guess that you may have set a default value for is_alive in your model defination

[CarlosCuevas]

@Neil-UWA Thanks, but there are many cases where it would be inappropriate to set a default value (exactly when you'd want to set a field as required).

[richardpringle]

Hey @CarlosCuevas, I'm guessing that somewhere in the code that is_alive = undefined is evaluating to false. I will look into the details of the issue.

Thanks!

[richardpringle]

@bajtos, I found the source of the issue: https://github.com/strongloop/strong-remoting/blob/master/lib/dynamic.js#L98-L117

Boolean(undefined) // returns false;

This is an easy fix, but before I do so, I just want to double check to make sure there aren't any use-cases where you wouldn't want the return value to remain "undefined" for the Dynamic.define('boolean', ... function in general.

[bajtos]

@richardpringle thanks for checking, a change like this can have subtle side effects, so it's best to consider it thoroughly.

We have recently discovered quite few flaws in strong-remoting's argument coercion (see strongloop/strong-remoting#264, #1806, strongloop/strong-remoting#208), there is a spike story to investigate the issues and come up with a more robust design, see https://github.com/strongloop-internal/scrum-loopback/issues/706.

So much for the background context.

As for fixing boolean conversion, see also strongloop/strong-remoting@b924a79 (which has been reverted as it did not work well).

I found the source of the issue: https://github.com/strongloop/strong-remoting/blob/master/lib/dynamic.js#L98-L117

return Boolean(undefined) // returns false;
// we should return undefined instead

This is an easy fix, but before I do so, I just want to double check to make sure there aren't any use-cases where you wouldn't want the return value to remain "undefined"

I am inclining towards changing boolean converter to preserve undefined values, as you are proposing @richardpringle.

@ritch @raymondfeng @STRML do you have any opinion on this? Can you see a use case where the proposed change would be a problem?

[STRML]

FYI @bajtos we can't see those internal issues.
On Mar 2, 2016 6:55 PM, "Miroslav Bajtoš" notifications@github.com wrote:

@richardpringle https://github.com/richardpringle thanks for checking,
a change like this can have subtle side effects, so it's best to consider
it thoroughly.

We have recently discovered quite few flaws in strong-remoting's argument
coercion (see strongloop/strong-remoting#264
strongloop/strong-remoting#264,
#1806
#1806, strongloop/
strong-r emoting#208
strongloop/strong-remoting#208), there is a
spike story to investigate the issues and come up with a more robust
design, see strongloop-internal/scrum-loopback#706
https://github.com/strongloop-internal/scrum-loopback/issues/706.

So much for the background context.

As for fixing boolean conversion, see also strongloop/strong-remoting@
b924a79
strongloop/strong-remoting@b924a79
(which has been reverted as it did not work well).

I found the source of the issue:
https://github.com/strongloop/strong-remoting/blob/master/lib/dynamic.js#L98-L117

return Boolean(undefined) // returns false;// we should return undefined instead

This is an easy fix, but before I do so, I just want to double check to
make sure there aren't any use-cases where you wouldn't want the return
value to remain "undefined"

I am inclining towards changing boolean converter to preserve undefined
values, as you are proposing @richardpringle
https://github.com/richardpringle.

@ritch https://github.com/ritch @raymondfeng
https://github.com/raymondfeng @STRML https://github.com/STRML do you
have any opinion on this? Can you see a use case where the proposed change
would be a problem?

—
Reply to this email directly or view it on GitHub
#2092 (comment)
.

[bajtos]

@STRML we can't see those internal issues.

That internal issue is just a placeholder that helps us to plan our work in sprints. It lists the three issues I mentioned in my comment above - strongloop/strong-remoting#264, #1806, strongloop/strong-remoting#208.

I know the internal issues are not ideal, but let's keep that matter out of the discussion in this particular GH issue.

[STRML]

Not familiar with the term "spike story", assumed there was something of
substance since you linked it.
On Mar 2, 2016 7:26 PM, "Miroslav Bajtoš" notifications@github.com wrote:

@STRML https://github.com/STRML we can't see those internal issues.

That internal issue is just a placeholder that helps us to plan our work
in sprints. It lists the three issues I mentioned in my comment above -
strongloop/strong-remoting#264
strongloop/strong-remoting#264,
#1806
#1806,
strongloop/strong-remoting#208
strongloop/strong-remoting#208.

I know the internal issues are not ideal, but let's keep that matter out
of the discussion in this particular GH issue.

—
Reply to this email directly or view it on GitHub
#2092 (comment)
.

[richardpringle]

@STRML: Spike - research and come up with a proposal for a change.

There's nothing of substance in "spike" story, @bajtos just wanted to let me know that we are aware there is a problem.